Bug 158073 – [Win] jsc.exe sometimes never exits.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Per Arne Vollan]

Sometimes jsc.exe never exits. Debugging shows that is waits for a lock under MachineThreads::removeThreadIfFound when calling jscExit().

[Per Arne Vollan]

Created attachment 279771 [details]
Patch

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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?

[Per Arne Vollan]

(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.

[Per Arne Vollan]

(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.

[Per Arne Vollan]

Created attachment 284328 [details]
Patch

[Mark Lam]

Comment on attachment 284328 [details]
Patch

r=me

[Per Arne Vollan]

(In reply to comment #7)
> Comment on attachment 284328 [details]
> Patch
> 
> r=me

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 284328 [details]
Patch

Clearing flags on attachment: 284328

Committed r203603: <http://trac.webkit.org/changeset/203603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 160096

[Saam Barati]

(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.

[Per Arne Vollan]

Created attachment 286962 [details]
Patch

[Per Arne Vollan]

This bug appears to be causing timeouts on Windows bots when running JSC stress tests.

[Brent Fulgham]

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)"?

[Per Arne Vollan]

(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?

[Per Arne Vollan]

Created attachment 297012 [details]
Patch

[Darin Adler]

Comment on attachment 297012 [details]
Patch

This seems like an inelegant solution, but I don’t have something better to suggest at this time.

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Committed r210237: <http://trac.webkit.org/changeset/210237>