Oilpan: Allocation should be allowed in pre-finalizers

Source/platform/heap/ThreadState.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 499 matching lines @@
         // TODO(Oilpan): Avoid calling weak callbacks for dead objects.
         // We can do that by checking isHeapObjectAlive(object) before
         // calling the weak callback, but in that case Callback::Item
         // needs to understand T*.
         item->call(visitor);
         return true;
     }
     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);
     return anchor;
 }
 
 Mutex& ThreadState::globalRootsMutex()
 {
     AtomicallyInitializedStaticReference(Mutex, mutex, new Mutex);
     return mutex;
@@ skipping 142 matching lines @@
     if (!isSweepingInProgress())
         return;
 
     // This check is here to prevent performIdleLazySweep() from being called
     // recursively. I'm not sure if it can happen but it would be safer to have
     // the check just in case.
     if (sweepForbidden())
         return;
 
     bool sweepCompleted = true;
-    ThreadState::SweepForbiddenScope scope(this);
+    SweepForbiddenScope scope(this);
     {
         if (isMainThread())
             ScriptForbiddenScope::enter();
 
         for (int i = 0; i < NumberOfHeaps; i++) {
             // lazySweepWithDeadline() won't check the deadline until it sweeps
             // 10 pages. So we give a small slack for safety.
             double slack = 0.001;
             double remainingBudget = deadlineSeconds - slack - Platform::current()->monotonicallyIncreasingTime();
             if (remainingBudget <= 0 || !m_heaps[i]->lazySweepWithDeadline(deadlineSeconds)) {
@@ skipping 250 matching lines @@
         m_gcState = NoGCScheduled;
     }
 }
 
 void ThreadState::preSweep()
 {
     checkThread();
     if (gcState() != EagerSweepScheduled && gcState() != LazySweepScheduled)
         return;
 
-    {
-        if (isMainThread())
-            ScriptForbiddenScope::enter();
+    threadLocalWeakProcessing();
 
-        SweepForbiddenScope forbiddenScope(this);
-        {
-            MarkingVisitor<Visitor::WeakProcessing> weakProcessingVisitor;
+    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.

 
-            // 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();
-            }
-        }
-
-        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();
 #endif
         completeSweep();
     } else {
         // The default behavior is lazy sweeping.
-        setGCState(Sweeping);
         eagerSweep();
 #if defined(ADDRESS_SANITIZER)
         poisonAllHeaps();
 #endif
         scheduleIdleLazySweep();
     }
 #else
-    setGCState(Sweeping);
     completeSweep();
 #endif
 
 #if ENABLE(GC_PROFILING)
     snapshotFreeListIfNecessary();
 #endif
 }
 
 #if defined(ADDRESS_SANITIZER)
 void ThreadState::poisonAllHeaps()
@@ skipping 21 matching lines @@
 {
     // Some objects need to be finalized promptly and cannot be handled
     // by lazy sweeping. Keep those in a designated heap and sweep it
     // eagerly.
     ASSERT(isSweepingInProgress());
 
     // Mirroring the completeSweep() condition; see its comment.
     if (sweepForbidden())
         return;
 
-    ThreadState::SweepForbiddenScope scope(this);
+    SweepForbiddenScope scope(this);
     {
         if (isMainThread())
             ScriptForbiddenScope::enter();
 
         m_heaps[EagerSweepHeapIndex]->completeSweep();
 
         if (isMainThread())
             ScriptForbiddenScope::exit();
     }
 }
 
 void ThreadState::completeSweep()
 {
     // If we are not in a sweeping phase, there is nothing to do here.
     if (!isSweepingInProgress())
         return;
 
     // completeSweep() can be called recursively if finalizers can allocate
     // memory and the allocation triggers completeSweep(). This check prevents
     // the sweeping from being executed recursively.
     if (sweepForbidden())
         return;
 
-    ThreadState::SweepForbiddenScope scope(this);
+    SweepForbiddenScope scope(this);
     {
         if (isMainThread())
             ScriptForbiddenScope::enter();
 
         TRACE_EVENT0("blink_gc", "ThreadState::completeSweep");
         double timeStamp = WTF::currentTimeMS();
 
         static_assert(EagerSweepHeapIndex == 0, "Eagerly swept heaps must be processed first.");
         for (int i = 0; i < NumberOfHeaps; i++)
             m_heaps[i]->completeSweep();
@@ skipping 212 matching lines @@
     if (sweepForbidden())
         return;
     auto it = m_preFinalizers.find(target);
     ASSERT(it != m_preFinalizers.end());
     m_preFinalizers.remove(it);
 }
 
 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))
             deadObjects.append(entry.key);
     }
     // FIXME: removeAll is inefficient.  It can shrink repeatedly.
     m_preFinalizers.removeAll(deadObjects);
+
+    if (isMainThread())
+        ScriptForbiddenScope::exit();
 }
 
 void ThreadState::clearHeapAges()
 {
     memset(m_heapAges, 0, sizeof(size_t) * NumberOfHeaps);
     memset(m_likelyToBePromptlyFreed.get(), 0, sizeof(int) * likelyToBePromptlyFreedArraySize);
     m_currentHeapAges = 0;
 }
 
 int ThreadState::heapIndexOfVectorHeapLeastRecentlyExpanded(int beginHeapIndex, int endHeapIndex)
@@ skipping 130 matching lines @@
         json->beginArray(it->key.ascii().data());
         for (size_t age = 0; age <= maxHeapObjectAge; ++age)
             json->pushInteger(it->value.ages[age]);
         json->endArray();
     }
     TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("blink_gc"), statsName, this, json.release());
 }
 #endif
 
 } // namespace blink