Changeset 179753 in webkit for trunk/Source/JavaScriptCore/heap/Heap.cpp

Timestamp:

Feb 6, 2015, 12:57:45 PM (11 years ago)

Author:

[email protected]

Message:

MachineThreads should be ref counted.
<​https://webkit.org/b/141317>

Reviewed by Filip Pizlo.

The VM's MachineThreads registry object is being referenced from other
threads as a raw pointer. In a scenario where the VM is destructed on
the main thread, there is no guarantee that another thread isn't still
holding a reference to the registry and will eventually invoke
removeThread() on it on thread exit. Hence, there's a possible use
after free scenario here.

The fix is to make MachineThreads ThreadSafeRefCounted, and have all
threads that references keep a RefPtr to it to ensure that it stays
alive until the very last thread is done with it.

• API/tests/testapi.mm:

(useVMFromOtherThread): - Renamed to be more descriptive.
(useVMFromOtherThreadAndOutliveVM):

• Added a test that has another thread which uses the VM outlive the VM to confirm that there is no crash.

However, I was not actually able to get the VM to crash without this
patch because I wasn't always able to the thread destructor to be
called. With this patch applied, I did verify with some logging that
the MachineThreads registry is only destructed after all threads
have removed themselves from it.

(threadMain): Deleted.

• heap/Heap.cpp:

(JSC::Heap::Heap):
(JSC::Heap::~Heap):
(JSC::Heap::gatherStackRoots):

• heap/Heap.h:

(JSC::Heap::machineThreads):

• heap/MachineStackMarker.cpp:

(JSC::MachineThreads::Thread::Thread):
(JSC::MachineThreads::addCurrentThread):
(JSC::MachineThreads::removeCurrentThread):

• heap/MachineStackMarker.h:

File:

• trunk/Source/JavaScriptCore/heap/Heap.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -312 +312 @@
     , m_storageSpace(this)
     , m_extraMemoryUsage(0)
-    , m_machineThreads(this)
     , m_sharedData(vm)
     , m_slotVisitor(m_sharedData)
@@ -341 +340 @@
 #endif
 {
+    m_machineThreads = adoptRef(new MachineThreads(this));
     m_storageSpace.init();
     if (Options::verifyHeap())
@@ -348 +348 @@
 Heap::~Heap()
 {
+    // We need to remove the main thread explicitly here because the main thread
+    // may not terminate for a while though the Heap (and VM) is being shut down.
+    m_machineThreads->removeCurrentThread();
 }
 
@@ -588 +591 @@
     GCPHASE(GatherStackRoots);
     m_jitStubRoutines.clearMarks();
-    m_machineThreads.gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
+    m_machineThreads->gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
 }