Finish completeSweep before shutting down V8

third_party/WebKit/Source/platform/heap/ThreadState.cpp

Index: third_party/WebKit/Source/platform/heap/ThreadState.cpp
diff --git a/third_party/WebKit/Source/platform/heap/ThreadState.cpp b/third_party/WebKit/Source/platform/heap/ThreadState.cpp
index 8446223a5880b38e074f72b38ad995b6393ba25a..91457e541122b590979f28d8c28bd2028862177a 100644
--- a/third_party/WebKit/Source/platform/heap/ThreadState.cpp
+++ b/third_party/WebKit/Source/platform/heap/ThreadState.cpp
@@ -209,6 +209,16 @@ void ThreadState::attachMainThread()
     attachedThreads().add(state);
 }
 
+
+void ThreadState::stopMainThread()
+{
+    ASSERT(isMainThread());
+
+    // Finish sweeping before shutting down V8. Otherwise, some destructor
+    // may access V8 and cause crashes.
+    completeSweep();
+}
+
 void ThreadState::detachMainThread()
 {
     // Enter a safe point before trying to acquire threadAttachMutex
@@ -218,23 +228,17 @@ void ThreadState::detachMainThread()
     ThreadState* state = mainThreadState();
     ASSERT(state == ThreadState::current());
     ASSERT(state->checkThread());
-    // You must call unregisterTraceDOMWrappers before detaching
-    // the main thread.
-    ASSERT(!state->m_isolate);
+    ASSERT(!state->isSweepingInProgress());
 
-    // 1. Finish sweeping.
-    state->completeSweep();
-    {
-        SafePointAwareMutexLocker locker(threadAttachMutex(), BlinkGC::NoHeapPointersOnStack);
+    SafePointAwareMutexLocker locker(threadAttachMutex(), BlinkGC::NoHeapPointersOnStack);

[haraken, 2016/03/10 07:28:53]

Ah, now I remember why I had to clear state->m_isolate before detaching the main
thread.

1) V8 is shut down.
2) ThreadState::detachMainThread() is called.
3) SafePointAwareMutexLocker triggers a GC.
4) The GC tries to iterate V8 wrappers. V8 is already gone.

#0 0x000004714f2b base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7fcaeee9dcb0 <unknown>
#2 0x00000293f208 v8::internal::GlobalHandles::IterateAllRootsWithClassIds()
#3 0x0000026f6746 v8::Isolate::VisitHandlesWithClassIds()
#4 0x000002cdda0f blink::V8GCController::traceDOMWrappers()
#5 0x000002c70b73 blink::ThreadState::visitPersistents()
#6 0x000002c70941 blink::ThreadState::visitPersistentRoots()
#7 0x000002c663cf blink::Heap::collectGarbage()
#8 0x000002c672e1 blink::Heap::collectAllGarbage()
#9 0x000002c765b6 blink::ThreadState::enterSafePoint()
#10 0x000002c7014a blink::SafePointAwareMutexLocker::SafePointAwareMutexLocker()
#11 0x000002c6ffd0 blink::ThreadState::detachMainThread()
#12 0x000002c8f1f7 blink::shutdownWithoutV8()
#13 0x00000144ca68
content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport()
#14 0x00000144cb49
content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport()
#15 0x000001448b6b content::UnitTestTestSuite::~UnitTestTestSuite()

Clearing m_isolate is helpful to prevent the Oilpan's GC from iterating V8
wrappers, but it's not a right solution because it's already problematic that a
GC is triggered after V8 is shut down. As described in this bug, some destructor
invoked in the GC may access V8 and cause crashes.

My proposal is 1) change the SafePointMutexLocker to MutexLocker or 2) just
remove the SafePointMutexLocker. It is already guaranteed that the main thread
is the last thread that gets detached. So we don't need to grab the mutex here.

What do you think?

[sof, 2016/03/10 07:35:41]

If you add an assert or two to capture those assumptions in detachMainThread(),
that approach sounds fine to me.

 
-        // 2. Add the main thread's heap pages to the orphaned pool.
-        state->cleanupPages();
+    // Add the main thread's heap pages to the orphaned pool.
+    state->cleanupPages();
 
-        // 3. Detach the main thread.
-        ASSERT(attachedThreads().contains(state));
-        attachedThreads().remove(state);
-        state->~ThreadState();
-    }
+    // Detach the main thread.
+    ASSERT(attachedThreads().contains(state));
+    attachedThreads().remove(state);
+    state->~ThreadState();
 }
 
 void ThreadState::attach()