[heap] Introduce a separate pass for destroying phantom handles

Phantom handles were processed at the same time as finalizers. This 
meant that if a finalizer kept an object alive the phantom handle
was still destroyed.

This becomes a problem in the context of Blink GCs where internal
fields are roots for Blink. Prematurely destroying a phantom handle
can lead to stale pointers.

Bug: chromium:772299
Change-Id: If02365c457be8ce48379ad357cce36baa9617cfb
Reviewed-on: https://chromium-review.googlesource.com/750625Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49086}

src/global-handles.cc

@@ -160,14 +160,21 @@ class GlobalHandles::Node {
 
   bool IsInUse() const { return state() != FREE; }
 
+  bool IsPhantomCallback() const {
+    return weakness_type() == PHANTOM_WEAK ||
+           weakness_type() == PHANTOM_WEAK_2_EMBEDDER_FIELDS;
+  }
+
+  bool IsPhantomResetHandle() const {
+    return weakness_type() == PHANTOM_WEAK_RESET_HANDLE;
+  }
+
   bool IsPendingPhantomCallback() const {
-    return state() == PENDING &&
-           (weakness_type() == PHANTOM_WEAK ||
-            weakness_type() == PHANTOM_WEAK_2_EMBEDDER_FIELDS);
+    return state() == PENDING && IsPhantomCallback();
   }
 
   bool IsPendingPhantomResetHandle() const {
-    return state() == PENDING && weakness_type() == PHANTOM_WEAK_RESET_HANDLE;
+    return state() == PENDING && IsPhantomResetHandle();
   }
 
   bool IsRetainer() const {
@@ -613,29 +620,44 @@ bool GlobalHandles::IsWeak(Object** location) {
 }
 
 DISABLE_CFI_PERF
-void GlobalHandles::IterateWeakRoots(RootVisitor* v) {
+void GlobalHandles::IterateWeakRootsForFinalizers(RootVisitor* v) {
   for (NodeIterator it(this); !it.done(); it.Advance()) {
     Node* node = it.node();
-    if (node->IsWeakRetainer()) {
-      // Pending weak phantom handles die immediately. Everything else survives.
-      if (node->IsPendingPhantomResetHandle()) {
+    if (node->IsWeakRetainer() && node->state() == Node::PENDING) {
+      DCHECK(!node->IsPhantomCallback());
+      DCHECK(!node->IsPhantomResetHandle());
+      // Finalizers need to survive.
+      v->VisitRootPointer(Root::kGlobalHandles, node->location());
+    }
+  }
+}
+
+DISABLE_CFI_PERF
+void GlobalHandles::IterateWeakRootsForPhantomHandles(
+    WeakSlotCallback should_reset_handle) {
+  for (NodeIterator it(this); !it.done(); it.Advance()) {
+    Node* node = it.node();
+    if (node->IsWeakRetainer() && should_reset_handle(node->location())) {
+      if (node->IsPhantomResetHandle()) {
+        node->MarkPending();
         node->ResetPhantomHandle();
         ++number_of_phantom_handle_resets_;
-      } else if (node->IsPendingPhantomCallback()) {
+      } else if (node->IsPhantomCallback()) {
+        node->MarkPending();
         node->CollectPhantomCallbackData(isolate(),
                                          &pending_phantom_callbacks_);
-      } else {
-        v->VisitRootPointer(Root::kGlobalHandles, node->location());
       }
     }
   }
 }
 
-
-void GlobalHandles::IdentifyWeakHandles(WeakSlotCallback f) {
+void GlobalHandles::IdentifyWeakHandles(WeakSlotCallback should_reset_handle) {
   for (NodeIterator it(this); !it.done(); it.Advance()) {
-    if (it.node()->IsWeak() && f(it.node()->location())) {
-      it.node()->MarkPending();
+    Node* node = it.node();
+    if (node->IsWeak() && should_reset_handle(node->location())) {
+      if (!node->IsPhantomCallback() && !node->IsPhantomResetHandle()) {
+        node->MarkPending();
+      }
     }
   }
 }

src/global-handles.h

@@ -133,12 +133,13 @@ class GlobalHandles {
   // and have class IDs
   void IterateWeakRootsInNewSpaceWithClassIds(v8::PersistentHandleVisitor* v);
 
-  // Iterates over all weak roots in heap.
-  void IterateWeakRoots(RootVisitor* v);
+  // Iterates over weak roots on the heap.
+  void IterateWeakRootsForFinalizers(RootVisitor* v);
+  void IterateWeakRootsForPhantomHandles(WeakSlotCallback should_reset_handle);
 
-  // Find all weak handles satisfying the callback predicate, mark
-  // them as pending.
-  void IdentifyWeakHandles(WeakSlotCallback f);
+  // Marks all handles that should be finalized based on the predicate
+  // |should_reset_handle| as pending.
+  void IdentifyWeakHandles(WeakSlotCallback should_reset_handle);
 
   // NOTE: Five ...NewSpace... functions below are used during
   // scavenge collections and iterate over sets of handles that are

src/heap/mark-compact.cc

@@ -2545,6 +2545,7 @@ void MarkCompactCollector::MarkLiveObjects() {
       TRACE_GC(heap()->tracer(),
                GCTracer::Scope::MC_MARK_WEAK_CLOSURE_EPHEMERAL);
       ProcessEphemeralMarking(false);
+      DCHECK(marking_worklist()->IsEmpty());
     }
 
     // The objects reachable from the roots, weak maps or object groups
@@ -2561,12 +2562,12 @@ void MarkCompactCollector::MarkLiveObjects() {
           &IsUnmarkedHeapObject);
       ProcessMarkingWorklist();
     }
-    // Then we mark the objects.
 
     {
       TRACE_GC(heap()->tracer(),
                GCTracer::Scope::MC_MARK_WEAK_CLOSURE_WEAK_ROOTS);
-      heap()->isolate()->global_handles()->IterateWeakRoots(&root_visitor);
+      heap()->isolate()->global_handles()->IterateWeakRootsForFinalizers(
+          &root_visitor);
       ProcessMarkingWorklist();
     }
 
@@ -2582,6 +2583,12 @@ void MarkCompactCollector::MarkLiveObjects() {
         TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_MARK_WRAPPER_EPILOGUE);
         heap()->local_embedder_heap_tracer()->TraceEpilogue();
       }
+      DCHECK(marking_worklist()->IsEmpty());
+    }
+
+    {
+      heap()->isolate()->global_handles()->IterateWeakRootsForPhantomHandles(
+          &IsUnmarkedHeapObject);
     }
   }
 

test/cctest/test-global-handles.cc

@@ -185,5 +185,42 @@ TEST(PhatomHandlesWithoutCallbacks) {
   CHECK_EQ(0u, isolate->NumberOfPhantomHandleResetsSinceLastCall());
 }
 
+namespace {
+
+void ResurrectingFinalizer(
+    const v8::WeakCallbackInfo<v8::Global<v8::Object>>& data) {
+  data.GetParameter()->ClearWeak();
+}
+
+}  // namespace
+
+TEST(Regress772299) {
+  CcTest::InitializeVM();
+  v8::Isolate* isolate = CcTest::isolate();
+
+  v8::Global<v8::Object> g1, g2;
+  {
+    v8::HandleScope scope(isolate);
+    v8::Local<v8::Object> o1 =
+        v8::Local<v8::Object>::New(isolate, v8::Object::New(isolate));
+    v8::Local<v8::Object> o2 =
+        v8::Local<v8::Object>::New(isolate, v8::Object::New(isolate));
+    o1->Set(isolate->GetCurrentContext(), v8_str("link"), o2).FromJust();
+    g1.Reset(isolate, o1);
+    g2.Reset(isolate, o2);
+    // g1 will be finalized but resurrected.
+    g1.SetWeak(&g1, ResurrectingFinalizer, v8::WeakCallbackType::kFinalizer);
+    // g2 will be a phantom handle that should not be reset as g1 transitively
+    // keeps it alive.
+    g2.SetWeak();
+  }
+
+  CcTest::CollectAllAvailableGarbage();
+  // Both, g1 and g2, should stay alive as the finalizer resurrects the root
+  // object that transitively keeps the other one alive.
+  CHECK(!g1.IsEmpty());
+  CHECK(!g2.IsEmpty());
+}
+
 }  // namespace internal
 }  // namespace v8