Skip to content

Commit 94e8282

Browse files
sygV8 LUCI CQ
syg
authored and
V8 LUCI CQ
committed
[symbol-as-weakmap-key] Fix DCHECKs and add CanBeHeldWeakly
There are a few DCHECKs that weren't updated to allow for Symbols as weak collection keys. This CL updates those DCHECKs and also does the following refactors for clarity: - Add Object::CanBeHeldWeakly - Rename GotoIfCannotBeWeakKey -> GotoIfCannotBeHeldWeakly to align with spec AO name Bug: chromium:1370400, chromium:1370402, v8:12947 Change-Id: I380840c8377497feae97e3fca37555dae0dcc255 Fixed: chromium:1370400, chromium:1370402 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3928150 Auto-Submit: Shu-yu Guo <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Commit-Queue: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#83507}
1 parent 699147d commit 94e8282

12 files changed

+66
-62
lines changed

‎src/builtins/builtins-collections-gen.cc

+20-20
Original file line numberDiff line numberDiff line change
@@ -425,25 +425,25 @@ TNode<IntPtrT> BaseCollectionsAssembler::EstimatedInitialSize(
425425
}
426426

427427
// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-canbeheldweakly-abstract-operation
428-
void BaseCollectionsAssembler::GotoIfCannotBeWeakKey(
429-
const TNode<Object> obj, Label* if_cannot_be_weak_key) {
428+
void BaseCollectionsAssembler::GotoIfCannotBeHeldWeakly(
429+
const TNode<Object> obj, Label* if_cannot_be_held_weakly) {
430430
Label check_symbol_key(this);
431431
Label end(this);
432-
GotoIf(TaggedIsSmi(obj), if_cannot_be_weak_key);
432+
GotoIf(TaggedIsSmi(obj), if_cannot_be_held_weakly);
433433
TNode<Uint16T> instance_type = LoadMapInstanceType(LoadMap(CAST(obj)));
434434
GotoIfNot(IsJSReceiverInstanceType(instance_type), &check_symbol_key);
435435
// TODO(v8:12547) Shared structs and arrays should only be able to point
436436
// to shared values in weak collections. For now, disallow them as weak
437437
// collection keys.
438-
GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_weak_key);
439-
GotoIf(IsJSSharedArrayInstanceType(instance_type), if_cannot_be_weak_key);
438+
GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_held_weakly);
439+
GotoIf(IsJSSharedArrayInstanceType(instance_type), if_cannot_be_held_weakly);
440440
Goto(&end);
441441
Bind(&check_symbol_key);
442-
GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_weak_key);
443-
GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_weak_key);
442+
GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_held_weakly);
443+
GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_held_weakly);
444444
TNode<Uint32T> flags = LoadSymbolFlags(CAST(obj));
445445
GotoIf(Word32And(flags, Symbol::IsInPublicSymbolTableBit::kMask),
446-
if_cannot_be_weak_key);
446+
if_cannot_be_held_weakly);
447447
Goto(&end);
448448
Bind(&end);
449449
}
@@ -2586,17 +2586,17 @@ TF_BUILTIN(WeakMapLookupHashIndex, WeakCollectionsBuiltinsAssembler) {
25862586
auto table = Parameter<EphemeronHashTable>(Descriptor::kTable);
25872587
auto key = Parameter<Object>(Descriptor::kKey);
25882588

2589-
Label if_cannot_be_weak_key(this);
2589+
Label if_cannot_be_held_weakly(this);
25902590

2591-
GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key);
2591+
GotoIfCannotBeHeldWeakly(key, &if_cannot_be_held_weakly);
25922592

2593-
TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_weak_key);
2593+
TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_held_weakly);
25942594
TNode<IntPtrT> capacity = LoadTableCapacity(table);
25952595
TNode<IntPtrT> key_index = FindKeyIndexForKey(
2596-
table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key);
2596+
table, key, hash, EntryMask(capacity), &if_cannot_be_held_weakly);
25972597
Return(SmiTag(ValueIndexFromKeyIndex(key_index)));
25982598

2599-
BIND(&if_cannot_be_weak_key);
2599+
BIND(&if_cannot_be_held_weakly);
26002600
Return(SmiConstant(-1));
26012601
}
26022602

@@ -2651,22 +2651,22 @@ TF_BUILTIN(WeakCollectionDelete, WeakCollectionsBuiltinsAssembler) {
26512651
auto collection = Parameter<JSWeakCollection>(Descriptor::kCollection);
26522652
auto key = Parameter<Object>(Descriptor::kKey);
26532653

2654-
Label call_runtime(this), if_cannot_be_weak_key(this);
2654+
Label call_runtime(this), if_cannot_be_held_weakly(this);
26552655

2656-
GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key);
2656+
GotoIfCannotBeHeldWeakly(key, &if_cannot_be_held_weakly);
26572657

2658-
TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_weak_key);
2658+
TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_held_weakly);
26592659
TNode<EphemeronHashTable> table = LoadTable(collection);
26602660
TNode<IntPtrT> capacity = LoadTableCapacity(table);
26612661
TNode<IntPtrT> key_index = FindKeyIndexForKey(
2662-
table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key);
2662+
table, key, hash, EntryMask(capacity), &if_cannot_be_held_weakly);
26632663
TNode<IntPtrT> number_of_elements = LoadNumberOfElements(table, -1);
26642664
GotoIf(ShouldShrink(capacity, number_of_elements), &call_runtime);
26652665

26662666
RemoveEntry(table, key_index, number_of_elements);
26672667
Return(TrueConstant());
26682668

2669-
BIND(&if_cannot_be_weak_key);
2669+
BIND(&if_cannot_be_held_weakly);
26702670
Return(FalseConstant());
26712671

26722672
BIND(&call_runtime);
@@ -2751,7 +2751,7 @@ TF_BUILTIN(WeakMapPrototypeSet, WeakCollectionsBuiltinsAssembler) {
27512751
"WeakMap.prototype.set");
27522752

27532753
Label throw_invalid_key(this);
2754-
GotoIfCannotBeWeakKey(key, &throw_invalid_key);
2754+
GotoIfCannotBeHeldWeakly(key, &throw_invalid_key);
27552755

27562756
Return(
27572757
CallBuiltin(Builtin::kWeakCollectionSet, context, receiver, key, value));
@@ -2769,7 +2769,7 @@ TF_BUILTIN(WeakSetPrototypeAdd, WeakCollectionsBuiltinsAssembler) {
27692769
"WeakSet.prototype.add");
27702770

27712771
Label throw_invalid_value(this);
2772-
GotoIfCannotBeWeakKey(value, &throw_invalid_value);
2772+
GotoIfCannotBeHeldWeakly(value, &throw_invalid_value);
27732773

27742774
Return(CallBuiltin(Builtin::kWeakCollectionSet, context, receiver, value,
27752775
TrueConstant()));

‎src/builtins/builtins-collections-gen.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ class BaseCollectionsAssembler : public CodeStubAssembler {
2727

2828
virtual ~BaseCollectionsAssembler() = default;
2929

30-
void GotoIfCannotBeWeakKey(const TNode<Object> obj,
31-
Label* if_cannot_be_weak_key);
30+
void GotoIfCannotBeHeldWeakly(const TNode<Object> obj,
31+
Label* if_cannot_be_held_weakly);
3232

3333
protected:
3434
enum Variant { kMap, kSet, kWeakMap, kWeakSet };

‎src/builtins/builtins-weak-refs.cc

+4-17
Original file line numberDiff line numberDiff line change
@@ -27,23 +27,10 @@ BUILTIN(FinalizationRegistryUnregister) {
2727

2828
// 4. If CanBeHeldWeakly(unregisterToken) is false, throw a TypeError
2929
// exception.
30-
if (v8_flags.harmony_symbol_as_weakmap_key) {
31-
if (!unregister_token->IsJSReceiver() &&
32-
(!unregister_token->IsSymbol() ||
33-
Handle<Symbol>::cast(unregister_token)->is_in_public_symbol_table())) {
34-
THROW_NEW_ERROR_RETURN_FAILURE(
35-
isolate,
36-
NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
37-
unregister_token));
38-
}
39-
} else {
40-
// 4. If Type(unregisterToken) is not Object, throw a TypeError exception.
41-
if (!unregister_token->IsJSReceiver()) {
42-
THROW_NEW_ERROR_RETURN_FAILURE(
43-
isolate,
44-
NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
45-
unregister_token));
46-
}
30+
if (!unregister_token->CanBeHeldWeakly()) {
31+
THROW_NEW_ERROR_RETURN_FAILURE(
32+
isolate, NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
33+
unregister_token));
4734
}
4835

4936
bool success = JSFinalizationRegistry::Unregister(

‎src/builtins/finalization-registry.tq

+3-3
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ extern transitioning macro
1616
RemoveFinalizationRegistryCellFromUnregisterTokenMap(
1717
JSFinalizationRegistry, WeakCell): void;
1818

19-
extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeWeakKey(JSAny):
19+
extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeHeldWeakly(JSAny):
2020
void labels NotWeakKey;
2121

2222
macro SplitOffTail(weakCell: WeakCell): WeakCell|Undefined {
@@ -140,7 +140,7 @@ FinalizationRegistryRegister(
140140
MessageTemplate::kIncompatibleMethodReceiver,
141141
'FinalizationRegistry.prototype.register', receiver);
142142
// 3. If CanBeHeldWeakly(target) is false, throw a TypeError exception.
143-
GotoIfCannotBeWeakKey(arguments[0])
143+
GotoIfCannotBeHeldWeakly(arguments[0])
144144
otherwise ThrowTypeError(MessageTemplate::kInvalidWeakRefsRegisterTarget);
145145

146146
const target = UnsafeCast<(JSReceiver | Symbol)>(arguments[0]);
@@ -159,7 +159,7 @@ FinalizationRegistryRegister(
159159
if (IsUndefined(unregisterTokenRaw)) {
160160
unregisterToken = Undefined;
161161
} else {
162-
GotoIfCannotBeWeakKey(unregisterTokenRaw)
162+
GotoIfCannotBeHeldWeakly(unregisterTokenRaw)
163163
otherwise ThrowTypeError(
164164
MessageTemplate::kInvalidWeakRefsUnregisterToken, unregisterTokenRaw);
165165
unregisterToken = UnsafeCast<(JSReceiver | Symbol)>(unregisterTokenRaw);

‎src/builtins/weak-ref.tq

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ WeakRefConstructor(
2424
}
2525

2626
// 2. If CanBeHeldWeakly(weakTarget) is false, throw a TypeError exception.
27-
GotoIfCannotBeWeakKey(weakTarget) otherwise ThrowTypeError(
27+
GotoIfCannotBeHeldWeakly(weakTarget) otherwise ThrowTypeError(
2828
MessageTemplate::kInvalidWeakRefsWeakRefConstructorTarget);
2929

3030
// 3. Let weakRef be ? OrdinaryCreateFromConstructor(NewTarget,

‎src/diagnostics/objects-debug.cc

+2-6
Original file line numberDiff line numberDiff line change
@@ -1300,9 +1300,7 @@ void JSSharedArray::JSSharedArrayVerify(Isolate* isolate) {
13001300
void WeakCell::WeakCellVerify(Isolate* isolate) {
13011301
CHECK(IsWeakCell());
13021302

1303-
CHECK(target().IsJSReceiver() || target().IsUndefined(isolate) ||
1304-
(target().IsSymbol() &&
1305-
!Symbol::cast(target()).is_in_public_symbol_table()));
1303+
CHECK(target().IsUndefined(isolate) || target().CanBeHeldWeakly());
13061304

13071305
CHECK(prev().IsWeakCell() || prev().IsUndefined(isolate));
13081306
if (prev().IsWeakCell()) {
@@ -1330,9 +1328,7 @@ void WeakCell::WeakCellVerify(Isolate* isolate) {
13301328
void JSWeakRef::JSWeakRefVerify(Isolate* isolate) {
13311329
CHECK(IsJSWeakRef());
13321330
JSObjectVerify(isolate);
1333-
CHECK(target().IsUndefined(isolate) || target().IsJSReceiver() ||
1334-
(target().IsSymbol() &&
1335-
!Symbol::cast(target()).is_in_public_symbol_table()));
1331+
CHECK(target().IsUndefined(isolate) || target().CanBeHeldWeakly());
13361332
}
13371333

13381334
void JSFinalizationRegistry::JSFinalizationRegistryVerify(Isolate* isolate) {

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

+2-4
Original file line numberDiff line numberDiff line change
@@ -170,9 +170,7 @@ void WeakCell::Nullify(Isolate* isolate,
170170
// only called for WeakCells which haven't been unregistered yet, so they will
171171
// be in the active_cells list. (The caller must guard against calling this
172172
// for unregistered WeakCells by checking that the target is not undefined.)
173-
DCHECK(target().IsJSReceiver() ||
174-
(target().IsSymbol() &&
175-
!Symbol::cast(target()).is_in_public_symbol_table()));
173+
DCHECK(target().CanBeHeldWeakly());
176174
set_target(ReadOnlyRoots(isolate).undefined_value());
177175

178176
JSFinalizationRegistry fr =
@@ -218,7 +216,7 @@ void WeakCell::RemoveFromFinalizationRegistryCells(Isolate* isolate) {
218216

219217
// It's important to set_target to undefined here. This guards that we won't
220218
// call Nullify (which assumes that the WeakCell is in active_cells).
221-
DCHECK(target().IsUndefined() || target().IsJSReceiver());
219+
DCHECK(target().IsUndefined() || target().CanBeHeldWeakly());
222220
set_target(ReadOnlyRoots(isolate).undefined_value());
223221

224222
JSFinalizationRegistry fr =

‎src/objects/objects-inl.h

+17
Original file line numberDiff line numberDiff line change
@@ -1206,6 +1206,23 @@ MaybeHandle<Object> Object::Share(Isolate* isolate, Handle<Object> value,
12061206
throw_if_cannot_be_shared);
12071207
}
12081208

1209+
// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-canbeheldweakly-abstract-operation
1210+
bool Object::CanBeHeldWeakly() const {
1211+
if (IsJSReceiver()) {
1212+
// TODO(v8:12547) Shared structs and arrays should only be able to point
1213+
// to shared values in weak collections. For now, disallow them as weak
1214+
// collection keys.
1215+
if (v8_flags.harmony_struct) {
1216+
return !IsJSSharedStruct() && !IsJSSharedArray();
1217+
}
1218+
return true;
1219+
}
1220+
if (v8_flags.harmony_symbol_as_weakmap_key) {
1221+
return IsSymbol() && !Symbol::cast(*this).is_in_public_symbol_table();
1222+
}
1223+
return false;
1224+
}
1225+
12091226
Handle<Object> ObjectHashTableShape::AsHandle(Handle<Object> key) {
12101227
return key;
12111228
}

‎src/objects/objects.h

+5
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,11 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
785785
Handle<HeapObject> value,
786786
ShouldThrow throw_if_cannot_be_shared);
787787

788+
// Whether this Object can be held weakly, i.e. whether it can be used as a
789+
// key in WeakMap, as a key in WeakSet, as the target of a WeakRef, or as a
790+
// target or unregister token of a FinalizationRegistry.
791+
inline bool CanBeHeldWeakly() const;
792+
788793
protected:
789794
inline Address field_address(size_t offset) const {
790795
return ptr() + offset - kHeapObjectTag;

‎src/runtime/runtime-collections.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionDelete) {
7979
int hash = args.smi_value_at(2);
8080

8181
#ifdef DEBUG
82-
DCHECK(key->IsJSReceiver());
82+
DCHECK(key->CanBeHeldWeakly());
8383
DCHECK(EphemeronHashTable::IsKey(ReadOnlyRoots(isolate), *key));
8484
Handle<EphemeronHashTable> table(
8585
EphemeronHashTable::cast(weak_collection->table()), isolate);
@@ -102,7 +102,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionSet) {
102102
int hash = args.smi_value_at(3);
103103

104104
#ifdef DEBUG
105-
DCHECK(key->IsJSReceiver());
105+
DCHECK(key->CanBeHeldWeakly());
106106
DCHECK(EphemeronHashTable::IsKey(ReadOnlyRoots(isolate), *key));
107107
Handle<EphemeronHashTable> table(
108108
EphemeronHashTable::cast(weak_collection->table()), isolate);

‎src/runtime/runtime-weak-refs.cc

+1-7
Original file line numberDiff line numberDiff line change
@@ -44,13 +44,7 @@ RUNTIME_FUNCTION(Runtime_JSWeakRefAddToKeptObjects) {
4444
HandleScope scope(isolate);
4545
DCHECK_EQ(1, args.length());
4646
Handle<HeapObject> object = args.at<HeapObject>(0);
47-
if (FLAG_harmony_symbol_as_weakmap_key) {
48-
DCHECK(object->IsJSReceiver() ||
49-
(object->IsSymbol() &&
50-
!(Handle<Symbol>::cast(object))->is_in_public_symbol_table()));
51-
} else {
52-
DCHECK(object->IsJSReceiver());
53-
}
47+
DCHECK(object->CanBeHeldWeakly());
5448

5549
isolate->heap()->KeepDuringJob(object);
5650

‎test/mjsunit/harmony/symbol-as-weakmap-key.js

+7
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,10 @@
9999
assertFalse(set.delete(key));
100100
assertFalse(set.has(key));
101101
})();
102+
103+
(function TestFinalizationRegistryUnregister() {
104+
const fr = new FinalizationRegistry(function() {});
105+
const key = {};
106+
fr.register(Symbol('foo'), "holdings", key);
107+
fr.unregister(key);
108+
})();

0 commit comments

Comments
 (0)