Changeset 179900 in webkit

Timestamp:

Feb 10, 2015, 4:12:18 PM (12 years ago)

Author:

bshafiei@apple.com

Message:

Merged r179576, r179648. rdar://problem/19709260

Location:

branches/safari-600.1.4.15-branch/Source/JavaScriptCore

Files:

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

branches/safari-600.1.4.15-branch/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();

branches/safari-600.1.4.15-branch/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-02-10  Babak Shafiei  <bshafiei@apple.com>
+
+        Merge r179576, r179648.
+
+    2015-02-04  Mark Lam  <mark.lam@apple.com>
+
+            r179576 introduce a deadlock potential during GC thread suspension.
+            <https://webkit.org/b/141268>
+
+            Reviewed by Michael Saboff.
+
+            http://trac.webkit.org/r179576 introduced a potential for deadlocking.
+            In the GC thread suspension loop, we currently delete
+            MachineThreads::Thread that we detect to be invalid.  This is unsafe
+            because we may have already suspended some threads, and one of those
+            suspended threads may still be holding the C heap lock which we need
+            for deleting the invalid thread.
+
+            The fix is to put the invalid threads in a separate toBeDeleted list,
+            and delete them only after GC has resumed all threads.
+
+            * heap/MachineStackMarker.cpp:
+            (JSC::MachineThreads::removeCurrentThread):
+            - Undo refactoring removeThreadWithLockAlreadyAcquired() out of
+            removeCurrentThread() since it is no longer needed.
+
+            (JSC::MachineThreads::tryCopyOtherThreadStacks):
+            - Put invalid Threads on a threadsToBeDeleted list, and delete those
+            Threads only after all threads have been resumed.
+
+            (JSC::MachineThreads::removeThreadWithLockAlreadyAcquired): Deleted.
+            * heap/MachineStackMarker.h:
+
+    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-10  Babak Shafiei  <bshafiei@apple.com>
 

branches/safari-600.1.4.15-branch/Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -241 +241 @@
 }
 
-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 +457 @@
         MutexLocker lock(m_registeredThreadsMutex);
 
+        Thread* threadsToBeDeleted = nullptr;
+
 #ifndef NDEBUG
         // Forbid malloc during the gather phase. The gather phase suspends
@@ -459 +465 @@
         fastMallocForbid();
 #endif
-        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;
+        Thread* previousThread = nullptr;
+        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));
+                    
+                    // Put the invalid thread on the threadsToBeDeleted list.
+                    // We can't just delete it here because we have suspended other
+                    // threads, and they may still be holding the C heap lock which
+                    // we need for deleting the invalid thread. Hence, we need to
+                    // defer the deletion till after we have resumed all threads.
+                    Thread* nextThread = thread->next;
+                    thread->next = threadsToBeDeleted;
+                    threadsToBeDeleted = thread;
+                    
+                    if (previousThread)
+                        previousThread->next = nextThread;
+                    else
+                        m_registeredThreads = nextThread;
+                    thread = nextThread;
+                    continue;
+                }
+#else
+                UNUSED_PARAM(numberOfThreads);
+                ASSERT_UNUSED(success, success);
+#endif
+            }
+            previousThread = thread;
+            thread = thread->next;
         }
 
@@ -479 +526 @@
         fastMallocAllow();
 #endif
+        for (Thread* thread = threadsToBeDeleted; thread; ) {
+            Thread* nextThread = thread->next;
+            delete thread;
+            thread = nextThread;
+        }
     }
 }