From 7aae66a85ff65f3a74a14cf4d4f02e84f82f1706 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 21 Jun 2023 16:01:50 +0200 Subject: [PATCH] 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 | 87 +++++++++++++++---- 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 | 74 ++++------------ src/node_contextify.h | 24 ----- .../test-dynamic-import-script-lifetime.js | 32 +++++++ .../test-vm-compile-function-leak.js | 19 ++++ 16 files changed, 185 insertions(+), 188 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 26ccd38be1ad6f..95ff24277aa06e 100644 --- a/lib/internal/modules/esm/create_dynamic_module.js +++ b/lib/internal/modules/esm/create_dynamic_module.js @@ -45,8 +45,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 5305c1eb8f37d9..72c60af37a3029 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -180,9 +180,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 }, importAssertions) => { return this.import(specifier, url, importAssertions); diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 80228e895fafcf..b143cd0ad34d0e 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -116,8 +116,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 5014c99b2a9eb3..e43eb21a52206e 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; function getDefaultConditions() { assert(defaultConditions !== undefined); @@ -73,21 +70,75 @@ function getConditionsSet(conditions) { return getDefaultConditionsSet(); } -function initializeImportMetaObject(wrap, meta) { - if (callbackMap.has(wrap)) { - const { initializeImportMeta } = callbackMap.get(wrap); +/** + * @callback ImportModuleDynamicallyCallback + * @param {string} specifier + * @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer + * @param {object} assertions + * @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); +} + +// 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(); @@ -151,7 +202,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 970e25d926dbb2..8005d037cebfa4 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") \ @@ -338,7 +339,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();