From 6d509668765fff4c4e112c1d15784a56227a4f12 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 19:52:25 +0100 Subject: [PATCH] deps: V8: cherry-pick 94e8282325a1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [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 Reviewed-by: Marja Hölttä Commit-Queue: Marja Hölttä Cr-Commit-Position: refs/heads/main@{#83507} Refs: https://github.com/v8/v8/commit/94e8282325a1129d46e800e6c13a79fd96e49a0f PR-URL: https://github.com/nodejs/node/pull/51004 Reviewed-By: Chengzhong Wu --- common.gypi | 2 +- .../src/builtins/builtins-collections-gen.cc | 38 +++++++++---------- .../src/builtins/builtins-collections-gen.h | 4 +- deps/v8/src/builtins/builtins-weak-refs.cc | 21 ++-------- deps/v8/src/builtins/finalization-registry.tq | 6 +-- deps/v8/src/builtins/weak-ref.tq | 2 +- deps/v8/src/diagnostics/objects-debug.cc | 8 +--- deps/v8/src/objects/js-weak-refs-inl.h | 6 +-- deps/v8/src/objects/objects-inl.h | 17 +++++++++ deps/v8/src/objects/objects.h | 5 +++ deps/v8/src/runtime/runtime-collections.cc | 4 +- deps/v8/src/runtime/runtime-weak-refs.cc | 8 +--- .../mjsunit/harmony/symbol-as-weakmap-key.js | 7 ++++ 13 files changed, 66 insertions(+), 62 deletions(-) diff --git a/common.gypi b/common.gypi index 2b3e0f97b61181..8f598c63aee632 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.31', + 'v8_embedder_string': '-node.32', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/builtins/builtins-collections-gen.cc b/deps/v8/src/builtins/builtins-collections-gen.cc index 313b66b176f7f5..19d77fa261ec61 100644 --- a/deps/v8/src/builtins/builtins-collections-gen.cc +++ b/deps/v8/src/builtins/builtins-collections-gen.cc @@ -399,23 +399,23 @@ TNode BaseCollectionsAssembler::EstimatedInitialSize( [=] { return IntPtrConstant(0); }); } -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 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(IsJSSharedStructInstanceType(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); } @@ -2573,17 +2573,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)); } @@ -2638,22 +2638,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); @@ -2735,7 +2735,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)); @@ -2753,7 +2753,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/deps/v8/src/builtins/builtins-collections-gen.h b/deps/v8/src/builtins/builtins-collections-gen.h index 6b5516a818b332..53ea23f12d9f6b 100644 --- a/deps/v8/src/builtins/builtins-collections-gen.h +++ b/deps/v8/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/deps/v8/src/builtins/builtins-weak-refs.cc b/deps/v8/src/builtins/builtins-weak-refs.cc index c76c3989d47507..a944159f247bec 100644 --- a/deps/v8/src/builtins/builtins-weak-refs.cc +++ b/deps/v8/src/builtins/builtins-weak-refs.cc @@ -27,23 +27,10 @@ BUILTIN(FinalizationRegistryUnregister) { // 4. If CanBeHeldWeakly(unregisterToken) is false, throw a TypeError // exception. - if (FLAG_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/deps/v8/src/builtins/finalization-registry.tq b/deps/v8/src/builtins/finalization-registry.tq index 3dd554b382cf91..4e4b4be068669f 100644 --- a/deps/v8/src/builtins/finalization-registry.tq +++ b/deps/v8/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/deps/v8/src/builtins/weak-ref.tq b/deps/v8/src/builtins/weak-ref.tq index 68f56fc111ad36..051831698534ce 100644 --- a/deps/v8/src/builtins/weak-ref.tq +++ b/deps/v8/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/deps/v8/src/diagnostics/objects-debug.cc b/deps/v8/src/diagnostics/objects-debug.cc index e4cb23da28934b..a4ab4e6299bef8 100644 --- a/deps/v8/src/diagnostics/objects-debug.cc +++ b/deps/v8/src/diagnostics/objects-debug.cc @@ -1240,9 +1240,7 @@ void JSSharedStruct::JSSharedStructVerify(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()) { @@ -1270,9 +1268,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/deps/v8/src/objects/js-weak-refs-inl.h b/deps/v8/src/objects/js-weak-refs-inl.h index 15f7d36b6fe72a..0385d59efb7e56 100644 --- a/deps/v8/src/objects/js-weak-refs-inl.h +++ b/deps/v8/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/deps/v8/src/objects/objects-inl.h b/deps/v8/src/objects/objects-inl.h index 82d8776ef34ca8..6a4f976029662c 100644 --- a/deps/v8/src/objects/objects-inl.h +++ b/deps/v8/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 (FLAG_harmony_struct) { + return !IsJSSharedStruct(); + } + return true; + } + if (FLAG_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/deps/v8/src/objects/objects.h b/deps/v8/src/objects/objects.h index 316f870e31f33c..a3ad333f0a91d9 100644 --- a/deps/v8/src/objects/objects.h +++ b/deps/v8/src/objects/objects.h @@ -770,6 +770,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/deps/v8/src/runtime/runtime-collections.cc b/deps/v8/src/runtime/runtime-collections.cc index 38279e2d0e27d6..98619ff0a35f07 100644 --- a/deps/v8/src/runtime/runtime-collections.cc +++ b/deps/v8/src/runtime/runtime-collections.cc @@ -82,7 +82,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); @@ -105,7 +105,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/deps/v8/src/runtime/runtime-weak-refs.cc b/deps/v8/src/runtime/runtime-weak-refs.cc index db3611dbb7d4eb..ff60813b434bef 100644 --- a/deps/v8/src/runtime/runtime-weak-refs.cc +++ b/deps/v8/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/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js b/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js index f644fccb00022d..284e78b30196b7 100644 --- a/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js +++ b/deps/v8/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); +})();