Oilpan: Allocation should be allowed in pre-finalizers

Source/platform/heap/ThreadState.cpp

Index: Source/platform/heap/ThreadState.cpp
diff --git a/Source/platform/heap/ThreadState.cpp b/Source/platform/heap/ThreadState.cpp
index ce2fc79e21c42f39a9d22b65e0e4a545ba34586f..22c590af2bc8db8788678311d29b9fb5278e5dfe 100644
--- a/Source/platform/heap/ThreadState.cpp
+++ b/Source/platform/heap/ThreadState.cpp
@@ -517,6 +517,31 @@ bool ThreadState::popAndInvokeThreadLocalWeakCallback(Visitor* visitor)
     return false;
 }
 
+void ThreadState::threadLocalWeakProcessing()
+{
+    ASSERT(!sweepForbidden());
+    TRACE_EVENT0("blink_gc", "ThreadState::threadLocalWeakProcessing");
+    SweepForbiddenScope forbiddenScope(this);
+    if (isMainThread())
+        ScriptForbiddenScope::enter();
+
+    // Disallow allocation during weak processing.
+    // It would be technically safe to allow allocations, but it is unsafe
+    // to mutate an object graph in a way in which a dead object gets
+    // resurrected or mutate a HashTable (because HashTable's weak processing
+    // assumes that the HashTable hasn't been mutated since the latest marking).
+    // Due to the complexity, we just forbid allocations.
+    NoAllocationScope noAllocationScope(this);
+
+    MarkingVisitor<Visitor::WeakProcessing> weakProcessingVisitor;
+
+    // Perform thread-specific weak processing.
+    while (popAndInvokeThreadLocalWeakCallback(&weakProcessingVisitor)) { }
+
+    if (isMainThread())
+        ScriptForbiddenScope::exit();
+}
+
 PersistentAnchor& ThreadState::globalRoots()
 {
     AtomicallyInitializedStaticReference(PersistentAnchor, anchor, new PersistentAnchor);
@@ -679,7 +704,7 @@ void ThreadState::performIdleLazySweep(double deadlineSeconds)
         return;
 
     bool sweepCompleted = true;
-    ThreadState::SweepForbiddenScope scope(this);
+    SweepForbiddenScope scope(this);
     {
         if (isMainThread())
             ScriptForbiddenScope::enter();
@@ -950,39 +975,23 @@ void ThreadState::preSweep()
     if (gcState() != EagerSweepScheduled && gcState() != LazySweepScheduled)
         return;
 
-    {
-        if (isMainThread())
-            ScriptForbiddenScope::enter();
-
-        SweepForbiddenScope forbiddenScope(this);
-        {
-            MarkingVisitor<Visitor::WeakProcessing> weakProcessingVisitor;
+    threadLocalWeakProcessing();
 
-            // Disallow allocation during weak processing.
-            NoAllocationScope noAllocationScope(this);
-            {
-                // Perform thread-specific weak processing.
-                TRACE_EVENT0("blink_gc", "ThreadState::threadLocalWeakProcessing");
-                while (popAndInvokeThreadLocalWeakCallback(&weakProcessingVisitor)) { }
-            }
-            {
-                TRACE_EVENT0("blink_gc", "ThreadState::invokePreFinalizers");
-                invokePreFinalizers();
-            }
-        }
+    GCState previousGCState = gcState();

[sof, 2015/06/18 12:18:03]

This runs the risk of triggering an unused variable warning, non-Oilpan.

[haraken, 2015/06/18 16:30:13]

Done.

+    setGCState(Sweeping);

[sof, 2015/06/18 12:18:03]

Moving this up before running the prefinalizers makes me a bit worried, but I'm
fine with giving it a go -- I cannot see it allowing problematic operations.

[haraken, 2015/06/18 16:30:13]

Moving setGCState(Sweeping) up here is necessary to avoid a GC from getting
scheduled during the pre-finalizers. scheduleGCIfNeeded() returns without
nothing if isSweepingProgress returns true.

I added a comment about this.

 
-        if (isMainThread())
-            ScriptForbiddenScope::exit();
-    }
+    // Allocation is allowed during the pre-finalizers and destructors.
+    // However, they must not mutate an object graph in a way in which
+    // a dead object gets resurrected.
+    invokePreFinalizers();
 
 #if defined(ADDRESS_SANITIZER)
     poisonEagerHeap(SetPoison);
 #endif
 
 #if ENABLE(LAZY_SWEEPING)
-    if (gcState() == EagerSweepScheduled) {
+    if (previousGCState == EagerSweepScheduled) {
         // Eager sweeping should happen only in testing.
-        setGCState(Sweeping);
         eagerSweep();
 #if defined(ADDRESS_SANITIZER)
         poisonAllHeaps();
@@ -990,7 +999,6 @@ void ThreadState::preSweep()
         completeSweep();
     } else {
         // The default behavior is lazy sweeping.
-        setGCState(Sweeping);
         eagerSweep();
 #if defined(ADDRESS_SANITIZER)
         poisonAllHeaps();
@@ -998,7 +1006,6 @@ void ThreadState::preSweep()
         scheduleIdleLazySweep();
     }
 #else
-    setGCState(Sweeping);
     completeSweep();
 #endif
 
@@ -1040,7 +1047,7 @@ void ThreadState::eagerSweep()
     if (sweepForbidden())
         return;
 
-    ThreadState::SweepForbiddenScope scope(this);
+    SweepForbiddenScope scope(this);
     {
         if (isMainThread())
             ScriptForbiddenScope::enter();
@@ -1064,7 +1071,7 @@ void ThreadState::completeSweep()
     if (sweepForbidden())
         return;
 
-    ThreadState::SweepForbiddenScope scope(this);
+    SweepForbiddenScope scope(this);
     {
         if (isMainThread())
             ScriptForbiddenScope::enter();
@@ -1297,6 +1304,13 @@ void ThreadState::unregisterPreFinalizerInternal(void* target)
 void ThreadState::invokePreFinalizers()
 {
     checkThread();
+    ASSERT(!sweepForbidden());
+    TRACE_EVENT0("blink_gc", "ThreadState::invokePreFinalizers");
+
+    if (isMainThread())
+        ScriptForbiddenScope::enter();
+
+    SweepForbiddenScope forbiddenScope(this);
     Vector<void*> deadObjects;
     for (auto& entry : m_preFinalizers) {
         if (entry.value(entry.key))
@@ -1304,6 +1318,9 @@ void ThreadState::invokePreFinalizers()
     }
     // FIXME: removeAll is inefficient.  It can shrink repeatedly.
     m_preFinalizers.removeAll(deadObjects);
+
+    if (isMainThread())
+        ScriptForbiddenScope::exit();
 }
 
 void ThreadState::clearHeapAges()