node:vm: break Strong<> cycle through NodeVMScriptFetcher importModuleDynamically callback

src/jsc/bindings/NodeVM.cpp

@@ -1570,8 +1570,11 @@ JSC_DEFINE_HOST_FUNCTION(vmModuleCompileFunction, (JSGlobalObject * globalObject
 
     fetcher->owner(vm, function);
 
-    if (!function) {
-        return throwVMError(globalObject, scope, "Failed to compile function"_s);
+    // NodeVMScriptFetcher only holds a Weak reference to the callback to avoid
+    // an uncollectable cycle; keep it alive for as long as the compiled
+    // function is reachable by storing it as a private property.
+    if (importer && importer.isCell()) {
+        function->putDirect(vm, builtinNames(vm).importerPrivateName(), importer, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly);
     }
 
     return JSValue::encode(function);

src/jsc/bindings/NodeVMScript.cpp

@@ -138,6 +138,7 @@ constructScript(JSGlobalObject* globalObject, CallFrame* callFrame, JSValue newT
     RETURN_IF_EXCEPTION(scope, {});
 
     fetcher->owner(vm, script);
+    script->setDynamicImportCallback(vm, importer);
 
     WTF::Vector<uint8_t>& cachedData = script->cachedData();
 
@@ -242,6 +243,7 @@ void NodeVMScript::visitChildrenImpl(JSCell* cell, Visitor& visitor)
     Base::visitChildren(thisObject, visitor);
     visitor.append(thisObject->m_cachedExecutable);
     visitor.append(thisObject->m_cachedBytecodeBuffer);
+    visitor.append(thisObject->m_dynamicImportCallback);
 }
 
 NodeVMScriptConstructor::NodeVMScriptConstructor(VM& vm, Structure* structure)

src/jsc/bindings/NodeVMScript.h

@@ -79,6 +79,11 @@ class NodeVMScript final : public JSC::JSDestructibleObject, public SigintReceiv
     void cachedDataRejected(TriState value) { m_cachedDataRejected = value; }
     bool sourceMapURLParsed() const { return m_sourceMapURLParsed; }
     void sourceMapURLParsed(bool value) { m_sourceMapURLParsed = value; }
+    void setDynamicImportCallback(JSC::VM& vm, JSC::JSValue value)
+    {
+        if (value && value.isCell())
+            m_dynamicImportCallback.set(vm, this, value);
+    }
 
     DECLARE_VISIT_CHILDREN;
 
@@ -87,6 +92,9 @@ class NodeVMScript final : public JSC::JSDestructibleObject, public SigintReceiv
     RefPtr<JSC::CachedBytecode> m_cachedBytecode;
     JSC::WriteBarrier<JSC::JSUint8Array> m_cachedBytecodeBuffer;
     JSC::WriteBarrier<JSC::ProgramExecutable> m_cachedExecutable;
+    // Keeps the importModuleDynamically callback alive; NodeVMScriptFetcher only
+    // holds a Weak reference to it to avoid an uncollectable Strong<> cycle.
+    JSC::WriteBarrier<JSC::Unknown> m_dynamicImportCallback;
     ScriptOptions m_options;
     bool m_cachedDataProduced = false;
     bool m_sourceMapURLParsed = false;

src/jsc/bindings/NodeVMScriptFetcher.h

@@ -16,7 +16,12 @@ class NodeVMScriptFetcher : public JSC::ScriptFetcher {
 
     Type fetcherType() const final { return Type::NodeVM; }
 
-    JSC::JSValue dynamicImportCallback() const { return m_dynamicImportCallback.get(); }
+    JSC::JSValue dynamicImportCallback() const
+    {
+        if (auto* cell = m_dynamicImportCallback.get())
+            return JSC::JSValue(cell);
+        return JSC::jsUndefined();
+    }
 
     JSC::JSValue owner() const
     {
@@ -42,19 +47,30 @@ class NodeVMScriptFetcher : public JSC::ScriptFetcher {
     }
 
 private:
-    JSC::Strong<JSC::Unknown> m_dynamicImportCallback;
-    // m_owner is the NodeVMScript / JSFunction / module wrapper that holds this
-    // fetcher via m_source -> SourceProvider -> SourceOrigin -> RefPtr<fetcher>.
-    // A Strong handle here would form an uncollectable cycle (the owner keeps
-    // the fetcher alive via RefPtr, and the fetcher would keep the owner alive
-    // as a GC root). Use Weak instead: when the owner is collected its
-    // SourceCode chain drops the last RefPtr to this fetcher.
+    // This fetcher is RefCounted and reachable from its owning JSCell via
+    // m_source -> SourceProvider -> SourceOrigin -> RefPtr<fetcher>. Holding
+    // either the owner or the importModuleDynamically callback via Strong<>
+    // would create an uncollectable cycle whenever the callback's closure can
+    // reach the owner (a common pattern in module linker caches). Both are
+    // therefore held weakly here; the owning NodeVMScript / NodeVMSourceTextModule /
+    // compiled JSFunction is responsible for keeping the callback alive via a
+    // normal GC edge (WriteBarrier / property) so that the Weak handle remains
+    // valid for as long as the owner is reachable.
+    //
+    // Note that nested closures share this fetcher via the SourceProvider but
+    // do not keep the owner alive, so a nested closure that outlives a dropped
+    // owner will observe a cleared m_dynamicImportCallback and import() will
+    // fail with ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING. Closing that gap without
+    // reintroducing the Strong<> cycle requires adding the fetcher as an opaque
+    // root from ScriptExecutable's visitChildren in WebKit.
+    JSC::Weak<JSC::JSCell> m_dynamicImportCallback;
     JSC::Weak<JSC::JSCell> m_owner;
     bool m_isUsingDefaultLoader = false;
 
-    NodeVMScriptFetcher(JSC::VM& vm, JSC::JSValue dynamicImportCallback, JSC::JSValue owner)
-        : m_dynamicImportCallback(vm, dynamicImportCallback)
+    NodeVMScriptFetcher(JSC::VM&, JSC::JSValue dynamicImportCallback, JSC::JSValue owner)
     {
+        if (dynamicImportCallback.isCell())
+            m_dynamicImportCallback = JSC::Weak<JSC::JSCell>(dynamicImportCallback.asCell());
         if (owner.isCell())
             m_owner = JSC::Weak<JSC::JSCell>(owner.asCell());
     }

src/jsc/bindings/NodeVMSourceTextModule.cpp

@@ -108,7 +108,7 @@ NodeVMSourceTextModule* NodeVMSourceTextModule::create(VM& vm, JSGlobalObject* g
     RETURN_IF_EXCEPTION(scope, nullptr);
     NodeVMSourceTextModule* ptr = new (NotNull, allocateCell<NodeVMSourceTextModule>(vm)) NodeVMSourceTextModule(
         vm, zigGlobalObject->NodeVMSourceTextModuleStructure(), WTF::move(identifier), contextValue,
-        WTF::move(sourceCode), moduleWrapper, initializeImportMeta);
+        WTF::move(sourceCode), moduleWrapper, initializeImportMeta, dynamicImportCallback);
     RETURN_IF_EXCEPTION(scope, nullptr);
     ptr->finishCreation(vm);
 
@@ -562,6 +562,7 @@ void NodeVMSourceTextModule::visitChildrenImpl(JSCell* cell, Visitor& visitor)
     visitor.append(vmModule->m_cachedExecutable);
     visitor.append(vmModule->m_cachedBytecodeBuffer);
     visitor.append(vmModule->m_initializeImportMeta);
+    visitor.append(vmModule->m_dynamicImportCallback);
 }
 
 DEFINE_VISIT_CHILDREN(NodeVMSourceTextModule);

src/jsc/bindings/NodeVMSourceTextModule.h

@@ -54,14 +54,18 @@ class NodeVMSourceTextModule final : public NodeVMModule {
     WriteBarrier<ModuleProgramExecutable> m_cachedExecutable;
     WriteBarrier<JSUint8Array> m_cachedBytecodeBuffer;
     WriteBarrier<Unknown> m_initializeImportMeta;
+    // Keeps the importModuleDynamically callback alive; NodeVMScriptFetcher only
+    // holds a Weak reference to it to avoid an uncollectable Strong<> cycle.
+    WriteBarrier<Unknown> m_dynamicImportCallback;
     RefPtr<CachedBytecode> m_bytecode;
     SourceCode m_sourceCode;
     bool m_hasTopLevelAwait { false };
     bool m_linkCalled { false };
 
-    NodeVMSourceTextModule(JSC::VM& vm, JSC::Structure* structure, WTF::String identifier, JSValue context, SourceCode sourceCode, JSValue moduleWrapper, JSValue initializeImportMeta)
+    NodeVMSourceTextModule(JSC::VM& vm, JSC::Structure* structure, WTF::String identifier, JSValue context, SourceCode sourceCode, JSValue moduleWrapper, JSValue initializeImportMeta, JSValue dynamicImportCallback)
         : Base(vm, structure, WTF::move(identifier), context, moduleWrapper)
         , m_initializeImportMeta(initializeImportMeta && !initializeImportMeta.isUndefined() ? initializeImportMeta : JSValue(), JSC::WriteBarrierEarlyInit)
+        , m_dynamicImportCallback(dynamicImportCallback && dynamicImportCallback.isCell() ? dynamicImportCallback : JSValue(), JSC::WriteBarrierEarlyInit)
         , m_sourceCode(WTF::move(sourceCode))
     {
     }

test/js/node/vm/vm-script-fetcher-leak.test.ts

@@ -56,4 +56,100 @@ describe("node:vm NodeVMScriptFetcher leak", () => {
 
     await expectMaxObjectTypeCount(expect, "Script", baseline + 20);
   });
+
+  // Regression: NodeVMScriptFetcher also held m_dynamicImportCallback via
+  // JSC::Strong, so any importModuleDynamically closure that could reach the
+  // resulting script/module (a common pattern in module linker caches) formed
+  // an uncollectable cycle: script -> m_source -> SourceProvider -> SourceOrigin
+  // -> RefPtr<NodeVMScriptFetcher> -> Strong<callback> -> closure -> script.
+
+  test("vm.Script with importModuleDynamically referencing the script should not leak", async () => {
+    const baseline = heapStats().objectTypeCounts.Script || 0;
+
+    function iteration() {
+      const holder: { script?: vm.Script } = {};
+      holder.script = new vm.Script("1 + 1", {
+        importModuleDynamically: () => {
+          // Closure references the script via `holder`, forming a cycle through
+          // the fetcher's callback reference.
+          return holder.script;
+        },
+      });
+    }
+    for (let i = 0; i < 500; i++) iteration();
+
+    await expectMaxObjectTypeCount(expect, "Script", baseline + 20);
+  });
+
+  test("vm.SourceTextModule with importModuleDynamically referencing the module should not leak", async () => {
+    const baseline = heapStats().objectTypeCounts.NodeVMSourceTextModule || 0;
+
+    function iteration() {
+      const cache = new Map<string, any>();
+      const mod = new vm.SourceTextModule("export const a = 1;", {
+        importModuleDynamically: specifier => cache.get(specifier),
+      });
+      cache.set("self", mod);
+    }
+    for (let i = 0; i < 500; i++) iteration();
+
+    await expectMaxObjectTypeCount(expect, "NodeVMSourceTextModule", baseline + 20);
+  });
+
+  test("vm.compileFunction with importModuleDynamically referencing the function should not leak", async () => {
+    const baseline = heapStats().objectTypeCounts.FunctionExecutable || 0;
+
+    function iteration() {
+      const holder: { fn?: Function } = {};
+      holder.fn = vm.compileFunction("return 1", [], {
+        importModuleDynamically: () => {
+          return holder.fn;
+        },
+      });
+    }
+    for (let i = 0; i < 500; i++) iteration();
+
+    await expectMaxObjectTypeCount(expect, "FunctionExecutable", baseline + 50);
+  });
+
+  test("vm.Script importModuleDynamically callback survives GC while script is alive", async () => {
+    // After making the fetcher's callback Weak, the owning script must keep the
+    // callback alive so that import() still works after a GC.
+    let called = 0;
+    const script = new vm.Script('import("kept").catch(() => {});', {
+      importModuleDynamically: () => {
+        called++;
+        throw new Error("callback reached");
+      },
+    });
+
+    Bun.gc(true);
+    await Bun.sleep(0);
+    Bun.gc(true);
+
+    script.runInThisContext();
+    await Bun.sleep(0);
+
+    // If the Weak handle had been cleared, import() would have rejected with
+    // ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING without invoking the callback.
+    expect(called).toBe(1);
+  });
+
+  test("vm.compileFunction importModuleDynamically callback survives GC while function is alive", async () => {
+    let called = 0;
+    const fn = vm.compileFunction('return import("kept").catch(() => {});', [], {
+      importModuleDynamically: () => {
+        called++;
+        throw new Error("callback reached");
+      },
+    });
+
+    Bun.gc(true);
+    await Bun.sleep(0);
+    Bun.gc(true);
+
+    await fn();
+
+    expect(called).toBe(1);
+  });
 });