[heap] Introduce separate pass for reseting phantom handles on Scavenge

Resetting phantom handles while keeping finalizers alive leads to the
problem of eagerly resetting a handle although another finalizer keeps
it (transitively) alive.

This becomes a problem with internal pointers to Blink as without
global handle a Blink GC is free to collect wrappables.

This CL untangles finalizers handling from phantom handle resets by
introducing a separate path for resetting.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

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

src/global-handles.cc

@@ -703,24 +703,49 @@ void GlobalHandles::MarkNewSpaceWeakUnmodifiedObjectsPending(
     DCHECK(node->is_in_new_space_list());
     if ((node->is_independent() || !node->is_active()) && node->IsWeak() &&
         is_unscavenged(isolate_->heap(), node->location())) {
-      node->MarkPending();
+      if (!node->IsPhantomCallback() && !node->IsPhantomResetHandle()) {
+        node->MarkPending();
+      }
     }
   }
 }
 
-void GlobalHandles::IterateNewSpaceWeakUnmodifiedRoots(RootVisitor* v) {
+void GlobalHandles::IterateNewSpaceWeakUnmodifiedRootsForFinalizers(
+    RootVisitor* v) {
   for (Node* node : new_space_nodes_) {
     DCHECK(node->is_in_new_space_list());
     if ((node->is_independent() || !node->is_active()) &&
-        node->IsWeakRetainer()) {
-      // Pending weak phantom handles die immediately. Everything else survives.
-      if (node->IsPendingPhantomResetHandle()) {
-        node->ResetPhantomHandle();
-        ++number_of_phantom_handle_resets_;
-      } else if (node->IsPendingPhantomCallback()) {
-        node->CollectPhantomCallbackData(isolate(),
-                                         &pending_phantom_callbacks_);
+        node->IsWeakRetainer() && (node->state() == Node::PENDING)) {
+      DCHECK(!node->IsPhantomCallback());
+      DCHECK(!node->IsPhantomResetHandle());
+      // Finalizers need to survive.
+      v->VisitRootPointer(Root::kGlobalHandles, node->location());
+    }
+  }
+}
+
+void GlobalHandles::IterateNewSpaceWeakUnmodifiedRootsForPhantomHandles(
+    RootVisitor* v, WeakSlotCallbackWithHeap should_reset_handle) {
+  for (Node* node : new_space_nodes_) {
+    DCHECK(node->is_in_new_space_list());
+    if ((node->is_independent() || !node->is_active()) &&
+        node->IsWeakRetainer() && (node->state() != Node::PENDING)) {
+      DCHECK(node->IsPhantomResetHandle() || node->IsPhantomCallback());
+      if (should_reset_handle(isolate_->heap(), node->location())) {
+        if (node->IsPhantomResetHandle()) {
+          node->MarkPending();
+          node->ResetPhantomHandle();
+          ++number_of_phantom_handle_resets_;
+
+        } else if (node->IsPhantomCallback()) {
+          node->MarkPending();
+          node->CollectPhantomCallbackData(isolate(),
+                                           &pending_phantom_callbacks_);
+        } else {
+          UNREACHABLE();
+        }
       } else {
+        // Node survived and needs to be visited.
         v->VisitRootPointer(Root::kGlobalHandles, node->location());
       }
     }

src/global-handles.h

@@ -161,7 +161,9 @@ class GlobalHandles {
 
   // Iterates over weak independent or unmodified handles.
   // See the note above.
-  void IterateNewSpaceWeakUnmodifiedRoots(RootVisitor* v);
+  void IterateNewSpaceWeakUnmodifiedRootsForFinalizers(RootVisitor* v);
+  void IterateNewSpaceWeakUnmodifiedRootsForPhantomHandles(
+      RootVisitor* v, WeakSlotCallbackWithHeap should_reset_handle);
 
   // Identify unmodified objects that are in weak state and marks them
   // unmodified

src/heap/heap.cc

@@ -1723,7 +1723,7 @@ void Heap::CheckNewSpaceExpansionCriteria() {
 }
 
 static bool IsUnscavengedHeapObject(Heap* heap, Object** p) {
-  return heap->InNewSpace(*p) &&
+  return heap->InFromSpace(*p) &&
          !HeapObject::cast(*p)->map_word().IsForwardingAddress();
 }
 
@@ -1958,9 +1958,18 @@ void Heap::Scavenge() {
                GCTracer::Scope::SCAVENGER_SCAVENGE_WEAK_GLOBAL_HANDLES_PROCESS);
       isolate()->global_handles()->MarkNewSpaceWeakUnmodifiedObjectsPending(
           &IsUnscavengedHeapObject);
-      isolate()->global_handles()->IterateNewSpaceWeakUnmodifiedRoots(
-          &root_scavenge_visitor);
+      isolate()
+          ->global_handles()
+          ->IterateNewSpaceWeakUnmodifiedRootsForFinalizers(
+              &root_scavenge_visitor);
       scavengers[kMainThreadId]->Process();
+
+      DCHECK(copied_list.IsGlobalEmpty());
+      DCHECK(promotion_list.IsGlobalEmpty());
+      isolate()
+          ->global_handles()
+          ->IterateNewSpaceWeakUnmodifiedRootsForPhantomHandles(
+              &root_scavenge_visitor, &IsUnscavengedHeapObject);
     }
 
     for (int i = 0; i < num_scavenge_tasks; i++) {

src/heap/mark-compact.cc

@@ -2296,8 +2296,13 @@ void MinorMarkCompactCollector::MarkLiveObjects() {
     TRACE_GC(heap()->tracer(), GCTracer::Scope::MINOR_MC_MARK_GLOBAL_HANDLES);
     isolate()->global_handles()->MarkNewSpaceWeakUnmodifiedObjectsPending(
         &IsUnmarkedObjectForYoungGeneration);
-    isolate()->global_handles()->IterateNewSpaceWeakUnmodifiedRoots(
-        &root_visitor);
+    isolate()
+        ->global_handles()
+        ->IterateNewSpaceWeakUnmodifiedRootsForFinalizers(&root_visitor);
+    isolate()
+        ->global_handles()
+        ->IterateNewSpaceWeakUnmodifiedRootsForPhantomHandles(
+            &root_visitor, &IsUnmarkedObjectForYoungGeneration);
     ProcessMarkingWorklist();
   }
 }

test/cctest/test-global-handles.cc

@@ -192,12 +192,9 @@ void ResurrectingFinalizer(
   data.GetParameter()->ClearWeak();
 }
 
-}  // namespace
-
-TEST(Regress772299) {
-  CcTest::InitializeVM();
-  v8::Isolate* isolate = CcTest::isolate();
-
+template <typename GCFunction>
+void TestFinalizerKeepsPhantomAlive(v8::Isolate* isolate,
+                                    GCFunction gc_function) {
   v8::Global<v8::Object> g1, g2;
   {
     v8::HandleScope scope(isolate);
@@ -205,6 +202,10 @@ TEST(Regress772299) {
         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));
+    CHECK(reinterpret_cast<Isolate*>(isolate)->heap()->InNewSpace(
+        *v8::Utils::OpenHandle(v8::Object::Cast(*o1))));
+    CHECK(reinterpret_cast<Isolate*>(isolate)->heap()->InNewSpace(
+        *v8::Utils::OpenHandle(v8::Object::Cast(*o2))));
     o1->Set(isolate->GetCurrentContext(), v8_str("link"), o2).FromJust();
     g1.Reset(isolate, o1);
     g2.Reset(isolate, o2);
@@ -215,12 +216,35 @@ TEST(Regress772299) {
     g2.SetWeak();
   }
 
-  CcTest::CollectAllAvailableGarbage();
+  gc_function(&g1, &g2);
   // 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
+
+TEST(FinalizerKeepsPhantomAliveOnMarkCompact) {
+  // See crbug.com/772299.
+  CcTest::InitializeVM();
+  TestFinalizerKeepsPhantomAlive(
+      CcTest::isolate(),
+      [](v8::Global<v8::Object>* g1, v8::Global<v8::Object>* g2) {
+        CcTest::CollectAllAvailableGarbage();
+      });
+}
+
+TEST(FinalizerKeepsPhantomAliveOnScavenge) {
+  CcTest::InitializeVM();
+  TestFinalizerKeepsPhantomAlive(
+      CcTest::isolate(),
+      [](v8::Global<v8::Object>* g1, v8::Global<v8::Object>* g2) {
+        g1->MarkIndependent();
+        g2->MarkIndependent();
+        CcTest::CollectGarbage(i::NEW_SPACE);
+      });
+}
+
 }  // namespace internal
 }  // namespace v8