Sometimes jsc.exe never exits. Debugging shows that is waits for a lock under MachineThreads::removeThreadIfFound when calling jscExit().
Created attachment 279771 [details] Patch
Comment on attachment 279771 [details] Patch Ideally I’d like to see a more elegant fix for this. It’s really unfortunate to need the MainThread concept at all in this file; that’s a more fragile higher level concept than the rest of what is here. This should be a lower level class than MainThread. The need to initialize the main thread in jsc.cpp shows this new Windows-only dependency we are creating on the MainThread concept. Is there some Windows-specific way to understand which thread is the main one. It also seems better to not pass the remove function to threadSpecificKeyCreate in the first place rather than adding the function but then having the function do nothing. So I would put the code in the MachineThreads constructor.
Comment on attachment 279771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279771&action=review r=me > Source/JavaScriptCore/jsc.cpp:2350 > // Initialize JSC before getting VM. > -#if ENABLE(SAMPLING_REGIONS) > +#if ENABLE(SAMPLING_REGIONS) || OS(WINDOWS) > WTF::initializeMainThread(); > #endif Why is Windows the only OS that needs to call WTF::initializeMainThread()? Is there some Windows-specific initialization that's needed, or does this get initialized in a different places on macOS/iOS?
(In reply to comment #3) > Comment on attachment 279771 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279771&action=review > > r=me > > > Source/JavaScriptCore/jsc.cpp:2350 > > // Initialize JSC before getting VM. > > -#if ENABLE(SAMPLING_REGIONS) > > +#if ENABLE(SAMPLING_REGIONS) || OS(WINDOWS) > > WTF::initializeMainThread(); > > #endif > > Why is Windows the only OS that needs to call WTF::initializeMainThread()? > > Is there some Windows-specific initialization that's needed, or does this > get initialized in a different places on macOS/iOS? Thanks for reviewing! I added this call, because the implementation on Windows of isMainThread() requires that WTF::initializeMainThread() is called first.
(In reply to comment #2) > Comment on attachment 279771 [details] > Patch > > Ideally I’d like to see a more elegant fix for this. > > It’s really unfortunate to need the MainThread concept at all in this file; > that’s a more fragile higher level concept than the rest of what is here. > This should be a lower level class than MainThread. The need to initialize > the main thread in jsc.cpp shows this new Windows-only dependency we are > creating on the MainThread concept. Is there some Windows-specific way to > understand which thread is the main one. > > It also seems better to not pass the remove function to > threadSpecificKeyCreate in the first place rather than adding the function > but then having the function do nothing. So I would put the code in the > MachineThreads constructor. Thanks for reviewing :) I also see that there is an additional difference in how the pthread and Windows thread specific implementation work. On Windows, the destructor is also called when threadSpecificKeyDelete is called, see https://msdn.microsoft.com/en-us/library/windows/desktop/ms682665(v=vs.85).aspx. This does not seem to be the case with the pthread implementation, as far as I can see. With this in mind, I think this patch needs a little more work.
Created attachment 284328 [details] Patch
Comment on attachment 284328 [details] Patch r=me
(In reply to comment #7) > Comment on attachment 284328 [details] > Patch > > r=me Thanks for reviewing!
Comment on attachment 284328 [details] Patch Clearing flags on attachment: 284328 Committed r203603: <http://trac.webkit.org/changeset/203603>
All reviewed patches have been landed. Closing bug.
Created attachment 284367 [details] Crash log This change seems to have caused CLoop tests to fail with this assertion: mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: x = new Function(); x.toString = Object.prototype.toString; x.charAt = String.prototype.charAt; x.charAt(17) = PASSED! mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: ASSERTION FAILED: m_apiLock->currentThreadIsHoldingLock() mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: /Volumes/Data/slave/elcapitan-cloop-debug/build/Source/JavaScriptCore/runtime/VM.cpp(364) : JSC::VM::~VM() mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 1 0x103a9b0e0 WTFCrash mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 2 0x103a43e55 JSC::VM::~VM() mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 3 0x103a45025 JSC::VM::~VM() mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 4 0x10328b6c9 WTF::ThreadSafeRefCounted<JSC::VM>::deref() mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 5 0x10328b670 WTF::Ref<JSC::VM>::~Ref() mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 6 0x10326dd35 WTF::Ref<JSC::VM>::~Ref() mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 7 0x103260526 jscmain(int, char**) mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 8 0x10326035b main mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 9 0x7fff8f36c5ad start mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 10 0x7 https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/8242
Re-opened since this is blocked by bug 160096
(In reply to comment #11) > Created attachment 284367 [details] > Crash log > > This change seems to have caused CLoop tests to fail with this assertion: > > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: x = new > Function(); x.toString = Object.prototype.toString; x.charAt = > String.prototype.charAt; x.charAt(17) = PASSED! > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: ASSERTION > FAILED: m_apiLock->currentThreadIsHoldingLock() > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: > /Volumes/Data/slave/elcapitan-cloop-debug/build/Source/JavaScriptCore/ > runtime/VM.cpp(364) : JSC::VM::~VM() > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 1 0x103a9b0e0 > WTFCrash > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 2 0x103a43e55 > JSC::VM::~VM() > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 3 0x103a45025 > JSC::VM::~VM() > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 4 0x10328b6c9 > WTF::ThreadSafeRefCounted<JSC::VM>::deref() > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 5 0x10328b670 > WTF::Ref<JSC::VM>::~Ref() > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 6 0x10326dd35 > WTF::Ref<JSC::VM>::~Ref() > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 7 0x103260526 > jscmain(int, char**) > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 8 0x10326035b > main > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 9 > 0x7fff8f36c5ad start > mozilla-tests.yaml/ecma/String/15.5.4.4-4.js.mozilla-llint: 10 0x7 > > > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/8242 I think this change will cause every debug JSC stress test to fail.
Created attachment 286962 [details] Patch
This bug appears to be causing timeouts on Windows bots when running JSC stress tests.
Comment on attachment 286962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286962&action=review > Source/JavaScriptCore/jsc.cpp:-2435 > -#if ENABLE(SAMPLING_REGIONS) Did you mean to remove this for all ports? Should it maybe be "#if ENABLE(SAMPLING_REGIONS) || PLATFORM(WIN)"?
(In reply to comment #16) > Comment on attachment 286962 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286962&action=review > > > Source/JavaScriptCore/jsc.cpp:-2435 > > -#if ENABLE(SAMPLING_REGIONS) > > Did you mean to remove this for all ports? Should it maybe be "#if > ENABLE(SAMPLING_REGIONS) || PLATFORM(WIN)"? It seems this is the only place where ENABLE(SAMPLING_REGIONS) is still used, so I assumed it was not removed when the other occurrences of ENABLE(SAMPLING_REGIONS) were removed. But perhaps it should still be used here?
Created attachment 297012 [details] Patch
Comment on attachment 297012 [details] Patch This seems like an inelegant solution, but I don’t have something better to suggest at this time.
(In reply to comment #19) > Comment on attachment 297012 [details] > Patch > > This seems like an inelegant solution, but I don’t have something better to > suggest at this time. Yes, I agree :) Thanks for reviewing!
Committed r210237: <http://trac.webkit.org/changeset/210237>