Changeset 179576 in webkit

Timestamp:

Feb 3, 2015 4:00:00 PM (9 years ago)

Author:

mark.lam@apple.com

Message:

Workaround a thread library bug where thread destructors may not get called.
<​https://webkit.org/b/141209>

Reviewed by Michael Saboff.

There's a bug where thread destructors may not get called. As far as
we know, this only manifests on darwin ports. We will work around this
by checking at GC time if the platform thread is still valid. If not,
we'll purge it from the VM's registeredThreads list before proceeding
with thread scanning activity.

Note: it is important that we do this invalid thread detection during
suspension, because the validity (and liveness) of the other thread is
only guaranteed while it is suspended.

• API/tests/testapi.mm:

(threadMain):

• Added a test to enter the VM from another thread before we GC on the main thread.

• heap/MachineStackMarker.cpp:

(JSC::MachineThreads::removeThreadWithLockAlreadyAcquired):
(JSC::MachineThreads::removeCurrentThread):

• refactored removeThreadWithLockAlreadyAcquired() out from removeCurrentThread() so that we can also call it for purging invalid threads.

(JSC::suspendThread):

• Added a return status to tell if the suspension succeeded or not.

(JSC::MachineThreads::tryCopyOtherThreadStacks):

• Check if the suspension failed, and purge the thread if we can't suspend it. Failure to suspend implies that the thread has terminated without calling its destructor.
• heap/MachineStackMarker.h:

Location:

trunk/Source/JavaScriptCore

Files:

• API/tests/testapi.mm (modified) (3 diffs)
• ChangeLog (modified) (1 diff)
• heap/MachineStackMarker.cpp (modified) (5 diffs)
• heap/MachineStackMarker.h (modified) (1 diff)

trunk/Source/JavaScriptCore/API/tests/testapi.mm

@@ -30 +30 @@
 #import "JSExportTests.h"
 
+#import <pthread.h>
+
 extern "C" void JSSynchronousGarbageCollectForDebugging(JSContextRef);
 extern "C" void JSSynchronousEdenCollectForDebugging(JSContextRef);
@@ -469 +471 @@
     }();
     return containsClass;
+}
+
+static void* threadMain(void* contextPtr)
+{
+    JSContext *context = (__bridge JSContext*)contextPtr;
+
+    // Do something to enter the VM.
+    TestObject *testObject = [TestObject testObject];
+    context[@"testObject"] = testObject;
+    pthread_exit(nullptr);
 }
 
@@ -1360 +1372 @@
     }
 
+    @autoreleasepool {
+        JSContext *context = [[JSContext alloc] init];
+        
+        pthread_t threadID;
+        pthread_create(&threadID, NULL, &threadMain, (__bridge void*)context);
+        pthread_join(threadID, nullptr);
+        JSSynchronousGarbageCollectForDebugging([context JSGlobalContextRef]);
+
+        checkResult(@"Did not crash after entering the VM from another thread", true);
+    }
+    
     currentThisInsideBlockGetterTest();
     runDateTests();

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-02-03  Mark Lam  <mark.lam@apple.com>
+
+        Workaround a thread library bug where thread destructors may not get called.
+        <https://webkit.org/b/141209>
+
+        Reviewed by Michael Saboff.
+
+        There's a bug where thread destructors may not get called.  As far as
+        we know, this only manifests on darwin ports.  We will work around this
+        by checking at GC time if the platform thread is still valid.  If not,
+        we'll purge it from the VM's registeredThreads list before proceeding
+        with thread scanning activity.
+
+        Note: it is important that we do this invalid thread detection during
+        suspension, because the validity (and liveness) of the other thread is
+        only guaranteed while it is suspended.
+
+        * API/tests/testapi.mm:
+        (threadMain):
+        - Added a test to enter the VM from another thread before we GC on
+          the main thread.
+
+        * heap/MachineStackMarker.cpp:
+        (JSC::MachineThreads::removeThreadWithLockAlreadyAcquired):
+        (JSC::MachineThreads::removeCurrentThread):
+        - refactored removeThreadWithLockAlreadyAcquired() out from
+          removeCurrentThread() so that we can also call it for purging invalid
+          threads.
+        (JSC::suspendThread):
+        - Added a return status to tell if the suspension succeeded or not.
+        (JSC::MachineThreads::tryCopyOtherThreadStacks):
+        - Check if the suspension failed, and purge the thread if we can't
+          suspend it.  Failure to suspend implies that the thread has
+          terminated without calling its destructor.
+        * heap/MachineStackMarker.h:
+
 2015-02-03  Joseph Pecoraro  <pecoraro@apple.com>
 

trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -191 +191 @@
 }
 
-void MachineThreads::removeCurrentThread()
-{
-    PlatformThread currentPlatformThread = getCurrentPlatformThread();
-
-    MutexLocker lock(m_registeredThreadsMutex);
-
-    if (equalThread(currentPlatformThread, m_registeredThreads->platformThread)) {
+template<typename PlatformThread>
+void MachineThreads::removeThreadWithLockAlreadyAcquired(PlatformThread threadToRemove)
+{
+    if (equalThread(threadToRemove, m_registeredThreads->platformThread)) {
         Thread* t = m_registeredThreads;
         m_registeredThreads = m_registeredThreads->next;
@@ -205 +202 @@
         Thread* t;
         for (t = m_registeredThreads->next; t; t = t->next) {
-            if (equalThread(t->platformThread, currentPlatformThread)) {
+            if (equalThread(t->platformThread, threadToRemove)) {
                 last->next = t->next;
                 break;
@@ -215 +212 @@
     }
 }
-
+    
+void MachineThreads::removeCurrentThread()
+{
+    PlatformThread currentPlatformThread = getCurrentPlatformThread();
+
+    MutexLocker lock(m_registeredThreadsMutex);
+    removeThreadWithLockAlreadyAcquired(currentPlatformThread);
+}
+    
 void MachineThreads::gatherFromCurrentThread(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks, void* stackCurrent, RegisterState& registers)
 {
@@ -227 +232 @@
 }
 
-static inline void suspendThread(const PlatformThread& platformThread)
-{
-#if OS(DARWIN)
-    thread_suspend(platformThread);
-#elif OS(WINDOWS)
-    SuspendThread(platformThread);
+static inline bool suspendThread(const PlatformThread& platformThread)
+{
+#if OS(DARWIN)
+    kern_return_t result = thread_suspend(platformThread);
+    return result == KERN_SUCCESS;
+#elif OS(WINDOWS)
+    bool threadIsSuspended = (SuspendThread(platformThread) != (DWORD)-1);
+    ASSERT(threadIsSuspended);
+    return threadIsSuspended;
 #elif USE(PTHREADS)
     pthread_kill(platformThread, SigThreadSuspendResume);
+    return true;
 #else
 #error Need a way to suspend threads on this platform
@@ -453 +462 @@
 
     PlatformThread currentPlatformThread = getCurrentPlatformThread();
-    for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
-        if (!equalThread(thread->platformThread, currentPlatformThread))
-            suspendThread(thread->platformThread);
+    int numberOfThreads = 0; // Using 0 to denote that we haven't counted the number of threads yet.
+    int index = 1;
+
+    for (Thread* thread = m_registeredThreads; thread; index++) {
+        if (!equalThread(thread->platformThread, currentPlatformThread)) {
+            bool success = suspendThread(thread->platformThread);
+#if OS(DARWIN)
+            if (!success) {
+                if (!numberOfThreads) {
+                    for (Thread* countedThread = m_registeredThreads; countedThread; countedThread = countedThread->next)
+                        numberOfThreads++;
+                }
+                
+                // Re-do the suspension to get the actual failure result for logging.
+                kern_return_t error = thread_suspend(thread->platformThread);
+                ASSERT(error != KERN_SUCCESS);
+
+                WTFReportError(__FILE__, __LINE__, WTF_PRETTY_FUNCTION,
+                    "JavaScript garbage collection encountered an invalid thread (err 0x%x): Thread [%d/%d: %p] platformThread %p.",
+                    error, index, numberOfThreads, thread, reinterpret_cast<void*>(thread->platformThread));
+                
+                Thread* nextThread = thread->next;
+                removeThreadWithLockAlreadyAcquired(thread->platformThread);
+                thread = nextThread;
+                continue;
+            }
+#else
+            UNUSED_PARAM(numberOfThreads);
+            ASSERT_UNUSED(success, success);
+#endif
+        }
+        thread = thread->next;
     }
 

trunk/Source/JavaScriptCore/heap/MachineStackMarker.h

@@ -59 +59 @@
         void removeCurrentThread();
 
+        template<typename PlatformThread>
+        void removeThreadWithLockAlreadyAcquired(PlatformThread);
+
         Mutex m_registeredThreadsMutex;
         Thread* m_registeredThreads;