vm: fix gc bug with modules and compiled functions

Backport-PR-URL: https://github.com/nodejs/node/pull/28779
PR-URL: https://github.com/nodejs/node/pull/28671
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Guy Bedford <guybedford@gmail.com>

7 files changed, 69 insertions, 45 deletions

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 00bc575b1f..a70fad83f9 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -717,7 +717,7 @@ Module.prototype._compile = function(content, filename) {
       } : undefined,
     });
   } else {
-    compiledWrapper = compileFunction(
+    const compiled = compileFunction(
       content,
       filename,
       0,
@@ -736,13 +736,14 @@ Module.prototype._compile = function(content, filename) {
     );
     if (experimentalModules) {
       const { callbackMap } = internalBinding('module_wrap');
-      callbackMap.set(compiledWrapper, {
+      callbackMap.set(compiled.cacheKey, {
         importModuleDynamically: async (specifier) => {
           const loader = await asyncESM.loaderPromise;
           return loader.import(specifier, normalizeReferrerURL(filename));
         }
       });
     }
+    compiledWrapper = compiled.function;
   }
 
   var inspectorWrapper = null;
diff --git a/lib/vm.js b/lib/vm.js
index a666a05c8a..ec6614e661 100644
--- a/lib/vm.js
+++ b/lib/vm.js
@@ -383,7 +383,7 @@ function compileFunction(code, params, options = {}) {
     }
   });
 
-  return _compileFunction(
+  const result = _compileFunction(
     code,
     filename,
     lineOffset,
@@ -394,6 +394,16 @@ function compileFunction(code, params, options = {}) {
     contextExtensions,
     params
   );
+
+  if (produceCachedData) {
+    result.function.cachedDataProduced = result.cachedDataProduced;
+  }
+
+  if (result.cachedData) {
+    result.function.cachedData = result.cachedData;
+  }
+
+  return result.function;
 }
 
 
diff --git a/src/env.cc b/src/env.cc
index df59eaaa94..38d5796f54 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -39,6 +39,7 @@ using v8::NewStringType;
 using v8::Number;
 using v8::Object;
 using v8::Private;
+using v8::ScriptOrModule;
 using v8::SnapshotCreator;
 using v8::StackTrace;
 using v8::String;
@@ -46,6 +47,7 @@ using v8::Symbol;
 using v8::TracingController;
 using v8::Undefined;
 using v8::Value;
+using v8::WeakCallbackInfo;
 using worker::Worker;
 
 int const Environment::kNodeContextTag = 0x6e6f64;
@@ -385,9 +387,22 @@ Environment::Environment(IsolateData* isolate_data,
   CreateProperties();
 }
 
-CompileFnEntry::CompileFnEntry(Environment* env, uint32_t id)
-    : env(env), id(id) {
-  env->compile_fn_entries.insert(this);
+static void WeakCallbackCompiledFn(
+    const WeakCallbackInfo<CompiledFnEntry>& data) {
+  CompiledFnEntry* entry = data.GetParameter();
+  entry->env->id_to_function_map.erase(entry->id);
+  delete entry;
+}
+
+CompiledFnEntry::CompiledFnEntry(Environment* env,
+                                 uint32_t id,
+                                 Local<ScriptOrModule> script)
+    : env(env),
+      id(id),
+      cache_key(env->isolate(), Object::New(env->isolate())),
+      script(env->isolate(), script) {
+  this->script.SetWeak(
+      this, WeakCallbackCompiledFn, v8::WeakCallbackType::kParameter);
 }
 
 Environment::~Environment() {
@@ -398,12 +413,6 @@ Environment::~Environment() {
   // CleanupHandles() should have removed all of them.
   CHECK(file_handle_read_wrap_freelist_.empty());
 
-  // dispose the Persistent references to the compileFunction
-  // wrappers used in the dynamic import callback
-  for (auto& entry : compile_fn_entries) {
-    delete entry;
-  }
-
   HandleScope handle_scope(isolate());
 
 #if HAVE_INSPECTOR
diff --git a/src/env.h b/src/env.h
index df389f3bea..3665c100d9 100644
--- a/src/env.h
+++ b/src/env.h
@@ -157,6 +157,7 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
   V(cached_data_produced_string, "cachedDataProduced")                         \
   V(cached_data_rejected_string, "cachedDataRejected")                         \
   V(cached_data_string, "cachedData")                                          \
+  V(cache_key_string, "cacheKey")                                              \
   V(change_string, "change")                                                   \
   V(channel_string, "channel")                                                 \
   V(chunks_sent_since_last_write_string, "chunksSentSinceLastWrite")           \
@@ -500,10 +501,14 @@ struct ContextInfo {
   bool is_default = false;
 };
 
-struct CompileFnEntry {
+struct CompiledFnEntry {
   Environment* env;
   uint32_t id;
-  CompileFnEntry(Environment* env, uint32_t id);
+  v8::Global<v8::Object> cache_key;
+  v8::Global<v8::ScriptOrModule> script;
+  CompiledFnEntry(Environment* env,
+                  uint32_t id,
+                  v8::Local<v8::ScriptOrModule> script);
 };
 
 // Listing the AsyncWrap provider types first enables us to cast directly
@@ -990,8 +995,7 @@ class Environment : public MemoryRetainer {
   std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
   std::unordered_map<uint32_t, contextify::ContextifyScript*>
       id_to_script_map;
-  std::unordered_set<CompileFnEntry*> compile_fn_entries;
-  std::unordered_map<uint32_t, v8::Global<v8::Function>> id_to_function_map;
+  std::unordered_map<uint32_t, CompiledFnEntry*> id_to_function_map;
 
   inline uint32_t get_next_module_id();
   inline uint32_t get_next_script_id();
diff --git a/src/module_wrap.cc b/src/module_wrap.cc
index bccfc2d2af..7df91bb870 100644
--- a/src/module_wrap.cc
+++ b/src/module_wrap.cc
@@ -1041,7 +1041,7 @@ static MaybeLocal<Promise> ImportModuleDynamically(
     ModuleWrap* wrap = ModuleWrap::GetFromID(env, id);
     object = wrap->object();
   } else if (type == ScriptType::kFunction) {
-    object = env->id_to_function_map.find(id)->second.Get(iso);
+    object = env->id_to_function_map.find(id)->second->cache_key.Get(iso);
   } else {
     UNREACHABLE();
   }
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 5be774fb91..6559e813c9 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -66,6 +66,7 @@ using v8::PropertyHandlerFlags;
 using v8::Script;
 using v8::ScriptCompiler;
 using v8::ScriptOrigin;
+using v8::ScriptOrModule;
 using v8::String;
 using v8::Symbol;
 using v8::Uint32;
@@ -305,15 +306,6 @@ void ContextifyContext::WeakCallback(
   delete context;
 }
 
-void ContextifyContext::WeakCallbackCompileFn(
-    const WeakCallbackInfo<CompileFnEntry>& data) {
-  CompileFnEntry* entry = data.GetParameter();
-  if (entry->env->compile_fn_entries.erase(entry) != 0) {
-    entry->env->id_to_function_map.erase(entry->id);
-    delete entry;
-  }
-}
-
 // static
 ContextifyContext* ContextifyContext::ContextFromContextifiedSandbox(
     Environment* env,
@@ -1117,9 +1109,11 @@ void ContextifyContext::CompileFunction(
     }
   }
 
+  Local<ScriptOrModule> script;
   MaybeLocal<Function> maybe_fn = ScriptCompiler::CompileFunctionInContext(
       parsing_context, &source, params.size(), params.data(),
-      context_extensions.size(), context_extensions.data(), options);
+      context_extensions.size(), context_extensions.data(), options,
+      v8::ScriptCompiler::NoCacheReason::kNoCacheNoReason, &script);
 
   if (maybe_fn.IsEmpty()) {
     if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
@@ -1129,13 +1123,17 @@ void ContextifyContext::CompileFunction(
     return;
   }
   Local<Function> fn = maybe_fn.ToLocalChecked();
-  env->id_to_function_map.emplace(std::piecewise_construct,
-                                  std::make_tuple(id),
-                                  std::make_tuple(isolate, fn));
-  CompileFnEntry* gc_entry = new CompileFnEntry(env, id);
-  env->id_to_function_map[id].SetWeak(gc_entry,
-      WeakCallbackCompileFn,
-      v8::WeakCallbackType::kParameter);
+
+  CompiledFnEntry* entry = new CompiledFnEntry(env, id, script);
+  env->id_to_function_map.emplace(id, entry);
+  Local<Object> cache_key = entry->cache_key.Get(isolate);
+
+  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 (produce_cached_data) {
     const std::unique_ptr<ScriptCompiler::CachedData> cached_data(
@@ -1146,18 +1144,22 @@ void ContextifyContext::CompileFunction(
           env,
           reinterpret_cast<const char*>(cached_data->data),
           cached_data->length);
-      if (fn->Set(
-          parsing_context,
-          env->cached_data_string(),
-          buf.ToLocalChecked()).IsNothing()) return;
+      if (result
+              ->Set(parsing_context,
+                    env->cached_data_string(),
+                    buf.ToLocalChecked())
+              .IsNothing())
+        return;
     }
-    if (fn->Set(
-        parsing_context,
-        env->cached_data_produced_string(),
-        Boolean::New(isolate, cached_data_produced)).IsNothing()) return;
+    if (result
+            ->Set(parsing_context,
+                  env->cached_data_produced_string(),
+                  Boolean::New(isolate, cached_data_produced))
+            .IsNothing())
+      return;
   }
 
-  args.GetReturnValue().Set(fn);
+  args.GetReturnValue().Set(result);
 }
 
 static void StartSigintWatchdog(const FunctionCallbackInfo<Value>& args) {
diff --git a/src/node_contextify.h b/src/node_contextify.h
index 288b51ef56..73461dc623 100644
--- a/src/node_contextify.h
+++ b/src/node_contextify.h
@@ -63,8 +63,6 @@ class ContextifyContext {
       const v8::FunctionCallbackInfo<v8::Value>& args);
   static void WeakCallback(
       const v8::WeakCallbackInfo<ContextifyContext>& data);
-  static void WeakCallbackCompileFn(
-      const v8::WeakCallbackInfo<CompileFnEntry>& data);
   static void PropertyGetterCallback(
       v8::Local<v8::Name> property,
       const v8::PropertyCallbackInfo<v8::Value>& args);