From a6524dd6ab419152eef7d168459b2ec0f3422433 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 18:13:50 +0100 Subject: [PATCH 01/22] deps: V8: cherry-pick c400af48b5ef MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [symbol-as-weakmap-key] Implement Symbol as WeakMap Keys Allow non-registered symbols as keys in weakmap and weakset. Allow non-registered symbols as target and unregisterToken in WeakRef and FinalizationRegistry. Bug: v8:12947 Change-Id: Ieb63bda66e3cc378879ac651e23300b71caed627 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3865056 Reviewed-by: Dominik Inführ Commit-Queue: Marja Hölttä Reviewed-by: Jakob Linke Cr-Commit-Position: refs/heads/main@{#83313} Refs: https://github.com/v8/v8/commit/c400af48b5ef3c5c54312fec8acd0d84e6071046 --- deps/v8/src/builtins/base.tq | 10 +- .../src/builtins/builtins-collections-gen.cc | 230 +++--------------- .../src/builtins/builtins-collections-gen.h | 186 ++++++++++++++ deps/v8/src/builtins/builtins-weak-refs.cc | 28 ++- deps/v8/src/builtins/cast.tq | 15 ++ deps/v8/src/builtins/finalization-registry.tq | 38 +-- deps/v8/src/builtins/weak-ref.tq | 20 +- deps/v8/src/codegen/code-stub-assembler.h | 6 + deps/v8/src/codegen/external-reference.cc | 5 + deps/v8/src/codegen/external-reference.h | 2 + deps/v8/src/common/message-template.h | 10 +- deps/v8/src/diagnostics/objects-debug.cc | 8 +- deps/v8/src/flags/flag-definitions.h | 3 +- deps/v8/src/heap/heap.cc | 2 +- deps/v8/src/heap/heap.h | 2 +- deps/v8/src/init/bootstrapper.cc | 1 + deps/v8/src/objects/js-weak-refs-inl.h | 11 +- deps/v8/src/objects/js-weak-refs.h | 4 +- deps/v8/src/objects/js-weak-refs.tq | 9 +- deps/v8/src/runtime/runtime-weak-refs.cc | 12 +- .../test/message/fail/weak-refs-register1.out | 4 +- .../message/fail/weak-refs-unregister.out | 4 +- .../mjsunit/harmony/symbol-as-weakmap-key.js | 99 ++++++++ .../test/mjsunit/harmony/weakrefs/basics.js | 4 +- .../weakrefs/symbol-as-weakref-target-gc.js | 39 +++ .../weakrefs/symbol-as-weakref-target.js | 41 ++++ 26 files changed, 533 insertions(+), 260 deletions(-) create mode 100644 deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js create mode 100644 deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target-gc.js create mode 100644 deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target.js diff --git a/deps/v8/src/builtins/base.tq b/deps/v8/src/builtins/base.tq index c34d9f6884cfb5..fe67d403b87879 100644 --- a/deps/v8/src/builtins/base.tq +++ b/deps/v8/src/builtins/base.tq @@ -438,9 +438,9 @@ extern enum MessageTemplate { kWasmTrapArrayOutOfBounds, kWasmTrapArrayTooLarge, kWeakRefsRegisterTargetAndHoldingsMustNotBeSame, - kWeakRefsRegisterTargetMustBeObject, - kWeakRefsUnregisterTokenMustBeObject, - kWeakRefsWeakRefConstructorTargetMustBeObject, + kInvalidWeakRefsRegisterTarget, + kInvalidWeakRefsUnregisterToken, + kInvalidWeakRefsWeakRefConstructorTarget, ... } @@ -906,10 +906,10 @@ macro Float64IsNaN(n: float64): bool { // The type of all tagged values that can safely be compared with TaggedEqual. @if(V8_ENABLE_WEBASSEMBLY) type TaggedWithIdentity = JSReceiver | FixedArrayBase | Oddball | Map | - WeakCell | Context | EmptyString | WasmInternalFunction; + WeakCell | Context | EmptyString | Symbol | WasmInternalFunction; @ifnot(V8_ENABLE_WEBASSEMBLY) type TaggedWithIdentity = JSReceiver | FixedArrayBase | Oddball | Map | - WeakCell | Context | EmptyString; + WeakCell | Context | EmptyString | Symbol; extern operator '==' macro TaggedEqual(TaggedWithIdentity, Object): bool; extern operator '==' macro TaggedEqual(Object, TaggedWithIdentity): bool; diff --git a/deps/v8/src/builtins/builtins-collections-gen.cc b/deps/v8/src/builtins/builtins-collections-gen.cc index 00cad7b314b756..313b66b176f7f5 100644 --- a/deps/v8/src/builtins/builtins-collections-gen.cc +++ b/deps/v8/src/builtins/builtins-collections-gen.cc @@ -22,130 +22,6 @@ namespace internal { template using TVariable = compiler::TypedCodeAssemblerVariable; -class BaseCollectionsAssembler : public CodeStubAssembler { - public: - explicit BaseCollectionsAssembler(compiler::CodeAssemblerState* state) - : CodeStubAssembler(state) {} - - virtual ~BaseCollectionsAssembler() = default; - - protected: - enum Variant { kMap, kSet, kWeakMap, kWeakSet }; - - // Adds an entry to a collection. For Maps, properly handles extracting the - // key and value from the entry (see LoadKeyValue()). - void AddConstructorEntry(Variant variant, TNode context, - TNode collection, TNode add_function, - TNode key_value, - Label* if_may_have_side_effects = nullptr, - Label* if_exception = nullptr, - TVariable* var_exception = nullptr); - - // Adds constructor entries to a collection. Choosing a fast path when - // possible. - void AddConstructorEntries(Variant variant, TNode context, - TNode native_context, - TNode collection, - TNode initial_entries); - - // Fast path for adding constructor entries. Assumes the entries are a fast - // JS array (see CodeStubAssembler::BranchIfFastJSArray()). - void AddConstructorEntriesFromFastJSArray(Variant variant, - TNode context, - TNode native_context, - TNode collection, - TNode fast_jsarray, - Label* if_may_have_side_effects); - - // Adds constructor entries to a collection using the iterator protocol. - void AddConstructorEntriesFromIterable(Variant variant, - TNode context, - TNode native_context, - TNode collection, - TNode iterable); - - // Constructs a collection instance. Choosing a fast path when possible. - TNode AllocateJSCollection(TNode context, - TNode constructor, - TNode new_target); - - // Fast path for constructing a collection instance if the constructor - // function has not been modified. - TNode AllocateJSCollectionFast(TNode constructor); - - // Fallback for constructing a collection instance if the constructor function - // has been modified. - TNode AllocateJSCollectionSlow(TNode context, - TNode constructor, - TNode new_target); - - // Allocates the backing store for a collection. - virtual TNode AllocateTable( - Variant variant, TNode at_least_space_for) = 0; - - // Main entry point for a collection constructor builtin. - void GenerateConstructor(Variant variant, - Handle constructor_function_name, - TNode new_target, TNode argc, - TNode context); - - // Retrieves the collection function that adds an entry. `set` for Maps and - // `add` for Sets. - TNode GetAddFunction(Variant variant, TNode context, - TNode collection); - - // Retrieves the collection constructor function. - TNode GetConstructor(Variant variant, - TNode native_context); - - // Retrieves the initial collection function that adds an entry. Should only - // be called when it is certain that a collection prototype's map hasn't been - // changed. - TNode GetInitialAddFunction(Variant variant, - TNode native_context); - - // Checks whether {collection}'s initial add/set function has been modified - // (depending on {variant}, loaded from {native_context}). - void GotoIfInitialAddFunctionModified(Variant variant, - TNode native_context, - TNode collection, - Label* if_modified); - - // Gets root index for the name of the add/set function. - RootIndex GetAddFunctionNameIndex(Variant variant); - - // Retrieves the offset to access the backing table from the collection. - int GetTableOffset(Variant variant); - - // Estimates the number of entries the collection will have after adding the - // entries passed in the constructor. AllocateTable() can use this to avoid - // the time of growing/rehashing when adding the constructor entries. - TNode EstimatedInitialSize(TNode initial_entries, - TNode is_fast_jsarray); - - void GotoIfCannotBeWeakKey(const TNode obj, - Label* if_cannot_be_weak_key); - - // Determines whether the collection's prototype has been modified. - TNode HasInitialCollectionPrototype(Variant variant, - TNode native_context, - TNode collection); - - // Gets the initial prototype map for given collection {variant}. - TNode GetInitialCollectionPrototype(Variant variant, - TNode native_context); - - // Loads an element from a fixed array. If the element is the hole, returns - // `undefined`. - TNode LoadAndNormalizeFixedArrayElement(TNode elements, - TNode index); - - // Loads an element from a fixed double array. If the element is the hole, - // returns `undefined`. - TNode LoadAndNormalizeFixedDoubleArrayElement( - TNode elements, TNode index); -}; - void BaseCollectionsAssembler::AddConstructorEntry( Variant variant, TNode context, TNode collection, TNode add_function, TNode key_value, @@ -525,12 +401,23 @@ TNode BaseCollectionsAssembler::EstimatedInitialSize( void BaseCollectionsAssembler::GotoIfCannotBeWeakKey( const TNode obj, Label* if_cannot_be_weak_key) { + Label check_symbol_key(this); + Label end(this); GotoIf(TaggedIsSmi(obj), if_cannot_be_weak_key); TNode instance_type = LoadMapInstanceType(LoadMap(CAST(obj))); - GotoIfNot(IsJSReceiverInstanceType(instance_type), if_cannot_be_weak_key); + 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); + Goto(&end); + Bind(&check_symbol_key); + GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_weak_key); + GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_weak_key); + TNode flags = LoadSymbolFlags(CAST(obj)); + GotoIf(Word32And(flags, Symbol::IsInPublicSymbolTableBit::kMask), + if_cannot_be_weak_key); + Goto(&end); + Bind(&end); } TNode BaseCollectionsAssembler::GetInitialCollectionPrototype( @@ -2427,67 +2314,6 @@ TF_BUILTIN(FindOrderedHashMapEntry, CollectionsBuiltinsAssembler) { Return(SmiConstant(-1)); } -class WeakCollectionsBuiltinsAssembler : public BaseCollectionsAssembler { - public: - explicit WeakCollectionsBuiltinsAssembler(compiler::CodeAssemblerState* state) - : BaseCollectionsAssembler(state) {} - - protected: - void AddEntry(TNode table, TNode key_index, - TNode key, TNode value, - TNode number_of_elements); - - TNode AllocateTable(Variant variant, - TNode at_least_space_for) override; - - // Generates and sets the identity for a JSRececiver. - TNode CreateIdentityHash(TNode receiver); - TNode EntryMask(TNode capacity); - - // Builds code that finds the EphemeronHashTable entry for a {key} using the - // comparison code generated by {key_compare}. The key index is returned if - // the {key} is found. - using KeyComparator = - std::function entry_key, Label* if_same)>; - TNode FindKeyIndex(TNode table, TNode key_hash, - TNode entry_mask, - const KeyComparator& key_compare); - - // Builds code that finds an EphemeronHashTable entry available for a new - // entry. - TNode FindKeyIndexForInsertion(TNode table, - TNode key_hash, - TNode entry_mask); - - // Builds code that finds the EphemeronHashTable entry with key that matches - // {key} and returns the entry's key index. If {key} cannot be found, jumps to - // {if_not_found}. - TNode FindKeyIndexForKey(TNode table, TNode key, - TNode hash, - TNode entry_mask, - Label* if_not_found); - - TNode InsufficientCapacityToAdd(TNode capacity, - TNode number_of_elements, - TNode number_of_deleted); - TNode KeyIndexFromEntry(TNode entry); - - TNode LoadNumberOfElements(TNode table, - int offset); - TNode LoadNumberOfDeleted(TNode table, - int offset = 0); - TNode LoadTable(TNode collection); - TNode LoadTableCapacity(TNode table); - - void RemoveEntry(TNode table, TNode key_index, - TNode number_of_elements); - TNode ShouldRehash(TNode number_of_elements, - TNode number_of_deleted); - TNode ShouldShrink(TNode capacity, - TNode number_of_elements); - TNode ValueIndexFromKeyIndex(TNode key_index); -}; - void WeakCollectionsBuiltinsAssembler::AddEntry( TNode table, TNode key_index, TNode key, TNode value, TNode number_of_elements) { @@ -2503,6 +2329,25 @@ void WeakCollectionsBuiltinsAssembler::AddEntry( SmiFromIntPtr(number_of_elements)); } +TNode WeakCollectionsBuiltinsAssembler::GetHash( + const TNode key, Label* if_no_hash) { + TVARIABLE(IntPtrT, var_hash); + Label if_symbol(this); + Label return_result(this); + GotoIfNot(IsJSReceiver(key), &if_symbol); + var_hash = LoadJSReceiverIdentityHash(CAST(key), if_no_hash); + Goto(&return_result); + Bind(&if_symbol); + CSA_DCHECK(this, IsSymbol(key)); + CSA_DCHECK(this, Word32BinaryNot( + Word32And(LoadSymbolFlags(CAST(key)), + Symbol::IsInPublicSymbolTableBit::kMask))); + var_hash = ChangeInt32ToIntPtr(LoadNameHash(CAST(key), nullptr)); + Goto(&return_result); + Bind(&return_result); + return var_hash.value(); +} + TNode WeakCollectionsBuiltinsAssembler::AllocateTable( Variant variant, TNode at_least_space_for) { // See HashTable::New(). @@ -2732,8 +2577,7 @@ TF_BUILTIN(WeakMapLookupHashIndex, WeakCollectionsBuiltinsAssembler) { GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key); - TNode hash = - LoadJSReceiverIdentityHash(CAST(key), &if_cannot_be_weak_key); + TNode hash = GetHash(CAST(key), &if_cannot_be_weak_key); TNode capacity = LoadTableCapacity(table); TNode key_index = FindKeyIndexForKey( table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key); @@ -2798,8 +2642,7 @@ TF_BUILTIN(WeakCollectionDelete, WeakCollectionsBuiltinsAssembler) { GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key); - TNode hash = - LoadJSReceiverIdentityHash(CAST(key), &if_cannot_be_weak_key); + TNode hash = GetHash(CAST(key), &if_cannot_be_weak_key); TNode table = LoadTable(collection); TNode capacity = LoadTableCapacity(table); TNode key_index = FindKeyIndexForKey( @@ -2823,10 +2666,10 @@ TF_BUILTIN(WeakCollectionDelete, WeakCollectionsBuiltinsAssembler) { TF_BUILTIN(WeakCollectionSet, WeakCollectionsBuiltinsAssembler) { auto context = Parameter(Descriptor::kContext); auto collection = Parameter(Descriptor::kCollection); - auto key = Parameter(Descriptor::kKey); + auto key = Parameter(Descriptor::kKey); auto value = Parameter(Descriptor::kValue); - CSA_DCHECK(this, IsJSReceiver(key)); + CSA_DCHECK(this, Word32Or(IsJSReceiver(key), IsSymbol(key))); Label call_runtime(this), if_no_hash(this), if_not_found(this); @@ -2834,7 +2677,7 @@ TF_BUILTIN(WeakCollectionSet, WeakCollectionsBuiltinsAssembler) { TNode capacity = LoadTableCapacity(table); TNode entry_mask = EntryMask(capacity); - TVARIABLE(IntPtrT, var_hash, LoadJSReceiverIdentityHash(key, &if_no_hash)); + TVARIABLE(IntPtrT, var_hash, GetHash(key, &if_no_hash)); TNode key_index = FindKeyIndexForKey(table, key, var_hash.value(), entry_mask, &if_not_found); @@ -2843,6 +2686,7 @@ TF_BUILTIN(WeakCollectionSet, WeakCollectionsBuiltinsAssembler) { BIND(&if_no_hash); { + CSA_DCHECK(this, IsJSReceiver(key)); var_hash = SmiUntag(CreateIdentityHash(key)); Goto(&if_not_found); } diff --git a/deps/v8/src/builtins/builtins-collections-gen.h b/deps/v8/src/builtins/builtins-collections-gen.h index a132557e3cd0a4..6b5516a818b332 100644 --- a/deps/v8/src/builtins/builtins-collections-gen.h +++ b/deps/v8/src/builtins/builtins-collections-gen.h @@ -20,6 +20,192 @@ void BranchIfIterableWithOriginalValueSetIterator( TNode context, compiler::CodeAssemblerLabel* if_true, compiler::CodeAssemblerLabel* if_false); +class BaseCollectionsAssembler : public CodeStubAssembler { + public: + explicit BaseCollectionsAssembler(compiler::CodeAssemblerState* state) + : CodeStubAssembler(state) {} + + virtual ~BaseCollectionsAssembler() = default; + + void GotoIfCannotBeWeakKey(const TNode obj, + Label* if_cannot_be_weak_key); + + protected: + enum Variant { kMap, kSet, kWeakMap, kWeakSet }; + + // Adds an entry to a collection. For Maps, properly handles extracting the + // key and value from the entry (see LoadKeyValue()). + void AddConstructorEntry(Variant variant, TNode context, + TNode collection, TNode add_function, + TNode key_value, + Label* if_may_have_side_effects = nullptr, + Label* if_exception = nullptr, + TVariable* var_exception = nullptr); + + // Adds constructor entries to a collection. Choosing a fast path when + // possible. + void AddConstructorEntries(Variant variant, TNode context, + TNode native_context, + TNode collection, + TNode initial_entries); + + // Fast path for adding constructor entries. Assumes the entries are a fast + // JS array (see CodeStubAssembler::BranchIfFastJSArray()). + void AddConstructorEntriesFromFastJSArray(Variant variant, + TNode context, + TNode native_context, + TNode collection, + TNode fast_jsarray, + Label* if_may_have_side_effects); + + // Adds constructor entries to a collection using the iterator protocol. + void AddConstructorEntriesFromIterable(Variant variant, + TNode context, + TNode native_context, + TNode collection, + TNode iterable); + + // Constructs a collection instance. Choosing a fast path when possible. + TNode AllocateJSCollection(TNode context, + TNode constructor, + TNode new_target); + + // Fast path for constructing a collection instance if the constructor + // function has not been modified. + TNode AllocateJSCollectionFast(TNode constructor); + + // Fallback for constructing a collection instance if the constructor function + // has been modified. + TNode AllocateJSCollectionSlow(TNode context, + TNode constructor, + TNode new_target); + + // Allocates the backing store for a collection. + virtual TNode AllocateTable( + Variant variant, TNode at_least_space_for) = 0; + + // Main entry point for a collection constructor builtin. + void GenerateConstructor(Variant variant, + Handle constructor_function_name, + TNode new_target, TNode argc, + TNode context); + + // Retrieves the collection function that adds an entry. `set` for Maps and + // `add` for Sets. + TNode GetAddFunction(Variant variant, TNode context, + TNode collection); + + // Retrieves the collection constructor function. + TNode GetConstructor(Variant variant, + TNode native_context); + + // Retrieves the initial collection function that adds an entry. Should only + // be called when it is certain that a collection prototype's map hasn't been + // changed. + TNode GetInitialAddFunction(Variant variant, + TNode native_context); + + // Checks whether {collection}'s initial add/set function has been modified + // (depending on {variant}, loaded from {native_context}). + void GotoIfInitialAddFunctionModified(Variant variant, + TNode native_context, + TNode collection, + Label* if_modified); + + // Gets root index for the name of the add/set function. + RootIndex GetAddFunctionNameIndex(Variant variant); + + // Retrieves the offset to access the backing table from the collection. + int GetTableOffset(Variant variant); + + // Estimates the number of entries the collection will have after adding the + // entries passed in the constructor. AllocateTable() can use this to avoid + // the time of growing/rehashing when adding the constructor entries. + TNode EstimatedInitialSize(TNode initial_entries, + TNode is_fast_jsarray); + + // Determines whether the collection's prototype has been modified. + TNode HasInitialCollectionPrototype(Variant variant, + TNode native_context, + TNode collection); + + // Gets the initial prototype map for given collection {variant}. + TNode GetInitialCollectionPrototype(Variant variant, + TNode native_context); + + // Loads an element from a fixed array. If the element is the hole, returns + // `undefined`. + TNode LoadAndNormalizeFixedArrayElement(TNode elements, + TNode index); + + // Loads an element from a fixed double array. If the element is the hole, + // returns `undefined`. + TNode LoadAndNormalizeFixedDoubleArrayElement( + TNode elements, TNode index); +}; + +class WeakCollectionsBuiltinsAssembler : public BaseCollectionsAssembler { + public: + explicit WeakCollectionsBuiltinsAssembler(compiler::CodeAssemblerState* state) + : BaseCollectionsAssembler(state) {} + + protected: + void AddEntry(TNode table, TNode key_index, + TNode key, TNode value, + TNode number_of_elements); + + TNode AllocateTable(Variant variant, + TNode at_least_space_for) override; + + TNode GetHash(const TNode key, Label* if_no_hash); + // Generates and sets the identity for a JSRececiver. + TNode CreateIdentityHash(TNode receiver); + TNode EntryMask(TNode capacity); + + // Builds code that finds the EphemeronHashTable entry for a {key} using the + // comparison code generated by {key_compare}. The key index is returned if + // the {key} is found. + using KeyComparator = + std::function entry_key, Label* if_same)>; + TNode FindKeyIndex(TNode table, TNode key_hash, + TNode entry_mask, + const KeyComparator& key_compare); + + // Builds code that finds an EphemeronHashTable entry available for a new + // entry. + TNode FindKeyIndexForInsertion(TNode table, + TNode key_hash, + TNode entry_mask); + + // Builds code that finds the EphemeronHashTable entry with key that matches + // {key} and returns the entry's key index. If {key} cannot be found, jumps to + // {if_not_found}. + TNode FindKeyIndexForKey(TNode table, TNode key, + TNode hash, + TNode entry_mask, + Label* if_not_found); + + TNode InsufficientCapacityToAdd(TNode capacity, + TNode number_of_elements, + TNode number_of_deleted); + TNode KeyIndexFromEntry(TNode entry); + + TNode LoadNumberOfElements(TNode table, + int offset); + TNode LoadNumberOfDeleted(TNode table, + int offset = 0); + TNode LoadTable(TNode collection); + TNode LoadTableCapacity(TNode table); + + void RemoveEntry(TNode table, TNode key_index, + TNode number_of_elements); + TNode ShouldRehash(TNode number_of_elements, + TNode number_of_deleted); + TNode ShouldShrink(TNode capacity, + TNode number_of_elements); + TNode ValueIndexFromKeyIndex(TNode key_index); +}; + } // namespace internal } // namespace v8 diff --git a/deps/v8/src/builtins/builtins-weak-refs.cc b/deps/v8/src/builtins/builtins-weak-refs.cc index aee330b4bd4f3c..c76c3989d47507 100644 --- a/deps/v8/src/builtins/builtins-weak-refs.cc +++ b/deps/v8/src/builtins/builtins-weak-refs.cc @@ -9,6 +9,7 @@ namespace v8 { namespace internal { +// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-finalization-registry.prototype.unregister BUILTIN(FinalizationRegistryUnregister) { HandleScope scope(isolate); const char* method_name = "FinalizationRegistry.prototype.unregister"; @@ -24,16 +25,29 @@ BUILTIN(FinalizationRegistryUnregister) { Handle unregister_token = args.atOrUndefined(isolate, 1); - // 4. If Type(unregisterToken) is not Object, throw a TypeError exception. - if (!unregister_token->IsJSReceiver()) { - THROW_NEW_ERROR_RETURN_FAILURE( - isolate, - NewTypeError(MessageTemplate::kWeakRefsUnregisterTokenMustBeObject, - unregister_token)); + // 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)); + } } bool success = JSFinalizationRegistry::Unregister( - finalization_registry, Handle::cast(unregister_token), + finalization_registry, Handle::cast(unregister_token), isolate); return *isolate->factory()->ToBoolean(success); diff --git a/deps/v8/src/builtins/cast.tq b/deps/v8/src/builtins/cast.tq index 5cb6e0bc92e0ed..0d347e3dd35cdc 100644 --- a/deps/v8/src/builtins/cast.tq +++ b/deps/v8/src/builtins/cast.tq @@ -697,6 +697,21 @@ Cast(o: HeapObject): JSReceiver|Null } } +Cast(implicit context: Context)(o: Object): JSReceiver|Symbol + labels CastError { + typeswitch (o) { + case (o: JSReceiver): { + return o; + } + case (o: Symbol): { + return o; + } + case (Object): { + goto CastError; + } + } +} + Cast(o: Object): Smi|PromiseReaction labels CastError { typeswitch (o) { case (o: Smi): { diff --git a/deps/v8/src/builtins/finalization-registry.tq b/deps/v8/src/builtins/finalization-registry.tq index 38cae7ed20b9ff..3dd554b382cf91 100644 --- a/deps/v8/src/builtins/finalization-registry.tq +++ b/deps/v8/src/builtins/finalization-registry.tq @@ -1,6 +1,7 @@ // Copyright 2020 the V8 project authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "src/builtins/builtins-collections-gen.h" namespace runtime { extern runtime @@ -15,6 +16,9 @@ extern transitioning macro RemoveFinalizationRegistryCellFromUnregisterTokenMap( JSFinalizationRegistry, WeakCell): void; +extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeWeakKey(JSAny): + void labels NotWeakKey; + macro SplitOffTail(weakCell: WeakCell): WeakCell|Undefined { const weakCellTail = weakCell.next; weakCell.next = Undefined; @@ -125,6 +129,7 @@ FinalizationRegistryConstructor( return finalizationRegistry; } +// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-finalization-registry.prototype.register transitioning javascript builtin FinalizationRegistryRegister( js-implicit context: NativeContext, receiver: JSAny)(...arguments): JSAny { @@ -134,33 +139,32 @@ FinalizationRegistryRegister( ThrowTypeError( MessageTemplate::kIncompatibleMethodReceiver, 'FinalizationRegistry.prototype.register', receiver); - // 3. If Type(target) is not Object, throw a TypeError exception. - const target = Cast(arguments[0]) otherwise ThrowTypeError( - MessageTemplate::kWeakRefsRegisterTargetMustBeObject); + // 3. If CanBeHeldWeakly(target) is false, throw a TypeError exception. + GotoIfCannotBeWeakKey(arguments[0]) + otherwise ThrowTypeError(MessageTemplate::kInvalidWeakRefsRegisterTarget); + + const target = UnsafeCast<(JSReceiver | Symbol)>(arguments[0]); const heldValue = arguments[1]; // 4. If SameValue(target, heldValue), throw a TypeError exception. if (target == heldValue) { ThrowTypeError( MessageTemplate::kWeakRefsRegisterTargetAndHoldingsMustNotBeSame); } - // 5. If Type(unregisterToken) is not Object, + // 5. If CanBeHeldWeakly(unregisterToken) is false, // a. If unregisterToken is not undefined, throw a TypeError exception. // b. Set unregisterToken to empty. const unregisterTokenRaw = arguments[2]; - let unregisterToken: JSReceiver|Undefined; - typeswitch (unregisterTokenRaw) { - case (Undefined): { - unregisterToken = Undefined; - } - case (unregisterTokenObj: JSReceiver): { - unregisterToken = unregisterTokenObj; - } - case (JSAny): deferred { - ThrowTypeError( - MessageTemplate::kWeakRefsUnregisterTokenMustBeObject, - unregisterTokenRaw); - } + let unregisterToken: JSReceiver|Undefined|Symbol; + + if (IsUndefined(unregisterTokenRaw)) { + unregisterToken = Undefined; + } else { + GotoIfCannotBeWeakKey(unregisterTokenRaw) + otherwise ThrowTypeError( + MessageTemplate::kInvalidWeakRefsUnregisterToken, unregisterTokenRaw); + unregisterToken = UnsafeCast<(JSReceiver | Symbol)>(unregisterTokenRaw); } + // 6. Let cell be the Record { [[WeakRefTarget]] : target, [[HeldValue]]: // heldValue, [[UnregisterToken]]: unregisterToken }. // Allocate the WeakCell object in the old space, because 1) WeakCell weakness diff --git a/deps/v8/src/builtins/weak-ref.tq b/deps/v8/src/builtins/weak-ref.tq index 56d3fc1c4314bf..68f56fc111ad36 100644 --- a/deps/v8/src/builtins/weak-ref.tq +++ b/deps/v8/src/builtins/weak-ref.tq @@ -2,15 +2,18 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include 'src/builtins/builtins-collections-gen.h' + namespace runtime { -extern runtime JSWeakRefAddToKeptObjects(implicit context: Context)(JSReceiver): - void; +extern runtime JSWeakRefAddToKeptObjects(implicit context: Context)( + JSReceiver | Symbol): void; } // namespace runtime namespace weakref { +// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-weak-ref-target transitioning javascript builtin WeakRefConstructor( js-implicit context: NativeContext, receiver: JSAny, newTarget: JSAny, @@ -19,15 +22,17 @@ WeakRefConstructor( if (newTarget == Undefined) { ThrowTypeError(MessageTemplate::kConstructorNotFunction, 'WeakRef'); } - // 2. If Type(target) is not Object, throw a TypeError exception. - const weakTarget = Cast(weakTarget) otherwise - ThrowTypeError( - MessageTemplate::kWeakRefsWeakRefConstructorTargetMustBeObject); + + // 2. If CanBeHeldWeakly(weakTarget) is false, throw a TypeError exception. + GotoIfCannotBeWeakKey(weakTarget) otherwise ThrowTypeError( + MessageTemplate::kInvalidWeakRefsWeakRefConstructorTarget); + // 3. Let weakRef be ? OrdinaryCreateFromConstructor(NewTarget, // "%WeakRefPrototype%", « [[WeakRefTarget]] »). const map = GetDerivedMap(target, UnsafeCast(newTarget)); const weakRef = UnsafeCast(AllocateFastOrSlowJSObjectFromMap(map)); // 4. Perfom ! AddToKeptObjects(target). + const weakTarget = UnsafeCast<(JSReceiver | Symbol)>(weakTarget); runtime::JSWeakRefAddToKeptObjects(weakTarget); // 5. Set weakRef.[[WeakRefTarget]] to target. weakRef.target = weakTarget; @@ -52,7 +57,8 @@ WeakRefDeref(js-implicit context: NativeContext, receiver: JSAny)(): JSAny { if (target != Undefined) { // JSWeakRefAddToKeptObjects might allocate and cause a GC, but it // won't clear `target` since we hold it here on the stack. - runtime::JSWeakRefAddToKeptObjects(UnsafeCast(target)); + runtime::JSWeakRefAddToKeptObjects( + UnsafeCast<(JSReceiver | Symbol)>(target)); } return target; } diff --git a/deps/v8/src/codegen/code-stub-assembler.h b/deps/v8/src/codegen/code-stub-assembler.h index bccdc34b746bdb..a1c8816c54b90b 100644 --- a/deps/v8/src/codegen/code-stub-assembler.h +++ b/deps/v8/src/codegen/code-stub-assembler.h @@ -2679,6 +2679,12 @@ class V8_EXPORT_PRIVATE CodeStubAssembler ExternalReference::address_of_shared_string_table_flag()); } + TNode HasHarmonySymbolAsWeakmapKeyFlag() { + return LoadRuntimeFlag( + ExternalReference:: + address_of_FLAG_harmony_symbol_as_weakmap_key()); + } + // True iff |object| is a Smi or a HeapNumber or a BigInt. TNode IsNumeric(TNode object); diff --git a/deps/v8/src/codegen/external-reference.cc b/deps/v8/src/codegen/external-reference.cc index 6d206663ba86b7..686a91da6dc4b5 100644 --- a/deps/v8/src/codegen/external-reference.cc +++ b/deps/v8/src/codegen/external-reference.cc @@ -550,6 +550,11 @@ ExternalReference::address_of_mock_arraybuffer_allocator_flag() { return ExternalReference(&FLAG_mock_arraybuffer_allocator); } +ExternalReference +ExternalReference::address_of_FLAG_harmony_symbol_as_weakmap_key() { + return ExternalReference(&FLAG_harmony_symbol_as_weakmap_key); +} + ExternalReference ExternalReference::address_of_builtin_subclassing_flag() { return ExternalReference(&FLAG_builtin_subclassing); } diff --git a/deps/v8/src/codegen/external-reference.h b/deps/v8/src/codegen/external-reference.h index 7715aa6a816f33..636c528dabfe41 100644 --- a/deps/v8/src/codegen/external-reference.h +++ b/deps/v8/src/codegen/external-reference.h @@ -92,6 +92,8 @@ class StatsCounter; #define EXTERNAL_REFERENCE_LIST(V) \ V(abort_with_reason, "abort_with_reason") \ + V(address_of_FLAG_harmony_symbol_as_weakmap_key, \ + "FLAG_harmony_symbol_as_weakmap_key") \ V(address_of_builtin_subclassing_flag, "FLAG_builtin_subclassing") \ V(address_of_double_abs_constant, "double_absolute_constant") \ V(address_of_double_neg_constant, "double_negate_constant") \ diff --git a/deps/v8/src/common/message-template.h b/deps/v8/src/common/message-template.h index 22fde49ffd1673..a37d278d6e17c9 100644 --- a/deps/v8/src/common/message-template.h +++ b/deps/v8/src/common/message-template.h @@ -633,17 +633,15 @@ namespace internal { T(TraceEventPhaseError, "Trace event phase must be a number.") \ T(TraceEventIDError, "Trace event id must be a number.") \ /* Weak refs */ \ - T(WeakRefsUnregisterTokenMustBeObject, \ - "unregisterToken ('%') must be an object") \ + T(InvalidWeakRefsUnregisterToken, "Invalid unregisterToken ('%')") \ T(WeakRefsCleanupMustBeCallable, \ "FinalizationRegistry: cleanup must be callable") \ - T(WeakRefsRegisterTargetMustBeObject, \ - "FinalizationRegistry.prototype.register: target must be an object") \ + T(InvalidWeakRefsRegisterTarget, \ + "FinalizationRegistry.prototype.register: invalid target") \ T(WeakRefsRegisterTargetAndHoldingsMustNotBeSame, \ "FinalizationRegistry.prototype.register: target and holdings must not " \ "be same") \ - T(WeakRefsWeakRefConstructorTargetMustBeObject, \ - "WeakRef: target must be an object") \ + T(InvalidWeakRefsWeakRefConstructorTarget, "WeakRef: invalid target") \ T(OptionalChainingNoNew, "Invalid optional chain from new expression") \ T(OptionalChainingNoSuper, "Invalid optional chain from super property") \ T(OptionalChainingNoTemplate, "Invalid tagged template on optional chain") \ diff --git a/deps/v8/src/diagnostics/objects-debug.cc b/deps/v8/src/diagnostics/objects-debug.cc index 42e9dff7e363fd..e4cb23da28934b 100644 --- a/deps/v8/src/diagnostics/objects-debug.cc +++ b/deps/v8/src/diagnostics/objects-debug.cc @@ -1240,7 +1240,9 @@ void JSSharedStruct::JSSharedStructVerify(Isolate* isolate) { void WeakCell::WeakCellVerify(Isolate* isolate) { CHECK(IsWeakCell()); - CHECK(target().IsJSReceiver() || target().IsUndefined(isolate)); + CHECK(target().IsJSReceiver() || target().IsUndefined(isolate) || + (target().IsSymbol() && + !Symbol::cast(target()).is_in_public_symbol_table())); CHECK(prev().IsWeakCell() || prev().IsUndefined(isolate)); if (prev().IsWeakCell()) { @@ -1268,7 +1270,9 @@ void WeakCell::WeakCellVerify(Isolate* isolate) { void JSWeakRef::JSWeakRefVerify(Isolate* isolate) { CHECK(IsJSWeakRef()); JSObjectVerify(isolate); - CHECK(target().IsUndefined(isolate) || target().IsJSReceiver()); + CHECK(target().IsUndefined(isolate) || target().IsJSReceiver() || + (target().IsSymbol() && + !Symbol::cast(target()).is_in_public_symbol_table())); } void JSFinalizationRegistry::JSFinalizationRegistryVerify(Isolate* isolate) { diff --git a/deps/v8/src/flags/flag-definitions.h b/deps/v8/src/flags/flag-definitions.h index f02ef309d69e45..55e380a49e7ef5 100644 --- a/deps/v8/src/flags/flag-definitions.h +++ b/deps/v8/src/flags/flag-definitions.h @@ -307,7 +307,8 @@ DEFINE_BOOL(harmony_shipping, true, "enable all shipped harmony features") "harmony ResizableArrayBuffer / GrowableSharedArrayBuffer") \ V(harmony_temporal, "Temporal") \ V(harmony_shadow_realm, "harmony ShadowRealm") \ - V(harmony_struct, "harmony structs and shared structs") + V(harmony_struct, "harmony structs and shared structs") \ + V(harmony_symbol_as_weakmap_key, "harmony symbols as weakmap keys") #ifdef V8_INTL_SUPPORT #define HARMONY_INPROGRESS(V) \ diff --git a/deps/v8/src/heap/heap.cc b/deps/v8/src/heap/heap.cc index 02eefd9e4fdc78..53871476c72905 100644 --- a/deps/v8/src/heap/heap.cc +++ b/deps/v8/src/heap/heap.cc @@ -6924,7 +6924,7 @@ void Heap::RemoveDirtyFinalizationRegistriesOnContext(NativeContext context) { set_dirty_js_finalization_registries_list_tail(prev); } -void Heap::KeepDuringJob(Handle target) { +void Heap::KeepDuringJob(Handle target) { DCHECK(weak_refs_keep_during_job().IsUndefined() || weak_refs_keep_during_job().IsOrderedHashSet()); Handle table; diff --git a/deps/v8/src/heap/heap.h b/deps/v8/src/heap/heap.h index b0a8757ca96ab6..a3817f90ed3014 100644 --- a/deps/v8/src/heap/heap.h +++ b/deps/v8/src/heap/heap.h @@ -955,7 +955,7 @@ class Heap { return is_finalization_registry_cleanup_task_posted_; } - V8_EXPORT_PRIVATE void KeepDuringJob(Handle target); + V8_EXPORT_PRIVATE void KeepDuringJob(Handle target); void ClearKeptObjects(); // =========================================================================== diff --git a/deps/v8/src/init/bootstrapper.cc b/deps/v8/src/init/bootstrapper.cc index d4c4122c22f4d7..fd75d5faa317de 100644 --- a/deps/v8/src/init/bootstrapper.cc +++ b/deps/v8/src/init/bootstrapper.cc @@ -4484,6 +4484,7 @@ void Genesis::InitializeConsole(Handle extras_binding) { EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_import_assertions) EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_private_brand_checks) EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_class_static_blocks) +EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_symbol_as_weakmap_key) EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_error_cause) #ifdef V8_INTL_SUPPORT diff --git a/deps/v8/src/objects/js-weak-refs-inl.h b/deps/v8/src/objects/js-weak-refs-inl.h index 76e6e075e5ded6..15f7d36b6fe72a 100644 --- a/deps/v8/src/objects/js-weak-refs-inl.h +++ b/deps/v8/src/objects/js-weak-refs-inl.h @@ -5,10 +5,9 @@ #ifndef V8_OBJECTS_JS_WEAK_REFS_INL_H_ #define V8_OBJECTS_JS_WEAK_REFS_INL_H_ -#include "src/objects/js-weak-refs.h" - #include "src/api/api-inl.h" #include "src/heap/heap-write-barrier-inl.h" +#include "src/objects/js-weak-refs.h" #include "src/objects/smi-inl.h" // Has to be the last include (doesn't have include guards): @@ -55,7 +54,7 @@ void JSFinalizationRegistry::RegisterWeakCellWithUnregisterToken( bool JSFinalizationRegistry::Unregister( Handle finalization_registry, - Handle unregister_token, Isolate* isolate) { + Handle unregister_token, Isolate* isolate) { // Iterate through the doubly linked list of WeakCells associated with the // key. Each WeakCell will be in the "active_cells" or "cleared_cells" list of // its FinalizationRegistry; remove it from there. @@ -66,7 +65,7 @@ bool JSFinalizationRegistry::Unregister( template bool JSFinalizationRegistry::RemoveUnregisterToken( - JSReceiver unregister_token, Isolate* isolate, + HeapObject unregister_token, Isolate* isolate, RemoveUnregisterTokenMode removal_mode, GCNotifyUpdatedSlotCallback gc_notify_updated_slot) { // This method is called from both FinalizationRegistry#unregister and for @@ -171,7 +170,9 @@ 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()); + DCHECK(target().IsJSReceiver() || + (target().IsSymbol() && + !Symbol::cast(target()).is_in_public_symbol_table())); set_target(ReadOnlyRoots(isolate).undefined_value()); JSFinalizationRegistry fr = diff --git a/deps/v8/src/objects/js-weak-refs.h b/deps/v8/src/objects/js-weak-refs.h index f678234ff81afc..64ff9573f6b2df 100644 --- a/deps/v8/src/objects/js-weak-refs.h +++ b/deps/v8/src/objects/js-weak-refs.h @@ -37,7 +37,7 @@ class JSFinalizationRegistry Handle weak_cell, Isolate* isolate); inline static bool Unregister( Handle finalization_registry, - Handle unregister_token, Isolate* isolate); + Handle unregister_token, Isolate* isolate); // RemoveUnregisterToken is called from both Unregister and during GC. Since // it modifies slots in key_map and WeakCells and the normal write barrier is @@ -49,7 +49,7 @@ class JSFinalizationRegistry }; template inline bool RemoveUnregisterToken( - JSReceiver unregister_token, Isolate* isolate, + HeapObject unregister_token, Isolate* isolate, RemoveUnregisterTokenMode removal_mode, GCNotifyUpdatedSlotCallback gc_notify_updated_slot); diff --git a/deps/v8/src/objects/js-weak-refs.tq b/deps/v8/src/objects/js-weak-refs.tq index c687ab50016776..c9fa8ee30bdb57 100644 --- a/deps/v8/src/objects/js-weak-refs.tq +++ b/deps/v8/src/objects/js-weak-refs.tq @@ -20,8 +20,8 @@ extern class JSFinalizationRegistry extends JSObject { extern class WeakCell extends HeapObject { finalization_registry: Undefined|JSFinalizationRegistry; - target: Undefined|JSReceiver; - unregister_token: Undefined|JSReceiver; + target: Undefined|JSReceiver|Symbol; + unregister_token: Undefined|JSReceiver|Symbol; holdings: JSAny; // For storing doubly linked lists of WeakCells in JSFinalizationRegistry's @@ -38,5 +38,6 @@ extern class WeakCell extends HeapObject { key_list_prev: Undefined|WeakCell; key_list_next: Undefined|WeakCell; } - -extern class JSWeakRef extends JSObject { target: Undefined|JSReceiver; } +extern class JSWeakRef extends JSObject { + target: Undefined|JSReceiver|Symbol; +} diff --git a/deps/v8/src/runtime/runtime-weak-refs.cc b/deps/v8/src/runtime/runtime-weak-refs.cc index f3c6f63ebcb6d7..db3611dbb7d4eb 100644 --- a/deps/v8/src/runtime/runtime-weak-refs.cc +++ b/deps/v8/src/runtime/runtime-weak-refs.cc @@ -2,10 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "src/runtime/runtime-utils.h" - #include "src/execution/arguments-inl.h" #include "src/objects/js-weak-refs-inl.h" +#include "src/runtime/runtime-utils.h" namespace v8 { namespace internal { @@ -44,7 +43,14 @@ RUNTIME_FUNCTION( RUNTIME_FUNCTION(Runtime_JSWeakRefAddToKeptObjects) { HandleScope scope(isolate); DCHECK_EQ(1, args.length()); - Handle object = args.at(0); + 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()); + } isolate->heap()->KeepDuringJob(object); diff --git a/deps/v8/test/message/fail/weak-refs-register1.out b/deps/v8/test/message/fail/weak-refs-register1.out index aa4cbc2fa22e1e..7e7bf12791be2d 100644 --- a/deps/v8/test/message/fail/weak-refs-register1.out +++ b/deps/v8/test/message/fail/weak-refs-register1.out @@ -1,6 +1,6 @@ -*%(basename)s:*: TypeError: FinalizationRegistry.prototype.register: target must be an object +*%(basename)s:*: TypeError: FinalizationRegistry.prototype.register: invalid target fg.register(1); ^ -TypeError: FinalizationRegistry.prototype.register: target must be an object +TypeError: FinalizationRegistry.prototype.register: invalid target at FinalizationRegistry.register () at *%(basename)s:*:4 diff --git a/deps/v8/test/message/fail/weak-refs-unregister.out b/deps/v8/test/message/fail/weak-refs-unregister.out index 52945869838778..a6a66ea709d747 100644 --- a/deps/v8/test/message/fail/weak-refs-unregister.out +++ b/deps/v8/test/message/fail/weak-refs-unregister.out @@ -1,6 +1,6 @@ -*%(basename)s:*: TypeError: unregisterToken ('1') must be an object +*%(basename)s:*: TypeError: Invalid unregisterToken ('1') fg.unregister(1); ^ -TypeError: unregisterToken ('1') must be an object +TypeError: Invalid unregisterToken ('1') at FinalizationRegistry.unregister () at *%(basename)s:*:4 diff --git a/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js b/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js new file mode 100644 index 00000000000000..0e97ea1accca58 --- /dev/null +++ b/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js @@ -0,0 +1,99 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --harmony-symbol-as-weakmap-key --expose-gc + +(function TestWeakMapWithNonRegisteredSymbolKey() { + const key = Symbol('123'); + const value = 1; + const map = new WeakMap(); + assertFalse(map.has(key)); + assertSame(undefined, map.get(key)); + assertFalse(map.delete(key)); + assertSame(map, map.set(key, value)); + assertSame(value, map.get(key)); + assertTrue(map.has(key)); + assertTrue(map.delete(key)); + assertFalse(map.has(key)); + assertSame(undefined, map.get(key)); + assertFalse(map.delete(key)); + assertFalse(map.has(key)); + assertSame(undefined, map.get(key)); +})(); + +(function TestWeakMapWithNonRegisteredSymbolKeyGC() { + const map = new WeakMap(); + + const outerKey = Symbol('234'); + const outerValue = 1; + map.set(outerKey, outerValue); + { + const innerKey = Symbol('123'); + const innerValue = 1; + map.set(innerKey, innerValue); + assertTrue(map.has(innerKey)); + assertSame(innerValue, map.get(innerKey)); + } + gc(); + assertTrue(map.has(outerKey)); + assertSame(outerValue, map.get(outerKey)); +})(); + +(function TestWeakMapWithRegisteredSymbolKey() { + const key = Symbol.for('123'); + const value = 1; + const map = new WeakMap(); + assertFalse(map.has(key)); + assertSame(undefined, map.get(key)); + assertFalse(map.delete(key)); + assertThrows(() => { + map.set(key, value); + }, TypeError, 'Invalid value used as weak map key'); + assertFalse(map.has(key)); + assertSame(undefined, map.get(key)); + assertFalse(map.delete(key)); + assertFalse(map.has(key)); + assertSame(undefined, map.get(key)); +})(); + +(function TestWeakSetWithNonRegisteredSymbolKey() { + const key = Symbol('123'); + const set = new WeakSet(); + assertFalse(set.has(key)); + assertFalse(set.delete(key)); + assertSame(set, set.add(key)); + assertTrue(set.has(key)); + assertTrue(set.delete(key)); + assertFalse(set.has(key)); + assertFalse(set.delete(key)); + assertFalse(set.has(key)); +})(); + +(function TestWeakSetWithNonRegisteredSymbolKeyGC() { + const set = new WeakSet(); + const outerKey = Symbol('234'); + set.add(outerKey); + { + const innerKey = Symbol('123'); + set.add(innerKey); + assertTrue(set.has(innerKey)); + } + gc(); + assertTrue(set.has(outerKey)); +})(); + +(function TestWeakSetWithRegisteredSymbolKey() { + const key = Symbol.for('123'); + const set = new WeakSet(); + assertFalse(set.has(key)); + assertFalse(set.delete(key)); + + assertThrows(() => { + assertSame(set, set.add(key)); + }, TypeError, 'Invalid value used in weak set'); + + assertFalse(set.has(key)); + assertFalse(set.delete(key)); + assertFalse(set.has(key)); +})(); diff --git a/deps/v8/test/mjsunit/harmony/weakrefs/basics.js b/deps/v8/test/mjsunit/harmony/weakrefs/basics.js index f879df9a2a63b0..7628c641bc7984 100644 --- a/deps/v8/test/mjsunit/harmony/weakrefs/basics.js +++ b/deps/v8/test/mjsunit/harmony/weakrefs/basics.js @@ -47,7 +47,7 @@ (function TestRegisterWithNonObjectTarget() { let fg = new FinalizationRegistry(() => {}); - let message = "FinalizationRegistry.prototype.register: target must be an object"; + let message = "FinalizationRegistry.prototype.register: invalid target"; assertThrows(() => fg.register(1, "holdings"), TypeError, message); assertThrows(() => fg.register(false, "holdings"), TypeError, message); assertThrows(() => fg.register("foo", "holdings"), TypeError, message); @@ -116,7 +116,7 @@ })(); (function TestWeakRefConstructorWithNonObject() { - let message = "WeakRef: target must be an object"; + let message = "WeakRef: invalid target"; assertThrows(() => new WeakRef(), TypeError, message); assertThrows(() => new WeakRef(1), TypeError, message); assertThrows(() => new WeakRef(false), TypeError, message); diff --git a/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target-gc.js b/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target-gc.js new file mode 100644 index 00000000000000..561bf4f058bdd7 --- /dev/null +++ b/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target-gc.js @@ -0,0 +1,39 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --harmony-symbol-as-weakmap-key --expose-gc --noincremental-marking + +(function TestWeakRefWithSymbolGC() { + let weakRef; + { + const innerKey = Symbol('123'); + weakRef = new WeakRef(innerKey); + } + // Since the WeakRef was created during this turn, it is not cleared by GC. + gc(); + assertNotEquals(undefined, weakRef.deref()); + // Next task. + setTimeout(() => { + gc(); + assertEquals(undefined, weakRef.deref()); + }, 0); +})(); + +(function TestFinalizationRegistryWithSymbolGC() { + let cleanUpCalled = false; + const fg = new FinalizationRegistry((target) => { + assertEquals('123', target); + cleanUpCalled = true; + }); + (function () { + const innerKey = Symbol('123'); + fg.register(innerKey, '123'); + })(); + gc(); + assertFalse(cleanUpCalled); + // Check that cleanup callback was called in a follow up task. + setTimeout(() => { + assertTrue(cleanUpCalled); + }, 0); +})(); diff --git a/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target.js b/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target.js new file mode 100644 index 00000000000000..1dc874ed83b3da --- /dev/null +++ b/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target.js @@ -0,0 +1,41 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --harmony-symbol-as-weakmap-key + +(function TestRegisterWithSymbolTarget() { + const fg = new FinalizationRegistry(() => { }); + fg.register(Symbol('123'), 'holdings'); + // Registered symbols cannot be the target. + assertThrows(() => fg.register(Symbol.for('123'), 'holdings'), TypeError); +})(); + +(function TestRegisterWithSymbolUnregisterToken() { + const fg = new FinalizationRegistry(() => { }); + fg.register({}, 'holdings', Symbol('123')); + // Registered symbols cannot be the unregister token. + assertThrows(() => fg.register({}, 'holdings', Symbol.for('123')), TypeError); +})(); + +(function TestRegisterSymbolAndHoldingsSameValue() { + const fg = new FinalizationRegistry(() => {}); + const obj = Symbol('123'); + // SameValue(target, holdings) not ok. + assertThrows(() => fg.register(obj, obj), TypeError); + const holdings = {a: 1}; + fg.register(obj, holdings); +})(); + +(function TestUnregisterWithSymbolUnregisterToken() { + const fg = new FinalizationRegistry(() => {}); + fg.unregister(Symbol('123')); + // Registered symbols cannot be the unregister token. + assertThrows(() => fg.unregister(Symbol.for('123')), TypeError); +})(); + +(function TestWeakRefConstructorWithSymbol() { + new WeakRef(Symbol('123')); + // Registered symbols cannot be the WeakRef target. + assertThrows(() => new WeakRef(Symbol.for('123')), TypeError); +})(); From 990caf332dfa4529c95dcff8ace137f359aa76d8 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 19:41:26 +0100 Subject: [PATCH 02/22] deps: V8: cherry-pick 7f5daed62d47 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [symbol-as-weakmap-key] Add tests to check weak collection size ... after gc. This CL also adds a runtime test function GetWeakCollectionSize to get the weak collection size. Bug: v8:12947 Change-Id: I4aff39165a54b63b3d690bfea71c2a439da01d00 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3905071 Reviewed-by: Marja Hölttä Commit-Queue: 王澳 Cr-Commit-Position: refs/heads/main@{#83464} Refs: https://github.com/v8/v8/commit/7f5daed62d47d1904bba9ff45443d7cab209b21a --- deps/v8/src/runtime/runtime-test.cc | 10 ++++++++++ deps/v8/src/runtime/runtime.h | 1 + .../test/mjsunit/harmony/symbol-as-weakmap-key.js | 14 ++++++++------ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/deps/v8/src/runtime/runtime-test.cc b/deps/v8/src/runtime/runtime-test.cc index fcaada5711d487..7dcc05ab485408 100644 --- a/deps/v8/src/runtime/runtime-test.cc +++ b/deps/v8/src/runtime/runtime-test.cc @@ -27,6 +27,7 @@ #include "src/logging/counters.h" #include "src/objects/heap-object-inl.h" #include "src/objects/js-array-inl.h" +#include "src/objects/js-collection-inl.h" #include "src/objects/js-function-inl.h" #include "src/objects/js-regexp-inl.h" #include "src/objects/managed-inl.h" @@ -1730,5 +1731,14 @@ RUNTIME_FUNCTION(Runtime_SharedGC) { return ReadOnlyRoots(isolate).undefined_value(); } +RUNTIME_FUNCTION(Runtime_GetWeakCollectionSize) { + HandleScope scope(isolate); + DCHECK_EQ(1, args.length()); + Handle collection = args.at(0); + + return Smi::FromInt( + EphemeronHashTable::cast(collection->table()).NumberOfElements()); +} + } // namespace internal } // namespace v8 diff --git a/deps/v8/src/runtime/runtime.h b/deps/v8/src/runtime/runtime.h index 877b277a5e26cb..d0f81cf2f7672a 100644 --- a/deps/v8/src/runtime/runtime.h +++ b/deps/v8/src/runtime/runtime.h @@ -498,6 +498,7 @@ namespace internal { F(GetInitializerFunction, 1, 1) \ F(GetOptimizationStatus, 1, 1) \ F(GetUndetectable, 0, 1) \ + F(GetWeakCollectionSize, 1, 1) \ F(GlobalPrint, 1, 1) \ F(HasDictionaryElements, 1, 1) \ F(HasDoubleElements, 1, 1) \ 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 0e97ea1accca58..f644fccb00022d 100644 --- a/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js +++ b/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --harmony-symbol-as-weakmap-key --expose-gc +// Flags: --harmony-symbol-as-weakmap-key --expose-gc --allow-natives-syntax --noincremental-marking (function TestWeakMapWithNonRegisteredSymbolKey() { const key = Symbol('123'); @@ -28,16 +28,17 @@ const outerKey = Symbol('234'); const outerValue = 1; map.set(outerKey, outerValue); - { + (function () { const innerKey = Symbol('123'); const innerValue = 1; map.set(innerKey, innerValue); assertTrue(map.has(innerKey)); assertSame(innerValue, map.get(innerKey)); - } + })(); gc(); assertTrue(map.has(outerKey)); assertSame(outerValue, map.get(outerKey)); + assertEquals(1, %GetWeakCollectionSize(map)); })(); (function TestWeakMapWithRegisteredSymbolKey() { @@ -74,13 +75,14 @@ const set = new WeakSet(); const outerKey = Symbol('234'); set.add(outerKey); - { + (function () { const innerKey = Symbol('123'); set.add(innerKey); assertTrue(set.has(innerKey)); - } - gc(); + })(); assertTrue(set.has(outerKey)); + gc(); + assertEquals(1, %GetWeakCollectionSize(set)); })(); (function TestWeakSetWithRegisteredSymbolKey() { From 8ade4aa8ae03b2c7d01a8764167c670c8fc93c16 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 19:44:47 +0100 Subject: [PATCH 03/22] deps: V8: cherry-pick 9a98f96b6d68 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [symbol-as-weakmap-key] Stage the feature Bug: v8:12947 Change-Id: I0a151a6b301ee93675cc9f87a4fa24cb1be76462 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3928061 Auto-Submit: Shu-yu Guo Commit-Queue: Marja Hölttä Reviewed-by: Marja Hölttä Cr-Commit-Position: refs/heads/main@{#83483} Refs: https://github.com/v8/v8/commit/9a98f96b6d68c203a773164df8d4abedd31991ac --- deps/v8/src/flags/flag-definitions.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/deps/v8/src/flags/flag-definitions.h b/deps/v8/src/flags/flag-definitions.h index 55e380a49e7ef5..4627f96c07ed60 100644 --- a/deps/v8/src/flags/flag-definitions.h +++ b/deps/v8/src/flags/flag-definitions.h @@ -307,8 +307,7 @@ DEFINE_BOOL(harmony_shipping, true, "enable all shipped harmony features") "harmony ResizableArrayBuffer / GrowableSharedArrayBuffer") \ V(harmony_temporal, "Temporal") \ V(harmony_shadow_realm, "harmony ShadowRealm") \ - V(harmony_struct, "harmony structs and shared structs") \ - V(harmony_symbol_as_weakmap_key, "harmony symbols as weakmap keys") + V(harmony_struct, "harmony structs and shared structs") #ifdef V8_INTL_SUPPORT #define HARMONY_INPROGRESS(V) \ @@ -319,8 +318,9 @@ DEFINE_BOOL(harmony_shipping, true, "enable all shipped harmony features") #endif // Features that are complete (but still behind the --harmony flag). -#define HARMONY_STAGED_BASE(V) \ - V(harmony_array_grouping, "harmony array grouping") +#define HARMONY_STAGED_BASE(V) \ + V(harmony_array_grouping, "harmony array grouping") \ + V(harmony_symbol_as_weakmap_key, "harmony symbols as weakmap keys") #ifdef V8_INTL_SUPPORT #define HARMONY_STAGED(V) \ From 80f54b7b306db3560a96d57c62d4ab6faa6a975a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 19:52:25 +0100 Subject: [PATCH 04/22] 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 --- .../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 ++++ 12 files changed, 65 insertions(+), 61 deletions(-) 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); +})(); From 213e73b105ecfbc8aa07175045beced5fe9dac8c Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 20:00:18 +0100 Subject: [PATCH 05/22] deps: V8: cherry-pick 3dd9576ce336 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [inspector] Support Symbols in EntryPreview The Symbols-as-WeakMap-keys proposal allows non-Symbol.for Symbol values in weak collections, which means it can show in EntryPreviews. Also apparently Symbols in regular Maps and Sets were also unsupported. Bug: v8:13350, v8:12947 Change-Id: Ib10476fa2f3c7f59af67933f0bf61640be1bbd97 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3930037 Reviewed-by: Benedikt Meurer Reviewed-by: Simon Zünd Commit-Queue: Shu-yu Guo Cr-Commit-Position: refs/heads/main@{#83518} Refs: https://github.com/v8/v8/commit/3dd9576ce33683b261da944ddba460b507ca5aa1 --- deps/v8/src/inspector/value-mirror.cc | 12 +++ ...t-preview-internal-properties-expected.txt | 82 +++++++++++++++++++ .../object-preview-internal-properties.js | 11 ++- 3 files changed, 104 insertions(+), 1 deletion(-) diff --git a/deps/v8/src/inspector/value-mirror.cc b/deps/v8/src/inspector/value-mirror.cc index c2fa9b46cc0067..a345a8db38675e 100644 --- a/deps/v8/src/inspector/value-mirror.cc +++ b/deps/v8/src/inspector/value-mirror.cc @@ -656,6 +656,18 @@ class SymbolMirror final : public ValueMirror { .build(); } + void buildEntryPreview( + v8::Local context, int* nameLimit, int* indexLimit, + std::unique_ptr* preview) const override { + *preview = + ObjectPreview::create() + .setType(RemoteObject::TypeEnum::Symbol) + .setDescription(descriptionForSymbol(context, m_symbol)) + .setOverflow(false) + .setProperties(std::make_unique>()) + .build(); + } + v8::Local v8Value() const override { return m_symbol; } protocol::Response buildWebDriverValue( diff --git a/deps/v8/test/inspector/debugger/object-preview-internal-properties-expected.txt b/deps/v8/test/inspector/debugger/object-preview-internal-properties-expected.txt index 56f992237a2ebd..1bdef8231b2523 100644 --- a/deps/v8/test/inspector/debugger/object-preview-internal-properties-expected.txt +++ b/deps/v8/test/inspector/debugger/object-preview-internal-properties-expected.txt @@ -159,6 +159,88 @@ expression: new WeakSet([{}]) ] +Running test: symbolsAsKeysInEntries +expression: new Map([[Symbol('key1'), 1]]) +{ + name : size + type : number + value : 1 +} +[[Entries]]: +[ + [0] : { + key : { + description : Symbol(key1) + overflow : false + properties : [ + ] + type : symbol + } + value : { + description : 1 + overflow : false + properties : [ + ] + type : number + } + } +] + +expression: new Set([Symbol('key2')]) +{ + name : size + type : number + value : 1 +} +[[Entries]]: +[ + [0] : { + value : { + description : Symbol(key2) + overflow : false + properties : [ + ] + type : symbol + } + } +] + +expression: new WeakMap([[Symbol('key3'), 2]]) +[[Entries]]: +[ + [0] : { + key : { + description : Symbol(key3) + overflow : false + properties : [ + ] + type : symbol + } + value : { + description : 2 + overflow : false + properties : [ + ] + type : number + } + } +] + +expression: new WeakSet([Symbol('key4')]) +[[Entries]]: +[ + [0] : { + value : { + description : Symbol(key4) + overflow : false + properties : [ + ] + type : symbol + } + } +] + + Running test: iteratorObject expression: (new Map([[1,2]])).entries() [[Entries]]: diff --git a/deps/v8/test/inspector/debugger/object-preview-internal-properties.js b/deps/v8/test/inspector/debugger/object-preview-internal-properties.js index f542683aa49159..d2b3a7816339e9 100644 --- a/deps/v8/test/inspector/debugger/object-preview-internal-properties.js +++ b/deps/v8/test/inspector/debugger/object-preview-internal-properties.js @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // -// Flags: --harmony-class-fields +// Flags: --harmony-symbol-as-weakmap-key let {session, contextGroup, Protocol} = InspectorTest.start("Check internal properties reported in object preview."); @@ -45,6 +45,15 @@ InspectorTest.runTestSuite([ .then(next); }, + function symbolsAsKeysInEntries(next) + { + checkExpression("new Map([[Symbol('key1'), 1]])") + .then(() => checkExpression("new Set([Symbol('key2')])")) + .then(() => checkExpression("new WeakMap([[Symbol('key3'), 2]])")) + .then(() => checkExpression("new WeakSet([Symbol('key4')])")) + .then(next); + }, + function iteratorObject(next) { checkExpression("(new Map([[1,2]])).entries()") From 0f33b00e86f1ecc9b53b86d4655946c02c3aab7c Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 20:01:24 +0100 Subject: [PATCH 06/22] deps: V8: cherry-pick 1fada6b36f8d Original commit message: [symbol-as-weakmap-key] Fix DCHECKs when clearing JS weakrefs Bug: chromium:1372500, v8:12947 Fixed: chromium:1372500 Change-Id: Id6330de5886e4ea72544b307c358e2190ea47d9c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3942586 Reviewed-by: Anton Bikineev Commit-Queue: Shu-yu Guo Cr-Commit-Position: refs/heads/main@{#83632} Refs: https://github.com/v8/v8/commit/1fada6b36f8df9c34f0f841e4ad51892e1984603 --- deps/v8/src/heap/mark-compact.cc | 5 +++-- .../harmony/regress/regress-crbug-1372500.js | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 deps/v8/test/mjsunit/harmony/regress/regress-crbug-1372500.js diff --git a/deps/v8/src/heap/mark-compact.cc b/deps/v8/src/heap/mark-compact.cc index ef0b67ca2b6274..0baf08ee74a35b 100644 --- a/deps/v8/src/heap/mark-compact.cc +++ b/deps/v8/src/heap/mark-compact.cc @@ -3027,7 +3027,7 @@ void MarkCompactCollector::ClearJSWeakRefs() { }; HeapObject target = HeapObject::cast(weak_cell.target()); if (!non_atomic_marking_state()->IsBlackOrGrey(target)) { - DCHECK(!target.IsUndefined()); + DCHECK(target.CanBeHeldWeakly()); // The value of the WeakCell is dead. JSFinalizationRegistry finalization_registry = JSFinalizationRegistry::cast(weak_cell.finalization_registry()); @@ -3049,6 +3049,7 @@ void MarkCompactCollector::ClearJSWeakRefs() { HeapObject unregister_token = weak_cell.unregister_token(); if (!non_atomic_marking_state()->IsBlackOrGrey(unregister_token)) { + DCHECK(unregister_token.CanBeHeldWeakly()); // The unregister token is dead. Remove any corresponding entries in the // key map. Multiple WeakCell with the same token will have all their // unregister_token field set to undefined when processing the first @@ -3057,7 +3058,7 @@ void MarkCompactCollector::ClearJSWeakRefs() { JSFinalizationRegistry finalization_registry = JSFinalizationRegistry::cast(weak_cell.finalization_registry()); finalization_registry.RemoveUnregisterToken( - JSReceiver::cast(unregister_token), isolate(), + unregister_token, isolate(), JSFinalizationRegistry::kKeepMatchedCellsInRegistry, gc_notify_updated_slot); } else { diff --git a/deps/v8/test/mjsunit/harmony/regress/regress-crbug-1372500.js b/deps/v8/test/mjsunit/harmony/regress/regress-crbug-1372500.js new file mode 100644 index 00000000000000..6264570fdd3bd7 --- /dev/null +++ b/deps/v8/test/mjsunit/harmony/regress/regress-crbug-1372500.js @@ -0,0 +1,14 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --harmony-symbol-as-weakmap-key --expose-gc + +// Register an object in a FinalizationRegistry with a Symbol as the unregister +// token. +let fr = new FinalizationRegistry(function () {}); +(function register() { + fr.register({}, "holdings", Symbol('unregisterToken')); +})(); +// The unregister token should be dead, trigger its collection. +gc(); From 59cf71f24b158d780924b99caf926787fed1d9a2 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 20:02:32 +0100 Subject: [PATCH 07/22] deps: V8: cherry-pick 705e374124ae Original commit message: [symbol-as-weakmap-key] Ship the proposal I2S with 3 LGTMs: https://groups.google.com/a/chromium.org/g/blink-dev/c/E6pDZP_TiBA/m/ZcXLwiz8AAAJ Bug: v8:12947 Change-Id: Ibce4abc8b0610afb2041d44cc9ed136db8b62c0d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4004610 Commit-Queue: Shu-yu Guo Reviewed-by: Camillo Bruni Cr-Commit-Position: refs/heads/main@{#84128} Refs: https://github.com/v8/v8/commit/705e374124ae026206e90519356cd980a1a515ba --- deps/v8/src/flags/flag-definitions.h | 6 +++--- deps/v8/test/mjsunit/es6/collections.js | 1 - deps/v8/test/mjsunit/harmony/weakrefs/basics.js | 3 --- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/deps/v8/src/flags/flag-definitions.h b/deps/v8/src/flags/flag-definitions.h index 4627f96c07ed60..b984651e9e3268 100644 --- a/deps/v8/src/flags/flag-definitions.h +++ b/deps/v8/src/flags/flag-definitions.h @@ -319,8 +319,7 @@ DEFINE_BOOL(harmony_shipping, true, "enable all shipped harmony features") // Features that are complete (but still behind the --harmony flag). #define HARMONY_STAGED_BASE(V) \ - V(harmony_array_grouping, "harmony array grouping") \ - V(harmony_symbol_as_weakmap_key, "harmony symbols as weakmap keys") + V(harmony_array_grouping, "harmony array grouping") #ifdef V8_INTL_SUPPORT #define HARMONY_STAGED(V) \ @@ -339,7 +338,8 @@ DEFINE_BOOL(harmony_shipping, true, "enable all shipped harmony features") V(harmony_error_cause, "harmony error cause property") \ V(harmony_object_has_own, "harmony Object.hasOwn") \ V(harmony_class_static_blocks, "harmony static initializer blocks") \ - V(harmony_array_find_last, "harmony array find last helpers") + V(harmony_array_find_last, "harmony array find last helpers") \ + V(harmony_symbol_as_weakmap_key, "harmony symbols as weakmap keys") #ifdef V8_INTL_SUPPORT #define HARMONY_SHIPPING(V) HARMONY_SHIPPING_BASE(V) diff --git a/deps/v8/test/mjsunit/es6/collections.js b/deps/v8/test/mjsunit/es6/collections.js index feae6294392365..2b608e1f5da10d 100644 --- a/deps/v8/test/mjsunit/es6/collections.js +++ b/deps/v8/test/mjsunit/es6/collections.js @@ -77,7 +77,6 @@ function TestInvalidCalls(m) { assertThrows(function () { m.set(null, 0) }, TypeError); assertThrows(function () { m.set(0, 0) }, TypeError); assertThrows(function () { m.set('a-key', 0) }, TypeError); - assertThrows(function () { m.set(Symbol(), 0) }, TypeError); } TestInvalidCalls(new WeakMap); diff --git a/deps/v8/test/mjsunit/harmony/weakrefs/basics.js b/deps/v8/test/mjsunit/harmony/weakrefs/basics.js index 7628c641bc7984..018dc6d5ab1def 100644 --- a/deps/v8/test/mjsunit/harmony/weakrefs/basics.js +++ b/deps/v8/test/mjsunit/harmony/weakrefs/basics.js @@ -51,7 +51,6 @@ assertThrows(() => fg.register(1, "holdings"), TypeError, message); assertThrows(() => fg.register(false, "holdings"), TypeError, message); assertThrows(() => fg.register("foo", "holdings"), TypeError, message); - assertThrows(() => fg.register(Symbol(), "holdings"), TypeError, message); assertThrows(() => fg.register(null, "holdings"), TypeError, message); assertThrows(() => fg.register(undefined, "holdings"), TypeError, message); })(); @@ -97,7 +96,6 @@ assertThrows(() => fg.unregister(1), TypeError); assertThrows(() => fg.unregister(1n), TypeError); assertThrows(() => fg.unregister('one'), TypeError); - assertThrows(() => fg.unregister(Symbol()), TypeError); assertThrows(() => fg.unregister(true), TypeError); assertThrows(() => fg.unregister(false), TypeError); assertThrows(() => fg.unregister(undefined), TypeError); @@ -121,7 +119,6 @@ assertThrows(() => new WeakRef(1), TypeError, message); assertThrows(() => new WeakRef(false), TypeError, message); assertThrows(() => new WeakRef("foo"), TypeError, message); - assertThrows(() => new WeakRef(Symbol()), TypeError, message); assertThrows(() => new WeakRef(null), TypeError, message); assertThrows(() => new WeakRef(undefined), TypeError, message); })(); From 86d8b58313c77bf4cb0d578ff9f001068b6bc367 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 1 Dec 2023 20:06:49 +0100 Subject: [PATCH 08/22] deps: V8: cherry-pick 71ff68830279 Original commit message: [symbol-as-weakmap-key] Remove --harmony-symbol-as-weakmap-key The proposal has shipped since Nov 2022. Bug: v8:12947 Change-Id: Ibb2e9cf1d22e4b754ec7625ae38175fc96007255 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4451066 Commit-Queue: Shu-yu Guo Reviewed-by: Adam Klein Cr-Commit-Position: refs/heads/main@{#87180} Refs: https://github.com/v8/v8/commit/71ff68830279b7ad6719db066b21f0489e871596 --- common.gypi | 2 +- deps/v8/src/builtins/builtins-collections-gen.cc | 2 +- deps/v8/src/builtins/builtins-weak-refs.cc | 10 +++------- deps/v8/src/builtins/finalization-registry.tq | 2 +- deps/v8/src/builtins/weak-ref.tq | 2 +- deps/v8/src/codegen/code-stub-assembler.h | 6 ------ deps/v8/src/codegen/external-reference.cc | 5 ----- deps/v8/src/codegen/external-reference.h | 2 -- deps/v8/src/flags/flag-definitions.h | 3 +-- deps/v8/src/init/bootstrapper.cc | 1 - deps/v8/src/objects/objects-inl.h | 7 ++----- .../debugger/object-preview-internal-properties.js | 2 -- .../mjsunit/harmony/regress/regress-crbug-1372500.js | 2 +- deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js | 2 +- .../harmony/weakrefs/symbol-as-weakref-target-gc.js | 2 +- .../harmony/weakrefs/symbol-as-weakref-target.js | 2 -- 16 files changed, 13 insertions(+), 39 deletions(-) diff --git a/common.gypi b/common.gypi index dba2631f218581..a58d48b0c5eb9a 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.28', + 'v8_embedder_string': '-node.29', ##### 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 19d77fa261ec61..d83038f7f65c40 100644 --- a/deps/v8/src/builtins/builtins-collections-gen.cc +++ b/deps/v8/src/builtins/builtins-collections-gen.cc @@ -399,6 +399,7 @@ TNode BaseCollectionsAssembler::EstimatedInitialSize( [=] { return IntPtrConstant(0); }); } +// https://tc39.es/ecma262/#sec-canbeheldweakly void BaseCollectionsAssembler::GotoIfCannotBeHeldWeakly( const TNode obj, Label* if_cannot_be_held_weakly) { Label check_symbol_key(this); @@ -411,7 +412,6 @@ void BaseCollectionsAssembler::GotoIfCannotBeHeldWeakly( GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_held_weakly); Goto(&end); Bind(&check_symbol_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), diff --git a/deps/v8/src/builtins/builtins-weak-refs.cc b/deps/v8/src/builtins/builtins-weak-refs.cc index a944159f247bec..eef9de1cb1d66f 100644 --- a/deps/v8/src/builtins/builtins-weak-refs.cc +++ b/deps/v8/src/builtins/builtins-weak-refs.cc @@ -9,23 +9,19 @@ namespace v8 { namespace internal { -// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-finalization-registry.prototype.unregister +// https://tc39.es/ecma262/#sec-finalization-registry.prototype.unregister BUILTIN(FinalizationRegistryUnregister) { HandleScope scope(isolate); const char* method_name = "FinalizationRegistry.prototype.unregister"; // 1. Let finalizationGroup be the this value. // - // 2. If Type(finalizationGroup) is not Object, throw a TypeError - // exception. - // - // 3. If finalizationGroup does not have a [[Cells]] internal slot, - // throw a TypeError exception. + // 2. Perform ? RequireInternalSlot(finalizationRegistry, [[Cells]]). CHECK_RECEIVER(JSFinalizationRegistry, finalization_registry, method_name); Handle unregister_token = args.atOrUndefined(isolate, 1); - // 4. If CanBeHeldWeakly(unregisterToken) is false, throw a TypeError + // 3. If CanBeHeldWeakly(unregisterToken) is false, throw a TypeError // exception. if (!unregister_token->CanBeHeldWeakly()) { THROW_NEW_ERROR_RETURN_FAILURE( diff --git a/deps/v8/src/builtins/finalization-registry.tq b/deps/v8/src/builtins/finalization-registry.tq index 4e4b4be068669f..c4e7f038ade07f 100644 --- a/deps/v8/src/builtins/finalization-registry.tq +++ b/deps/v8/src/builtins/finalization-registry.tq @@ -129,7 +129,7 @@ FinalizationRegistryConstructor( return finalizationRegistry; } -// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-finalization-registry.prototype.register +// https://tc39.es/ecma262/#sec-finalization-registry.prototype.register transitioning javascript builtin FinalizationRegistryRegister( js-implicit context: NativeContext, receiver: JSAny)(...arguments): JSAny { diff --git a/deps/v8/src/builtins/weak-ref.tq b/deps/v8/src/builtins/weak-ref.tq index 051831698534ce..ea0842c1ac2dd2 100644 --- a/deps/v8/src/builtins/weak-ref.tq +++ b/deps/v8/src/builtins/weak-ref.tq @@ -13,7 +13,7 @@ extern runtime JSWeakRefAddToKeptObjects(implicit context: Context)( namespace weakref { -// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-weak-ref-target +// https://tc39.es/ecma262/#sec-weak-ref-target transitioning javascript builtin WeakRefConstructor( js-implicit context: NativeContext, receiver: JSAny, newTarget: JSAny, diff --git a/deps/v8/src/codegen/code-stub-assembler.h b/deps/v8/src/codegen/code-stub-assembler.h index a1c8816c54b90b..bccdc34b746bdb 100644 --- a/deps/v8/src/codegen/code-stub-assembler.h +++ b/deps/v8/src/codegen/code-stub-assembler.h @@ -2679,12 +2679,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler ExternalReference::address_of_shared_string_table_flag()); } - TNode HasHarmonySymbolAsWeakmapKeyFlag() { - return LoadRuntimeFlag( - ExternalReference:: - address_of_FLAG_harmony_symbol_as_weakmap_key()); - } - // True iff |object| is a Smi or a HeapNumber or a BigInt. TNode IsNumeric(TNode object); diff --git a/deps/v8/src/codegen/external-reference.cc b/deps/v8/src/codegen/external-reference.cc index 686a91da6dc4b5..6d206663ba86b7 100644 --- a/deps/v8/src/codegen/external-reference.cc +++ b/deps/v8/src/codegen/external-reference.cc @@ -550,11 +550,6 @@ ExternalReference::address_of_mock_arraybuffer_allocator_flag() { return ExternalReference(&FLAG_mock_arraybuffer_allocator); } -ExternalReference -ExternalReference::address_of_FLAG_harmony_symbol_as_weakmap_key() { - return ExternalReference(&FLAG_harmony_symbol_as_weakmap_key); -} - ExternalReference ExternalReference::address_of_builtin_subclassing_flag() { return ExternalReference(&FLAG_builtin_subclassing); } diff --git a/deps/v8/src/codegen/external-reference.h b/deps/v8/src/codegen/external-reference.h index 636c528dabfe41..7715aa6a816f33 100644 --- a/deps/v8/src/codegen/external-reference.h +++ b/deps/v8/src/codegen/external-reference.h @@ -92,8 +92,6 @@ class StatsCounter; #define EXTERNAL_REFERENCE_LIST(V) \ V(abort_with_reason, "abort_with_reason") \ - V(address_of_FLAG_harmony_symbol_as_weakmap_key, \ - "FLAG_harmony_symbol_as_weakmap_key") \ V(address_of_builtin_subclassing_flag, "FLAG_builtin_subclassing") \ V(address_of_double_abs_constant, "double_absolute_constant") \ V(address_of_double_neg_constant, "double_negate_constant") \ diff --git a/deps/v8/src/flags/flag-definitions.h b/deps/v8/src/flags/flag-definitions.h index b984651e9e3268..c101c5730c98f2 100644 --- a/deps/v8/src/flags/flag-definitions.h +++ b/deps/v8/src/flags/flag-definitions.h @@ -338,8 +338,7 @@ DEFINE_BOOL(harmony_shipping, true, "enable all shipped harmony features") V(harmony_error_cause, "harmony error cause property") \ V(harmony_object_has_own, "harmony Object.hasOwn") \ V(harmony_class_static_blocks, "harmony static initializer blocks") \ - V(harmony_array_find_last, "harmony array find last helpers") \ - V(harmony_symbol_as_weakmap_key, "harmony symbols as weakmap keys") + V(harmony_array_find_last, "harmony array find last helpers") #ifdef V8_INTL_SUPPORT #define HARMONY_SHIPPING(V) HARMONY_SHIPPING_BASE(V) diff --git a/deps/v8/src/init/bootstrapper.cc b/deps/v8/src/init/bootstrapper.cc index fd75d5faa317de..d4c4122c22f4d7 100644 --- a/deps/v8/src/init/bootstrapper.cc +++ b/deps/v8/src/init/bootstrapper.cc @@ -4484,7 +4484,6 @@ void Genesis::InitializeConsole(Handle extras_binding) { EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_import_assertions) EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_private_brand_checks) EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_class_static_blocks) -EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_symbol_as_weakmap_key) EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_error_cause) #ifdef V8_INTL_SUPPORT diff --git a/deps/v8/src/objects/objects-inl.h b/deps/v8/src/objects/objects-inl.h index 6a4f976029662c..a1f73d23a9ca86 100644 --- a/deps/v8/src/objects/objects-inl.h +++ b/deps/v8/src/objects/objects-inl.h @@ -1206,7 +1206,7 @@ MaybeHandle Object::Share(Isolate* isolate, Handle value, throw_if_cannot_be_shared); } -// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-canbeheldweakly-abstract-operation +// https://tc39.es/ecma262/#sec-canbeheldweakly bool Object::CanBeHeldWeakly() const { if (IsJSReceiver()) { // TODO(v8:12547) Shared structs and arrays should only be able to point @@ -1217,10 +1217,7 @@ bool Object::CanBeHeldWeakly() const { } return true; } - if (FLAG_harmony_symbol_as_weakmap_key) { - return IsSymbol() && !Symbol::cast(*this).is_in_public_symbol_table(); - } - return false; + return IsSymbol() && !Symbol::cast(*this).is_in_public_symbol_table(); } Handle ObjectHashTableShape::AsHandle(Handle key) { diff --git a/deps/v8/test/inspector/debugger/object-preview-internal-properties.js b/deps/v8/test/inspector/debugger/object-preview-internal-properties.js index d2b3a7816339e9..026c4bc1c364c9 100644 --- a/deps/v8/test/inspector/debugger/object-preview-internal-properties.js +++ b/deps/v8/test/inspector/debugger/object-preview-internal-properties.js @@ -1,8 +1,6 @@ // Copyright 2016 the V8 project authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// -// Flags: --harmony-symbol-as-weakmap-key let {session, contextGroup, Protocol} = InspectorTest.start("Check internal properties reported in object preview."); diff --git a/deps/v8/test/mjsunit/harmony/regress/regress-crbug-1372500.js b/deps/v8/test/mjsunit/harmony/regress/regress-crbug-1372500.js index 6264570fdd3bd7..bccca0b40761fe 100644 --- a/deps/v8/test/mjsunit/harmony/regress/regress-crbug-1372500.js +++ b/deps/v8/test/mjsunit/harmony/regress/regress-crbug-1372500.js @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --harmony-symbol-as-weakmap-key --expose-gc +// Flags: --expose-gc // Register an object in a FinalizationRegistry with a Symbol as the unregister // token. 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 284e78b30196b7..9070b9309a5aa1 100644 --- a/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js +++ b/deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --harmony-symbol-as-weakmap-key --expose-gc --allow-natives-syntax --noincremental-marking +// Flags: --expose-gc --allow-natives-syntax --noincremental-marking (function TestWeakMapWithNonRegisteredSymbolKey() { const key = Symbol('123'); diff --git a/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target-gc.js b/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target-gc.js index 561bf4f058bdd7..3f663d2b0e0f52 100644 --- a/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target-gc.js +++ b/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target-gc.js @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --harmony-symbol-as-weakmap-key --expose-gc --noincremental-marking +// Flags: --expose-gc --noincremental-marking (function TestWeakRefWithSymbolGC() { let weakRef; diff --git a/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target.js b/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target.js index 1dc874ed83b3da..a969f388717a50 100644 --- a/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target.js +++ b/deps/v8/test/mjsunit/harmony/weakrefs/symbol-as-weakref-target.js @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --harmony-symbol-as-weakmap-key - (function TestRegisterWithSymbolTarget() { const fg = new FinalizationRegistry(() => { }); fg.register(Symbol('123'), 'holdings'); From c8ed93a98fcea6949250a5f1c7847c1bb872bb9a Mon Sep 17 00:00:00 2001 From: Jithil P Ponnan Date: Sat, 7 Oct 2023 01:33:46 +1100 Subject: [PATCH 09/22] lib: fix compileFunction throws range error for negative numbers PR-URL: https://github.com/nodejs/node/pull/49855 Fixes: https://github.com/nodejs/node/issues/49848 Reviewed-By: Yagiz Nizipli Reviewed-By: Benjamin Gruenbaum --- lib/internal/vm.js | 6 ++-- .../test-vm-compile-function-lineoffset.js | 34 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 test/es-module/test-vm-compile-function-lineoffset.js diff --git a/lib/internal/vm.js b/lib/internal/vm.js index b14ba13e7e4cfb..d2063c78e6315d 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -16,7 +16,7 @@ const { validateObject, validateString, validateStringArray, - validateUint32, + validateInt32, } = require('internal/validators'); const { ERR_INVALID_ARG_TYPE, @@ -46,8 +46,8 @@ function internalCompileFunction(code, params, options) { } = options; validateString(filename, 'options.filename'); - validateUint32(columnOffset, 'options.columnOffset'); - validateUint32(lineOffset, 'options.lineOffset'); + validateInt32(columnOffset, 'options.columnOffset'); + validateInt32(lineOffset, 'options.lineOffset'); if (cachedData !== undefined) validateBuffer(cachedData, 'options.cachedData'); validateBoolean(produceCachedData, 'options.produceCachedData'); diff --git a/test/es-module/test-vm-compile-function-lineoffset.js b/test/es-module/test-vm-compile-function-lineoffset.js new file mode 100644 index 00000000000000..47846a3aa6eb8b --- /dev/null +++ b/test/es-module/test-vm-compile-function-lineoffset.js @@ -0,0 +1,34 @@ +'use strict'; + +require('../common'); + +const assert = require('assert'); +const { compileFunction } = require('node:vm'); + +const min = -2147483648; +const max = 2147483647; + +compileFunction('', [], { lineOffset: min, columnOffset: min }); +compileFunction('', [], { lineOffset: max, columnOffset: max }); + +assert.throws( + () => { + compileFunction('', [], { lineOffset: min - 1, columnOffset: max }); + }, + { + code: 'ERR_OUT_OF_RANGE', + name: 'RangeError', + message: /The value of "options\.lineOffset" is out of range/, + } +); + +assert.throws( + () => { + compileFunction('', [], { lineOffset: min, columnOffset: min - 1 }); + }, + { + code: 'ERR_OUT_OF_RANGE', + name: 'RangeError', + message: /The value of "options\.columnOffset" is out of range/, + } +); From f1251ef8d6a76a3451c33867fd8a7002a762f346 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 2 Aug 2023 04:14:35 +0200 Subject: [PATCH 10/22] src: cast v8::Object::GetInternalField() return value to v8::Value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In preparation of https://chromium-review.googlesource.com/c/v8/v8/+/4707972 which changes the return value to v8::Data. PR-URL: https://github.com/nodejs/node/pull/48943 Reviewed-By: Juan José Arboleda Reviewed-By: Mohammed Keyvanzadeh Reviewed-By: Tobias Nießen Reviewed-By: Chengzhong Wu Reviewed-By: Stephen Belanger Reviewed-By: Jiawen Geng --- src/base_object-inl.h | 3 ++- src/module_wrap.cc | 6 ++++-- src/node_file.cc | 2 +- src/node_task_queue.cc | 2 +- src/node_zlib.cc | 3 ++- src/stream_base.cc | 5 +++-- 6 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index f003f1390b864f..0148c75427985e 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -127,7 +127,8 @@ template void BaseObject::InternalFieldGet( v8::Local property, const v8::PropertyCallbackInfo& info) { - info.GetReturnValue().Set(info.This()->GetInternalField(Field)); + info.GetReturnValue().Set( + info.This()->GetInternalField(Field).As()); } template diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 21b9a702aebaf4..2e3d444f64cd15 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -78,7 +78,7 @@ ModuleWrap::~ModuleWrap() { } Local ModuleWrap::context() const { - Local obj = object()->GetInternalField(kContextObjectSlot); + Local obj = object()->GetInternalField(kContextObjectSlot).As(); if (obj.IsEmpty()) return {}; return obj.As()->GetCreationContext().ToLocalChecked(); } @@ -684,7 +684,9 @@ MaybeLocal ModuleWrap::SyntheticModuleEvaluationStepsCallback( TryCatchScope try_catch(env); Local synthetic_evaluation_steps = - obj->object()->GetInternalField(kSyntheticEvaluationStepsSlot) + obj->object() + ->GetInternalField(kSyntheticEvaluationStepsSlot) + .As() .As(); obj->object()->SetInternalField( kSyntheticEvaluationStepsSlot, Undefined(isolate)); diff --git a/src/node_file.cc b/src/node_file.cc index 7f627ac458492c..ebbb67c226775d 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -438,7 +438,7 @@ MaybeLocal FileHandle::ClosePromise() { Local context = env()->context(); Local close_resolver = - object()->GetInternalField(FileHandle::kClosingPromiseSlot); + object()->GetInternalField(FileHandle::kClosingPromiseSlot).As(); if (!close_resolver.IsEmpty() && !close_resolver->IsUndefined()) { CHECK(close_resolver->IsPromise()); return close_resolver.As(); diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index 5d0e2b0d4c7ba1..1a0cb082a2534f 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -50,7 +50,7 @@ static Maybe GetAssignedPromiseWrapAsyncId(Environment* env, // be an object. If it's not, we just ignore it. Ideally v8 would // have had GetInternalField returning a MaybeLocal but this works // for now. - Local promiseWrap = promise->GetInternalField(0); + Local promiseWrap = promise->GetInternalField(0).As(); if (promiseWrap->IsObject()) { Local maybe_async_id; if (!promiseWrap.As()->Get(env->context(), id_symbol) diff --git a/src/node_zlib.cc b/src/node_zlib.cc index fac116f9e6b3e2..0c4ae0fc794347 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -423,7 +423,8 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { UpdateWriteResult(); // call the write() cb - Local cb = object()->GetInternalField(kWriteJSCallback); + Local cb = + object()->GetInternalField(kWriteJSCallback).template As(); MakeCallback(cb.As(), 0, nullptr); if (pending_close_) diff --git a/src/stream_base.cc b/src/stream_base.cc index f1769ca52970fe..b9dfc645e2b49c 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -470,8 +470,9 @@ MaybeLocal StreamBase::CallJSOnreadMethod(ssize_t nread, AsyncWrap* wrap = GetAsyncWrap(); CHECK_NOT_NULL(wrap); - Local onread = wrap->object()->GetInternalField( - StreamBase::kOnReadFunctionField); + Local onread = wrap->object() + ->GetInternalField(StreamBase::kOnReadFunctionField) + .As(); CHECK(onread->IsFunction()); return wrap->MakeCallback(onread.As(), arraysize(argv), argv); } From af9edb4bdb64492ec874d01054c2cb91325ef2ff Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 26 Sep 2023 17:31:20 +0200 Subject: [PATCH 11/22] deps: add v8::Object::SetInternalFieldForNodeCore() This is a non-ABI breaking solution for https://github.com/v8/v8/commit/b60a03df4cebafb4c92ee644d11617ad73889e5e and https://github.com/v8/v8/commit/0aa622e12893e9921c01a34ce9507b544e599c4a which are necessary for backporting vm-related memory fixes to v18.x. PR-URL: https://github.com/nodejs/node/pull/49874 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Jiawen Geng --- deps/v8/include/v8-object.h | 17 +++++++++++++++++ deps/v8/src/api/api.cc | 23 +++++++++++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/deps/v8/include/v8-object.h b/deps/v8/include/v8-object.h index bad299fc42948d..49369317b72c39 100644 --- a/deps/v8/include/v8-object.h +++ b/deps/v8/include/v8-object.h @@ -20,6 +20,8 @@ class Function; class FunctionTemplate; template class PropertyCallbackInfo; +class Module; +class UnboundScript; /** * A private symbol @@ -480,6 +482,21 @@ class V8_EXPORT Object : public Value { /** Sets the value in an internal field. */ void SetInternalField(int index, Local value); + /** + * Warning: These are Node.js-specific extentions used to avoid breaking + * changes in Node.js v18.x. They do not exist in V8 upstream and will + * not exist in Node.js v21.x. Node.js embedders and addon authors should + * not use them from v18.x. + */ +#ifndef NODE_WANT_INTERNALS + V8_DEPRECATED("This extention should only be used by Node.js core") +#endif + void SetInternalFieldForNodeCore(int index, Local value); +#ifndef NODE_WANT_INTERNALS + V8_DEPRECATED("This extention should only be used by Node.js core") +#endif + void SetInternalFieldForNodeCore(int index, Local value); + /** * Gets a 2-byte-aligned native pointer from an internal field. This field * must have been set by SetAlignedPointerInInternalField, everything else diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc index 8d224bea2e2261..3b1a81680b0323 100644 --- a/deps/v8/src/api/api.cc +++ b/deps/v8/src/api/api.cc @@ -5948,14 +5948,33 @@ Local v8::Object::SlowGetInternalField(int index) { return Utils::ToLocal(value); } -void v8::Object::SetInternalField(int index, v8::Local value) { - i::Handle obj = Utils::OpenHandle(this); +template +void SetInternalFieldImpl(v8::Object* receiver, int index, v8::Local value) { + i::Handle obj = Utils::OpenHandle(receiver); const char* location = "v8::Object::SetInternalField()"; if (!InternalFieldOK(obj, index, location)) return; i::Handle val = Utils::OpenHandle(*value); i::Handle::cast(obj)->SetEmbedderField(index, *val); } +void v8::Object::SetInternalField(int index, v8::Local value) { + SetInternalFieldImpl(this, index, value); +} + +/** + * These are Node.js-specific extentions used to avoid breaking changes in + * Node.js v20.x. + */ +void v8::Object::SetInternalFieldForNodeCore(int index, + v8::Local value) { + SetInternalFieldImpl(this, index, value); +} + +void v8::Object::SetInternalFieldForNodeCore(int index, + v8::Local value) { + SetInternalFieldImpl(this, index, value); +} + void* v8::Object::SlowGetAlignedPointerFromInternalField(int index) { i::Handle obj = Utils::OpenHandle(this); const char* location = "v8::Object::GetAlignedPointerFromInternalField()"; From 9e8629cfbdf85552f8cea72456e64407a63c90f4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 8 Sep 2023 13:08:09 +0200 Subject: [PATCH 12/22] src: set ModuleWrap internal fields only once There is no need to initialize the internal fields to undefined and then initialize them to something else in the caller. Simply pass the internal fields into the constructor to initialize them just once. PR-URL: https://github.com/nodejs/node/pull/49391 Reviewed-By: Darshan Sen Reviewed-By: Yagiz Nizipli Reviewed-By: Stephen Belanger Reviewed-By: Chengzhong Wu --- src/module_wrap.cc | 40 +++++++++++++++++++++++----------------- src/module_wrap.h | 4 +++- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 2e3d444f64cd15..a1b7633a31d1df 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -52,16 +52,22 @@ using v8::Value; ModuleWrap::ModuleWrap(Environment* env, Local object, Local module, - Local url) - : BaseObject(env, object), - module_(env->isolate(), module), - id_(env->get_next_module_id()) { + Local url, + Local context_object, + Local synthetic_evaluation_step) + : BaseObject(env, object), + module_(env->isolate(), module), + id_(env->get_next_module_id()) { env->id_to_module_map.emplace(id_, this); - Local undefined = Undefined(env->isolate()); object->SetInternalField(kURLSlot, url); - object->SetInternalField(kSyntheticEvaluationStepsSlot, undefined); - object->SetInternalField(kContextObjectSlot, undefined); + object->SetInternalField(kSyntheticEvaluationStepsSlot, + synthetic_evaluation_step); + object->SetInternalField(kContextObjectSlot, context_object); + + if (!synthetic_evaluation_step->IsUndefined()) { + synthetic_ = true; + } } ModuleWrap::~ModuleWrap() { @@ -79,7 +85,9 @@ ModuleWrap::~ModuleWrap() { Local ModuleWrap::context() const { Local obj = object()->GetInternalField(kContextObjectSlot).As(); - if (obj.IsEmpty()) return {}; + // If this fails, there is likely a bug e.g. ModuleWrap::context() is accessed + // before the ModuleWrap constructor completes. + CHECK(obj->IsObject()); return obj.As()->GetCreationContext().ToLocalChecked(); } @@ -227,18 +235,16 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { return; } - ModuleWrap* obj = new ModuleWrap(env, that, module, url); - - if (synthetic) { - obj->synthetic_ = true; - obj->object()->SetInternalField(kSyntheticEvaluationStepsSlot, args[3]); - } - // Use the extras object as an object whose GetCreationContext() will be the // original `context`, since the `Context` itself strictly speaking cannot // be stored in an internal field. - obj->object()->SetInternalField(kContextObjectSlot, - context->GetExtrasBindingObject()); + Local context_object = context->GetExtrasBindingObject(); + Local synthetic_evaluation_step = + synthetic ? args[3] : Undefined(env->isolate()).As(); + + ModuleWrap* obj = new ModuleWrap( + env, that, module, url, context_object, synthetic_evaluation_step); + obj->contextify_context_ = contextify_context; env->hash_to_module_map.emplace(module->GetIdentityHash(), obj); diff --git a/src/module_wrap.h b/src/module_wrap.h index ce4610a461a2b4..00a22b79a801ab 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -72,7 +72,9 @@ class ModuleWrap : public BaseObject { ModuleWrap(Environment* env, v8::Local object, v8::Local module, - v8::Local url); + v8::Local url, + v8::Local context_object, + v8::Local synthetic_evaluation_step); ~ModuleWrap() override; static void New(const v8::FunctionCallbackInfo& args); From bb3462a02c436b334213c4f3c5a8690da84158b9 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 21 Jun 2023 16:01:50 +0200 Subject: [PATCH 13/22] module: use symbol in WeakMap to manage host defined options Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in https://github.com/nodejs/node/pull/46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: https://github.com/nodejs/node/pull/48510 Refs: https://github.com/nodejs/node/issues/44211 Refs: https://github.com/nodejs/node/issues/42080 Refs: https://github.com/nodejs/node/issues/47096 Refs: https://github.com/nodejs/node/issues/43205 Refs: https://github.com/nodejs/node/issues/38695 Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Stephen Belanger --- .../modules/esm/create_dynamic_module.js | 5 +- lib/internal/modules/esm/loader.js | 5 +- lib/internal/modules/esm/translators.js | 5 +- lib/internal/modules/esm/utils.js | 89 ++++++++++++++----- lib/internal/vm.js | 7 +- lib/internal/vm/module.js | 16 ++-- lib/vm.js | 5 +- src/env-inl.h | 10 --- src/env.h | 8 -- src/env_properties.h | 2 +- src/module_wrap.cc | 66 +++++--------- src/module_wrap.h | 8 +- src/node_contextify.cc | 75 ++++------------ src/node_contextify.h | 24 ----- .../test-dynamic-import-script-lifetime.js | 32 +++++++ .../test-vm-compile-function-leak.js | 19 ++++ 16 files changed, 185 insertions(+), 191 deletions(-) create mode 100644 test/es-module/test-dynamic-import-script-lifetime.js create mode 100644 test/es-module/test-vm-compile-function-leak.js diff --git a/lib/internal/modules/esm/create_dynamic_module.js b/lib/internal/modules/esm/create_dynamic_module.js index c0060c47e93b5a..f5077acb78ace2 100644 --- a/lib/internal/modules/esm/create_dynamic_module.js +++ b/lib/internal/modules/esm/create_dynamic_module.js @@ -69,8 +69,9 @@ import.meta.done(); if (imports.length) { reflect.imports = { __proto__: null }; } - const { setCallbackForWrap } = require('internal/modules/esm/utils'); - setCallbackForWrap(m, { + const { registerModule } = require('internal/modules/esm/utils'); + registerModule(m, { + __proto__: null, initializeImportMeta: (meta, wrap) => { meta.exports = reflect.exports; if (reflect.imports) { diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 6e42f1a5db5a8a..992311f8922c54 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -210,9 +210,10 @@ class ModuleLoader { ) { const evalInstance = (url) => { const { ModuleWrap } = internalBinding('module_wrap'); - const { setCallbackForWrap } = require('internal/modules/esm/utils'); + const { registerModule } = require('internal/modules/esm/utils'); const module = new ModuleWrap(url, undefined, source, 0, 0); - setCallbackForWrap(module, { + registerModule(module, { + __proto__: null, initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }), importModuleDynamically: (specifier, { url }, importAttributes) => { return this.import(specifier, url, importAttributes); diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 39af99c6c3b5b7..d1bc2754fc3ce3 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -150,8 +150,9 @@ translators.set('module', async function moduleStrategy(url, source, isMain) { maybeCacheSourceMap(url, source); debug(`Translating StandardModule ${url}`); const module = new ModuleWrap(url, undefined, source, 0, 0); - const { setCallbackForWrap } = require('internal/modules/esm/utils'); - setCallbackForWrap(module, { + const { registerModule } = require('internal/modules/esm/utils'); + registerModule(module, { + __proto__: null, initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }), importModuleDynamically, }); diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index df729ef96c7172..b6b98569849ef4 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -7,6 +7,11 @@ const { ObjectFreeze, } = primordials; +const { + privateSymbols: { + host_defined_option_symbol, + }, +} = internalBinding('util'); const { ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING, ERR_INVALID_ARG_VALUE, @@ -21,16 +26,8 @@ const { setImportModuleDynamicallyCallback, setInitializeImportMetaObjectCallback, } = internalBinding('module_wrap'); -const { - getModuleFromWrap, -} = require('internal/vm/module'); const assert = require('internal/assert'); -const callbackMap = new SafeWeakMap(); -function setCallbackForWrap(wrap, data) { - callbackMap.set(wrap, data); -} - let defaultConditions; /** * Returns the default conditions for ES module loading. @@ -83,34 +80,86 @@ function getConditionsSet(conditions) { return getDefaultConditionsSet(); } +/** + * @callback ImportModuleDynamicallyCallback + * @param {string} specifier + * @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer + * @param {object} attributes + * @returns { Promise } + */ + +/** + * @callback InitializeImportMetaCallback + * @param {object} meta + * @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer + */ + +/** + * @typedef {{ + * callbackReferrer: ModuleWrap|ContextifyScript|Function|vm.Module + * initializeImportMeta? : InitializeImportMetaCallback, + * importModuleDynamically? : ImportModuleDynamicallyCallback + * }} ModuleRegistry + */ + +/** + * @type {WeakMap} + */ +const moduleRegistries = new SafeWeakMap(); + +/** + * V8 would make sure that as long as import() can still be initiated from + * the referrer, the symbol referenced by |host_defined_option_symbol| should + * be alive, which in term would keep the settings object alive through the + * WeakMap, and in turn that keeps the referrer object alive, which would be + * passed into the callbacks. + * The reference goes like this: + * [v8::internal::Script] (via host defined options) ----1--> [idSymbol] + * [callbackReferrer] (via host_defined_option_symbol) ------2------^ | + * ^----------3---- (via WeakMap)------ + * 1+3 makes sure that as long as import() can still be initiated, the + * referrer wrap is still around and can be passed into the callbacks. + * 2 is only there so that we can get the id symbol to configure the + * weak map. + * @param {ModuleWrap|ContextifyScript|Function} referrer The referrer to + * get the id symbol from. This is different from callbackReferrer which + * could be set by the caller. + * @param {ModuleRegistry} registry + */ +function registerModule(referrer, registry) { + const idSymbol = referrer[host_defined_option_symbol]; + // To prevent it from being GC'ed. + registry.callbackReferrer ??= referrer; + moduleRegistries.set(idSymbol, registry); +} + /** * Defines the `import.meta` object for a given module. - * @param {object} wrap - Reference to the module. + * @param {symbol} symbol - Reference to the module. * @param {Record} meta - The import.meta object to initialize. */ -function initializeImportMetaObject(wrap, meta) { - if (callbackMap.has(wrap)) { - const { initializeImportMeta } = callbackMap.get(wrap); +function initializeImportMetaObject(symbol, meta) { + if (moduleRegistries.has(symbol)) { + const { initializeImportMeta, callbackReferrer } = moduleRegistries.get(symbol); if (initializeImportMeta !== undefined) { - meta = initializeImportMeta(meta, getModuleFromWrap(wrap) || wrap); + meta = initializeImportMeta(meta, callbackReferrer); } } } /** * Asynchronously imports a module dynamically using a callback function. The native callback. - * @param {object} wrap - Reference to the module. + * @param {symbol} symbol - Reference to the module. * @param {string} specifier - The module specifier string. * @param {Record} attributes - The import attributes object. * @returns {Promise} - The imported module object. * @throws {ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING} - If the callback function is missing. */ -async function importModuleDynamicallyCallback(wrap, specifier, attributes) { - if (callbackMap.has(wrap)) { - const { importModuleDynamically } = callbackMap.get(wrap); +async function importModuleDynamicallyCallback(symbol, specifier, attributes) { + if (moduleRegistries.has(symbol)) { + const { importModuleDynamically, callbackReferrer } = moduleRegistries.get(symbol); if (importModuleDynamically !== undefined) { - return importModuleDynamically( - specifier, getModuleFromWrap(wrap) || wrap, attributes); + return importModuleDynamically(specifier, callbackReferrer, attributes); } } throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING(); @@ -176,7 +225,7 @@ async function initializeHooks() { } module.exports = { - setCallbackForWrap, + registerModule, initializeESM, initializeHooks, getDefaultConditions, diff --git a/lib/internal/vm.js b/lib/internal/vm.js index d2063c78e6315d..70dbd866e66bd7 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -100,9 +100,10 @@ function internalCompileFunction(code, params, options) { const { importModuleDynamicallyWrap } = require('internal/vm/module'); const wrapped = importModuleDynamicallyWrap(importModuleDynamically); const func = result.function; - const { setCallbackForWrap } = require('internal/modules/esm/utils'); - setCallbackForWrap(result.cacheKey, { - importModuleDynamically: (s, _k, i) => wrapped(s, func, i), + const { registerModule } = require('internal/modules/esm/utils'); + registerModule(func, { + __proto__: null, + importModuleDynamically: wrapped, }); } diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index 19d93e1abfbd42..439908a891b10c 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -12,7 +12,6 @@ const { ObjectSetPrototypeOf, ReflectApply, SafePromiseAllReturnVoid, - SafeWeakMap, Symbol, SymbolToStringTag, TypeError, @@ -70,7 +69,6 @@ const STATUS_MAP = { let globalModuleId = 0; const defaultModuleName = 'vm:module'; -const wrapToModuleMap = new SafeWeakMap(); const kWrap = Symbol('kWrap'); const kContext = Symbol('kContext'); @@ -121,17 +119,18 @@ class Module { }); } + let registry = { __proto__: null }; if (sourceText !== undefined) { this[kWrap] = new ModuleWrap(identifier, context, sourceText, options.lineOffset, options.columnOffset, options.cachedData); - const { setCallbackForWrap } = require('internal/modules/esm/utils'); - setCallbackForWrap(this[kWrap], { + registry = { + __proto__: null, initializeImportMeta: options.initializeImportMeta, importModuleDynamically: options.importModuleDynamically ? importModuleDynamicallyWrap(options.importModuleDynamically) : undefined, - }); + }; } else { assert(syntheticEvaluationSteps); this[kWrap] = new ModuleWrap(identifier, context, @@ -139,7 +138,11 @@ class Module { syntheticEvaluationSteps); } - wrapToModuleMap.set(this[kWrap], this); + // This will take precedence over the referrer as the object being + // passed into the callbacks. + registry.callbackReferrer = this; + const { registerModule } = require('internal/modules/esm/utils'); + registerModule(this[kWrap], registry); this[kContext] = context; } @@ -446,5 +449,4 @@ module.exports = { SourceTextModule, SyntheticModule, importModuleDynamicallyWrap, - getModuleFromWrap: (wrap) => wrapToModuleMap.get(wrap), }; diff --git a/lib/vm.js b/lib/vm.js index b48e79c282541b..5ca04d6fb41758 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -106,8 +106,9 @@ class Script extends ContextifyScript { validateFunction(importModuleDynamically, 'options.importModuleDynamically'); const { importModuleDynamicallyWrap } = require('internal/vm/module'); - const { setCallbackForWrap } = require('internal/modules/esm/utils'); - setCallbackForWrap(this, { + const { registerModule } = require('internal/modules/esm/utils'); + registerModule(this, { + __proto__: null, importModuleDynamically: importModuleDynamicallyWrap(importModuleDynamically), }); diff --git a/src/env-inl.h b/src/env-inl.h index b959fb48d29ce1..2ee4e709d82d72 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -345,16 +345,6 @@ inline AliasedInt32Array& Environment::stream_base_state() { return stream_base_state_; } -inline uint32_t Environment::get_next_module_id() { - return module_id_counter_++; -} -inline uint32_t Environment::get_next_script_id() { - return script_id_counter_++; -} -inline uint32_t Environment::get_next_function_id() { - return function_id_counter_++; -} - ShouldNotAbortOnUncaughtScope::ShouldNotAbortOnUncaughtScope( Environment* env) : env_(env) { diff --git a/src/env.h b/src/env.h index 06250f32e14863..1f5dbc65790d97 100644 --- a/src/env.h +++ b/src/env.h @@ -698,14 +698,6 @@ class Environment : public MemoryRetainer { builtins::BuiltinLoader* builtin_loader(); std::unordered_multimap hash_to_module_map; - std::unordered_map id_to_module_map; - std::unordered_map - id_to_script_map; - std::unordered_map id_to_function_map; - - inline uint32_t get_next_module_id(); - inline uint32_t get_next_script_id(); - inline uint32_t get_next_function_id(); EnabledDebugList* enabled_debug_list() { return &enabled_debug_list_; } diff --git a/src/env_properties.h b/src/env_properties.h index 5fc5106c0c5ad2..47d41826014c48 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -21,6 +21,7 @@ V(arrow_message_private_symbol, "node:arrowMessage") \ V(contextify_context_private_symbol, "node:contextify:context") \ V(decorated_private_symbol, "node:decorated") \ + V(host_defined_option_symbol, "node:host_defined_option_symbol") \ V(napi_type_tag, "node:napi:type_tag") \ V(napi_wrapper, "node:napi:wrapper") \ V(untransferable_object_private_symbol, "node:untransferableObject") \ @@ -337,7 +338,6 @@ V(blocklist_constructor_template, v8::FunctionTemplate) \ V(contextify_global_template, v8::ObjectTemplate) \ V(contextify_wrapper_template, v8::ObjectTemplate) \ - V(compiled_fn_entry_template, v8::ObjectTemplate) \ V(crypto_key_object_handle_constructor, v8::FunctionTemplate) \ V(env_proxy_template, v8::ObjectTemplate) \ V(env_proxy_ctor_template, v8::FunctionTemplate) \ diff --git a/src/module_wrap.cc b/src/module_wrap.cc index a1b7633a31d1df..576cc3123a8138 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -38,13 +38,13 @@ using v8::MaybeLocal; using v8::MicrotaskQueue; using v8::Module; using v8::ModuleRequest; -using v8::Number; using v8::Object; using v8::PrimitiveArray; using v8::Promise; using v8::ScriptCompiler; using v8::ScriptOrigin; using v8::String; +using v8::Symbol; using v8::UnboundModuleScript; using v8::Undefined; using v8::Value; @@ -55,11 +55,7 @@ ModuleWrap::ModuleWrap(Environment* env, Local url, Local context_object, Local synthetic_evaluation_step) - : BaseObject(env, object), - module_(env->isolate(), module), - id_(env->get_next_module_id()) { - env->id_to_module_map.emplace(id_, this); - + : BaseObject(env, object), module_(env->isolate(), module) { object->SetInternalField(kURLSlot, url); object->SetInternalField(kSyntheticEvaluationStepsSlot, synthetic_evaluation_step); @@ -73,7 +69,6 @@ ModuleWrap::ModuleWrap(Environment* env, ModuleWrap::~ModuleWrap() { HandleScope scope(env()->isolate()); Local module = module_.Get(env()->isolate()); - env()->id_to_module_map.erase(id_); auto range = env()->hash_to_module_map.equal_range(module->GetIdentityHash()); for (auto it = range.first; it != range.second; ++it) { if (it->second == this) { @@ -102,14 +97,6 @@ ModuleWrap* ModuleWrap::GetFromModule(Environment* env, return nullptr; } -ModuleWrap* ModuleWrap::GetFromID(Environment* env, uint32_t id) { - auto module_wrap_it = env->id_to_module_map.find(id); - if (module_wrap_it == env->id_to_module_map.end()) { - return nullptr; - } - return module_wrap_it->second; -} - // new ModuleWrap(url, context, source, lineOffset, columnOffset) // new ModuleWrap(url, context, exportNames, syntheticExecutionFunction) void ModuleWrap::New(const FunctionCallbackInfo& args) { @@ -154,8 +141,8 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { Local host_defined_options = PrimitiveArray::New(isolate, HostDefinedOptions::kLength); - host_defined_options->Set(isolate, HostDefinedOptions::kType, - Number::New(isolate, ScriptType::kModule)); + Local id_symbol = Symbol::New(isolate, url); + host_defined_options->Set(isolate, HostDefinedOptions::kID, id_symbol); ShouldNotAbortOnUncaughtScope no_abort_scope(env); TryCatchScope try_catch(env); @@ -235,6 +222,11 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { return; } + if (that->SetPrivate(context, env->host_defined_option_symbol(), id_symbol) + .IsNothing()) { + return; + } + // Use the extras object as an object whose GetCreationContext() will be the // original `context`, since the `Context` itself strictly speaking cannot // be stored in an internal field. @@ -249,9 +241,6 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { env->hash_to_module_map.emplace(module->GetIdentityHash(), obj); - host_defined_options->Set(isolate, HostDefinedOptions::kID, - Number::New(isolate, obj->id())); - that->SetIntegrityLevel(context, IntegrityLevel::kFrozen); args.GetReturnValue().Set(that); } @@ -586,35 +575,16 @@ static MaybeLocal ImportModuleDynamically( Local object; - int type = options->Get(context, HostDefinedOptions::kType) - .As() - ->Int32Value(context) - .ToChecked(); - uint32_t id = options->Get(context, HostDefinedOptions::kID) - .As() - ->Uint32Value(context) - .ToChecked(); - if (type == ScriptType::kScript) { - contextify::ContextifyScript* wrap = env->id_to_script_map.find(id)->second; - object = wrap->object(); - } else if (type == ScriptType::kModule) { - ModuleWrap* wrap = ModuleWrap::GetFromID(env, id); - object = wrap->object(); - } else if (type == ScriptType::kFunction) { - auto it = env->id_to_function_map.find(id); - CHECK_NE(it, env->id_to_function_map.end()); - object = it->second->object(); - } else { - UNREACHABLE(); - } + Local id = + options->Get(context, HostDefinedOptions::kID).As(); Local attributes = createImportAttributesContainer(env, isolate, import_attributes); Local import_args[] = { - object, - Local(specifier), - attributes, + id, + Local(specifier), + attributes, }; Local result; @@ -658,7 +628,13 @@ void ModuleWrap::HostInitializeImportMetaObjectCallback( Local wrap = module_wrap->object(); Local callback = env->host_initialize_import_meta_object_callback(); - Local args[] = { wrap, meta }; + Local id; + if (!wrap->GetPrivate(context, env->host_defined_option_symbol()) + .ToLocal(&id)) { + return; + } + DCHECK(id->IsSymbol()); + Local args[] = {id, meta}; TryCatchScope try_catch(env); USE(callback->Call( context, Undefined(env->isolate()), arraysize(args), args)); diff --git a/src/module_wrap.h b/src/module_wrap.h index 00a22b79a801ab..3d3c646baa5011 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -26,9 +26,8 @@ enum ScriptType : int { }; enum HostDefinedOptions : int { - kType = 8, - kID = 9, - kLength = 10, + kID = 8, + kLength = 9, }; class ModuleWrap : public BaseObject { @@ -55,9 +54,7 @@ class ModuleWrap : public BaseObject { tracker->TrackField("resolve_cache", resolve_cache_); } - inline uint32_t id() { return id_; } v8::Local context() const; - static ModuleWrap* GetFromID(node::Environment*, uint32_t id); SET_MEMORY_INFO_NAME(ModuleWrap) SET_SELF_SIZE(ModuleWrap) @@ -109,7 +106,6 @@ class ModuleWrap : public BaseObject { contextify::ContextifyContext* contextify_context_ = nullptr; bool synthetic_ = false; bool linked_ = false; - uint32_t id_; }; } // namespace loader diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 3da8746e2c46bb..7f174dbe9c829b 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -60,7 +60,6 @@ using v8::MicrotasksPolicy; using v8::Name; using v8::NamedPropertyHandlerConfiguration; using v8::Nothing; -using v8::Number; using v8::Object; using v8::ObjectTemplate; using v8::PrimitiveArray; @@ -73,11 +72,11 @@ using v8::Script; using v8::ScriptCompiler; using v8::ScriptOrigin; using v8::String; +using v8::Symbol; using v8::Uint32; using v8::UnboundScript; using v8::Value; using v8::WeakCallbackInfo; -using v8::WeakCallbackType; // The vm module executes code in a sandboxed environment with a different // global object than the rest of the code. This is achieved by applying @@ -833,10 +832,9 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { Local host_defined_options = PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - host_defined_options->Set(isolate, loader::HostDefinedOptions::kType, - Number::New(isolate, loader::ScriptType::kScript)); - host_defined_options->Set(isolate, loader::HostDefinedOptions::kID, - Number::New(isolate, contextify_script->id())); + Local id_symbol = Symbol::New(isolate, filename); + host_defined_options->Set( + isolate, loader::HostDefinedOptions::kID, id_symbol); ScriptOrigin origin(isolate, filename, @@ -880,6 +878,12 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { new_cached_data.reset(ScriptCompiler::CreateCodeCache(v8_script)); } + if (contextify_script->object() + ->SetPrivate(context, env->host_defined_option_symbol(), id_symbol) + .IsNothing()) { + return; + } + if (StoreCodeCacheResult(env, args.This(), compile_options, @@ -1117,17 +1121,11 @@ bool ContextifyScript::EvalMachine(Local context, ContextifyScript::ContextifyScript(Environment* env, Local object) - : BaseObject(env, object), - id_(env->get_next_script_id()) { + : BaseObject(env, object) { MakeWeak(); - env->id_to_script_map.emplace(id_, this); -} - - -ContextifyScript::~ContextifyScript() { - env()->id_to_script_map.erase(id_); } +ContextifyScript::~ContextifyScript() {} void ContextifyContext::CompileFunction( const FunctionCallbackInfo& args) { @@ -1197,18 +1195,12 @@ void ContextifyContext::CompileFunction( data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength()); } - // Get the function id - uint32_t id = env->get_next_function_id(); - // Set host_defined_options Local host_defined_options = PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); + Local id_symbol = Symbol::New(isolate, filename); host_defined_options->Set( - isolate, - loader::HostDefinedOptions::kType, - Number::New(isolate, loader::ScriptType::kFunction)); - host_defined_options->Set( - isolate, loader::HostDefinedOptions::kID, Number::New(isolate, id)); + isolate, loader::HostDefinedOptions::kID, id_symbol); ScriptOrigin origin(isolate, filename, @@ -1273,21 +1265,14 @@ void ContextifyContext::CompileFunction( } return; } - - Local cache_key; - if (!env->compiled_fn_entry_template()->NewInstance( - context).ToLocal(&cache_key)) { + if (fn->SetPrivate(context, env->host_defined_option_symbol(), id_symbol) + .IsNothing()) { return; } - CompiledFnEntry* entry = new CompiledFnEntry(env, cache_key, id, fn); - env->id_to_function_map.emplace(id, entry); Local result = Object::New(isolate); if (result->Set(parsing_context, env->function_string(), fn).IsNothing()) return; - if (result->Set(parsing_context, env->cache_key_string(), cache_key) - .IsNothing()) - return; if (result ->Set(parsing_context, env->source_map_url_string(), @@ -1312,25 +1297,6 @@ void ContextifyContext::CompileFunction( args.GetReturnValue().Set(result); } -void CompiledFnEntry::WeakCallback( - const WeakCallbackInfo& data) { - CompiledFnEntry* entry = data.GetParameter(); - delete entry; -} - -CompiledFnEntry::CompiledFnEntry(Environment* env, - Local object, - uint32_t id, - Local fn) - : BaseObject(env, object), id_(id), fn_(env->isolate(), fn) { - fn_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); -} - -CompiledFnEntry::~CompiledFnEntry() { - env()->id_to_function_map.erase(id_); - fn_.ClearWeak(); -} - static void StartSigintWatchdog(const FunctionCallbackInfo& args) { int ret = SigintWatchdogHelper::GetInstance()->Start(); args.GetReturnValue().Set(ret == 0); @@ -1418,15 +1384,6 @@ void Initialize(Local target, SetMethodNoSideEffect( context, target, "watchdogHasPendingSigint", WatchdogHasPendingSigint); - { - Local tpl = FunctionTemplate::New(env->isolate()); - tpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "CompiledFnEntry")); - tpl->InstanceTemplate()->SetInternalFieldCount( - CompiledFnEntry::kInternalFieldCount); - - env->set_compiled_fn_entry_template(tpl->InstanceTemplate()); - } - Local constants = Object::New(env->isolate()); Local measure_memory = Object::New(env->isolate()); Local memory_execution = Object::New(env->isolate()); diff --git a/src/node_contextify.h b/src/node_contextify.h index 76c89318bb6cbf..575f542ca7aa5a 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -171,32 +171,8 @@ class ContextifyScript : public BaseObject { std::shared_ptr microtask_queue, const v8::FunctionCallbackInfo& args); - inline uint32_t id() { return id_; } - private: v8::Global script_; - uint32_t id_; -}; - -class CompiledFnEntry final : public BaseObject { - public: - SET_NO_MEMORY_INFO() - SET_MEMORY_INFO_NAME(CompiledFnEntry) - SET_SELF_SIZE(CompiledFnEntry) - - CompiledFnEntry(Environment* env, - v8::Local object, - uint32_t id, - v8::Local fn); - ~CompiledFnEntry(); - - bool IsNotIndicativeOfMemoryLeakAtExit() const override { return true; } - - private: - uint32_t id_; - v8::Global fn_; - - static void WeakCallback(const v8::WeakCallbackInfo& data); }; v8::Maybe StoreCodeCacheResult( diff --git a/test/es-module/test-dynamic-import-script-lifetime.js b/test/es-module/test-dynamic-import-script-lifetime.js new file mode 100644 index 00000000000000..7ccea887109e05 --- /dev/null +++ b/test/es-module/test-dynamic-import-script-lifetime.js @@ -0,0 +1,32 @@ +// Flags: --expose-gc --experimental-vm-modules + +'use strict'; + +// This tests that vm.Script would not get GC'ed while the script can still +// initiate dynamic import. +// See https://github.com/nodejs/node/issues/43205. + +require('../common'); +const vm = require('vm'); + +const code = ` +new Promise(resolve => { + setTimeout(() => { + gc(); // vm.Script should not be GC'ed while the script is alive. + resolve(); + }, 1); +}).then(() => import('foo'));`; + +// vm.runInThisContext creates a vm.Script underneath, which should not be GC'ed +// while import() can still be initiated. +vm.runInThisContext(code, { + async importModuleDynamically() { + const m = new vm.SyntheticModule(['bar'], () => { + m.setExport('bar', 1); + }); + + await m.link(() => {}); + await m.evaluate(); + return m; + } +}); diff --git a/test/es-module/test-vm-compile-function-leak.js b/test/es-module/test-vm-compile-function-leak.js new file mode 100644 index 00000000000000..ff061cdaec7a01 --- /dev/null +++ b/test/es-module/test-vm-compile-function-leak.js @@ -0,0 +1,19 @@ +// Flags: --max-old-space-size=16 --trace-gc +'use strict'; + +// This tests that vm.compileFunction with dynamic import callback does not leak. +// See https://github.com/nodejs/node/issues/44211 +require('../common'); +const vm = require('vm'); +let count = 0; + +function main() { + // Try to reach the maximum old space size. + vm.compileFunction(`"${Math.random().toString().repeat(512)}"`, [], { + async importModuleDynamically() {}, + }); + if (count++ < 2048) { + setTimeout(main, 1); + } +} +main(); From 05778c4c0f9ef15b8bc50d3614c2872d1ef7f89f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 21 Jun 2023 18:04:35 +0200 Subject: [PATCH 14/22] module: fix leak of vm.SyntheticModule Previously we maintain a strong persistent reference to the ModuleWrap to retrieve the ID-to-ModuleWrap mapping from the HostImportModuleDynamicallyCallback using the number ID stored in the host-defined options. As a result the ModuleWrap would be kept alive until the Environment is shut down, which would be a leak for user code. With the new symbol-based host-defined option we can just get the ModuleWrap from the JS-land WeakMap so there's now no need to maintain this strong reference. This would at least fix the leak for vm.SyntheticModule. vm.SourceTextModule is still leaking due to the strong persistent reference to the v8::Module. PR-URL: https://github.com/nodejs/node/pull/48510 Refs: https://github.com/nodejs/node/issues/44211 Refs: https://github.com/nodejs/node/issues/42080 Refs: https://github.com/nodejs/node/issues/47096 Refs: https://github.com/nodejs/node/issues/43205 Refs: https://github.com/nodejs/node/issues/38695 Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Stephen Belanger --- src/module_wrap.cc | 1 + .../test-vm-synthetic-module-leak.js | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 test/es-module/test-vm-synthetic-module-leak.js diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 576cc3123a8138..b1fbbee5257400 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -64,6 +64,7 @@ ModuleWrap::ModuleWrap(Environment* env, if (!synthetic_evaluation_step->IsUndefined()) { synthetic_ = true; } + MakeWeak(); } ModuleWrap::~ModuleWrap() { diff --git a/test/es-module/test-vm-synthetic-module-leak.js b/test/es-module/test-vm-synthetic-module-leak.js new file mode 100644 index 00000000000000..9de02cb22f1128 --- /dev/null +++ b/test/es-module/test-vm-synthetic-module-leak.js @@ -0,0 +1,23 @@ +// Flags: --experimental-vm-modules --max-old-space-size=16 +'use strict'; + +// This tests that vm.SyntheticModule does not leak. +// See https://github.com/nodejs/node/issues/44211 +require('../common'); +const vm = require('vm'); + +let count = 0; +async function createModule() { + // Try to reach the maximum old space size. + const m = new vm.SyntheticModule(['bar'], () => { + m.setExport('bar', new Array(512).fill('----')); + }); + await m.link(() => {}); + await m.evaluate(); + if (count++ < 4 * 1024) { + setTimeout(createModule, 1); + } + return m; +} + +createModule(); From 084959984d864cb82707d36f965aac9916688a42 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 23 Jun 2023 18:32:57 +0200 Subject: [PATCH 15/22] module: fix the leak in SourceTextModule and ContextifySript Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: https://github.com/nodejs/node/pull/48510 Refs: https://github.com/nodejs/node/issues/44211 Refs: https://github.com/nodejs/node/issues/42080 Refs: https://github.com/nodejs/node/issues/47096 Refs: https://github.com/nodejs/node/issues/43205 Refs: https://github.com/nodejs/node/issues/38695 Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Stephen Belanger --- src/module_wrap.cc | 9 +++++--- src/module_wrap.h | 3 ++- src/node_contextify.cc | 4 ++++ src/node_contextify.h | 5 ++++ .../test-vm-contextified-script-leak.js | 19 +++++++++++++++ .../test-vm-source-text-module-leak.js | 23 +++++++++++++++++++ 6 files changed, 59 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-vm-contextified-script-leak.js create mode 100644 test/es-module/test-vm-source-text-module-leak.js diff --git a/src/module_wrap.cc b/src/module_wrap.cc index b1fbbee5257400..cf9425f4786144 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -55,7 +55,10 @@ ModuleWrap::ModuleWrap(Environment* env, Local url, Local context_object, Local synthetic_evaluation_step) - : BaseObject(env, object), module_(env->isolate(), module) { + : BaseObject(env, object), + module_(env->isolate(), module), + module_hash_(module->GetIdentityHash()) { + object->SetInternalFieldForNodeCore(kModuleSlot, module); object->SetInternalField(kURLSlot, url); object->SetInternalField(kSyntheticEvaluationStepsSlot, synthetic_evaluation_step); @@ -65,12 +68,12 @@ ModuleWrap::ModuleWrap(Environment* env, synthetic_ = true; } MakeWeak(); + module_.SetWeak(); } ModuleWrap::~ModuleWrap() { HandleScope scope(env()->isolate()); - Local module = module_.Get(env()->isolate()); - auto range = env()->hash_to_module_map.equal_range(module->GetIdentityHash()); + auto range = env()->hash_to_module_map.equal_range(module_hash_); for (auto it = range.first; it != range.second; ++it) { if (it->second == this) { env()->hash_to_module_map.erase(it); diff --git a/src/module_wrap.h b/src/module_wrap.h index 3d3c646baa5011..1fc801edced9c5 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -33,7 +33,7 @@ enum HostDefinedOptions : int { class ModuleWrap : public BaseObject { public: enum InternalFields { - kModuleWrapBaseField = BaseObject::kInternalFieldCount, + kModuleSlot = BaseObject::kInternalFieldCount, kURLSlot, kSyntheticEvaluationStepsSlot, kContextObjectSlot, // Object whose creation context is the target Context @@ -106,6 +106,7 @@ class ModuleWrap : public BaseObject { contextify::ContextifyContext* contextify_context_ = nullptr; bool synthetic_ = false; bool linked_ = false; + int module_hash_; }; } // namespace loader diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 7f174dbe9c829b..f631d26e0d0117 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -871,7 +871,11 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { "ContextifyScript::New"); return; } + contextify_script->script_.Reset(isolate, v8_script); + contextify_script->script_.SetWeak(); + contextify_script->object()->SetInternalFieldForNodeCore(kUnboundScriptSlot, + v8_script); std::unique_ptr new_cached_data; if (produce_cached_data) { diff --git a/src/node_contextify.h b/src/node_contextify.h index 575f542ca7aa5a..771de6edb515d2 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -149,6 +149,11 @@ class ContextifyContext : public BaseObject { class ContextifyScript : public BaseObject { public: + enum InternalFields { + kUnboundScriptSlot = BaseObject::kInternalFieldCount, + kInternalFieldCount + }; + SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(ContextifyScript) SET_SELF_SIZE(ContextifyScript) diff --git a/test/es-module/test-vm-contextified-script-leak.js b/test/es-module/test-vm-contextified-script-leak.js new file mode 100644 index 00000000000000..7498b46ab80cfa --- /dev/null +++ b/test/es-module/test-vm-contextified-script-leak.js @@ -0,0 +1,19 @@ +// Flags: --max-old-space-size=16 --trace-gc +'use strict'; + +// This tests that vm.Script with dynamic import callback does not leak. +// See: https://github.com/nodejs/node/issues/33439 +require('../common'); +const vm = require('vm'); +let count = 0; + +function main() { + // Try to reach the maximum old space size. + new vm.Script(`"${Math.random().toString().repeat(512)}";`, { + async importModuleDynamically() {}, + }); + if (count++ < 2 * 1024) { + setTimeout(main, 1); + } +} +main(); diff --git a/test/es-module/test-vm-source-text-module-leak.js b/test/es-module/test-vm-source-text-module-leak.js new file mode 100644 index 00000000000000..bf7f70c670e34c --- /dev/null +++ b/test/es-module/test-vm-source-text-module-leak.js @@ -0,0 +1,23 @@ +// Flags: --experimental-vm-modules --max-old-space-size=16 --trace-gc +'use strict'; + +// This tests that vm.SourceTextModule() does not leak. +// See: https://github.com/nodejs/node/issues/33439 +require('../common'); + +const vm = require('vm'); +let count = 0; +async function createModule() { + // Try to reach the maximum old space size. + const m = new vm.SourceTextModule(` + const bar = new Array(512).fill("----"); + export { bar }; +`); + await m.link(() => {}); + await m.evaluate(); + if (count++ < 4096) { + setTimeout(createModule, 1); + } + return m; +} +createModule(); From 2feffae731a3e5c876bf9862c1a937c6c98bb4ad Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 16 Sep 2023 15:07:30 +0200 Subject: [PATCH 16/22] test: add checkIfCollectable to test/common/gc.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/49671 Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli Reviewed-By: Michaël Zasso --- test/common/gc.js | 70 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 test/common/gc.js diff --git a/test/common/gc.js b/test/common/gc.js new file mode 100644 index 00000000000000..af637af7bedcd6 --- /dev/null +++ b/test/common/gc.js @@ -0,0 +1,70 @@ +'use strict'; + +// TODO(joyeecheung): merge ongc.js and gcUntil from common/index.js +// into this. + +// This function can be used to check if an object factor leaks or not, +// but it needs to be used with care: +// 1. The test should be set up with an ideally small +// --max-old-space-size or --max-heap-size, which combined with +// the maxCount parameter can reproduce a leak of the objects +// created by fn(). +// 2. This works under the assumption that if *none* of the objects +// created by fn() can be garbage-collected, the test would crash due +// to OOM. +// 3. If *any* of the objects created by fn() can be garbage-collected, +// it is considered leak-free. The FinalizationRegistry is used to +// terminate the test early once we detect any of the object is +// garbage-collected to make the test less prone to false positives. +// This may be especially important for memory management relying on +// emphemeron GC which can be inefficient to deal with extremely fast +// heap growth. +// Note that this can still produce false positives. When the test using +// this function still crashes due to OOM, inspect the heap to confirm +// if a leak is present (e.g. using heap snapshots). +// The generateSnapshotAt parameter can be used to specify a count +// interval to create the heap snapshot which may enforce a more thorough GC. +// This can be tried for code paths that require it for the GC to catch up +// with heap growth. However this type of forced GC can be in conflict with +// other logic in V8 such as bytecode aging, and it can slow down the test +// significantly, so it should be used scarcely and only as a last resort. +async function checkIfCollectable( + fn, maxCount = 4096, generateSnapshotAt = Infinity, logEvery = 128) { + let anyFinalized = false; + let count = 0; + + const f = new FinalizationRegistry(() => { + anyFinalized = true; + }); + + async function createObject() { + const obj = await fn(); + f.register(obj); + if (count++ < maxCount && !anyFinalized) { + setImmediate(createObject, 1); + } + // This can force a more thorough GC, but can slow the test down + // significantly in a big heap. Use it with care. + if (count % generateSnapshotAt === 0) { + // XXX(joyeecheung): This itself can consume a bit of JS heap memory, + // but the other alternative writeHeapSnapshot can run into disk space + // not enough problems in the CI & be slower depending on file system. + // Just do this for now as long as it works and only invent some + // internal voodoo when we absolutely have no other choice. + require('v8').getHeapSnapshot().pause().read(); + console.log(`Generated heap snapshot at ${count}`); + } + if (count % logEvery === 0) { + console.log(`Created ${count} objects`); + } + if (anyFinalized) { + console.log(`Found finalized object at ${count}, stop testing`); + } + } + + createObject(); +} + +module.exports = { + checkIfCollectable, +}; From 466c30498ed05474d4d32793332fc6cdf55cdcad Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 16 Sep 2023 15:08:18 +0200 Subject: [PATCH 17/22] test: use checkIfCollectable in vm leak tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously we simply create a lot of the target objects and check if the process crash due to OOM. Due to how we use emphemeron GC to handle memory management, which is inefficient but necessary for correctness, the tests can produce false positives as the GC isn't efficient enough to catch up with a very fast heap growth. This patch uses a new checkIfCollectable() utility to terminate the test early once we detect that any of the target object can actually be garbage collected. This should lower the chance of false positives. As a drive-by this also allows us to use setImmediate() to grow the heap even faster to make the tests run faster. PR-URL: https://github.com/nodejs/node/pull/49671 Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli Reviewed-By: Michaël Zasso --- .../es-module/test-vm-compile-function-leak.js | 13 +++++-------- .../test-vm-contextified-script-leak.js | 11 ++++------- .../test-vm-source-text-module-leak.js | 18 ++++++++---------- .../es-module/test-vm-synthetic-module-leak.js | 11 +++-------- 4 files changed, 20 insertions(+), 33 deletions(-) diff --git a/test/es-module/test-vm-compile-function-leak.js b/test/es-module/test-vm-compile-function-leak.js index ff061cdaec7a01..f9f04588fdc7c3 100644 --- a/test/es-module/test-vm-compile-function-leak.js +++ b/test/es-module/test-vm-compile-function-leak.js @@ -4,16 +4,13 @@ // This tests that vm.compileFunction with dynamic import callback does not leak. // See https://github.com/nodejs/node/issues/44211 require('../common'); +const { checkIfCollectable } = require('../common/gc'); const vm = require('vm'); -let count = 0; -function main() { - // Try to reach the maximum old space size. - vm.compileFunction(`"${Math.random().toString().repeat(512)}"`, [], { +async function createCompiledFunction() { + return vm.compileFunction(`"${Math.random().toString().repeat(512)}"`, [], { async importModuleDynamically() {}, }); - if (count++ < 2048) { - setTimeout(main, 1); - } } -main(); + +checkIfCollectable(createCompiledFunction, 2048); diff --git a/test/es-module/test-vm-contextified-script-leak.js b/test/es-module/test-vm-contextified-script-leak.js index 7498b46ab80cfa..60212dd4bbbf68 100644 --- a/test/es-module/test-vm-contextified-script-leak.js +++ b/test/es-module/test-vm-contextified-script-leak.js @@ -4,16 +4,13 @@ // This tests that vm.Script with dynamic import callback does not leak. // See: https://github.com/nodejs/node/issues/33439 require('../common'); +const { checkIfCollectable } = require('../common/gc'); const vm = require('vm'); -let count = 0; -function main() { +async function createContextifyScript() { // Try to reach the maximum old space size. - new vm.Script(`"${Math.random().toString().repeat(512)}";`, { + return new vm.Script(`"${Math.random().toString().repeat(512)}";`, { async importModuleDynamically() {}, }); - if (count++ < 2 * 1024) { - setTimeout(main, 1); - } } -main(); +checkIfCollectable(createContextifyScript, 2048); diff --git a/test/es-module/test-vm-source-text-module-leak.js b/test/es-module/test-vm-source-text-module-leak.js index bf7f70c670e34c..d05e812ac32c95 100644 --- a/test/es-module/test-vm-source-text-module-leak.js +++ b/test/es-module/test-vm-source-text-module-leak.js @@ -4,20 +4,18 @@ // This tests that vm.SourceTextModule() does not leak. // See: https://github.com/nodejs/node/issues/33439 require('../common'); - +const { checkIfCollectable } = require('../common/gc'); const vm = require('vm'); -let count = 0; -async function createModule() { + +async function createSourceTextModule() { // Try to reach the maximum old space size. const m = new vm.SourceTextModule(` - const bar = new Array(512).fill("----"); - export { bar }; -`); + const bar = new Array(512).fill("----"); + export { bar }; + `); await m.link(() => {}); await m.evaluate(); - if (count++ < 4096) { - setTimeout(createModule, 1); - } return m; } -createModule(); + +checkIfCollectable(createSourceTextModule, 4096, 1024); diff --git a/test/es-module/test-vm-synthetic-module-leak.js b/test/es-module/test-vm-synthetic-module-leak.js index 9de02cb22f1128..bc0e4689535327 100644 --- a/test/es-module/test-vm-synthetic-module-leak.js +++ b/test/es-module/test-vm-synthetic-module-leak.js @@ -4,20 +4,15 @@ // This tests that vm.SyntheticModule does not leak. // See https://github.com/nodejs/node/issues/44211 require('../common'); +const { checkIfCollectable } = require('../common/gc'); const vm = require('vm'); -let count = 0; -async function createModule() { - // Try to reach the maximum old space size. +async function createSyntheticModule() { const m = new vm.SyntheticModule(['bar'], () => { m.setExport('bar', new Array(512).fill('----')); }); await m.link(() => {}); await m.evaluate(); - if (count++ < 4 * 1024) { - setTimeout(createModule, 1); - } return m; } - -createModule(); +checkIfCollectable(createSyntheticModule, 4096); From fc003be91a843bbddb40bce547670d948320db29 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 23 Sep 2023 05:46:22 +0200 Subject: [PATCH 18/22] test: deflake test-vm-contextified-script-leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Similar to the test-vm-source-text-module-leak fix, use a snapshot to force a thorough GC in order to prevent false positives. PR-URL: https://github.com/nodejs/node/pull/49710 Refs: https://github.com/nodejs/reliability/issues/669 Reviewed-By: Franziska Hinkelmann Reviewed-By: Michaël Zasso Reviewed-By: Luigi Pinca Reviewed-By: Rich Trott --- test/es-module/test-vm-contextified-script-leak.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-vm-contextified-script-leak.js b/test/es-module/test-vm-contextified-script-leak.js index 60212dd4bbbf68..a8625fc94d246e 100644 --- a/test/es-module/test-vm-contextified-script-leak.js +++ b/test/es-module/test-vm-contextified-script-leak.js @@ -13,4 +13,4 @@ async function createContextifyScript() { async importModuleDynamically() {}, }); } -checkIfCollectable(createContextifyScript, 2048); +checkIfCollectable(createContextifyScript, 2048, 512); From d1831879ed16a7988b1d447406d22f524c8817ff Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 5 Oct 2023 02:11:04 +0200 Subject: [PATCH 19/22] vm: use default HDO when importModuleDynamically is not set This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: https://github.com/nodejs/node/pull/49950 Refs: https://github.com/nodejs/node/issues/35375 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chengzhong Wu Reviewed-By: Stephen Belanger --- .../vm/compile-script-in-isolate-cache.js | 35 +++++++++++++++++++ lib/internal/modules/esm/utils.js | 11 ++++++ lib/vm.js | 11 ++++-- src/env_properties.h | 1 + src/node_contextify.cc | 16 +++++++-- .../test-vm-no-dynamic-import-callback.js | 13 +++++++ 6 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 benchmark/vm/compile-script-in-isolate-cache.js create mode 100644 test/parallel/test-vm-no-dynamic-import-callback.js diff --git a/benchmark/vm/compile-script-in-isolate-cache.js b/benchmark/vm/compile-script-in-isolate-cache.js new file mode 100644 index 00000000000000..7eceb0eba0d215 --- /dev/null +++ b/benchmark/vm/compile-script-in-isolate-cache.js @@ -0,0 +1,35 @@ +'use strict'; + +// This benchmarks compiling scripts that hit the in-isolate compilation +// cache (by having the same source). +const common = require('../common.js'); +const fs = require('fs'); +const vm = require('vm'); +const fixtures = require('../../test/common/fixtures.js'); +const scriptPath = fixtures.path('snapshot', 'typescript.js'); + +const bench = common.createBenchmark(main, { + type: ['with-dynamic-import-callback', 'without-dynamic-import-callback'], + n: [100], +}); + +const scriptSource = fs.readFileSync(scriptPath, 'utf8'); + +function main({ n, type }) { + let script; + bench.start(); + const options = {}; + switch (type) { + case 'with-dynamic-import-callback': + // Use a dummy callback for now until we really need to benchmark it. + options.importModuleDynamically = async () => {}; + break; + case 'without-dynamic-import-callback': + break; + } + for (let i = 0; i < n; i++) { + script = new vm.Script(scriptSource, options); + } + bench.end(n); + script.runInThisContext(); +} diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index b6b98569849ef4..365d7ed7136b94 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -12,6 +12,10 @@ const { host_defined_option_symbol, }, } = internalBinding('util'); +const { + default_host_defined_options, +} = internalBinding('symbols'); + const { ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING, ERR_INVALID_ARG_VALUE, @@ -128,6 +132,13 @@ const moduleRegistries = new SafeWeakMap(); */ function registerModule(referrer, registry) { const idSymbol = referrer[host_defined_option_symbol]; + if (idSymbol === default_host_defined_options) { + // The referrer is compiled without custom callbacks, so there is + // no registry to hold on to. We'll throw + // ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING when a callback is + // needed. + return; + } // To prevent it from being GC'ed. registry.callbackReferrer ??= referrer; moduleRegistries.set(idSymbol, registry); diff --git a/lib/vm.js b/lib/vm.js index 5ca04d6fb41758..06e72114f2361a 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -87,6 +87,12 @@ class Script extends ContextifyScript { } validateBoolean(produceCachedData, 'options.produceCachedData'); + if (importModuleDynamically !== undefined) { + // Check that it's either undefined or a function before we pass + // it into the native constructor. + validateFunction(importModuleDynamically, + 'options.importModuleDynamically'); + } // Calling `ReThrow()` on a native TryCatch does not generate a new // abort-on-uncaught-exception check. A dummy try/catch in JS land // protects against that. @@ -97,14 +103,13 @@ class Script extends ContextifyScript { columnOffset, cachedData, produceCachedData, - parsingContext); + parsingContext, + importModuleDynamically !== undefined); } catch (e) { throw e; /* node-do-not-add-exception-line */ } if (importModuleDynamically !== undefined) { - validateFunction(importModuleDynamically, - 'options.importModuleDynamically'); const { importModuleDynamicallyWrap } = require('internal/vm/module'); const { registerModule } = require('internal/modules/esm/utils'); registerModule(this, { diff --git a/src/env_properties.h b/src/env_properties.h index 47d41826014c48..c00c1987457712 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -31,6 +31,7 @@ // Symbols are per-isolate primitives but Environment proxies them // for the sake of convenience. #define PER_ISOLATE_SYMBOL_PROPERTIES(V) \ + V(default_host_defined_options, "default_host_defined_options") \ V(fs_use_promises_symbol, "fs_use_promises_symbol") \ V(async_id_symbol, "async_id_symbol") \ V(handle_onclose_symbol, "handle_onclose") \ diff --git a/src/node_contextify.cc b/src/node_contextify.cc index f631d26e0d0117..8b58fa7a2bccb4 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -787,10 +787,12 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { bool produce_cached_data = false; Local parsing_context = context; + bool needs_custom_host_defined_options = false; if (argc > 2) { // new ContextifyScript(code, filename, lineOffset, columnOffset, - // cachedData, produceCachedData, parsingContext) - CHECK_EQ(argc, 7); + // cachedData, produceCachedData, parsingContext, + // needsCustomHostDefinedOptions) + CHECK_EQ(argc, 8); CHECK(args[2]->IsNumber()); line_offset = args[2].As()->Value(); CHECK(args[3]->IsNumber()); @@ -809,6 +811,9 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(sandbox); parsing_context = sandbox->context(); } + if (args[7]->IsTrue()) { + needs_custom_host_defined_options = true; + } } ContextifyScript* contextify_script = @@ -832,7 +837,12 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { Local host_defined_options = PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - Local id_symbol = Symbol::New(isolate, filename); + // We need a default host defined options that's the same for all scripts + // not needing custom module callbacks for so that the isolate compilation + // cache can be hit. + Local id_symbol = needs_custom_host_defined_options + ? Symbol::New(isolate, filename) + : env->default_host_defined_options(); host_defined_options->Set( isolate, loader::HostDefinedOptions::kID, id_symbol); diff --git a/test/parallel/test-vm-no-dynamic-import-callback.js b/test/parallel/test-vm-no-dynamic-import-callback.js new file mode 100644 index 00000000000000..3651f997598b21 --- /dev/null +++ b/test/parallel/test-vm-no-dynamic-import-callback.js @@ -0,0 +1,13 @@ +'use strict'; + +require('../common'); +const { Script } = require('vm'); +const assert = require('assert'); + +assert.rejects(async () => { + const script = new Script('import("fs")'); + const imported = script.runInThisContext(); + await imported; +}, { + code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING' +}); From c8be386a6e0a4f5de4fc70d018f4f263d7a74bb5 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 5 Oct 2023 03:27:11 +0200 Subject: [PATCH 20/22] vm: unify host-defined option generation in vm.compileFunction Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: https://github.com/nodejs/node/pull/50137 Refs: https://github.com/nodejs/node/issues/35375 Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Chengzhong Wu Reviewed-By: Antoine du Hamel --- lib/internal/vm.js | 25 ++++++++++++++++++- lib/vm.js | 12 +++------ src/node_contextify.cc | 20 ++++++--------- .../test-vm-no-dynamic-import-callback.js | 13 +++++++--- 4 files changed, 46 insertions(+), 24 deletions(-) diff --git a/lib/internal/vm.js b/lib/internal/vm.js index 70dbd866e66bd7..7ad76ebc873f19 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -2,12 +2,16 @@ const { ArrayPrototypeForEach, + Symbol, } = primordials; const { compileFunction, isContext: _isContext, } = internalBinding('contextify'); +const { + default_host_defined_options, +} = internalBinding('symbols'); const { validateArray, validateBoolean, @@ -28,12 +32,27 @@ function isContext(object) { return _isContext(object); } +function getHostDefinedOptionId(importModuleDynamically, filename) { + if (importModuleDynamically !== undefined) { + // Check that it's either undefined or a function before we pass + // it into the native constructor. + validateFunction(importModuleDynamically, + 'options.importModuleDynamically'); + } + if (importModuleDynamically === undefined) { + // We need a default host defined options that are the same for all + // scripts not needing custom module callbacks so that the isolate + // compilation cache can be hit. + return default_host_defined_options; + } + return Symbol(filename); +} + function internalCompileFunction(code, params, options) { validateString(code, 'code'); if (params !== undefined) { validateStringArray(params, 'params'); } - const { filename = '', columnOffset = 0, @@ -70,6 +89,8 @@ function internalCompileFunction(code, params, options) { validateObject(extension, name, { __proto__: null, nullable: true }); }); + const hostDefinedOptionId = + getHostDefinedOptionId(importModuleDynamically, filename); const result = compileFunction( code, filename, @@ -80,6 +101,7 @@ function internalCompileFunction(code, params, options) { parsingContext, contextExtensions, params, + hostDefinedOptionId, ); if (produceCachedData) { @@ -111,6 +133,7 @@ function internalCompileFunction(code, params, options) { } module.exports = { + getHostDefinedOptionId, internalCompileFunction, isContext, }; diff --git a/lib/vm.js b/lib/vm.js index 06e72114f2361a..833a6e547b09c2 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -42,7 +42,6 @@ const { const { validateBoolean, validateBuffer, - validateFunction, validateInt32, validateObject, validateOneOf, @@ -55,6 +54,7 @@ const { kVmBreakFirstLineSymbol, } = require('internal/util'); const { + getHostDefinedOptionId, internalCompileFunction, isContext, } = require('internal/vm'); @@ -87,12 +87,8 @@ class Script extends ContextifyScript { } validateBoolean(produceCachedData, 'options.produceCachedData'); - if (importModuleDynamically !== undefined) { - // Check that it's either undefined or a function before we pass - // it into the native constructor. - validateFunction(importModuleDynamically, - 'options.importModuleDynamically'); - } + const hostDefinedOptionId = + getHostDefinedOptionId(importModuleDynamically, filename); // Calling `ReThrow()` on a native TryCatch does not generate a new // abort-on-uncaught-exception check. A dummy try/catch in JS land // protects against that. @@ -104,7 +100,7 @@ class Script extends ContextifyScript { cachedData, produceCachedData, parsingContext, - importModuleDynamically !== undefined); + hostDefinedOptionId); } catch (e) { throw e; /* node-do-not-add-exception-line */ } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 8b58fa7a2bccb4..65a7acbcfb6bc6 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -787,11 +787,11 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { bool produce_cached_data = false; Local parsing_context = context; - bool needs_custom_host_defined_options = false; + Local id_symbol; if (argc > 2) { // new ContextifyScript(code, filename, lineOffset, columnOffset, // cachedData, produceCachedData, parsingContext, - // needsCustomHostDefinedOptions) + // hostDefinedOptionId) CHECK_EQ(argc, 8); CHECK(args[2]->IsNumber()); line_offset = args[2].As()->Value(); @@ -811,9 +811,8 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(sandbox); parsing_context = sandbox->context(); } - if (args[7]->IsTrue()) { - needs_custom_host_defined_options = true; - } + CHECK(args[7]->IsSymbol()); + id_symbol = args[7].As(); } ContextifyScript* contextify_script = @@ -837,12 +836,6 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { Local host_defined_options = PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - // We need a default host defined options that's the same for all scripts - // not needing custom module callbacks for so that the isolate compilation - // cache can be hit. - Local id_symbol = needs_custom_host_defined_options - ? Symbol::New(isolate, filename) - : env->default_host_defined_options(); host_defined_options->Set( isolate, loader::HostDefinedOptions::kID, id_symbol); @@ -1201,6 +1194,10 @@ void ContextifyContext::CompileFunction( params_buf = args[8].As(); } + // Argument 10: host-defined option symbol + CHECK(args[9]->IsSymbol()); + Local id_symbol = args[9].As(); + // Read cache from cached data buffer ScriptCompiler::CachedData* cached_data = nullptr; if (!cached_data_buf.IsEmpty()) { @@ -1212,7 +1209,6 @@ void ContextifyContext::CompileFunction( // Set host_defined_options Local host_defined_options = PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - Local id_symbol = Symbol::New(isolate, filename); host_defined_options->Set( isolate, loader::HostDefinedOptions::kID, id_symbol); diff --git a/test/parallel/test-vm-no-dynamic-import-callback.js b/test/parallel/test-vm-no-dynamic-import-callback.js index 3651f997598b21..35b553d587c6e4 100644 --- a/test/parallel/test-vm-no-dynamic-import-callback.js +++ b/test/parallel/test-vm-no-dynamic-import-callback.js @@ -1,7 +1,7 @@ 'use strict'; -require('../common'); -const { Script } = require('vm'); +const common = require('../common'); +const { Script, compileFunction } = require('vm'); const assert = require('assert'); assert.rejects(async () => { @@ -10,4 +10,11 @@ assert.rejects(async () => { await imported; }, { code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING' -}); +}).then(common.mustCall()); + +assert.rejects(async () => { + const imported = compileFunction('return import("fs")')(); + await imported; +}, { + code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING' +}).then(common.mustCall()); From 222877e99ecc5d50a3dcb8f3641f9c480286fd6a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 11 Oct 2023 04:50:50 +0200 Subject: [PATCH 21/22] vm: use internal versions of compileFunction and Script Instead of using the public versions of the vm APIs internally, use the internal versions so that we can skip unnecessary argument validation. The public versions would need special care to the generation of host-defined options to hit the isolate compilation cache when imporModuleDynamically isn't used, while internally it's almost always used, so this allows us to handle the host-defined options separately. PR-URL: https://github.com/nodejs/node/pull/50137 Refs: https://github.com/nodejs/node/issues/35375 Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Chengzhong Wu Reviewed-By: Antoine du Hamel --- lib/internal/modules/cjs/loader.js | 72 +++++++++------- lib/internal/process/execution.js | 37 +++++--- lib/internal/vm.js | 130 +++++++++++++++-------------- lib/repl.js | 85 ++++++++++--------- lib/vm.js | 62 ++++++++++++-- test/message/eval_messages.out | 14 ++-- test/message/stdin_messages.out | 18 ++-- 7 files changed, 245 insertions(+), 173 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 316996a8c329a1..69473df97ede09 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -53,6 +53,7 @@ const { SafeMap, SafeWeakMap, String, + Symbol, StringPrototypeCharAt, StringPrototypeCharCodeAt, StringPrototypeEndsWith, @@ -85,7 +86,12 @@ const { setOwnProperty, getLazy, } = require('internal/util'); -const { internalCompileFunction } = require('internal/vm'); +const { + internalCompileFunction, + makeContextifyScript, + runScriptInThisContext, +} = require('internal/vm'); + const assert = require('internal/assert'); const fs = require('fs'); const path = require('path'); @@ -1236,7 +1242,6 @@ Module.prototype.require = function(id) { let resolvedArgv; let hasPausedEntry = false; /** @type {import('vm').Script} */ -let Script; /** * Wraps the given content in a script and runs it in a new context. @@ -1245,46 +1250,49 @@ let Script; * @param {Module} cjsModuleInstance The CommonJS loader instance */ function wrapSafe(filename, content, cjsModuleInstance) { + const hostDefinedOptionId = Symbol(`cjs:${filename}`); + async function importModuleDynamically(specifier, _, importAttributes) { + const cascadedLoader = getCascadedLoader(); + return cascadedLoader.import(specifier, normalizeReferrerURL(filename), + importAttributes); + } if (patched) { - const wrapper = Module.wrap(content); - if (Script === undefined) { - ({ Script } = require('vm')); - } - const script = new Script(wrapper, { - filename, - lineOffset: 0, - importModuleDynamically: async (specifier, _, importAttributes) => { - const cascadedLoader = getCascadedLoader(); - return cascadedLoader.import(specifier, normalizeReferrerURL(filename), - importAttributes); - }, - }); + const wrapped = Module.wrap(content); + const script = makeContextifyScript( + wrapped, // code + filename, // filename + 0, // lineOffset + 0, // columnOffset + undefined, // cachedData + false, // produceCachedData + undefined, // parsingContext + hostDefinedOptionId, // hostDefinedOptionId + importModuleDynamically, // importModuleDynamically + ); // Cache the source map for the module if present. if (script.sourceMapURL) { maybeCacheSourceMap(filename, content, this, false, undefined, script.sourceMapURL); } - return script.runInThisContext({ - displayErrors: true, - }); + return runScriptInThisContext(script, true, false); } + const params = [ 'exports', 'require', 'module', '__filename', '__dirname' ]; try { - const result = internalCompileFunction(content, [ - 'exports', - 'require', - 'module', - '__filename', - '__dirname', - ], { - filename, - importModuleDynamically(specifier, _, importAttributes) { - const cascadedLoader = getCascadedLoader(); - return cascadedLoader.import(specifier, normalizeReferrerURL(filename), - importAttributes); - }, - }); + const result = internalCompileFunction( + content, // code, + filename, // filename + 0, // lineOffset + 0, // columnOffset, + undefined, // cachedData + false, // produceCachedData + undefined, // parsingContext + undefined, // contextExtensions + params, // params + hostDefinedOptionId, // hostDefinedOptionId + importModuleDynamically, // importModuleDynamically + ); // Cache the source map for the module if present. if (result.sourceMapURL) { diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index c3a63f63d1d613..838ed93a3e2afb 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -1,6 +1,7 @@ 'use strict'; const { + Symbol, RegExpPrototypeExec, globalThis, } = primordials; @@ -24,7 +25,9 @@ const { emitAfter, popAsyncContext, } = require('internal/async_hooks'); - +const { + makeContextifyScript, runScriptInThisContext, +} = require('internal/vm'); // shouldAbortOnUncaughtToggle is a typed array for faster // communication with JS. const { shouldAbortOnUncaughtToggle } = internalBinding('util'); @@ -52,8 +55,7 @@ function evalModule(source, print) { function evalScript(name, body, breakFirstLine, print, shouldLoadESM = false) { const CJSModule = require('internal/modules/cjs/loader').Module; - const { kVmBreakFirstLineSymbol } = require('internal/util'); - const { pathToFileURL } = require('url'); + const { pathToFileURL } = require('internal/url'); const cwd = tryGetCwd(); const origModule = globalThis.module; // Set e.g. when called from the REPL. @@ -78,16 +80,25 @@ function evalScript(name, body, breakFirstLine, print, shouldLoadESM = false) { `; globalThis.__filename = name; RegExpPrototypeExec(/^/, ''); // Necessary to reset RegExp statics before user code runs. - const result = module._compile(script, `${name}-wrapper`)(() => - require('vm').runInThisContext(body, { - filename: name, - displayErrors: true, - [kVmBreakFirstLineSymbol]: !!breakFirstLine, - importModuleDynamically(specifier, _, importAttributes) { - const loader = asyncESM.esmLoader; - return loader.import(specifier, baseUrl, importAttributes); - }, - })); + const result = module._compile(script, `${name}-wrapper`)(() => { + const hostDefinedOptionId = Symbol(name); + async function importModuleDynamically(specifier, _, importAttributes) { + const loader = asyncESM.esmLoader; + return loader.import(specifier, baseUrl, importAttributes); + } + const script = makeContextifyScript( + body, // code + name, // filename, + 0, // lineOffset + 0, // columnOffset, + undefined, // cachedData + false, // produceCachedData + undefined, // parsingContext + hostDefinedOptionId, // hostDefinedOptionId + importModuleDynamically, // importModuleDynamically + ); + return runScriptInThisContext(script, true, !!breakFirstLine); + }); if (print) { const { log } = require('internal/console/global'); log(result); diff --git a/lib/internal/vm.js b/lib/internal/vm.js index 7ad76ebc873f19..780f996b320aca 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -1,30 +1,25 @@ 'use strict'; const { - ArrayPrototypeForEach, + ReflectApply, Symbol, } = primordials; const { + ContextifyScript, compileFunction, isContext: _isContext, } = internalBinding('contextify'); +const { + runInContext, +} = ContextifyScript.prototype; const { default_host_defined_options, } = internalBinding('symbols'); const { - validateArray, - validateBoolean, - validateBuffer, validateFunction, validateObject, - validateString, - validateStringArray, - validateInt32, } = require('internal/validators'); -const { - ERR_INVALID_ARG_TYPE, -} = require('internal/errors').codes; function isContext(object) { validateObject(object, 'object', { __proto__: null, allowArray: true }); @@ -48,49 +43,20 @@ function getHostDefinedOptionId(importModuleDynamically, filename) { return Symbol(filename); } -function internalCompileFunction(code, params, options) { - validateString(code, 'code'); - if (params !== undefined) { - validateStringArray(params, 'params'); - } - const { - filename = '', - columnOffset = 0, - lineOffset = 0, - cachedData = undefined, - produceCachedData = false, - parsingContext = undefined, - contextExtensions = [], - importModuleDynamically, - } = options; - - validateString(filename, 'options.filename'); - validateInt32(columnOffset, 'options.columnOffset'); - validateInt32(lineOffset, 'options.lineOffset'); - if (cachedData !== undefined) - validateBuffer(cachedData, 'options.cachedData'); - validateBoolean(produceCachedData, 'options.produceCachedData'); - if (parsingContext !== undefined) { - if ( - typeof parsingContext !== 'object' || - parsingContext === null || - !isContext(parsingContext) - ) { - throw new ERR_INVALID_ARG_TYPE( - 'options.parsingContext', - 'Context', - parsingContext, - ); - } - } - validateArray(contextExtensions, 'options.contextExtensions'); - ArrayPrototypeForEach(contextExtensions, (extension, i) => { - const name = `options.contextExtensions[${i}]`; - validateObject(extension, name, { __proto__: null, nullable: true }); +function registerImportModuleDynamically(referrer, importModuleDynamically) { + const { importModuleDynamicallyWrap } = require('internal/vm/module'); + const { registerModule } = require('internal/modules/esm/utils'); + registerModule(referrer, { + __proto__: null, + importModuleDynamically: + importModuleDynamicallyWrap(importModuleDynamically), }); +} - const hostDefinedOptionId = - getHostDefinedOptionId(importModuleDynamically, filename); +function internalCompileFunction( + code, filename, lineOffset, columnOffset, + cachedData, produceCachedData, parsingContext, contextExtensions, + params, hostDefinedOptionId, importModuleDynamically) { const result = compileFunction( code, filename, @@ -117,23 +83,65 @@ function internalCompileFunction(code, params, options) { } if (importModuleDynamically !== undefined) { - validateFunction(importModuleDynamically, - 'options.importModuleDynamically'); - const { importModuleDynamicallyWrap } = require('internal/vm/module'); - const wrapped = importModuleDynamicallyWrap(importModuleDynamically); - const func = result.function; - const { registerModule } = require('internal/modules/esm/utils'); - registerModule(func, { - __proto__: null, - importModuleDynamically: wrapped, - }); + registerImportModuleDynamically(result.function, importModuleDynamically); } return result; } +function makeContextifyScript(code, + filename, + lineOffset, + columnOffset, + cachedData, + produceCachedData, + parsingContext, + hostDefinedOptionId, + importModuleDynamically) { + let script; + // Calling `ReThrow()` on a native TryCatch does not generate a new + // abort-on-uncaught-exception check. A dummy try/catch in JS land + // protects against that. + try { // eslint-disable-line no-useless-catch + script = new ContextifyScript(code, + filename, + lineOffset, + columnOffset, + cachedData, + produceCachedData, + parsingContext, + hostDefinedOptionId); + } catch (e) { + throw e; /* node-do-not-add-exception-line */ + } + + if (importModuleDynamically !== undefined) { + registerImportModuleDynamically(script, importModuleDynamically); + } + return script; +} + +// Internal version of vm.Script.prototype.runInThisContext() which skips +// argument validation. +function runScriptInThisContext(script, displayErrors, breakOnFirstLine) { + return ReflectApply( + runInContext, + script, + [ + null, // sandbox - use current context + -1, // timeout + displayErrors, // displayErrors + false, // breakOnSigint + breakOnFirstLine, // breakOnFirstLine + ], + ); +} + module.exports = { getHostDefinedOptionId, internalCompileFunction, isContext, + makeContextifyScript, + registerImportModuleDynamically, + runScriptInThisContext, }; diff --git a/lib/repl.js b/lib/repl.js index b2d143619ae093..2e54fabba2cff3 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -118,6 +118,9 @@ const { } = require('internal/util'); const { inspect } = require('internal/util/inspect'); const vm = require('vm'); + +const { runInThisContext, runInContext } = vm.Script.prototype; + const path = require('path'); const fs = require('fs'); const { Interface } = require('readline'); @@ -186,7 +189,9 @@ const { extensionFormatMap, legacyExtensionFormatMap, } = require('internal/modules/esm/formats'); - +const { + makeContextifyScript, +} = require('internal/vm'); let nextREPLResourceNumber = 1; // This prevents v8 code cache from getting confused and using a different // cache from a resource of the same name @@ -430,8 +435,6 @@ function REPLServer(prompt, } function defaultEval(code, context, file, cb) { - const asyncESM = require('internal/process/esm_loader'); - let result, script, wrappedErr; let err = null; let wrappedCmd = false; @@ -449,6 +452,21 @@ function REPLServer(prompt, wrappedCmd = true; } + const hostDefinedOptionId = Symbol(`eval:${file}`); + let parentURL; + try { + const { pathToFileURL } = require('internal/url'); + // Adding `/repl` prevents dynamic imports from loading relative + // to the parent of `process.cwd()`. + parentURL = pathToFileURL(path.join(process.cwd(), 'repl')).href; + } catch { + // Continue regardless of error. + } + async function importModuleDynamically(specifier, _, importAttributes) { + const asyncESM = require('internal/process/esm_loader'); + return asyncESM.esmLoader.import(specifier, parentURL, + importAttributes); + } // `experimentalREPLAwait` is set to true by default. // Shall be false in case `--no-experimental-repl-await` flag is used. if (experimentalREPLAwait && StringPrototypeIncludes(code, 'await')) { @@ -466,28 +484,21 @@ function REPLServer(prompt, } catch (e) { let recoverableError = false; if (e.name === 'SyntaxError') { - let parentURL; - try { - const { pathToFileURL } = require('url'); - // Adding `/repl` prevents dynamic imports from loading relative - // to the parent of `process.cwd()`. - parentURL = pathToFileURL(path.join(process.cwd(), 'repl')).href; - } catch { - // Continue regardless of error. - } - // Remove all "await"s and attempt running the script // in order to detect if error is truly non recoverable const fallbackCode = SideEffectFreeRegExpPrototypeSymbolReplace(/\bawait\b/g, code, ''); try { - vm.createScript(fallbackCode, { - filename: file, - displayErrors: true, - importModuleDynamically: (specifier, _, importAttributes) => { - return asyncESM.esmLoader.import(specifier, parentURL, - importAttributes); - }, - }); + makeContextifyScript( + fallbackCode, // code + file, // filename, + 0, // lineOffset + 0, // columnOffset, + undefined, // cachedData + false, // produceCachedData + undefined, // parsingContext + hostDefinedOptionId, // hostDefinedOptionId + importModuleDynamically, // importModuleDynamically + ); } catch (fallbackError) { if (isRecoverableError(fallbackError, fallbackCode)) { recoverableError = true; @@ -507,15 +518,6 @@ function REPLServer(prompt, return cb(null); if (err === null) { - let parentURL; - try { - const { pathToFileURL } = require('url'); - // Adding `/repl` prevents dynamic imports from loading relative - // to the parent of `process.cwd()`. - parentURL = pathToFileURL(path.join(process.cwd(), 'repl')).href; - } catch { - // Continue regardless of error. - } while (true) { try { if (self.replMode === module.exports.REPL_MODE_STRICT && @@ -524,14 +526,17 @@ function REPLServer(prompt, // value for statements and declarations that don't return a value. code = `'use strict'; void 0;\n${code}`; } - script = vm.createScript(code, { - filename: file, - displayErrors: true, - importModuleDynamically: (specifier, _, importAttributes) => { - return asyncESM.esmLoader.import(specifier, parentURL, - importAttributes); - }, - }); + script = makeContextifyScript( + code, // code + file, // filename, + 0, // lineOffset + 0, // columnOffset, + undefined, // cachedData + false, // produceCachedData + undefined, // parsingContext + hostDefinedOptionId, // hostDefinedOptionId + importModuleDynamically, // importModuleDynamically + ); } catch (e) { debug('parse error %j', code, e); if (wrappedCmd) { @@ -591,9 +596,9 @@ function REPLServer(prompt, }; if (self.useGlobal) { - result = script.runInThisContext(scriptOptions); + result = ReflectApply(runInThisContext, script, [scriptOptions]); } else { - result = script.runInContext(context, scriptOptions); + result = ReflectApply(runInContext, script, [context, scriptOptions]); } } finally { if (self.breakEvalOnSigint) { diff --git a/lib/vm.js b/lib/vm.js index 833a6e547b09c2..6bea7e5e74de9c 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -40,12 +40,14 @@ const { ERR_INVALID_ARG_TYPE, } = require('internal/errors').codes; const { + validateArray, validateBoolean, validateBuffer, validateInt32, - validateObject, validateOneOf, + validateObject, validateString, + validateStringArray, validateUint32, } = require('internal/validators'); const { @@ -57,6 +59,7 @@ const { getHostDefinedOptionId, internalCompileFunction, isContext, + registerImportModuleDynamically, } = require('internal/vm'); const kParsingContext = Symbol('script parsing context'); @@ -106,13 +109,7 @@ class Script extends ContextifyScript { } if (importModuleDynamically !== undefined) { - const { importModuleDynamicallyWrap } = require('internal/vm/module'); - const { registerModule } = require('internal/modules/esm/utils'); - registerModule(this, { - __proto__: null, - importModuleDynamically: - importModuleDynamicallyWrap(importModuleDynamically), - }); + registerImportModuleDynamically(this, importModuleDynamically); } } @@ -301,7 +298,54 @@ function runInThisContext(code, options) { } function compileFunction(code, params, options = kEmptyObject) { - return internalCompileFunction(code, params, options).function; + validateString(code, 'code'); + if (params !== undefined) { + validateStringArray(params, 'params'); + } + const { + filename = '', + columnOffset = 0, + lineOffset = 0, + cachedData = undefined, + produceCachedData = false, + parsingContext = undefined, + contextExtensions = [], + importModuleDynamically, + } = options; + + validateString(filename, 'options.filename'); + validateInt32(columnOffset, 'options.columnOffset'); + validateInt32(lineOffset, 'options.lineOffset'); + if (cachedData !== undefined) + validateBuffer(cachedData, 'options.cachedData'); + validateBoolean(produceCachedData, 'options.produceCachedData'); + if (parsingContext !== undefined) { + if ( + typeof parsingContext !== 'object' || + parsingContext === null || + !isContext(parsingContext) + ) { + throw new ERR_INVALID_ARG_TYPE( + 'options.parsingContext', + 'Context', + parsingContext, + ); + } + } + validateArray(contextExtensions, 'options.contextExtensions'); + ArrayPrototypeForEach(contextExtensions, (extension, i) => { + const name = `options.contextExtensions[${i}]`; + validateObject(extension, name, { __proto__: null, nullable: true }); + }); + + const hostDefinedOptionId = + getHostDefinedOptionId(importModuleDynamically, filename); + + return internalCompileFunction( + code, filename, lineOffset, columnOffset, + cachedData, produceCachedData, parsingContext, contextExtensions, + params, hostDefinedOptionId, importModuleDynamically, + ).function; } const measureMemoryModes = { diff --git a/test/message/eval_messages.out b/test/message/eval_messages.out index 3f44332c03a470..e07bbe4d6acd3c 100644 --- a/test/message/eval_messages.out +++ b/test/message/eval_messages.out @@ -2,10 +2,9 @@ [eval]:1 with(this){__filename} ^^^^ + SyntaxError: Strict mode code may not include a with statement - at new Script (node:vm:*:*) - at createScript (node:vm:*:*) - at Object.runInThisContext (node:vm:*:*) + at makeContextifyScript (node:internal/vm:*:*) at node:internal/process/execution:*:* at [eval]-wrapper:*:* at runScript (node:internal/process/execution:*:*) @@ -21,8 +20,7 @@ throw new Error("hello") Error: hello at [eval]:1:7 - at Script.runInThisContext (node:vm:*:*) - at Object.runInThisContext (node:vm:*:*) + at runScriptInThisContext (node:internal/vm:*:*) at node:internal/process/execution:*:* at [eval]-wrapper:*:* at runScript (node:internal/process/execution:*:*) @@ -37,8 +35,7 @@ throw new Error("hello") Error: hello at [eval]:1:7 - at Script.runInThisContext (node:vm:*:*) - at Object.runInThisContext (node:vm:*:*) + at runScriptInThisContext (node:internal/vm:*:*) at node:internal/process/execution:*:* at [eval]-wrapper:*:* at runScript (node:internal/process/execution:*:*) @@ -53,8 +50,7 @@ var x = 100; y = x; ReferenceError: y is not defined at [eval]:1:16 - at Script.runInThisContext (node:vm:*:*) - at Object.runInThisContext (node:vm:*:*) + at runScriptInThisContext (node:internal/vm:*:*) at node:internal/process/execution:*:* at [eval]-wrapper:*:* at runScript (node:internal/process/execution:*:*) diff --git a/test/message/stdin_messages.out b/test/message/stdin_messages.out index 3fd047aacb11a2..6afc8a62d7fcd9 100644 --- a/test/message/stdin_messages.out +++ b/test/message/stdin_messages.out @@ -4,9 +4,7 @@ with(this){__filename} ^^^^ SyntaxError: Strict mode code may not include a with statement - at new Script (node:vm:*) - at createScript (node:vm:*) - at Object.runInThisContext (node:vm:*) + at makeContextifyScript (node:internal/vm:*:*) at node:internal/process/execution:*:* at [stdin]-wrapper:*:* at runScript (node:internal/process/execution:*:*) @@ -14,6 +12,8 @@ SyntaxError: Strict mode code may not include a with statement at node:internal/main/eval_stdin:*:* at Socket. (node:internal/process/execution:*:*) at Socket.emit (node:events:*:*) + at endReadableNT (node:internal/streams/readable:*:*) + at process.processTicksAndRejections (node:internal/process/task_queues:*:*) Node.js * 42 @@ -24,8 +24,7 @@ throw new Error("hello") Error: hello at [stdin]:1:7 - at Script.runInThisContext (node:vm:*) - at Object.runInThisContext (node:vm:*) + at runScriptInThisContext (node:internal/vm:*:*) at node:internal/process/execution:*:* at [stdin]-wrapper:*:* at runScript (node:internal/process/execution:*:*) @@ -33,6 +32,7 @@ Error: hello at node:internal/main/eval_stdin:*:* at Socket. (node:internal/process/execution:*:*) at Socket.emit (node:events:*:*) + at endReadableNT (node:internal/streams/readable:*:*) Node.js * [stdin]:1 @@ -41,8 +41,7 @@ throw new Error("hello") Error: hello at [stdin]:1:* - at Script.runInThisContext (node:vm:*) - at Object.runInThisContext (node:vm:*) + at runScriptInThisContext (node:internal/vm:*:*) at node:internal/process/execution:*:* at [stdin]-wrapper:*:* at runScript (node:internal/process/execution:*:*) @@ -50,6 +49,7 @@ Error: hello at node:internal/main/eval_stdin:*:* at Socket. (node:internal/process/execution:*:*) at Socket.emit (node:events:*:*) + at endReadableNT (node:internal/streams/readable:*:*) Node.js * 100 @@ -59,8 +59,7 @@ let x = 100; y = x; ReferenceError: y is not defined at [stdin]:1:16 - at Script.runInThisContext (node:vm:*) - at Object.runInThisContext (node:vm:*) + at runScriptInThisContext (node:internal/vm:*:*) at node:internal/process/execution:*:* at [stdin]-wrapper:*:* at runScript (node:internal/process/execution:*:*) @@ -68,6 +67,7 @@ ReferenceError: y is not defined at node:internal/main/eval_stdin:*:* at Socket. (node:internal/process/execution:*:*) at Socket.emit (node:events:*:*) + at endReadableNT (node:internal/streams/readable:*:*) Node.js * From a2b3f6ee5a1c82783aba807fa841f815fb7e9dec Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 5 Oct 2023 23:08:44 +0200 Subject: [PATCH 22/22] vm: reject in importModuleDynamically without --experimental-vm-modules Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: https://github.com/nodejs/node/pull/50137 Refs: https://github.com/nodejs/node/issues/35375 Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Chengzhong Wu Reviewed-By: Antoine du Hamel --- doc/api/errors.md | 6 ++++ doc/api/vm.md | 24 ++++++++++++---- lib/internal/errors.js | 3 ++ lib/internal/modules/esm/utils.js | 8 +++++- lib/internal/vm.js | 16 +++++++++++ src/env_properties.h | 3 +- ...vm-dynamic-import-callback-missing-flag.js | 28 +++++++++++++++++++ 7 files changed, 81 insertions(+), 7 deletions(-) create mode 100644 test/parallel/test-vm-dynamic-import-callback-missing-flag.js diff --git a/doc/api/errors.md b/doc/api/errors.md index fee0ed89e4b844..88116a7ef6bd60 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2985,6 +2985,12 @@ An attempt was made to use something that was already closed. While using the Performance Timing API (`perf_hooks`), no valid performance entry types are found. + + +### `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG` + +A dynamic import callback was invoked without `--experimental-vm-modules`. + ### `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING` diff --git a/doc/api/vm.md b/doc/api/vm.md index f405d71ef299d4..a8ba0903ad6d68 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -98,7 +98,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API. We do not recommend - using it in a production environment. + using it in a production environment. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `script` {vm.Script} * `importAttributes` {Object} The `"assert"` value passed to the @@ -765,6 +767,9 @@ changes: * `importModuleDynamically` {Function} Called during evaluation of this module when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. + If `--experimental-vm-modules` isn't set, this callback will be ignored + and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `module` {vm.Module} * `importAttributes` {Object} The `"assert"` value passed to the @@ -1022,7 +1027,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API, and should not be - considered stable. + considered stable. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `function` {Function} * `importAttributes` {Object} The `"assert"` value passed to the @@ -1246,7 +1253,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API. We do not recommend - using it in a production environment. + using it in a production environment. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `script` {vm.Script} * `importAttributes` {Object} The `"assert"` value passed to the @@ -1345,7 +1354,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API. We do not recommend - using it in a production environment. + using it in a production environment. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `script` {vm.Script} * `importAttributes` {Object} The `"assert"` value passed to the @@ -1425,7 +1436,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API. We do not recommend - using it in a production environment. + using it in a production environment. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `script` {vm.Script} * `importAttributes` {Object} The `"assert"` value passed to the @@ -1589,6 +1602,7 @@ are not controllable through the timeout either. [Source Text Module Record]: https://tc39.es/ecma262/#sec-source-text-module-records [Synthetic Module Record]: https://heycam.github.io/webidl/#synthetic-module-records [V8 Embedder's Guide]: https://v8.dev/docs/embed#contexts +[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`]: errors.md#err_vm_dynamic_import_callback_missing_flag [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`]: errors.md#err_vm_dynamic_import_callback_missing [`ERR_VM_MODULE_STATUS`]: errors.md#err_vm_module_status [`Error`]: errors.md#class-error diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 5d488843f60ab8..6e1974b1e642b3 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1716,6 +1716,9 @@ E('ERR_VALID_PERFORMANCE_ENTRY_TYPE', 'At least one valid performance entry type is required', Error); E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING', 'A dynamic import callback was not specified.', TypeError); +E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG', + 'A dynamic import callback was invoked without --experimental-vm-modules', + TypeError); E('ERR_VM_MODULE_ALREADY_LINKED', 'Module has already been linked', Error); E('ERR_VM_MODULE_CANNOT_CREATE_CACHED_DATA', 'Cached data cannot be created for a module which has been evaluated', Error); diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index 365d7ed7136b94..b265e61aea4012 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -14,9 +14,11 @@ const { } = internalBinding('util'); const { default_host_defined_options, + vm_dynamic_import_missing_flag, } = internalBinding('symbols'); const { + ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG, ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING, ERR_INVALID_ARG_VALUE, } = require('internal/errors').codes; @@ -132,7 +134,8 @@ const moduleRegistries = new SafeWeakMap(); */ function registerModule(referrer, registry) { const idSymbol = referrer[host_defined_option_symbol]; - if (idSymbol === default_host_defined_options) { + if (idSymbol === default_host_defined_options || + idSymbol === vm_dynamic_import_missing_flag) { // The referrer is compiled without custom callbacks, so there is // no registry to hold on to. We'll throw // ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING when a callback is @@ -173,6 +176,9 @@ async function importModuleDynamicallyCallback(symbol, specifier, attributes) { return importModuleDynamically(specifier, callbackReferrer, attributes); } } + if (symbol === vm_dynamic_import_missing_flag) { + throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG(); + } throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING(); } diff --git a/lib/internal/vm.js b/lib/internal/vm.js index 780f996b320aca..624e6825b8b45d 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -15,12 +15,18 @@ const { } = ContextifyScript.prototype; const { default_host_defined_options, + vm_dynamic_import_missing_flag, } = internalBinding('symbols'); const { validateFunction, validateObject, } = require('internal/validators'); +const { + getOptionValue, +} = require('internal/options'); + + function isContext(object) { validateObject(object, 'object', { __proto__: null, allowArray: true }); @@ -40,6 +46,16 @@ function getHostDefinedOptionId(importModuleDynamically, filename) { // compilation cache can be hit. return default_host_defined_options; } + // We should've thrown here immediately when we introduced + // --experimental-vm-modules and importModuleDynamically, but since + // users are already using this callback to throw a similar error, + // we also defer the error to the time when an actual import() is called + // to avoid breaking them. To ensure that the isolate compilation + // cache can still be hit, use a constant sentinel symbol here. + if (!getOptionValue('--experimental-vm-modules')) { + return vm_dynamic_import_missing_flag; + } + return Symbol(filename); } diff --git a/src/env_properties.h b/src/env_properties.h index c00c1987457712..f0ca211cf5ac44 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -44,7 +44,8 @@ V(owner_symbol, "owner_symbol") \ V(onpskexchange_symbol, "onpskexchange") \ V(resource_symbol, "resource_symbol") \ - V(trigger_async_id_symbol, "trigger_async_id_symbol") + V(trigger_async_id_symbol, "trigger_async_id_symbol") \ + V(vm_dynamic_import_missing_flag, "vm_dynamic_import_missing_flag") // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. diff --git a/test/parallel/test-vm-dynamic-import-callback-missing-flag.js b/test/parallel/test-vm-dynamic-import-callback-missing-flag.js new file mode 100644 index 00000000000000..4b0d09ca3674a7 --- /dev/null +++ b/test/parallel/test-vm-dynamic-import-callback-missing-flag.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +const { Script, compileFunction } = require('vm'); +const assert = require('assert'); + +assert( + !process.execArgv.includes('--experimental-vm-modules'), + 'This test must be run without --experimental-vm-modules'); + +assert.rejects(async () => { + const script = new Script('import("fs")', { + importModuleDynamically: common.mustNotCall(), + }); + const imported = script.runInThisContext(); + await imported; +}, { + code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG' +}).then(common.mustCall()); + +assert.rejects(async () => { + const imported = compileFunction('return import("fs")', [], { + importModuleDynamically: common.mustNotCall(), + })(); + await imported; +}, { + code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG' +}).then(common.mustCall());