[weakrefs] Make unregister_token undefined on popped WeakCells

syg · Commit Bot · commit 93c0be4b7cdc · 2020-07-07T17:24:31.000Z
The unregister_token slot is iterated as a custom weak pointer slot, which means the heap verifier treats it as a strong slot. Currently, popped WeakCells (that is, WeakCells for which the owning FinalizationRegistry's finalizer has already been invoked) neither clears out the unregister_token slot nor marks it, which trips the heap verifier. Bug: chromium:1102161 Change-Id: I0a803f12379fc9df6935bc8331b3d5ecb199571a Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2284202 Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Auto-Submit: Shu-yu Guo <syg@chromium.org> Cr-Commit-Position: refs/heads/master@{#68723}
diff --git a/src/heap/mark-compact.cc b/src/heap/mark-compact.cc
@@ -2541,6 +2541,11 @@ void MarkCompactCollector::ClearJSWeakRefs() {
             matched_cell.set_unregister_token(undefined);
           },
           gc_notify_updated_slot);
+      // The following is necessary because in the case that weak_cell has
+      // already been popped and removed from the FinalizationRegistry, the call
+      // to JSFinalizationRegistry::RemoveUnregisterToken above will not find
+      // weak_cell itself to clear its unregister token.
+      weak_cell.set_unregister_token(undefined);
     } else {
       // The unregister_token is alive.
       ObjectSlot slot = weak_cell.RawField(WeakCell::kUnregisterTokenOffset);
diff --git a/test/cctest/test-js-weak-refs.cc b/test/cctest/test-js-weak-refs.cc
@@ -968,5 +968,52 @@ TEST(JSWeakRefTenuredInWorklist) {
   CHECK(weak_ref->target().IsUndefined(isolate));
 }
 
+TEST(UnregisterTokenHeapVerifier) {
+  FLAG_harmony_weak_refs = true;
+  if (!FLAG_incremental_marking) return;
+  ManualGCScope manual_gc_scope;
+#ifdef VERIFY_HEAP
+  FLAG_verify_heap = true;
+#endif
+
+  CcTest::InitializeVM();
+  v8::Isolate* isolate = CcTest::isolate();
+  Heap* heap = CcTest::heap();
+  v8::HandleScope outer_scope(isolate);
+
+  {
+    // Make a new FinalizationRegistry and register an object with an unregister
+    // token that's unreachable after the IIFE returns.
+    v8::HandleScope scope(isolate);
+    CompileRun(
+        "var token = {}; "
+        "var registry = new FinalizationRegistry(function ()  {}); "
+        "(function () { "
+        "  let o = {}; "
+        "  registry.register(o, {}, token); "
+        "})();");
+  }
+
+  // GC so the WeakCell corresponding to o is moved from the active_cells to
+  // cleared_cells.
+  CcTest::CollectAllGarbage();
+  CcTest::CollectAllGarbage();
+
+  {
+    // Override the unregister token to make the original object collectible.
+    v8::HandleScope scope(isolate);
+    CompileRun("token = 0;");
+  }
+
+  heap::SimulateIncrementalMarking(heap, true);
+
+  // Pump message loop to run the finalizer task, then the incremental marking
+  // task. The finalizer task will pop the WeakCell from the cleared list. This
+  // should make the unregister_token slot undefined. That slot is iterated as a
+  // custom weak pointer, so if it is not made undefined, the verifier as part
+  // of the incremental marking task will crash.
+  EmptyMessageQueues(isolate);
+}
+
 }  // namespace internal
 }  // namespace v8
