Modified: trunk/Source/_javascript_Core/ChangeLog (179647 => 179648)
--- trunk/Source/_javascript_Core/ChangeLog 2015-02-05 00:32:27 UTC (rev 179647)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-02-05 00:32:47 UTC (rev 179648)
@@ -1,3 +1,32 @@
+2015-02-04 Mark Lam <[email protected]>
+
+ 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 <[email protected]>
Web Inspector: Clean up Object Property Descriptor Collection
Modified: trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp (179647 => 179648)
--- trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp 2015-02-05 00:32:27 UTC (rev 179647)
+++ trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp 2015-02-05 00:32:47 UTC (rev 179648)
@@ -183,10 +183,12 @@
static_cast<MachineThreads*>(p)->removeCurrentThread();
}
-template<typename PlatformThread>
-void MachineThreads::removeThreadWithLockAlreadyAcquired(PlatformThread threadToRemove)
+void MachineThreads::removeCurrentThread()
{
- if (equalThread(threadToRemove, m_registeredThreads->platformThread)) {
+ PlatformThread currentPlatformThread = getCurrentPlatformThread();
+
+ MutexLocker lock(m_registeredThreadsMutex);
+ if (equalThread(currentPlatformThread, m_registeredThreads->platformThread)) {
Thread* t = m_registeredThreads;
m_registeredThreads = m_registeredThreads->next;
delete t;
@@ -194,7 +196,7 @@
Thread* last = m_registeredThreads;
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;
}
@@ -205,14 +207,6 @@
}
}
-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)
{
void* registersBegin = ®isters;
@@ -456,7 +450,9 @@
PlatformThread currentPlatformThread = getCurrentPlatformThread();
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)) {
bool success = suspendThread(thread->platformThread);
@@ -474,9 +470,20 @@
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;
- removeThreadWithLockAlreadyAcquired(thread->platformThread);
+ thread->next = threadsToBeDeleted;
+ threadsToBeDeleted = thread;
+
+ if (previousThread)
+ previousThread->next = nextThread;
+ else
+ m_registeredThreads = nextThread;
thread = nextThread;
continue;
}
@@ -485,6 +492,7 @@
ASSERT_UNUSED(success, success);
#endif
}
+ previousThread = thread;
thread = thread->next;
}
@@ -498,6 +506,12 @@
resumeThread(thread->platformThread);
}
+ for (Thread* thread = threadsToBeDeleted; thread; ) {
+ Thread* nextThread = thread->next;
+ delete thread;
+ thread = nextThread;
+ }
+
return *size <= capacity;
}
Modified: trunk/Source/_javascript_Core/heap/MachineStackMarker.h (179647 => 179648)
--- trunk/Source/_javascript_Core/heap/MachineStackMarker.h 2015-02-05 00:32:27 UTC (rev 179647)
+++ trunk/Source/_javascript_Core/heap/MachineStackMarker.h 2015-02-05 00:32:47 UTC (rev 179648)
@@ -57,9 +57,6 @@
static void removeThread(void*);
void removeCurrentThread();
- template<typename PlatformThread>
- void removeThreadWithLockAlreadyAcquired(PlatformThread);
-
Mutex m_registeredThreadsMutex;
Thread* m_registeredThreads;
WTF::ThreadSpecificKey m_threadSpecific;
