Bug 156647 – iTunes crashing JavaScriptCore.dll

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

For Windows we create thread specific API built upon the TLS APIs in Windows.  The thread specific key code for windows does not call any destructor when the key is destroyed.  Instead the thread specific key destructors are called from the function ThreadSpecificThreadExit() and it is called from wtfThreadEntryPoint() right before the thread exits.

The problem here is that wtfThreadEntryPoint() is only called for threads created by WTF typically via JavaScriptCore or WebKit.  The crash happens when a thread created by some other means calls into JavaScript and gets added to the list of threads that MachineThread tracks AND that thread later exits.  In that case, the thread specific key destructor MachineThread::removeThread() will never be called by the "foreign" thread.  MachineThread tries to manage a thread, including walking its stack, but the thread no longer exists.

The fix is to change the WTF ThreadSpecific wrappers to use FLS, which does of a callback when the fiber exits.  Since we don't support multiple fibers, there will be only one fiber per thread.  This makes FLS work just like TLS for our purposes.

[Michael Saboff]

rdar://problem/25435085

[Michael Saboff]

Created attachment 276522 [details]
Patch

[Geoffrey Garen]

Comment on attachment 276522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276522&action=review

> Source/WTF/ChangeLog:21
> +        The fix is to change from using TLS to using FLS (Fiber Local Storage).  Although
> +        Windows allows multiple fibers per thread, WebKit is not designed to work with a
> +        multiple fibers per thread.  When ther is only one fiber per thread, FLS works just
> +        like TLS, but it has the destroy callback.

Let's double-check that our clients only use one fiber per thread.

[Geoffrey Garen]

See for example https://bugs.webkit.org/show_bug.cgi?id=102411.

[Michael Saboff]

Created attachment 276528 [details]
Patch - Fixed cast issue I didn't see with VS 2015 Express

[Michael Saboff]

(In reply to comment #3)
> Comment on attachment 276522 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276522&action=review
> 
> > Source/WTF/ChangeLog:21
> > +        The fix is to change from using TLS to using FLS (Fiber Local Storage).  Although
> > +        Windows allows multiple fibers per thread, WebKit is not designed to work with a
> > +        multiple fibers per thread.  When ther is only one fiber per thread, FLS works just
> > +        like TLS, but it has the destroy callback.
> 
> Let's double-check that our clients only use one fiber per thread.

I check with the iTunes team and they only use one fiber per thread.

[Michael Saboff]

Created attachment 276531 [details]
Patch - Fixed the other cast that the EWS bot didn't catch the first time

[Michael Saboff]

Committed r199621: <http://trac.webkit.org/changeset/199621>

[Darin Adler]

Comment on attachment 276531 [details]
Patch - Fixed the other cast that the EWS bot didn't catch the first time

View in context: https://bugs.webkit.org/attachment.cgi?id=276531&action=review

> Source/WTF/wtf/ThreadSpecific.h:170
> +    DWORD flsKey = FlsAlloc(reinterpret_cast<PFLS_CALLBACK_FUNCTION>(destructor));

Is this reinterpret_cast safe? If so, why?

> Source/WTF/wtf/ThreadSpecific.h:196
> +    DWORD flsKey = FlsAlloc(reinterpret_cast<PFLS_CALLBACK_FUNCTION>(destroy));

Same question.

[Darin Adler]

Apparently Geoff already reviewed this, but it was still set to review? for some reason.

[Csaba Osztrogonác]

(In reply to comment #8)
> Committed r199621: <http://trac.webkit.org/changeset/199621>

It made 50+ tests crash/timeout on Apple Windows bots:
https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/57479

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156708

[Michael Saboff]

It appears that at least some of the test crashes are due to running out of FLS indices.  We require one for every ThreadSpecificKey and according to online documentation, it looks like there are only 128 per process.  I'm looking into how we can reduce the number of keys to fix this issue.

There may be other reasons for the test timeouts / crashes.  I'll check after trying this first fix.

[Michael Saboff]

Created attachment 276691 [details]
Updated Patch - Reduced number of ThreadSpecific values in JSC

I fixed the running out of FLS keys crash with this patch, however I wasn't able to reproduce all of the crashes seen on the build bots.

[WebKit Commit Bot]

Attachment 276691 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WARNING: Not running on native Windows.
Total errors found: 4 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 276692 [details]
Removed tab characters

[Saam Barati]

Comment on attachment 276692 [details]
Removed tab characters

View in context: https://bugs.webkit.org/attachment.cgi?id=276692&action=review

r=me

> Source/WTF/ChangeLog:14
> +        This mechanism was put in place for Windos because we layer the WTF::ThreadSpecific

Typo: Windows

> Source/WTF/ChangeLog:20
> +        multiple fibers per thread.  When ther is only one fiber per thread, FLS works just

Typo: there

[Michael Saboff]

Committed r199726: <http://trac.webkit.org/changeset/199726>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156748

[Michael Saboff]

The WebKit bots build and test 32 bits.  I had been building and testing 64 bits.  Rolled out the latest change.  Building 32 bits now and will test after that is done.

[Michael Saboff]

Created attachment 276786 [details]
Patch with fixes for 32 bit Windows

[Mark Lam]

Comment on attachment 276786 [details]
Patch with fixes for 32 bit Windows

View in context: https://bugs.webkit.org/attachment.cgi?id=276786&action=review

> Source/WTF/ChangeLog:14
> +        This mechanism was put in place for Windos because we layer the WTF::ThreadSpecific

still have typo: /Windos/Windows/.

> Source/WTF/ChangeLog:16
> +        a thread exiting callback the way that pthread_create_key.

I think you meant to say "that pthread_create_key has."

> Source/WTF/ChangeLog:20
> +        multiple fibers per thread.  When ther is only one fiber per thread, FLS works just

typo: /ther/there/.

[Michael Saboff]

Committed r199762: <http://trac.webkit.org/changeset/199762>