Changeset 179648 in webkit

Timestamp:

Feb 4, 2015 4:32:47 PM (9 years ago)

Author:

mark.lam@apple.com

Message:

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:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/MachineStackMarker.cpp (modified) (7 diffs)
• heap/MachineStackMarker.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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-04  Joseph Pecoraro  <pecoraro@apple.com>
 

trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -184 +184 @@
 }
 
-template<typename PlatformThread>
-void MachineThreads::removeThreadWithLockAlreadyAcquired(PlatformThread threadToRemove)
-{
-    if (equalThread(threadToRemove, m_registeredThreads->platformThread)) {
+void MachineThreads::removeCurrentThread()
+{
+    PlatformThread currentPlatformThread = getCurrentPlatformThread();
+
+    MutexLocker lock(m_registeredThreadsMutex);
+    if (equalThread(currentPlatformThread, m_registeredThreads->platformThread)) {
         Thread* t = m_registeredThreads;
         m_registeredThreads = m_registeredThreads->next;
@@ -195 +197 @@
         Thread* t;
         for (t = m_registeredThreads->next; t; t = t->next) {
-            if (equalThread(t->platformThread, threadToRemove)) {
+            if (equalThread(t->platformThread, currentPlatformThread)) {
                 last->next = t->next;
                 break;
@@ -204 +206 @@
         delete t;
     }
-}
-    
-void MachineThreads::removeCurrentThread()
-{
-    PlatformThread currentPlatformThread = getCurrentPlatformThread();
-
-    MutexLocker lock(m_registeredThreadsMutex);
-    removeThreadWithLockAlreadyAcquired(currentPlatformThread);
 }
     
@@ -457 +451 @@
     int numberOfThreads = 0; // Using 0 to denote that we haven't counted the number of threads yet.
     int index = 1;
-
+    Thread* threadsToBeDeleted = nullptr;
+
+    Thread* previousThread = nullptr;
     for (Thread* thread = m_registeredThreads; thread; index++) {
         if (!equalThread(thread->platformThread, currentPlatformThread)) {
@@ -475 +471 @@
                     "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;
-                removeThreadWithLockAlreadyAcquired(thread->platformThread);
+                thread->next = threadsToBeDeleted;
+                threadsToBeDeleted = thread;
+
+                if (previousThread)
+                    previousThread->next = nextThread;
+                else
+                    m_registeredThreads = nextThread;
                 thread = nextThread;
                 continue;
@@ -486 +493 @@
 #endif
         }
+        previousThread = thread;
         thread = thread->next;
     }
@@ -499 +507 @@
     }
 
+    for (Thread* thread = threadsToBeDeleted; thread; ) {
+        Thread* nextThread = thread->next;
+        delete thread;
+        thread = nextThread;
+    }
+    
     return *size <= capacity;
 }

trunk/Source/JavaScriptCore/heap/MachineStackMarker.h

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