vm: fix gc bug with modules and compiled functions

common.gypi

@@ -38,7 +38,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.15',
+    'v8_embedder_string': '-node.16',
 
     ##### V8 defaults for Node.js #####
 

deps/v8/include/v8.h

@@ -1701,7 +1701,8 @@ class V8_EXPORT ScriptCompiler {
       Local<String> arguments[], size_t context_extension_count,
       Local<Object> context_extensions[],
       CompileOptions options = kNoCompileOptions,
-      NoCacheReason no_cache_reason = kNoCacheNoReason);
+      NoCacheReason no_cache_reason = kNoCacheNoReason,
+      Local<ScriptOrModule>* script_or_module_out = nullptr);
 
   /**
    * Creates and returns code cache for the specified unbound_script.

deps/v8/src/api.cc

@@ -2499,70 +2499,83 @@ MaybeLocal<Function> ScriptCompiler::CompileFunctionInContext(
     Local<Context> v8_context, Source* source, size_t arguments_count,
     Local<String> arguments[], size_t context_extension_count,
     Local<Object> context_extensions[], CompileOptions options,
-    NoCacheReason no_cache_reason) {
-  PREPARE_FOR_EXECUTION(v8_context, ScriptCompiler, CompileFunctionInContext,
-                        Function);
-  TRACE_EVENT_CALL_STATS_SCOPED(isolate, "v8", "V8.ScriptCompiler");
+    NoCacheReason no_cache_reason,
+    Local<ScriptOrModule>* script_or_module_out) {
+  Local<Function> result;
 
-  DCHECK(options == CompileOptions::kConsumeCodeCache ||
-         options == CompileOptions::kEagerCompile ||
-         options == CompileOptions::kNoCompileOptions);
+  {
+    PREPARE_FOR_EXECUTION(v8_context, ScriptCompiler, CompileFunctionInContext,
+                          Function);
+    TRACE_EVENT_CALL_STATS_SCOPED(isolate, "v8", "V8.ScriptCompiler");
 
-  i::Handle<i::Context> context = Utils::OpenHandle(*v8_context);
+    DCHECK(options == CompileOptions::kConsumeCodeCache ||
+           options == CompileOptions::kEagerCompile ||
+           options == CompileOptions::kNoCompileOptions);
 
-  DCHECK(context->IsNativeContext());
-  i::Handle<i::SharedFunctionInfo> outer_info(
-      context->empty_function()->shared(), isolate);
-
-  i::Handle<i::JSFunction> fun;
-  i::Handle<i::FixedArray> arguments_list =
-      isolate->factory()->NewFixedArray(static_cast<int>(arguments_count));
-  for (int i = 0; i < static_cast<int>(arguments_count); i++) {
-    i::Handle<i::String> argument = Utils::OpenHandle(*arguments[i]);
-    if (!IsIdentifier(isolate, argument)) return Local<Function>();
-    arguments_list->set(i, *argument);
-  }
-
-  for (size_t i = 0; i < context_extension_count; ++i) {
-    i::Handle<i::JSReceiver> extension =
-        Utils::OpenHandle(*context_extensions[i]);
-    if (!extension->IsJSObject()) return Local<Function>();
-    context = isolate->factory()->NewWithContext(
-        context,
-        i::ScopeInfo::CreateForWithScope(
-            isolate,
-            context->IsNativeContext()
-                ? i::Handle<i::ScopeInfo>::null()
-                : i::Handle<i::ScopeInfo>(context->scope_info(), isolate)),
-        extension);
-  }
+    i::Handle<i::Context> context = Utils::OpenHandle(*v8_context);
 
-  i::Compiler::ScriptDetails script_details = GetScriptDetails(
-      isolate, source->resource_name, source->resource_line_offset,
-      source->resource_column_offset, source->source_map_url,
-      source->host_defined_options);
+    DCHECK(context->IsNativeContext());
 
-  i::ScriptData* script_data = nullptr;
-  if (options == kConsumeCodeCache) {
-    DCHECK(source->cached_data);
-    // ScriptData takes care of pointer-aligning the data.
-    script_data = new i::ScriptData(source->cached_data->data,
-                                    source->cached_data->length);
+    i::Handle<i::FixedArray> arguments_list =
+        isolate->factory()->NewFixedArray(static_cast<int>(arguments_count));
+    for (int i = 0; i < static_cast<int>(arguments_count); i++) {
+      i::Handle<i::String> argument = Utils::OpenHandle(*arguments[i]);
+      if (!IsIdentifier(isolate, argument)) return Local<Function>();
+      arguments_list->set(i, *argument);
+    }
+
+    for (size_t i = 0; i < context_extension_count; ++i) {
+      i::Handle<i::JSReceiver> extension =
+          Utils::OpenHandle(*context_extensions[i]);
+      if (!extension->IsJSObject()) return Local<Function>();
+      context = isolate->factory()->NewWithContext(
+          context,
+          i::ScopeInfo::CreateForWithScope(
+              isolate,
+              context->IsNativeContext()
+                  ? i::Handle<i::ScopeInfo>::null()
+                  : i::Handle<i::ScopeInfo>(context->scope_info(), isolate)),
+          extension);
+    }
+
+    i::Compiler::ScriptDetails script_details = GetScriptDetails(
+        isolate, source->resource_name, source->resource_line_offset,
+        source->resource_column_offset, source->source_map_url,
+        source->host_defined_options);
+
+    i::ScriptData* script_data = nullptr;
+    if (options == kConsumeCodeCache) {
+      DCHECK(source->cached_data);
+      // ScriptData takes care of pointer-aligning the data.
+      script_data = new i::ScriptData(source->cached_data->data,
+                                      source->cached_data->length);
+    }
+
+    i::Handle<i::JSFunction> scoped_result;
+    has_pending_exception =
+        !i::Compiler::GetWrappedFunction(
+             Utils::OpenHandle(*source->source_string), arguments_list, context,
+             script_details, source->resource_options, script_data, options,
+             no_cache_reason)
+             .ToHandle(&scoped_result);
+    if (options == kConsumeCodeCache) {
+      source->cached_data->rejected = script_data->rejected();
+    }
+    delete script_data;
+    RETURN_ON_FAILED_EXECUTION(Function);
+    result = handle_scope.Escape(Utils::CallableToLocal(scoped_result));
   }
 
-  i::Handle<i::JSFunction> result;
-  has_pending_exception =
-      !i::Compiler::GetWrappedFunction(
-           Utils::OpenHandle(*source->source_string), arguments_list, context,
-           script_details, source->resource_options, script_data, options,
-           no_cache_reason)
-           .ToHandle(&result);
-  if (options == kConsumeCodeCache) {
-    source->cached_data->rejected = script_data->rejected();
+  if (script_or_module_out != nullptr) {
+    i::Handle<i::JSFunction> function =
+        i::Handle<i::JSFunction>::cast(Utils::OpenHandle(*result));
+    i::Isolate* isolate = function->GetIsolate();
+    i::Handle<i::SharedFunctionInfo> shared(function->shared(), isolate);
+    i::Handle<i::Script> script(i::Script::cast(shared->script()), isolate);
+    *script_or_module_out = v8::Utils::ScriptOrModuleToLocal(script);
   }
-  delete script_data;
-  RETURN_ON_FAILED_EXECUTION(Function);
-  RETURN_ESCAPED(Utils::CallableToLocal(result));
+
+  return result;
 }
 
 void ScriptCompiler::ScriptStreamingTask::Run() { data_->task->Run(); }

deps/v8/test/cctest/test-compiler.cc

@@ -647,11 +647,16 @@ TEST(CompileFunctionInContextScriptOrigin) {
                           v8::Integer::New(CcTest::isolate(), 22),
                           v8::Integer::New(CcTest::isolate(), 41));
   v8::ScriptCompiler::Source script_source(v8_str("throw new Error()"), origin);
+  Local<ScriptOrModule> script;
   v8::Local<v8::Function> fun =
-      v8::ScriptCompiler::CompileFunctionInContext(env.local(), &script_source,
-                                                   0, nullptr, 0, nullptr)
+      v8::ScriptCompiler::CompileFunctionInContext(
+          env.local(), &script_source, 0, nullptr, 0, nullptr,
+          v8::ScriptCompiler::CompileOptions::kNoCompileOptions,
+          v8::ScriptCompiler::NoCacheReason::kNoCacheNoReason, &script)
           .ToLocalChecked();
   CHECK(!fun.IsEmpty());
+  CHECK(!script.IsEmpty());
+  CHECK(script->GetResourceName()->StrictEquals(v8_str("test")));
   v8::TryCatch try_catch(CcTest::isolate());
   CcTest::isolate()->SetCaptureStackTraceForUncaughtExceptions(true);
   CHECK(fun->Call(env.local(), env->Global(), 0, nullptr).IsEmpty());

lib/internal/modules/cjs/loader.js

@@ -705,7 +705,7 @@ function wrapSafe(filename, content) {
     });
   }
 
-  const compiledWrapper = compileFunction(
+  const compiled = compileFunction(
     content,
     filename,
     0,
@@ -725,15 +725,15 @@ function wrapSafe(filename, content) {
 
   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));
       }
     });
   }
 
-  return compiledWrapper;
+  return compiled.function;
 }
 
 // Run the file contents in the correct scope or sandbox. Expose

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

src/env.cc

@@ -39,13 +39,15 @@ using v8::NewStringType;
 using v8::Number;
 using v8::Object;
 using v8::Private;
+using v8::ScriptOrModule;
 using v8::SnapshotCreator;
 using v8::StackTrace;
 using v8::String;
 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

src/env.h

@@ -179,6 +179,7 @@ constexpr size_t kFsStatsBufferLength =
   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")           \
@@ -522,10 +523,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
@@ -1005,8 +1010,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();

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