From 94e8282325a1129d46e800e6c13a79fd96e49a0f Mon Sep 17 00:00:00 2001 From: Shu-yu Guo Date: Mon, 3 Oct 2022 18:05:26 -0700 Subject: [PATCH] [symbol-as-weakmap-key] Fix DCHECKs and add CanBeHeldWeakly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Marja Hölttä Commit-Queue: Marja Hölttä Cr-Commit-Position: refs/heads/main@{#83507} --- src/builtins/builtins-collections-gen.cc | 40 +++++++++---------- src/builtins/builtins-collections-gen.h | 4 +- src/builtins/builtins-weak-refs.cc | 21 ++-------- src/builtins/finalization-registry.tq | 6 +-- src/builtins/weak-ref.tq | 2 +- src/diagnostics/objects-debug.cc | 8 +--- src/objects/js-weak-refs-inl.h | 6 +-- src/objects/objects-inl.h | 17 ++++++++ src/objects/objects.h | 5 +++ src/runtime/runtime-collections.cc | 4 +- src/runtime/runtime-weak-refs.cc | 8 +--- test/mjsunit/harmony/symbol-as-weakmap-key.js | 7 ++++ 12 files changed, 66 insertions(+), 62 deletions(-) diff --git a/src/builtins/builtins-collections-gen.cc b/src/builtins/builtins-collections-gen.cc index 1eb6a161ce88..f6edbb6bc028 100644 --- a/src/builtins/builtins-collections-gen.cc +++ b/src/builtins/builtins-collections-gen.cc @@ -425,25 +425,25 @@ TNode BaseCollectionsAssembler::EstimatedInitialSize( } // https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-canbeheldweakly-abstract-operation -void BaseCollectionsAssembler::GotoIfCannotBeWeakKey( - const TNode obj, Label* if_cannot_be_weak_key) { +void BaseCollectionsAssembler::GotoIfCannotBeHeldWeakly( + const TNode obj, Label* if_cannot_be_held_weakly) { Label check_symbol_key(this); Label end(this); - GotoIf(TaggedIsSmi(obj), if_cannot_be_weak_key); + GotoIf(TaggedIsSmi(obj), if_cannot_be_held_weakly); TNode instance_type = LoadMapInstanceType(LoadMap(CAST(obj))); GotoIfNot(IsJSReceiverInstanceType(instance_type), &check_symbol_key); // TODO(v8:12547) Shared structs and arrays should only be able to point // to shared values in weak collections. For now, disallow them as weak // collection keys. - GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_weak_key); - GotoIf(IsJSSharedArrayInstanceType(instance_type), if_cannot_be_weak_key); + GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_held_weakly); + GotoIf(IsJSSharedArrayInstanceType(instance_type), if_cannot_be_held_weakly); Goto(&end); Bind(&check_symbol_key); - GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_weak_key); - GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_weak_key); + GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_held_weakly); + GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_held_weakly); TNode flags = LoadSymbolFlags(CAST(obj)); GotoIf(Word32And(flags, Symbol::IsInPublicSymbolTableBit::kMask), - if_cannot_be_weak_key); + if_cannot_be_held_weakly); Goto(&end); Bind(&end); } @@ -2586,17 +2586,17 @@ TF_BUILTIN(WeakMapLookupHashIndex, WeakCollectionsBuiltinsAssembler) { auto table = Parameter(Descriptor::kTable); auto key = Parameter(Descriptor::kKey); - Label if_cannot_be_weak_key(this); + Label if_cannot_be_held_weakly(this); - GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key); + GotoIfCannotBeHeldWeakly(key, &if_cannot_be_held_weakly); - TNode hash = GetHash(CAST(key), &if_cannot_be_weak_key); + TNode hash = GetHash(CAST(key), &if_cannot_be_held_weakly); TNode capacity = LoadTableCapacity(table); TNode key_index = FindKeyIndexForKey( - table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key); + table, key, hash, EntryMask(capacity), &if_cannot_be_held_weakly); Return(SmiTag(ValueIndexFromKeyIndex(key_index))); - BIND(&if_cannot_be_weak_key); + BIND(&if_cannot_be_held_weakly); Return(SmiConstant(-1)); } @@ -2651,22 +2651,22 @@ TF_BUILTIN(WeakCollectionDelete, WeakCollectionsBuiltinsAssembler) { auto collection = Parameter(Descriptor::kCollection); auto key = Parameter(Descriptor::kKey); - Label call_runtime(this), if_cannot_be_weak_key(this); + Label call_runtime(this), if_cannot_be_held_weakly(this); - GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key); + GotoIfCannotBeHeldWeakly(key, &if_cannot_be_held_weakly); - TNode hash = GetHash(CAST(key), &if_cannot_be_weak_key); + TNode hash = GetHash(CAST(key), &if_cannot_be_held_weakly); TNode table = LoadTable(collection); TNode capacity = LoadTableCapacity(table); TNode key_index = FindKeyIndexForKey( - table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key); + table, key, hash, EntryMask(capacity), &if_cannot_be_held_weakly); TNode number_of_elements = LoadNumberOfElements(table, -1); GotoIf(ShouldShrink(capacity, number_of_elements), &call_runtime); RemoveEntry(table, key_index, number_of_elements); Return(TrueConstant()); - BIND(&if_cannot_be_weak_key); + BIND(&if_cannot_be_held_weakly); Return(FalseConstant()); BIND(&call_runtime); @@ -2751,7 +2751,7 @@ TF_BUILTIN(WeakMapPrototypeSet, WeakCollectionsBuiltinsAssembler) { "WeakMap.prototype.set"); Label throw_invalid_key(this); - GotoIfCannotBeWeakKey(key, &throw_invalid_key); + GotoIfCannotBeHeldWeakly(key, &throw_invalid_key); Return( CallBuiltin(Builtin::kWeakCollectionSet, context, receiver, key, value)); @@ -2769,7 +2769,7 @@ TF_BUILTIN(WeakSetPrototypeAdd, WeakCollectionsBuiltinsAssembler) { "WeakSet.prototype.add"); Label throw_invalid_value(this); - GotoIfCannotBeWeakKey(value, &throw_invalid_value); + GotoIfCannotBeHeldWeakly(value, &throw_invalid_value); Return(CallBuiltin(Builtin::kWeakCollectionSet, context, receiver, value, TrueConstant())); diff --git a/src/builtins/builtins-collections-gen.h b/src/builtins/builtins-collections-gen.h index 464d434a9fc7..6dd2381ddd37 100644 --- a/src/builtins/builtins-collections-gen.h +++ b/src/builtins/builtins-collections-gen.h @@ -27,8 +27,8 @@ class BaseCollectionsAssembler : public CodeStubAssembler { virtual ~BaseCollectionsAssembler() = default; - void GotoIfCannotBeWeakKey(const TNode obj, - Label* if_cannot_be_weak_key); + void GotoIfCannotBeHeldWeakly(const TNode obj, + Label* if_cannot_be_held_weakly); protected: enum Variant { kMap, kSet, kWeakMap, kWeakSet }; diff --git a/src/builtins/builtins-weak-refs.cc b/src/builtins/builtins-weak-refs.cc index 00742d64fba5..a944159f247b 100644 --- a/src/builtins/builtins-weak-refs.cc +++ b/src/builtins/builtins-weak-refs.cc @@ -27,23 +27,10 @@ BUILTIN(FinalizationRegistryUnregister) { // 4. If CanBeHeldWeakly(unregisterToken) is false, throw a TypeError // exception. - if (v8_flags.harmony_symbol_as_weakmap_key) { - if (!unregister_token->IsJSReceiver() && - (!unregister_token->IsSymbol() || - Handle::cast(unregister_token)->is_in_public_symbol_table())) { - THROW_NEW_ERROR_RETURN_FAILURE( - isolate, - NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken, - unregister_token)); - } - } else { - // 4. If Type(unregisterToken) is not Object, throw a TypeError exception. - if (!unregister_token->IsJSReceiver()) { - THROW_NEW_ERROR_RETURN_FAILURE( - isolate, - NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken, - unregister_token)); - } + if (!unregister_token->CanBeHeldWeakly()) { + THROW_NEW_ERROR_RETURN_FAILURE( + isolate, NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken, + unregister_token)); } bool success = JSFinalizationRegistry::Unregister( diff --git a/src/builtins/finalization-registry.tq b/src/builtins/finalization-registry.tq index 3dd554b382cf..4e4b4be06866 100644 --- a/src/builtins/finalization-registry.tq +++ b/src/builtins/finalization-registry.tq @@ -16,7 +16,7 @@ extern transitioning macro RemoveFinalizationRegistryCellFromUnregisterTokenMap( JSFinalizationRegistry, WeakCell): void; -extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeWeakKey(JSAny): +extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeHeldWeakly(JSAny): void labels NotWeakKey; macro SplitOffTail(weakCell: WeakCell): WeakCell|Undefined { @@ -140,7 +140,7 @@ FinalizationRegistryRegister( MessageTemplate::kIncompatibleMethodReceiver, 'FinalizationRegistry.prototype.register', receiver); // 3. If CanBeHeldWeakly(target) is false, throw a TypeError exception. - GotoIfCannotBeWeakKey(arguments[0]) + GotoIfCannotBeHeldWeakly(arguments[0]) otherwise ThrowTypeError(MessageTemplate::kInvalidWeakRefsRegisterTarget); const target = UnsafeCast<(JSReceiver | Symbol)>(arguments[0]); @@ -159,7 +159,7 @@ FinalizationRegistryRegister( if (IsUndefined(unregisterTokenRaw)) { unregisterToken = Undefined; } else { - GotoIfCannotBeWeakKey(unregisterTokenRaw) + GotoIfCannotBeHeldWeakly(unregisterTokenRaw) otherwise ThrowTypeError( MessageTemplate::kInvalidWeakRefsUnregisterToken, unregisterTokenRaw); unregisterToken = UnsafeCast<(JSReceiver | Symbol)>(unregisterTokenRaw); diff --git a/src/builtins/weak-ref.tq b/src/builtins/weak-ref.tq index 68f56fc111ad..051831698534 100644 --- a/src/builtins/weak-ref.tq +++ b/src/builtins/weak-ref.tq @@ -24,7 +24,7 @@ WeakRefConstructor( } // 2. If CanBeHeldWeakly(weakTarget) is false, throw a TypeError exception. - GotoIfCannotBeWeakKey(weakTarget) otherwise ThrowTypeError( + GotoIfCannotBeHeldWeakly(weakTarget) otherwise ThrowTypeError( MessageTemplate::kInvalidWeakRefsWeakRefConstructorTarget); // 3. Let weakRef be ? OrdinaryCreateFromConstructor(NewTarget, diff --git a/src/diagnostics/objects-debug.cc b/src/diagnostics/objects-debug.cc index 55e6adb85220..0f91a0fc813a 100644 --- a/src/diagnostics/objects-debug.cc +++ b/src/diagnostics/objects-debug.cc @@ -1300,9 +1300,7 @@ void JSSharedArray::JSSharedArrayVerify(Isolate* isolate) { void WeakCell::WeakCellVerify(Isolate* isolate) { CHECK(IsWeakCell()); - CHECK(target().IsJSReceiver() || target().IsUndefined(isolate) || - (target().IsSymbol() && - !Symbol::cast(target()).is_in_public_symbol_table())); + CHECK(target().IsUndefined(isolate) || target().CanBeHeldWeakly()); CHECK(prev().IsWeakCell() || prev().IsUndefined(isolate)); if (prev().IsWeakCell()) { @@ -1330,9 +1328,7 @@ void WeakCell::WeakCellVerify(Isolate* isolate) { void JSWeakRef::JSWeakRefVerify(Isolate* isolate) { CHECK(IsJSWeakRef()); JSObjectVerify(isolate); - CHECK(target().IsUndefined(isolate) || target().IsJSReceiver() || - (target().IsSymbol() && - !Symbol::cast(target()).is_in_public_symbol_table())); + CHECK(target().IsUndefined(isolate) || target().CanBeHeldWeakly()); } void JSFinalizationRegistry::JSFinalizationRegistryVerify(Isolate* isolate) { diff --git a/src/objects/js-weak-refs-inl.h b/src/objects/js-weak-refs-inl.h index 15f7d36b6fe7..0385d59efb7e 100644 --- a/src/objects/js-weak-refs-inl.h +++ b/src/objects/js-weak-refs-inl.h @@ -170,9 +170,7 @@ void WeakCell::Nullify(Isolate* isolate, // only called for WeakCells which haven't been unregistered yet, so they will // be in the active_cells list. (The caller must guard against calling this // for unregistered WeakCells by checking that the target is not undefined.) - DCHECK(target().IsJSReceiver() || - (target().IsSymbol() && - !Symbol::cast(target()).is_in_public_symbol_table())); + DCHECK(target().CanBeHeldWeakly()); set_target(ReadOnlyRoots(isolate).undefined_value()); JSFinalizationRegistry fr = @@ -218,7 +216,7 @@ void WeakCell::RemoveFromFinalizationRegistryCells(Isolate* isolate) { // It's important to set_target to undefined here. This guards that we won't // call Nullify (which assumes that the WeakCell is in active_cells). - DCHECK(target().IsUndefined() || target().IsJSReceiver()); + DCHECK(target().IsUndefined() || target().CanBeHeldWeakly()); set_target(ReadOnlyRoots(isolate).undefined_value()); JSFinalizationRegistry fr = diff --git a/src/objects/objects-inl.h b/src/objects/objects-inl.h index ec64b6ffde6a..8d34d53a7f4b 100644 --- a/src/objects/objects-inl.h +++ b/src/objects/objects-inl.h @@ -1206,6 +1206,23 @@ MaybeHandle Object::Share(Isolate* isolate, Handle value, throw_if_cannot_be_shared); } +// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-canbeheldweakly-abstract-operation +bool Object::CanBeHeldWeakly() const { + if (IsJSReceiver()) { + // TODO(v8:12547) Shared structs and arrays should only be able to point + // to shared values in weak collections. For now, disallow them as weak + // collection keys. + if (v8_flags.harmony_struct) { + return !IsJSSharedStruct() && !IsJSSharedArray(); + } + return true; + } + if (v8_flags.harmony_symbol_as_weakmap_key) { + return IsSymbol() && !Symbol::cast(*this).is_in_public_symbol_table(); + } + return false; +} + Handle ObjectHashTableShape::AsHandle(Handle key) { return key; } diff --git a/src/objects/objects.h b/src/objects/objects.h index 52931c410887..286067cb3a5c 100644 --- a/src/objects/objects.h +++ b/src/objects/objects.h @@ -785,6 +785,11 @@ class Object : public TaggedImpl { Handle value, ShouldThrow throw_if_cannot_be_shared); + // Whether this Object can be held weakly, i.e. whether it can be used as a + // key in WeakMap, as a key in WeakSet, as the target of a WeakRef, or as a + // target or unregister token of a FinalizationRegistry. + inline bool CanBeHeldWeakly() const; + protected: inline Address field_address(size_t offset) const { return ptr() + offset - kHeapObjectTag; diff --git a/src/runtime/runtime-collections.cc b/src/runtime/runtime-collections.cc index cdf822c0c0e7..a326efaa5e75 100644 --- a/src/runtime/runtime-collections.cc +++ b/src/runtime/runtime-collections.cc @@ -79,7 +79,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionDelete) { int hash = args.smi_value_at(2); #ifdef DEBUG - DCHECK(key->IsJSReceiver()); + DCHECK(key->CanBeHeldWeakly()); DCHECK(EphemeronHashTable::IsKey(ReadOnlyRoots(isolate), *key)); Handle table( EphemeronHashTable::cast(weak_collection->table()), isolate); @@ -102,7 +102,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionSet) { int hash = args.smi_value_at(3); #ifdef DEBUG - DCHECK(key->IsJSReceiver()); + DCHECK(key->CanBeHeldWeakly()); DCHECK(EphemeronHashTable::IsKey(ReadOnlyRoots(isolate), *key)); Handle table( EphemeronHashTable::cast(weak_collection->table()), isolate); diff --git a/src/runtime/runtime-weak-refs.cc b/src/runtime/runtime-weak-refs.cc index db3611dbb7d4..ff60813b434b 100644 --- a/src/runtime/runtime-weak-refs.cc +++ b/src/runtime/runtime-weak-refs.cc @@ -44,13 +44,7 @@ RUNTIME_FUNCTION(Runtime_JSWeakRefAddToKeptObjects) { HandleScope scope(isolate); DCHECK_EQ(1, args.length()); Handle object = args.at(0); - if (FLAG_harmony_symbol_as_weakmap_key) { - DCHECK(object->IsJSReceiver() || - (object->IsSymbol() && - !(Handle::cast(object))->is_in_public_symbol_table())); - } else { - DCHECK(object->IsJSReceiver()); - } + DCHECK(object->CanBeHeldWeakly()); isolate->heap()->KeepDuringJob(object); diff --git a/test/mjsunit/harmony/symbol-as-weakmap-key.js b/test/mjsunit/harmony/symbol-as-weakmap-key.js index f644fccb0002..284e78b30196 100644 --- a/test/mjsunit/harmony/symbol-as-weakmap-key.js +++ b/test/mjsunit/harmony/symbol-as-weakmap-key.js @@ -99,3 +99,10 @@ assertFalse(set.delete(key)); assertFalse(set.has(key)); })(); + +(function TestFinalizationRegistryUnregister() { + const fr = new FinalizationRegistry(function() {}); + const key = {}; + fr.register(Symbol('foo'), "holdings", key); + fr.unregister(key); +})();