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
#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: #48510
Backport-PR-URL: #51004
Refs: #44211
Refs: #42080
Refs: #47096
Refs: #43205
Refs: #38695
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Author: joyeecheung

lib/internal/modules/esm/create_dynamic_module.js

@@ -69,8 +69,9 @@ import.meta.done();
   if (imports.length) {
     reflect.imports = { __proto__: null };
   }
-  const { setCallbackForWrap } = require('internal/modules/esm/utils');
-  setCallbackForWrap(m, {
+  const { registerModule } = require('internal/modules/esm/utils');
+  registerModule(m, {
+    __proto__: null,
     initializeImportMeta: (meta, wrap) => {
       meta.exports = reflect.exports;
       if (reflect.imports) {

lib/internal/modules/esm/loader.js

@@ -210,9 +210,10 @@ class ModuleLoader {
   ) {
     const evalInstance = (url) => {
       const { ModuleWrap } = internalBinding('module_wrap');
-      const { setCallbackForWrap } = require('internal/modules/esm/utils');
+      const { registerModule } = require('internal/modules/esm/utils');
       const module = new ModuleWrap(url, undefined, source, 0, 0);
-      setCallbackForWrap(module, {
+      registerModule(module, {
+        __proto__: null,
         initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }),
         importModuleDynamically: (specifier, { url }, importAttributes) => {
           return this.import(specifier, url, importAttributes);

lib/internal/modules/esm/translators.js

@@ -150,8 +150,9 @@ translators.set('module', async function moduleStrategy(url, source, isMain) {
   maybeCacheSourceMap(url, source);
   debug(`Translating StandardModule ${url}`);
   const module = new ModuleWrap(url, undefined, source, 0, 0);
-  const { setCallbackForWrap } = require('internal/modules/esm/utils');
-  setCallbackForWrap(module, {
+  const { registerModule } = require('internal/modules/esm/utils');
+  registerModule(module, {
+    __proto__: null,
     initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }),
     importModuleDynamically,
   });

lib/internal/modules/esm/utils.js

@@ -7,6 +7,11 @@ const {
   ObjectFreeze,
 } = primordials;
 
+const {
+  privateSymbols: {
+    host_defined_option_symbol,
+  },
+} = internalBinding('util');
 const {
   ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING,
   ERR_INVALID_ARG_VALUE,
@@ -21,16 +26,8 @@ const {
   setImportModuleDynamicallyCallback,
   setInitializeImportMetaObjectCallback,
 } = internalBinding('module_wrap');
-const {
-  getModuleFromWrap,
-} = require('internal/vm/module');
 const assert = require('internal/assert');
 
-const callbackMap = new SafeWeakMap();
-function setCallbackForWrap(wrap, data) {
-  callbackMap.set(wrap, data);
-}
-
 let defaultConditions;
 /**
  * Returns the default conditions for ES module loading.
@@ -83,34 +80,86 @@ function getConditionsSet(conditions) {
   return getDefaultConditionsSet();
 }
 
+/**
+ * @callback ImportModuleDynamicallyCallback
+ * @param {string} specifier
+ * @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer
+ * @param {object} attributes
+ * @returns { Promise<void> }
+ */
+
+/**
+ * @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<symbol, ModuleRegistry>}
+ */
+const moduleRegistries = new SafeWeakMap();
+
+/**
+ * V8 would make sure that as long as import() can still be initiated from
+ * the referrer, the symbol referenced by |host_defined_option_symbol| should
+ * be alive, which in term would keep the settings object alive through the
+ * WeakMap, and in turn that keeps the referrer object alive, which would be
+ * passed into the callbacks.
+ * The reference goes like this:
+ * [v8::internal::Script] (via host defined options) ----1--> [idSymbol]
+ * [callbackReferrer] (via host_defined_option_symbol) ------2------^  |
+ *                                 ^----------3---- (via WeakMap)------
+ * 1+3 makes sure that as long as import() can still be initiated, the
+ * referrer wrap is still around and can be passed into the callbacks.
+ * 2 is only there so that we can get the id symbol to configure the
+ * weak map.
+ * @param {ModuleWrap|ContextifyScript|Function} referrer The referrer to
+ *   get the id symbol from. This is different from callbackReferrer which
+ *   could be set by the caller.
+ * @param {ModuleRegistry} registry
+ */
+function registerModule(referrer, registry) {
+  const idSymbol = referrer[host_defined_option_symbol];
+  // To prevent it from being GC'ed.
+  registry.callbackReferrer ??= referrer;
+  moduleRegistries.set(idSymbol, registry);
+}
+
 /**
  * Defines the `import.meta` object for a given module.
- * @param {object} wrap - Reference to the module.
+ * @param {symbol} symbol - Reference to the module.
  * @param {Record<string, string | Function>} meta - The import.meta object to initialize.
  */
-function initializeImportMetaObject(wrap, meta) {
-  if (callbackMap.has(wrap)) {
-    const { initializeImportMeta } = callbackMap.get(wrap);
+function initializeImportMetaObject(symbol, meta) {
+  if (moduleRegistries.has(symbol)) {
+    const { initializeImportMeta, callbackReferrer } = moduleRegistries.get(symbol);
     if (initializeImportMeta !== undefined) {
-      meta = initializeImportMeta(meta, getModuleFromWrap(wrap) || wrap);
+      meta = initializeImportMeta(meta, callbackReferrer);
     }
   }
 }
 
 /**
  * Asynchronously imports a module dynamically using a callback function. The native callback.
- * @param {object} wrap - Reference to the module.
+ * @param {symbol} symbol - Reference to the module.
  * @param {string} specifier - The module specifier string.
  * @param {Record<string, string>} attributes - The import attributes object.
  * @returns {Promise<import('internal/modules/esm/loader.js').ModuleExports>} - The imported module object.
  * @throws {ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING} - If the callback function is missing.
  */
-async function importModuleDynamicallyCallback(wrap, specifier, attributes) {
-  if (callbackMap.has(wrap)) {
-    const { importModuleDynamically } = callbackMap.get(wrap);
+async function importModuleDynamicallyCallback(symbol, specifier, attributes) {
+  if (moduleRegistries.has(symbol)) {
+    const { importModuleDynamically, callbackReferrer } = moduleRegistries.get(symbol);
     if (importModuleDynamically !== undefined) {
-      return importModuleDynamically(
-        specifier, getModuleFromWrap(wrap) || wrap, attributes);
+      return importModuleDynamically(specifier, callbackReferrer, attributes);
     }
   }
   throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING();
@@ -176,7 +225,7 @@ async function initializeHooks() {
 }
 
 module.exports = {
-  setCallbackForWrap,
+  registerModule,
   initializeESM,
   initializeHooks,
   getDefaultConditions,

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,
     });
   }
 

lib/internal/vm/module.js

@@ -12,7 +12,6 @@ const {
   ObjectSetPrototypeOf,
   ReflectApply,
   SafePromiseAllReturnVoid,
-  SafeWeakMap,
   Symbol,
   SymbolToStringTag,
   TypeError,
@@ -70,7 +69,6 @@ const STATUS_MAP = {
 
 let globalModuleId = 0;
 const defaultModuleName = 'vm:module';
-const wrapToModuleMap = new SafeWeakMap();
 
 const kWrap = Symbol('kWrap');
 const kContext = Symbol('kContext');
@@ -121,25 +119,30 @@ 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,
                                    syntheticExportNames,
                                    syntheticEvaluationSteps);
     }
 
-    wrapToModuleMap.set(this[kWrap], this);
+    // This will take precedence over the referrer as the object being
+    // passed into the callbacks.
+    registry.callbackReferrer = this;
+    const { registerModule } = require('internal/modules/esm/utils');
+    registerModule(this[kWrap], registry);
 
     this[kContext] = context;
   }
@@ -446,5 +449,4 @@ module.exports = {
   SourceTextModule,
   SyntheticModule,
   importModuleDynamicallyWrap,
-  getModuleFromWrap: (wrap) => wrapToModuleMap.get(wrap),
 };

lib/vm.js

@@ -106,8 +106,9 @@ class Script extends ContextifyScript {
       validateFunction(importModuleDynamically,
                        'options.importModuleDynamically');
       const { importModuleDynamicallyWrap } = require('internal/vm/module');
-      const { setCallbackForWrap } = require('internal/modules/esm/utils');
-      setCallbackForWrap(this, {
+      const { registerModule } = require('internal/modules/esm/utils');
+      registerModule(this, {
+        __proto__: null,
         importModuleDynamically:
           importModuleDynamicallyWrap(importModuleDynamically),
       });

src/env-inl.h

@@ -345,16 +345,6 @@ inline AliasedInt32Array& Environment::stream_base_state() {
   return stream_base_state_;
 }
 
-inline uint32_t Environment::get_next_module_id() {
-  return module_id_counter_++;
-}
-inline uint32_t Environment::get_next_script_id() {
-  return script_id_counter_++;
-}
-inline uint32_t Environment::get_next_function_id() {
-  return function_id_counter_++;
-}
-
 ShouldNotAbortOnUncaughtScope::ShouldNotAbortOnUncaughtScope(
     Environment* env)
     : env_(env) {

src/env.h

@@ -698,14 +698,6 @@ class Environment : public MemoryRetainer {
   builtins::BuiltinLoader* builtin_loader();
 
   std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
-  std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
-  std::unordered_map<uint32_t, contextify::ContextifyScript*>
-      id_to_script_map;
-  std::unordered_map<uint32_t, contextify::CompiledFnEntry*> 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_; }
 

src/env_properties.h

@@ -21,6 +21,7 @@
   V(arrow_message_private_symbol, "node:arrowMessage")                         \
   V(contextify_context_private_symbol, "node:contextify:context")              \
   V(decorated_private_symbol, "node:decorated")                                \
+  V(host_defined_option_symbol, "node:host_defined_option_symbol")             \
   V(napi_type_tag, "node:napi:type_tag")                                       \
   V(napi_wrapper, "node:napi:wrapper")                                         \
   V(untransferable_object_private_symbol, "node:untransferableObject")         \
@@ -337,7 +338,6 @@
   V(blocklist_constructor_template, v8::FunctionTemplate)                      \
   V(contextify_global_template, v8::ObjectTemplate)                            \
   V(contextify_wrapper_template, v8::ObjectTemplate)                           \
-  V(compiled_fn_entry_template, v8::ObjectTemplate)                            \
   V(crypto_key_object_handle_constructor, v8::FunctionTemplate)                \
   V(env_proxy_template, v8::ObjectTemplate)                                    \
   V(env_proxy_ctor_template, v8::FunctionTemplate)                             \

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<String> url,
                        Local<Object> context_object,
                        Local<Value> 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 = 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<Value>& args) {
@@ -154,8 +141,8 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
 
   Local<PrimitiveArray> host_defined_options =
       PrimitiveArray::New(isolate, HostDefinedOptions::kLength);
-  host_defined_options->Set(isolate, HostDefinedOptions::kType,
-                            Number::New(isolate, ScriptType::kModule));
+  Local<Symbol> 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<Value>& 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<Value>& 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<Promise> ImportModuleDynamically(
 
   Local<Value> object;
 
-  int type = options->Get(context, HostDefinedOptions::kType)
-                 .As<Number>()
-                 ->Int32Value(context)
-                 .ToChecked();
-  uint32_t id = options->Get(context, HostDefinedOptions::kID)
-                    .As<Number>()
-                    ->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<Symbol> id =
+      options->Get(context, HostDefinedOptions::kID).As<Symbol>();
 
   Local<Object> attributes =
       createImportAttributesContainer(env, isolate, import_attributes);
 
   Local<Value> import_args[] = {
-    object,
-    Local<Value>(specifier),
-    attributes,
+      id,
+      Local<Value>(specifier),
+      attributes,
   };
 
   Local<Value> result;
@@ -658,7 +628,13 @@ void ModuleWrap::HostInitializeImportMetaObjectCallback(
   Local<Object> wrap = module_wrap->object();
   Local<Function> callback =
       env->host_initialize_import_meta_object_callback();
-  Local<Value> args[] = { wrap, meta };
+  Local<Value> id;
+  if (!wrap->GetPrivate(context, env->host_defined_option_symbol())
+           .ToLocal(&id)) {
+    return;
+  }
+  DCHECK(id->IsSymbol());
+  Local<Value> args[] = {id, meta};
   TryCatchScope try_catch(env);
   USE(callback->Call(
         context, Undefined(env->isolate()), arraysize(args), args));

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<v8::Context> 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

src/node_contextify.cc

@@ -60,7 +60,6 @@ using v8::MicrotasksPolicy;
 using v8::Name;
 using v8::NamedPropertyHandlerConfiguration;
 using v8::Nothing;
-using v8::Number;
 using v8::Object;
 using v8::ObjectTemplate;
 using v8::PrimitiveArray;
@@ -73,11 +72,11 @@ using v8::Script;
 using v8::ScriptCompiler;
 using v8::ScriptOrigin;
 using v8::String;
+using v8::Symbol;
 using v8::Uint32;
 using v8::UnboundScript;
 using v8::Value;
 using v8::WeakCallbackInfo;
-using v8::WeakCallbackType;
 
 // The vm module executes code in a sandboxed environment with a different
 // global object than the rest of the code. This is achieved by applying
@@ -833,10 +832,9 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
 
   Local<PrimitiveArray> 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<Symbol> id_symbol = Symbol::New(isolate, filename);
+  host_defined_options->Set(
+      isolate, loader::HostDefinedOptions::kID, id_symbol);
 
   ScriptOrigin origin(isolate,
                       filename,
@@ -880,6 +878,12 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
     new_cached_data.reset(ScriptCompiler::CreateCodeCache(v8_script));
   }
 
+  if (contextify_script->object()
+          ->SetPrivate(context, env->host_defined_option_symbol(), id_symbol)
+          .IsNothing()) {
+    return;
+  }
+
   if (StoreCodeCacheResult(env,
                            args.This(),
                            compile_options,
@@ -1117,17 +1121,11 @@ bool ContextifyScript::EvalMachine(Local<Context> context,
 
 
 ContextifyScript::ContextifyScript(Environment* env, Local<Object> 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<Value>& args) {
@@ -1197,18 +1195,12 @@ void ContextifyContext::CompileFunction(
       data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
   }
 
-  // Get the function id
-  uint32_t id = env->get_next_function_id();
-
   // Set host_defined_options
   Local<PrimitiveArray> host_defined_options =
       PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
+  Local<Symbol> id_symbol = Symbol::New(isolate, filename);
   host_defined_options->Set(
-      isolate,
-      loader::HostDefinedOptions::kType,
-      Number::New(isolate, loader::ScriptType::kFunction));
-  host_defined_options->Set(
-      isolate, loader::HostDefinedOptions::kID, Number::New(isolate, id));
+      isolate, loader::HostDefinedOptions::kID, id_symbol);
 
   ScriptOrigin origin(isolate,
                       filename,
@@ -1273,21 +1265,14 @@ void ContextifyContext::CompileFunction(
     }
     return;
   }
-
-  Local<Object> 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<Object> result = Object::New(isolate);
   if (result->Set(parsing_context, env->function_string(), fn).IsNothing())
     return;
-  if (result->Set(parsing_context, env->cache_key_string(), cache_key)
-          .IsNothing())
-    return;
   if (result
           ->Set(parsing_context,
                 env->source_map_url_string(),
@@ -1312,25 +1297,6 @@ void ContextifyContext::CompileFunction(
   args.GetReturnValue().Set(result);
 }
 
-void CompiledFnEntry::WeakCallback(
-    const WeakCallbackInfo<CompiledFnEntry>& data) {
-  CompiledFnEntry* entry = data.GetParameter();
-  delete entry;
-}
-
-CompiledFnEntry::CompiledFnEntry(Environment* env,
-                                 Local<Object> object,
-                                 uint32_t id,
-                                 Local<Function> 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<Value>& args) {
   int ret = SigintWatchdogHelper::GetInstance()->Start();
   args.GetReturnValue().Set(ret == 0);
@@ -1418,15 +1384,6 @@ void Initialize(Local<Object> target,
   SetMethodNoSideEffect(
       context, target, "watchdogHasPendingSigint", WatchdogHasPendingSigint);
 
-  {
-    Local<FunctionTemplate> tpl = FunctionTemplate::New(env->isolate());
-    tpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "CompiledFnEntry"));
-    tpl->InstanceTemplate()->SetInternalFieldCount(
-        CompiledFnEntry::kInternalFieldCount);
-
-    env->set_compiled_fn_entry_template(tpl->InstanceTemplate());
-  }
-
   Local<Object> constants = Object::New(env->isolate());
   Local<Object> measure_memory = Object::New(env->isolate());
   Local<Object> memory_execution = Object::New(env->isolate());

src/node_contextify.h

@@ -171,32 +171,8 @@ class ContextifyScript : public BaseObject {
                           std::shared_ptr<v8::MicrotaskQueue> microtask_queue,
                           const v8::FunctionCallbackInfo<v8::Value>& args);
 
-  inline uint32_t id() { return id_; }
-
  private:
   v8::Global<v8::UnboundScript> 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<v8::Object> object,
-                  uint32_t id,
-                  v8::Local<v8::Function> fn);
-  ~CompiledFnEntry();
-
-  bool IsNotIndicativeOfMemoryLeakAtExit() const override { return true; }
-
- private:
-  uint32_t id_;
-  v8::Global<v8::Function> fn_;
-
-  static void WeakCallback(const v8::WeakCallbackInfo<CompiledFnEntry>& data);
 };
 
 v8::Maybe<bool> StoreCodeCacheResult(

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;
+  }
+});

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();