From 684efdcfeb9bd35f4309367ac1db4da4729b349b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 21 Jun 2023 16:01:50 +0200 Subject: [PATCH 1/3] 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. --- .../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 14b78f660f1782..5d1307565885de 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 74756857f70044..82d892a96f1936 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(); @@ -149,7 +200,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 687422d1a4fb0b..db471d4efb8d1d 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -22,6 +22,7 @@ V(contextify_context_private_symbol, "node:contextify:context") \ V(decorated_private_symbol, "node:decorated") \ V(transfer_mode_private_symbol, "node:transfer_mode") \ + V(host_defined_option_symbol, "node:host_defined_option_symbol") \ V(js_transferable_wrapper_private_symbol, "node:js_transferable_wrapper") \ V(napi_type_tag, "node:napi:type_tag") \ V(napi_wrapper, "node:napi:wrapper") \ @@ -342,7 +343,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 3df92b191f88d671e07324da0aa57887b1aac7c5 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 21 Jun 2023 18:04:35 +0200 Subject: [PATCH 2/3] 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. --- 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 b439ba653f4ef39d74e41d86c54b8efff2b7b4ae Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 23 Jun 2023 18:32:57 +0200 Subject: [PATCH 3/3] 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. --- src/module_wrap.cc | 9 +++++--- src/module_wrap.h | 3 ++- src/node_contextify.cc | 3 +++ src/node_contextify.h | 5 ++++ .../test-vm-contextified-script-leak.js | 19 +++++++++++++++ .../test-vm-source-text-module-leak.js | 23 +++++++++++++++++++ 6 files changed, 58 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..a4190c9290e609 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->SetInternalField(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..b64faf812c6c47 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -855,7 +855,10 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { "ContextifyScript::New"); return; } + contextify_script->script_.Reset(isolate, v8_script); + contextify_script->script_.SetWeak(); + contextify_script->object()->SetInternalField(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();