iTunes crashing JavaScriptCore.dll

https://bugs.webkit.org/show_bug.cgi?id=156647

Reviewed by Saam Barati.

Source/JavaScriptCore:

Given that there there are only 128 FLS indices compared to over a 1000 for TLS, I
eliminated the thread specific m_threadSpecificForThread and instead we look for the
current thread in m_registeredThreads list when we need it.  In most cases there
will only be one thread.

* heap/MachineStackMarker.cpp:
(JSC::MachineThreads::MachineThreads):
(JSC::MachineThreads::~MachineThreads):
(JSC::MachineThreads::addCurrentThread):
(JSC::MachineThreads::machineThreadForCurrentThread):
(JSC::MachineThreads::removeThread):
* heap/MachineStackMarker.h:

Source/WTF:

If a thread was created without using the WTF thread apis and that thread uses
a JavaScript VM and that thread exits with the VM still around, JSC won't know
that the thread has exited.  Currently, we use ThreadSpecificThreadExit() to
clean up any thread specific keys.  Cleaning up these keys is how JSC is
notified of a thread exit.  We only call ThreadSpecificThreadExit() from
wtfThreadEntryPoint() when the thread entry point function returns.
This mechanism was put in place for Windows because we layer the WTF::ThreadSpecific
functionality on top of TLS (Thread Local Storage), but TLS doesn't have
a thread exiting callback the way that pthread_create_key.

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 there is only one fiber per thread, FLS works just
like TLS, but it has the destroy callback.

I restructured the Windows version of WTF::ThreadSpecific to be almost the same
as the pthread version.

* wtf/ThreadSpecific.h:
(WTF::threadSpecificKeyCreate):
(WTF::threadSpecificKeyDelete):
(WTF::threadSpecificSet):
(WTF::threadSpecificGet):
(WTF::ThreadSpecific<T>::ThreadSpecific):
(WTF::ThreadSpecific<T>::~ThreadSpecific):
(WTF::ThreadSpecific<T>::get):
(WTF::ThreadSpecific<T>::set):
(WTF::ThreadSpecific<T>::destroy):
Restructured to use FLS.  Renamed TLS* to FLS*.

* wtf/ThreadSpecificWin.cpp:
(WTF::flsKeyCount):
(WTF::flsKeys):
Renamed from tlsKey*() to flsKey*().

(WTF::destructorsList): Deleted.
(WTF::destructorsMutex): Deleted.
(WTF::PlatformThreadSpecificKey::PlatformThreadSpecificKey): Deleted.
(WTF::PlatformThreadSpecificKey::~PlatformThreadSpecificKey): Deleted.
(WTF::PlatformThreadSpecificKey::setValue): Deleted.
(WTF::PlatformThreadSpecificKey::value): Deleted.
(WTF::PlatformThreadSpecificKey::callDestructor): Deleted.
(WTF::tlsKeyCount): Deleted.
(WTF::tlsKeys): Deleted.
(WTF::threadSpecificKeyCreate): Deleted.
(WTF::threadSpecificKeyDelete): Deleted.
(WTF::threadSpecificSet): Deleted.
(WTF::threadSpecificGet): Deleted.
(WTF::ThreadSpecificThreadExit): Deleted.

* wtf/ThreadingWin.cpp:
(WTF::wtfThreadEntryPoint): Eliminated call to ThreadSpecificThreadExit.

LayoutTests:

Disabled fast/workers/dedicated-worker-lifecycle.html as it creates
more workers that we have ThreadSpecific keys.  We need at least one
key per JSC VM we create.  I didn't want to weaken the test for other
platforms.

* platform/win/TestExpectations:


Canonical link: https://commits.webkit.org/174861@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@199726 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,17 @@
+2016-04-19  Michael Saboff  <msaboff@apple.com>
+
+        iTunes crashing JavaScriptCore.dll
+        https://bugs.webkit.org/show_bug.cgi?id=156647
+
+        Reviewed by Saam Barati.
+
+        Disabled fast/workers/dedicated-worker-lifecycle.html as it creates
+        more workers that we have ThreadSpecific keys.  We need at least one
+        key per JSC VM we create.  I didn't want to weaken the test for other
+        platforms.
+
+        * platform/win/TestExpectations:
+
 2016-04-19  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [INTL] Use @thisNumberValue instead of `instanceof @Number`

LayoutTests/platform/win/TestExpectations

@@ -437,6 +437,10 @@ storage/storagequota-query-usage.html [ Skip ]
 storage/storagequota-request-quota.html [ Skip ]
 fast/workers/worker-storagequota-query-usage.html [ Skip ]
 
+# The number of workers in this test exceeds the number of
+# ThreadSpecificValues we have on Windows.
+fast/workers/dedicated-worker-lifecycle.html [ Skip ] 
+
 # Tests that require ENABLE(DOWNLOAD_ATTRIBUTE).
 fast/dom/HTMLAnchorElement/anchor-nodownload.html [ Skip ]
 fast/dom/HTMLAnchorElement/anchor-download.html [ Skip ]

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,23 @@
+2016-04-19  Michael Saboff  <msaboff@apple.com>
+
+        iTunes crashing JavaScriptCore.dll
+        https://bugs.webkit.org/show_bug.cgi?id=156647
+
+        Reviewed by Saam Barati.
+
+        Given that there there are only 128 FLS indices compared to over a 1000 for TLS, I
+        eliminated the thread specific m_threadSpecificForThread and instead we look for the
+        current thread in m_registeredThreads list when we need it.  In most cases there
+        will only be one thread.
+
+        * heap/MachineStackMarker.cpp:
+        (JSC::MachineThreads::MachineThreads):
+        (JSC::MachineThreads::~MachineThreads):
+        (JSC::MachineThreads::addCurrentThread):
+        (JSC::MachineThreads::machineThreadForCurrentThread):
+        (JSC::MachineThreads::removeThread):
+        * heap/MachineStackMarker.h:
+
 2016-04-19  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [INTL] Use @thisNumberValue instead of `instanceof @Number`

Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -192,22 +192,19 @@ static inline PlatformThread getCurrentPlatformThread()
 MachineThreads::MachineThreads(Heap* heap)
     : m_registeredThreads(0)
     , m_threadSpecificForMachineThreads(0)
-    , m_threadSpecificForThread(0)
 #if !ASSERT_DISABLED
     , m_heap(heap)
 #endif
 {
     UNUSED_PARAM(heap);
     threadSpecificKeyCreate(&m_threadSpecificForMachineThreads, removeThread);
-    threadSpecificKeyCreate(&m_threadSpecificForThread, nullptr);
     activeMachineThreadsManager().add(this);
 }
 
 MachineThreads::~MachineThreads()
 {
     activeMachineThreadsManager().remove(this);
     threadSpecificKeyDelete(m_threadSpecificForMachineThreads);
-    threadSpecificKeyDelete(m_threadSpecificForThread);
 
     LockHolder registeredThreadsLock(m_registeredThreadsMutex);
     for (Thread* t = m_registeredThreads; t;) {
@@ -234,18 +231,6 @@ bool MachineThreads::Thread::operator==(const PlatformThread& other) const
 #endif
 }
 
-#ifndef NDEBUG
-static bool isThreadInList(Thread* listHead, Thread* target)
-{
-    for (Thread* thread = listHead; thread; thread = thread->next) {
-        if (thread == target)
-            return true;
-    }
-
-    return false;
-}
-#endif
-
 void MachineThreads::addCurrentThread()
 {
     ASSERT(!m_heap->vm()->hasExclusiveThread() || m_heap->vm()->exclusiveThread() == std::this_thread::get_id());
@@ -254,15 +239,12 @@ void MachineThreads::addCurrentThread()
 #ifndef NDEBUG
         LockHolder lock(m_registeredThreadsMutex);
         ASSERT(threadSpecificGet(m_threadSpecificForMachineThreads) == this);
-        ASSERT(threadSpecificGet(m_threadSpecificForThread));
-        ASSERT(isThreadInList(m_registeredThreads, static_cast<Thread*>(threadSpecificGet(m_threadSpecificForThread))));
 #endif
         return;
     }
 
     Thread* thread = Thread::createForCurrentThread();
     threadSpecificSet(m_threadSpecificForMachineThreads, this);
-    threadSpecificSet(m_threadSpecificForThread, thread);
 
     LockHolder lock(m_registeredThreadsMutex);
 
@@ -272,14 +254,15 @@ void MachineThreads::addCurrentThread()
 
 Thread* MachineThreads::machineThreadForCurrentThread()
 {
-    Thread* result = static_cast<Thread*>(threadSpecificGet(m_threadSpecificForThread));
-    RELEASE_ASSERT(result);
-#ifndef NDEBUG
     LockHolder lock(m_registeredThreadsMutex);
-    ASSERT(isThreadInList(m_registeredThreads, result));
-#endif
+    PlatformThread platformThread = getCurrentPlatformThread();
+    for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
+        if (*thread == platformThread)
+            return thread;
+    }
 
-    return result;
+    RELEASE_ASSERT_NOT_REACHED();
+    return nullptr;
 }
 
 void MachineThreads::removeThread(void* p)

Source/JavaScriptCore/heap/MachineStackMarker.h

@@ -159,7 +159,6 @@ class MachineThreads {
     Lock m_registeredThreadsMutex;
     Thread* m_registeredThreads;
     WTF::ThreadSpecificKey m_threadSpecificForMachineThreads;
-    WTF::ThreadSpecificKey m_threadSpecificForThread;
 #if !ASSERT_DISABLED
     Heap* m_heap;
 #endif

Source/WTF/ChangeLog

@@ -1,3 +1,63 @@
+2016-04-19  Michael Saboff  <msaboff@apple.com>
+
+        iTunes crashing JavaScriptCore.dll
+        https://bugs.webkit.org/show_bug.cgi?id=156647
+
+        Reviewed by Saam Barati.
+
+        If a thread was created without using the WTF thread apis and that thread uses
+        a JavaScript VM and that thread exits with the VM still around, JSC won't know
+        that the thread has exited.  Currently, we use ThreadSpecificThreadExit() to
+        clean up any thread specific keys.  Cleaning up these keys is how JSC is
+        notified of a thread exit.  We only call ThreadSpecificThreadExit() from
+        wtfThreadEntryPoint() when the thread entry point function returns.
+        This mechanism was put in place for Windows because we layer the WTF::ThreadSpecific
+        functionality on top of TLS (Thread Local Storage), but TLS doesn't have
+        a thread exiting callback the way that pthread_create_key.
+
+        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 there is only one fiber per thread, FLS works just
+        like TLS, but it has the destroy callback.
+
+        I restructured the Windows version of WTF::ThreadSpecific to be almost the same
+        as the pthread version.
+
+        * wtf/ThreadSpecific.h:
+        (WTF::threadSpecificKeyCreate):
+        (WTF::threadSpecificKeyDelete):
+        (WTF::threadSpecificSet):
+        (WTF::threadSpecificGet):
+        (WTF::ThreadSpecific<T>::ThreadSpecific):
+        (WTF::ThreadSpecific<T>::~ThreadSpecific):
+        (WTF::ThreadSpecific<T>::get):
+        (WTF::ThreadSpecific<T>::set):
+        (WTF::ThreadSpecific<T>::destroy):
+        Restructured to use FLS.  Renamed TLS* to FLS*.
+
+        * wtf/ThreadSpecificWin.cpp:
+        (WTF::flsKeyCount):
+        (WTF::flsKeys):
+        Renamed from tlsKey*() to flsKey*().
+
+        (WTF::destructorsList): Deleted.
+        (WTF::destructorsMutex): Deleted.
+        (WTF::PlatformThreadSpecificKey::PlatformThreadSpecificKey): Deleted.
+        (WTF::PlatformThreadSpecificKey::~PlatformThreadSpecificKey): Deleted.
+        (WTF::PlatformThreadSpecificKey::setValue): Deleted.
+        (WTF::PlatformThreadSpecificKey::value): Deleted.
+        (WTF::PlatformThreadSpecificKey::callDestructor): Deleted.
+        (WTF::tlsKeyCount): Deleted.
+        (WTF::tlsKeys): Deleted.
+        (WTF::threadSpecificKeyCreate): Deleted.
+        (WTF::threadSpecificKeyDelete): Deleted.
+        (WTF::threadSpecificSet): Deleted.
+        (WTF::threadSpecificGet): Deleted.
+        (WTF::ThreadSpecificThreadExit): Deleted.
+
+        * wtf/ThreadingWin.cpp:
+        (WTF::wtfThreadEntryPoint): Eliminated call to ThreadSpecificThreadExit.
+
 2016-04-19  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [GTK] Use Generic WorkQueue instead of WorkQueueGLib

Source/WTF/wtf/ThreadSpecific.h

@@ -94,9 +94,6 @@ template<typename T> class ThreadSpecific {
 
         T* value;
         ThreadSpecific<T>* owner;
-#if OS(WINDOWS)
-        void (*destructor)(void*);
-#endif
     };
 
 #if USE(PTHREADS)
@@ -158,47 +155,64 @@ inline void ThreadSpecific<T>::set(T* ptr)
 
 #elif OS(WINDOWS)
 
-// The maximum number of TLS keys that can be created. For simplification, we assume that:
+// The maximum number of FLS keys that can be created. For simplification, we assume that:
 // 1) Once the instance of ThreadSpecific<> is created, it will not be destructed until the program dies.
 // 2) We do not need to hold many instances of ThreadSpecific<> data. This fixed number should be far enough.
-const int kMaxTlsKeySize = 256;
+const int kMaxFlsKeySize = 128;
+
+WTF_EXPORT_PRIVATE long& flsKeyCount();
+WTF_EXPORT_PRIVATE DWORD* flsKeys();
+
+typedef DWORD ThreadSpecificKey;
+
+inline void threadSpecificKeyCreate(ThreadSpecificKey* key, void (*destructor)(void *))
+{
+    DWORD flsKey = FlsAlloc(reinterpret_cast<PFLS_CALLBACK_FUNCTION>(destructor));
+    if (flsKey == FLS_OUT_OF_INDEXES)
+        CRASH();
+
+    *key = flsKey;
+}
 
-WTF_EXPORT_PRIVATE long& tlsKeyCount();
-WTF_EXPORT_PRIVATE DWORD* tlsKeys();
+inline void threadSpecificKeyDelete(ThreadSpecificKey key)
+{
+    FlsFree(key);
+}
 
-class PlatformThreadSpecificKey;
-typedef PlatformThreadSpecificKey* ThreadSpecificKey;
+inline void threadSpecificSet(ThreadSpecificKey key, void* data)
+{
+    FlsSetValue(key, data);
+}
 
-WTF_EXPORT_PRIVATE void threadSpecificKeyCreate(ThreadSpecificKey*, void (*)(void *));
-WTF_EXPORT_PRIVATE void threadSpecificKeyDelete(ThreadSpecificKey);
-WTF_EXPORT_PRIVATE void threadSpecificSet(ThreadSpecificKey, void*);
-WTF_EXPORT_PRIVATE void* threadSpecificGet(ThreadSpecificKey);
+inline void* threadSpecificGet(ThreadSpecificKey key)
+{
+    return FlsGetValue(key);
+}
 
 template<typename T>
 inline ThreadSpecific<T>::ThreadSpecific()
     : m_index(-1)
 {
-    DWORD tlsKey = TlsAlloc();
-    if (tlsKey == TLS_OUT_OF_INDEXES)
+    DWORD flsKey = FlsAlloc(reinterpret_cast<PFLS_CALLBACK_FUNCTION>(destroy));
+    if (flsKey == FLS_OUT_OF_INDEXES)
         CRASH();
 
-    m_index = InterlockedIncrement(&tlsKeyCount()) - 1;
-    if (m_index >= kMaxTlsKeySize)
+    m_index = InterlockedIncrement(&flsKeyCount()) - 1;
+    if (m_index >= kMaxFlsKeySize)
         CRASH();
-    tlsKeys()[m_index] = tlsKey;
+    flsKeys()[m_index] = flsKey;
 }
 
 template<typename T>
 inline ThreadSpecific<T>::~ThreadSpecific()
 {
-    // Does not invoke destructor functions. They will be called from ThreadSpecificThreadExit when the thread is detached.
-    TlsFree(tlsKeys()[m_index]);
+    FlsFree(flsKeys()[m_index]);
 }
 
 template<typename T>
 inline T* ThreadSpecific<T>::get()
 {
-    Data* data = static_cast<Data*>(TlsGetValue(tlsKeys()[m_index]));
+    Data* data = static_cast<Data*>(FlsGetValue(flsKeys()[m_index]));
     return data ? data->value : 0;
 }
 
@@ -207,8 +221,7 @@ inline void ThreadSpecific<T>::set(T* ptr)
 {
     ASSERT(!get());
     Data* data = new Data(ptr, this);
-    data->destructor = &ThreadSpecific<T>::destroy;
-    TlsSetValue(tlsKeys()[m_index], data);
+    FlsSetValue(flsKeys()[m_index], data);
 }
 
 #else
@@ -232,7 +245,7 @@ inline void ThreadSpecific<T>::destroy(void* ptr)
 #if USE(PTHREADS)
     pthread_setspecific(data->owner->m_key, 0);
 #elif OS(WINDOWS)
-    TlsSetValue(tlsKeys()[data->owner->m_index], 0);
+    FlsSetValue(flsKeys()[data->owner->m_index], 0);
 #else
 #error ThreadSpecific is not implemented for this platform.
 #endif