[weakrefs] Set unregister_token to undefined when unregistering

Bug: chromium:1321078
Change-Id: I426327ffc3d7eebdb562c01a87039a93dfb79a88
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3620836
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80349}

src/heap/mark-compact.cc

@@ -3198,14 +3198,11 @@ void MarkCompactCollector::ClearJSWeakRefs() {
       // unregister_token field set to undefined when processing the first
       // WeakCell. Like above, we're modifying pointers during GC, so record the
       // slots.
-      HeapObject undefined = ReadOnlyRoots(isolate()).undefined_value();
       JSFinalizationRegistry finalization_registry =
           JSFinalizationRegistry::cast(weak_cell.finalization_registry());
       finalization_registry.RemoveUnregisterToken(
           JSReceiver::cast(unregister_token), isolate(),
-          [undefined](WeakCell matched_cell) {
-            matched_cell.set_unregister_token(undefined);
-          },
+          JSFinalizationRegistry::kKeepMatchedCellsInRegistry,
           gc_notify_updated_slot);
     } else {
       // The unregister_token is alive.

src/objects/js-weak-refs-inl.h

@@ -60,16 +60,14 @@ bool JSFinalizationRegistry::Unregister(
   // key. Each WeakCell will be in the "active_cells" or "cleared_cells" list of
   // its FinalizationRegistry; remove it from there.
   return finalization_registry->RemoveUnregisterToken(
-      *unregister_token, isolate,
-      [isolate](WeakCell matched_cell) {
-        matched_cell.RemoveFromFinalizationRegistryCells(isolate);
-      },
+      *unregister_token, isolate, kRemoveMatchedCellsFromRegistry,
       [](HeapObject, ObjectSlot, Object) {});
 }
 
-template <typename MatchCallback, typename GCNotifyUpdatedSlotCallback>
+template <typename GCNotifyUpdatedSlotCallback>
 bool JSFinalizationRegistry::RemoveUnregisterToken(
-    JSReceiver unregister_token, Isolate* isolate, MatchCallback match_callback,
+    JSReceiver unregister_token, Isolate* isolate,
+    RemoveUnregisterTokenMode removal_mode,
     GCNotifyUpdatedSlotCallback gc_notify_updated_slot) {
   // This method is called from both FinalizationRegistry#unregister and for
   // removing weakly-held dead unregister tokens. The latter is during GC so
@@ -107,7 +105,16 @@ bool JSFinalizationRegistry::RemoveUnregisterToken(
     value = weak_cell.key_list_next();
     if (weak_cell.unregister_token() == unregister_token) {
       // weak_cell has the same unregister token; remove it from the key list.
-      match_callback(weak_cell);
+      switch (removal_mode) {
+        case kRemoveMatchedCellsFromRegistry:
+          weak_cell.RemoveFromFinalizationRegistryCells(isolate);
+          break;
+        case kKeepMatchedCellsInRegistry:
+          // Do nothing.
+          break;
+      }
+      // Clear unregister token-related fields.
+      weak_cell.set_unregister_token(undefined);
       weak_cell.set_key_list_prev(undefined);
       weak_cell.set_key_list_next(undefined);
       was_present = true;

src/objects/js-weak-refs.h

@@ -43,10 +43,14 @@ class JSFinalizationRegistry
   // it modifies slots in key_map and WeakCells and the normal write barrier is
   // disabled during GC, we need to tell the GC about the modified slots via the
   // gc_notify_updated_slot function.
-  template <typename MatchCallback, typename GCNotifyUpdatedSlotCallback>
+  enum RemoveUnregisterTokenMode {
+    kRemoveMatchedCellsFromRegistry,
+    kKeepMatchedCellsInRegistry
+  };
+  template <typename GCNotifyUpdatedSlotCallback>
   inline bool RemoveUnregisterToken(
       JSReceiver unregister_token, Isolate* isolate,
-      MatchCallback match_callback,
+      RemoveUnregisterTokenMode removal_mode,
       GCNotifyUpdatedSlotCallback gc_notify_updated_slot);
 
   // Returns true if the cleared_cells list is non-empty.

src/objects/objects.cc

@@ -7024,7 +7024,7 @@ void JSFinalizationRegistry::RemoveCellFromUnregisterTokenMap(
   }
 
   // weak_cell is now removed from the unregister token map, so clear its
-  // unregister token-related fields for heap verification.
+  // unregister token-related fields.
   weak_cell.set_unregister_token(undefined);
   weak_cell.set_key_list_prev(undefined);
   weak_cell.set_key_list_next(undefined);

test/cctest/test-js-weak-refs.cc

@@ -853,9 +853,7 @@ TEST(TestRemoveUnregisterToken) {
 
   finalization_registry->RemoveUnregisterToken(
       JSReceiver::cast(*token2), isolate,
-      [undefined](WeakCell matched_cell) {
-        matched_cell.set_unregister_token(*undefined);
-      },
+      JSFinalizationRegistry::kKeepMatchedCellsInRegistry,
       [](HeapObject, ObjectSlot, Object) {});
 
   // Both weak_cell2a and weak_cell2b remain on the weak cell chains.
@@ -1025,5 +1023,52 @@ TEST(UnregisterTokenHeapVerifier) {
   EmptyMessageQueues(isolate);
 }
 
+TEST(UnregisteredAndUnclearedCellHeapVerifier) {
+  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 a token.
+    v8::HandleScope scope(isolate);
+    CompileRun(
+        "var token = {}; "
+        "var registry = new FinalizationRegistry(function () {}); "
+        "registry.register({}, undefined, token);");
+  }
+
+  // Start incremental marking to activate the marking barrier.
+  heap::SimulateIncrementalMarking(heap, false);
+
+  {
+    // Make a WeakCell list with length >1, then unregister with the token to
+    // the WeakCell from the registry. The linked list manipulation keeps the
+    // unregistered WeakCell alive (i.e. not put into cleared_cells) due to the
+    // marking barrier from incremental marking. Then make the original token
+    // collectible.
+    v8::HandleScope scope(isolate);
+    CompileRun(
+        "registry.register({}); "
+        "registry.unregister(token); "
+        "token = 0;");
+  }
+
+  // Trigger GC.
+  CcTest::CollectAllGarbage();
+  CcTest::CollectAllGarbage();
+
+  // Pump message loop to run the finalizer task, then the incremental marking
+  // task. The verifier will verify that live WeakCells don't point to dead
+  // unregister tokens.
+  EmptyMessageQueues(isolate);
+}
+
 }  // namespace internal
 }  // namespace v8