From 0fbbe49cf65a460403700c9ba3078801a1f9f423 Mon Sep 17 00:00:00 2001 From: Steven Date: Sun, 24 Sep 2023 17:10:51 -0400 Subject: [PATCH 01/26] doc: promote fetch/webstreams from experimental to stable PR-URL: https://github.com/nodejs/node/pull/45684 Reviewed-By: Mohammed Keyvanzadeh Reviewed-By: Antoine du Hamel Reviewed-By: Robert Nagy Reviewed-By: James M Snell Reviewed-By: Yagiz Nizipli Reviewed-By: Moshe Atlow Reviewed-By: Matteo Collina Reviewed-By: Rafael Gonzaga --- doc/api/cli.md | 2 +- doc/api/globals.md | 36 +++++++++++++++++++++++++----------- doc/api/webstreams.md | 6 +++++- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index f50b22f729c283..936fd097c1333f 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1089,7 +1089,7 @@ Silence deprecation warnings. added: v18.0.0 --> -Disable experimental support for the [Fetch API][]. +Disable exposition of [Fetch API][] on the global scope. ### `--no-experimental-global-customevent` diff --git a/doc/api/globals.md b/doc/api/globals.md index 8285064c2e25e4..84b0cc02dcc26f 100644 --- a/doc/api/globals.md +++ b/doc/api/globals.md @@ -476,13 +476,16 @@ added: - v17.5.0 - v16.15.0 changes: + - version: + - REPLACEME + pr-url: https://github.com/nodejs/node/pull/45684 + description: No longer experimental. - version: v18.0.0 pr-url: https://github.com/nodejs/node/pull/41811 description: No longer behind `--experimental-global-fetch` CLI flag. --> -> Stability: 1 - Experimental. Disable this API with the [`--no-experimental-fetch`][] -> CLI flag. +> Stability: 2 - Stable A browser-compatible implementation of the [`fetch()`][] function. @@ -503,13 +506,16 @@ added: - v17.6.0 - v16.15.0 changes: + - version: + - REPLACEME + pr-url: https://github.com/nodejs/node/pull/45684 + description: No longer experimental. - version: v18.0.0 pr-url: https://github.com/nodejs/node/pull/41811 description: No longer behind `--experimental-global-fetch` CLI flag. --> -> Stability: 1 - Experimental. Disable this API with the [`--no-experimental-fetch`][] -> CLI flag. +> Stability: 2 - Stable A browser-compatible implementation of {FormData}. @@ -539,13 +545,16 @@ added: - v17.5.0 - v16.15.0 changes: + - version: + - REPLACEME + pr-url: https://github.com/nodejs/node/pull/45684 + description: No longer experimental. - version: v18.0.0 pr-url: https://github.com/nodejs/node/pull/41811 description: No longer behind `--experimental-global-fetch` CLI flag. --> -> Stability: 1 - Experimental. Disable this API with the [`--no-experimental-fetch`][] -> CLI flag. +> Stability: 2 - Stable A browser-compatible implementation of {Headers}. @@ -776,13 +785,16 @@ added: - v17.5.0 - v16.15.0 changes: + - version: + - REPLACEME + pr-url: https://github.com/nodejs/node/pull/45684 + description: No longer experimental. - version: v18.0.0 pr-url: https://github.com/nodejs/node/pull/41811 description: No longer behind `--experimental-global-fetch` CLI flag. --> -> Stability: 1 - Experimental. Disable this API with the [`--no-experimental-fetch`][] -> CLI flag. +> Stability: 2 - Stable A browser-compatible implementation of {Response}. @@ -793,13 +805,16 @@ added: - v17.5.0 - v16.15.0 changes: + - version: + - REPLACEME + pr-url: https://github.com/nodejs/node/pull/45684 + description: No longer experimental. - version: v18.0.0 pr-url: https://github.com/nodejs/node/pull/41811 description: No longer behind `--experimental-global-fetch` CLI flag. --> -> Stability: 1 - Experimental. Disable this API with the [`--no-experimental-fetch`][] -> CLI flag. +> Stability: 2 - Stable A browser-compatible implementation of {Request}. @@ -999,7 +1014,6 @@ A browser-compatible implementation of [`WritableStreamDefaultWriter`][]. [CommonJS module]: modules.md [ECMAScript module]: esm.md [Web Crypto API]: webcrypto.md -[`--no-experimental-fetch`]: cli.md#--no-experimental-fetch [`--no-experimental-global-customevent`]: cli.md#--no-experimental-global-customevent [`--no-experimental-global-webcrypto`]: cli.md#--no-experimental-global-webcrypto [`AbortController`]: https://developer.mozilla.org/en-US/docs/Web/API/AbortController diff --git a/doc/api/webstreams.md b/doc/api/webstreams.md index a8a111caef6414..6d75212aba4a3f 100644 --- a/doc/api/webstreams.md +++ b/doc/api/webstreams.md @@ -5,12 +5,16 @@ -> Stability: 1 - Experimental. +> Stability: 2 - Stable An implementation of the [WHATWG Streams Standard][]. From 6a88c6a7af695787d67d20677834a373dfaac3c0 Mon Sep 17 00:00:00 2001 From: "mert.altin" Date: Sat, 23 Sep 2023 12:13:31 +0300 Subject: [PATCH 02/26] doc: add mertcanaltin as a triager PR-URL: https://github.com/nodejs/node/pull/49826 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli Reviewed-By: Qingyu Deng Reviewed-By: Gireesh Punathil Reviewed-By: Daeyeon Jeong Reviewed-By: Moshe Atlow Reviewed-By: Darshan Sen Reviewed-By: Mohammed Keyvanzadeh Reviewed-By: Chemi Atlow Reviewed-By: LiviaMedeiros --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 76eb5c3c60ec6d..a9eafa7595ef93 100644 --- a/README.md +++ b/README.md @@ -721,6 +721,8 @@ maintaining the Node.js project. **Akhil Marsonya** <> (he/him) * [meixg](https://github.com/meixg) - **Xuguang Mei** <> (he/him) +* [mertcanaltin](https://github.com/mertcanaltin) - + **Mert Can Altin** <> * [Mesteery](https://github.com/Mesteery) - **Mestery** <> (he/him) * [preveen-stack](https://github.com/preveen-stack) - From 96009289fb44936a8b612deca3980b28096eac12 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Mon, 25 Sep 2023 12:23:58 +0800 Subject: [PATCH 03/26] node-api: enable uncaught exceptions policy by default This enables the option `--force-node-api-uncaught-exceptions-policy` for a specific Node-API addon when it is compiled with `NAPI_EXPERIMENTAL` (and this would be the default behavior when `NAPI_VERSION` 10 releases). This would not break existing Node-API addons. PR-URL: https://github.com/nodejs/node/pull/49313 Refs: https://github.com/nodejs/node/pull/36510 Reviewed-By: Michael Dawson Reviewed-By: Gabriel Schulhof --- doc/api/n-api.md | 8 ++ src/node_api.cc | 16 ++-- test/js-native-api/test_reference/binding.gyp | 6 ++ .../test_reference/test_finalizer.c | 71 ++++++++++++++++++ .../test_reference/test_finalizer.js | 4 +- .../test_reference/test_reference.c | 75 +++++-------------- test/node-api/test_buffer/binding.gyp | 4 + test/node-api/test_buffer/test_buffer.c | 50 ++----------- test/node-api/test_buffer/test_finalizer.c | 61 +++++++++++++++ test/node-api/test_buffer/test_finalizer.js | 2 +- .../test_threadsafe_function/binding.gyp | 14 ++++ ...n.js => test_legacy_uncaught_exception.js} | 2 +- .../test_uncaught_exception.c | 62 +++++++++++++++ .../test_uncaught_exception.js | 26 +------ .../test_uncaught_exception_v9.js | 8 ++ .../uncaught_exception.js | 31 ++++++++ 16 files changed, 307 insertions(+), 133 deletions(-) create mode 100644 test/js-native-api/test_reference/test_finalizer.c create mode 100644 test/node-api/test_buffer/test_finalizer.c rename test/node-api/test_threadsafe_function/{test_force_uncaught_exception.js => test_legacy_uncaught_exception.js} (85%) create mode 100644 test/node-api/test_threadsafe_function/test_uncaught_exception.c create mode 100644 test/node-api/test_threadsafe_function/test_uncaught_exception_v9.js create mode 100644 test/node-api/test_threadsafe_function/uncaught_exception.js diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 10b6dac61c4490..feb9fa6061ca4e 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -6245,6 +6245,13 @@ napi_create_threadsafe_function(napi_env env, [`napi_threadsafe_function_call_js`][] provides more details. * `[out] result`: The asynchronous thread-safe JavaScript function. +**Change History:** + +* Experimental (`NAPI_EXPERIMENTAL` is defined): + + Uncaught exceptions thrown in `call_js_cb` are handled with the + [`'uncaughtException'`][] event, instead of being ignored. + ### `napi_get_threadsafe_function_context` + +Type: Documentation-only + +`F_OK`, `R_OK`, `W_OK` and `X_OK` getters exposed directly on `node:fs` are +deprecated. Get them from `fs.constants` or `fs.promises.constants` instead. + [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf [RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3 [RFC 8247 Section 2.4]: https://www.rfc-editor.org/rfc/rfc8247#section-2.4 diff --git a/doc/api/fs.md b/doc/api/fs.md index 499711f5240af3..d223fa5fb7ca5f 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -1820,6 +1820,10 @@ concurrent modifications on the same file or data corruption may occur. [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); +} + +// The native callback +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); } } } -async function importModuleDynamicallyCallback(wrap, specifier, assertions) { - if (callbackMap.has(wrap)) { - const { importModuleDynamically } = callbackMap.get(wrap); +// The native callback +async function importModuleDynamicallyCallback(symbol, specifier, assertions) { + if (moduleRegistries.has(symbol)) { + const { importModuleDynamically, callbackReferrer } = moduleRegistries.get(symbol); if (importModuleDynamically !== undefined) { - return importModuleDynamically( - specifier, getModuleFromWrap(wrap) || wrap, assertions); + return importModuleDynamically(specifier, callbackReferrer, assertions); } } throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING(); @@ -142,7 +193,7 @@ async function initializeHooks() { } module.exports = { - setCallbackForWrap, + registerModule, initializeESM, initializeHooks, getDefaultConditions, diff --git a/lib/internal/vm.js b/lib/internal/vm.js index b14ba13e7e4cfb..ba5e2324667374 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 3d2d25064b62cd..c77ee9b107e0ad 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -11,7 +11,6 @@ const { ObjectSetPrototypeOf, ReflectApply, SafePromiseAllReturnVoid, - SafeWeakMap, Symbol, SymbolToStringTag, TypeError, @@ -69,7 +68,6 @@ const STATUS_MAP = { let globalModuleId = 0; const defaultModuleName = 'vm:module'; -const wrapToModuleMap = new SafeWeakMap(); const kWrap = Symbol('kWrap'); const kContext = Symbol('kContext'); @@ -120,17 +118,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, @@ -138,7 +137,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; } @@ -445,5 +448,4 @@ module.exports = { SourceTextModule, SyntheticModule, importModuleDynamicallyWrap, - getModuleFromWrap: (wrap) => wrapToModuleMap.get(wrap), }; diff --git a/lib/vm.js b/lib/vm.js index 515c7afb4aedb9..4b9bedec3f4934 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -105,8 +105,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 7802304b1891ae..793dc72e0dbad8 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -393,16 +393,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 c02fc6bd62dd78..afe67d2237ae69 100644 --- a/src/env.h +++ b/src/env.h @@ -746,14 +746,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 24de82429ec892..26e45695957b7a 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") \ @@ -339,7 +340,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 b96106a39744b9..7fb192bbdcd4cc 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 assertions = createImportAssertionContainer(env, isolate, import_assertions); Local import_args[] = { - object, - Local(specifier), - assertions, + id, + Local(specifier), + assertions, }; 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 a3d3386763af85..b96d629f3e5ae4 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 a557f5bd9f3b35..5ec80a7b6666cb 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 @@ -817,10 +816,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, @@ -864,6 +862,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, @@ -1116,17 +1120,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) { @@ -1196,18 +1194,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, @@ -1272,21 +1264,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(), @@ -1311,25 +1296,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); @@ -1381,14 +1347,6 @@ void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethodNoSideEffect( isolate, target, "watchdogHasPendingSigint", WatchdogHasPendingSigint); - { - Local tpl = FunctionTemplate::New(isolate); - tpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "CompiledFnEntry")); - tpl->InstanceTemplate()->SetInternalFieldCount( - CompiledFnEntry::kInternalFieldCount); - - isolate_data->set_compiled_fn_entry_template(tpl->InstanceTemplate()); - } SetMethod(isolate, target, "measureMemory", MeasureMemory); } diff --git a/src/node_contextify.h b/src/node_contextify.h index 2bcc15b5f55ad3..1098a7745257ac 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -151,32 +151,8 @@ class ContextifyScript : public BaseObject { v8::MicrotaskQueue* 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 23edd1dbce34c6f042f5111fd4109d5cdafd3e8b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 21 Jun 2023 18:04:35 +0200 Subject: [PATCH 22/26] 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 7fb192bbdcd4cc..56699764ef38a7 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 2920904784aed61d00e11ceba57c615f6d234c1f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 23 Jun 2023 18:32:57 +0200 Subject: [PATCH 23/26] 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 56699764ef38a7..a1b0f812391486 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 b96d629f3e5ae4..6435bad40936fe 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 5ec80a7b6666cb..75208c7293863e 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -855,7 +855,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 1098a7745257ac..d1dddbf374d563 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -128,6 +128,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 7d916e21b4721a919b077c96a79e9605057bc785 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 16 Sep 2023 15:07:30 +0200 Subject: [PATCH 24/26] 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 8a47a18b1e46fd598200fc685316f99d93bca001 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 16 Sep 2023 15:08:18 +0200 Subject: [PATCH 25/26] 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 1985233484294d54c1edf34af08831d56337efa4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 23 Sep 2023 05:46:22 +0200 Subject: [PATCH 26/26] 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);