Skip to content

Commit dd3289d

Browse files
sygV8 LUCI CQ
syg
authored and
V8 LUCI CQ
committed
[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 <[email protected]> Reviewed-by: Michael Lippautz <[email protected]> Cr-Commit-Position: refs/heads/main@{#80349}
1 parent 08a5a57 commit dd3289d

5 files changed

Lines changed: 70 additions & 17 deletions

File tree

‎src/heap/mark-compact.cc‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3198,14 +3198,11 @@ void MarkCompactCollector::ClearJSWeakRefs() {
31983198
// unregister_token field set to undefined when processing the first
31993199
// WeakCell. Like above, we're modifying pointers during GC, so record the
32003200
// slots.
3201-
HeapObject undefined = ReadOnlyRoots(isolate()).undefined_value();
32023201
JSFinalizationRegistry finalization_registry =
32033202
JSFinalizationRegistry::cast(weak_cell.finalization_registry());
32043203
finalization_registry.RemoveUnregisterToken(
32053204
JSReceiver::cast(unregister_token), isolate(),
3206-
[undefined](WeakCell matched_cell) {
3207-
matched_cell.set_unregister_token(undefined);
3208-
},
3205+
JSFinalizationRegistry::kKeepMatchedCellsInRegistry,
32093206
gc_notify_updated_slot);
32103207
} else {
32113208
// The unregister_token is alive.

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

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,16 +60,14 @@ bool JSFinalizationRegistry::Unregister(
6060
// key. Each WeakCell will be in the "active_cells" or "cleared_cells" list of
6161
// its FinalizationRegistry; remove it from there.
6262
return finalization_registry->RemoveUnregisterToken(
63-
*unregister_token, isolate,
64-
[isolate](WeakCell matched_cell) {
65-
matched_cell.RemoveFromFinalizationRegistryCells(isolate);
66-
},
63+
*unregister_token, isolate, kRemoveMatchedCellsFromRegistry,
6764
[](HeapObject, ObjectSlot, Object) {});
6865
}
6966

70-
template <typename MatchCallback, typename GCNotifyUpdatedSlotCallback>
67+
template <typename GCNotifyUpdatedSlotCallback>
7168
bool JSFinalizationRegistry::RemoveUnregisterToken(
72-
JSReceiver unregister_token, Isolate* isolate, MatchCallback match_callback,
69+
JSReceiver unregister_token, Isolate* isolate,
70+
RemoveUnregisterTokenMode removal_mode,
7371
GCNotifyUpdatedSlotCallback gc_notify_updated_slot) {
7472
// This method is called from both FinalizationRegistry#unregister and for
7573
// removing weakly-held dead unregister tokens. The latter is during GC so
@@ -107,7 +105,16 @@ bool JSFinalizationRegistry::RemoveUnregisterToken(
107105
value = weak_cell.key_list_next();
108106
if (weak_cell.unregister_token() == unregister_token) {
109107
// weak_cell has the same unregister token; remove it from the key list.
110-
match_callback(weak_cell);
108+
switch (removal_mode) {
109+
case kRemoveMatchedCellsFromRegistry:
110+
weak_cell.RemoveFromFinalizationRegistryCells(isolate);
111+
break;
112+
case kKeepMatchedCellsInRegistry:
113+
// Do nothing.
114+
break;
115+
}
116+
// Clear unregister token-related fields.
117+
weak_cell.set_unregister_token(undefined);
111118
weak_cell.set_key_list_prev(undefined);
112119
weak_cell.set_key_list_next(undefined);
113120
was_present = true;

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,14 @@ class JSFinalizationRegistry
4343
// it modifies slots in key_map and WeakCells and the normal write barrier is
4444
// disabled during GC, we need to tell the GC about the modified slots via the
4545
// gc_notify_updated_slot function.
46-
template <typename MatchCallback, typename GCNotifyUpdatedSlotCallback>
46+
enum RemoveUnregisterTokenMode {
47+
kRemoveMatchedCellsFromRegistry,
48+
kKeepMatchedCellsInRegistry
49+
};
50+
template <typename GCNotifyUpdatedSlotCallback>
4751
inline bool RemoveUnregisterToken(
4852
JSReceiver unregister_token, Isolate* isolate,
49-
MatchCallback match_callback,
53+
RemoveUnregisterTokenMode removal_mode,
5054
GCNotifyUpdatedSlotCallback gc_notify_updated_slot);
5155

5256
// Returns true if the cleared_cells list is non-empty.

‎src/objects/objects.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7024,7 +7024,7 @@ void JSFinalizationRegistry::RemoveCellFromUnregisterTokenMap(
70247024
}
70257025

70267026
// weak_cell is now removed from the unregister token map, so clear its
7027-
// unregister token-related fields for heap verification.
7027+
// unregister token-related fields.
70287028
weak_cell.set_unregister_token(undefined);
70297029
weak_cell.set_key_list_prev(undefined);
70307030
weak_cell.set_key_list_next(undefined);

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

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -853,9 +853,7 @@ TEST(TestRemoveUnregisterToken) {
853853

854854
finalization_registry->RemoveUnregisterToken(
855855
JSReceiver::cast(*token2), isolate,
856-
[undefined](WeakCell matched_cell) {
857-
matched_cell.set_unregister_token(*undefined);
858-
},
856+
JSFinalizationRegistry::kKeepMatchedCellsInRegistry,
859857
[](HeapObject, ObjectSlot, Object) {});
860858

861859
// Both weak_cell2a and weak_cell2b remain on the weak cell chains.
@@ -1025,5 +1023,52 @@ TEST(UnregisterTokenHeapVerifier) {
10251023
EmptyMessageQueues(isolate);
10261024
}
10271025

1026+
TEST(UnregisteredAndUnclearedCellHeapVerifier) {
1027+
if (!FLAG_incremental_marking) return;
1028+
ManualGCScope manual_gc_scope;
1029+
#ifdef VERIFY_HEAP
1030+
FLAG_verify_heap = true;
1031+
#endif
1032+
1033+
CcTest::InitializeVM();
1034+
v8::Isolate* isolate = CcTest::isolate();
1035+
Heap* heap = CcTest::heap();
1036+
v8::HandleScope outer_scope(isolate);
1037+
1038+
{
1039+
// Make a new FinalizationRegistry and register an object with a token.
1040+
v8::HandleScope scope(isolate);
1041+
CompileRun(
1042+
"var token = {}; "
1043+
"var registry = new FinalizationRegistry(function () {}); "
1044+
"registry.register({}, undefined, token);");
1045+
}
1046+
1047+
// Start incremental marking to activate the marking barrier.
1048+
heap::SimulateIncrementalMarking(heap, false);
1049+
1050+
{
1051+
// Make a WeakCell list with length >1, then unregister with the token to
1052+
// the WeakCell from the registry. The linked list manipulation keeps the
1053+
// unregistered WeakCell alive (i.e. not put into cleared_cells) due to the
1054+
// marking barrier from incremental marking. Then make the original token
1055+
// collectible.
1056+
v8::HandleScope scope(isolate);
1057+
CompileRun(
1058+
"registry.register({}); "
1059+
"registry.unregister(token); "
1060+
"token = 0;");
1061+
}
1062+
1063+
// Trigger GC.
1064+
CcTest::CollectAllGarbage();
1065+
CcTest::CollectAllGarbage();
1066+
1067+
// Pump message loop to run the finalizer task, then the incremental marking
1068+
// task. The verifier will verify that live WeakCells don't point to dead
1069+
// unregister tokens.
1070+
EmptyMessageQueues(isolate);
1071+
}
1072+
10281073
} // namespace internal
10291074
} // namespace v8

0 commit comments

Comments
 (0)