Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Walkthrough
ChangesGC cycle fix for importModuleDynamically callbacks
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 1:26 AM PT - Jun 23rd, 2026
✅ @robobun, your commit 9d2ad4deaa20d41e871a3f61499374df2caaa1b1 passed in 🧪 To try this PR locally: bunx bun-pr 29866That installs a local version of the PR into your bun-29866 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // 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); | ||
| } |
There was a problem hiding this comment.
🔴 The callback's lifetime is now anchored to the outer wrapper (NodeVMScript / the JSFunction returned by compileFunction / NodeVMSourceTextModule), but the fetcher is reachable from the SourceProvider, which is shared by every nested closure parsed from that source. So an inner closure — e.g. vm.compileFunction('return () => import("x")', [], {importModuleDynamically})(), or a closure a vm.Script installs on globalThis — can outlive the wrapper; once the wrapper is collected the Weak<> clears and import() from the surviving closure throws ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING. Pre-PR the Strong<> tied callback lifetime to the SourceProvider (i.e. "any code from this source alive ⇒ callback alive"), which is the contract import() actually needs; the new survives-GC tests only hold the outer owner so they don't catch this.
Extended reasoning...
What this PR changes
The fetcher's m_dynamicImportCallback is downgraded from Strong<Unknown> to Weak<JSCell>. To compensate, each call site adds a normal GC edge from a designated "owner" to the callback:
NodeVMScript:m_dynamicImportCallbackWriteBarriervisited invisitChildren(NodeVMScript.cpp:140 / NodeVMScript.h:80-95).vm.compileFunction: a privateimporterPrivateNameproperty on the returned outerJSFunction(NodeVM.cpp:1338-1343).NodeVMSourceTextModule:m_dynamicImportCallbackWriteBarrier.
The intended invariant is owner alive ⇒ callback alive ⇒ Weak handle valid.
Why the owner is the wrong anchor
NodeVMScriptFetcher is reachable via SourceCode → SourceProvider → SourceOrigin → RefPtr<ScriptFetcher>. In JSC, every nested FunctionExecutable parsed from a given source is a sub-range view into the same SourceProvider. So an inner closure created by running the script/function keeps the SourceProvider — and therefore the fetcher — alive independently of the outer wrapper object.
import() resolves its host hook via callerSourceOrigin(vm), which walks to the executing code's SourceOrigin → fetcher (see Zig::GlobalObject::moduleLoaderImportModule → NodeVM::importModule, NodeVM.cpp:271-280). For an inner closure that path reaches the same fetcher even after the outer wrapper is gone.
The inner closure's scope chain references the outer activation/JSLexicalEnvironment, not the outer JSFunction object, and certainly not the NodeVMScript wrapper. None of the three new GC edges are reachable from an inner closure. So:
inner closure alive ⇒ SourceProvider alive ⇒ fetcher alive, but wrapper dead ⇒
Weak<callback>cleared.
fetcher->dynamicImportCallback() then returns jsUndefined() (NodeVMScriptFetcher.h:19-24), and NodeVM::importModule falls into the !dynamicImportCallback.isCallable() branch and throws ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING.
Step-by-step repro (vm.compileFunction)
let fn = vm.compileFunction('return () => import("x");', [], {
importModuleDynamically: () => mod,
});
const inner = fn(); // inner's FunctionExecutable shares fn's SourceProvider → fetcher
fn = null; // drop the only thing rooting the callback (private property on fn)
Bun.gc(true);
await inner(); // fetcher still alive via inner's SourceProvider, but Weak<callback>
// has been cleared → ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSINGvm.compileFunctioncreates a fetcher whosem_dynamicImportCallbackisWeak<callback>, and storescallbackas a private property on the returned outerfn.fn()allocates an inner arrow closure; itsFunctionExecutableholds aSourceCodethat points tofn'sSourceProvider, which holdsSourceOrigin{…, Ref<fetcher>}.fn = nullremoves the only GC root forcallback(the private property).inner's scope chain does not reference the outerJSFunctionobject.- GC collects
fnandcallback; the fetcher'sWeak<>clears. The fetcher itself survives becauseinner → FunctionExecutable → SourceProvider → SourceOrigin → RefPtr<fetcher>. inner()evaluatesimport("x"), JSC walks to the caller's SourceOrigin, finds the fetcher, callsdynamicImportCallback()→jsUndefined(), andNodeVM::importModulethrowsERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING.
The same shape applies to new vm.Script(...).runInThisContext() installing a closure on globalThis and then dropping the Script — a very common pattern.
Why the new tests don't catch it
Both "survives GC" tests (vm.Script and vm.compileFunction) hold the outer script/fn across the GC and only then call it. They never exercise the case where an inner closure outlives the wrapper.
Comparison with pre-PR / Node.js
Pre-PR, Strong<callback> lived on the fetcher, so callback lifetime == fetcher lifetime == SourceProvider lifetime == "any code from this source is alive" — exactly the contract import() needs. Node.js gets the equivalent guarantee by stashing the registry key in host_defined_options on the v8::Script, which propagates to inner SFIs. This PR narrows the guarantee to "outer wrapper alive", which is strictly weaker and a behavioral regression.
Suggested direction
The callback needs to be rooted for the lifetime of the SourceProvider, not the wrapper. One option that still avoids the original cycle is to make the SourceProvider (or a small JSCell hung off it and visited via SourceProvider heap-cell visitation) own the callback via a WriteBarrier, so the GC can see closure → executable → provider → callback as a normal edge rather than an unconditional root. Alternatively, keep the Strong<> on the fetcher but break the cycle by giving the wrapper a WeakHandleOwner that clears the Strong<> in finalize() when the wrapper dies — though that re-introduces the "inner closure outlives wrapper" problem in the opposite direction, so the SourceProvider-anchored edge is the more correct fix.
There was a problem hiding this comment.
You're right — confirmed empirically:
// with this PR
compileFunction: outerFin=49 cbFin=49
inner import: calls=0 code=ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING
// main (Strong<>)
compileFunction: outerFin=49 cbFin=0
inner import: calls=1
The SourceProvider-scoped guarantee ("any code from this source alive ⇒ callback alive") is the correct contract, but expressing it without Strong<> needs an addOpaqueRoot(fetcher) in FunctionExecutable/GlobalExecutable::visitChildrenImpl plus a WeakHandleOwner::isReachableFromOpaqueRoots on the fetcher's Weak<callback> — ScriptExecutable doesn't visit SourceProvider/SourceOrigin/ScriptFetcher at all today, and neither WebCore ScriptFetcher subclass holds JS values, so there's no existing hook. That's a WebKit change.
Without it, owner-scoped is the tightest bound reachable from Bun's side. This PR picks that over the status-quo leak because:
- The leak fires on the common shape (any
importModuleDynamicallyclosure that can reach the result — module linker caches, the pattern in the original report) and has no workaround. - The regression fires on the narrow shape (inner closure outlives a dropped wrapper and then calls
import()) and has a workaround: keep theScript/Module/compiled function referenced. m_owneris alreadyWeakon main with the same wrapper-scoped limitation (inner closure after wrapper collection already seesowner === undefinedin the callback args).
Happy to follow up with the USE(BUN_JSC_ADDITIONS) opaque-root hook in WebKit to restore the full guarantee — that should go in as its own change since it touches the engine.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
We should probably use JSC::Weak and then have a hasPendingActivity? Can we be smarter aobu tthe GC lifetime here?
The callback is // fetcher side
m_dynamicImportCallback = Weak<JSCell>(cb, &ownerSingleton, /*context*/ this);
bool NodeVMScriptFetcherWeakOwner::isReachableFromOpaqueRoots(..., void* ctx, AbstractSlotVisitor& v, ...) {
return v.containsOpaqueRoot(ctx); // ctx == fetcher*
}…but something GC-visited has to add the fetcher as an opaque root whenever any executable from that source is live. Today nothing does: // FunctionExecutable::visitChildrenImpl / GlobalExecutable::visitChildrenImpl
if (auto* p = thisObject->source().provider())
if (auto* f = p->sourceOrigin().fetcher(); f && f->fetcherType() == ScriptFetcher::Type::NodeVM)
visitor.addOpaqueRoot(f);That needs a WebKit bump (CI uses the prebuilt). Happy to do that as the follow-up; without it, owner-scoped (what this PR does) is the tightest bound reachable from Bun's side. In the meantime this trades:
|
6f6a0c0 to
0ed715a
Compare
0ed715a to
207e4c7
Compare
|
Stale PR review: keep open, rework. The leak is real and still on main. The lifetime in this diff is not the wanted one, and the changes-requested review is still open. The diff anchors the callback to the outer wrapper ( The wanted shape scopes the callback to the source. The callback stays alive while any code compiled from that
|
|
Closing in favor of #43724. It fixes the same I built #43724 at 045b123 and ran the 5 tests from this PR against that build. All 5 pass. On 1.4.3-canary.1+367d939d9 the 3 leak tests fail with 500 of 500 objects retained. The nested closure case from the review thread here also works on that build. A closure that outlives its |
The three self-cycle shapes from #29866 (module linker caches): Script, SourceTextModule and compileFunction, one per fetcher visit site. All three retain 500/500 on the unfixed runtime.
The three self-cycle shapes from #29866 (module linker caches): Script, SourceTextModule and compileFunction, one per fetcher visit site. All three retain 500/500 on the unfixed runtime.
2 participants
What
NodeVMScriptFetcher(aRefCountedobject, not GC-managed) heldm_dynamicImportCallbackasJSC::Strong<Unknown>, which is an unconditional GC root. The fetcher is kept alive by its owningNodeVMScript/NodeVMSourceTextModule/ compiledJSFunctionvia theRefPtrchainm_source → SourceProvider → SourceOrigin → fetcher.Whenever the user's
importModuleDynamicallyclosure could reach the resulting script/module — which is the typical shape of a module linker cache — this formed an uncollectable cycle:The owner could never be collected, so the
RefPtrchain never dropped to zero, so theStrongroot never went away.Fix
The fetcher now holds
m_dynamicImportCallbackasJSC::Weak<JSCell>, mirroring the existing treatment ofm_owner. The callback's lifetime is instead tied to the owner via a normal GC edge:NodeVMScript/NodeVMSourceTextModule: newWriteBarrier<Unknown> m_dynamicImportCallbackvisited invisitChildren.vm.compileFunction: the callback is stored as a private (non-enumerable) property on the returnedJSFunction.So: owner alive ⇒ callback alive ⇒
Weakhandle valid; owner unreachable ⇒ whole graph collectable.Tests
Added to
test/js/node/vm/vm-script-fetcher-leak.test.ts:vm.Script/vm.SourceTextModule/vm.compileFunctionwith animportModuleDynamicallyclosure that references the resulting object no longer leak (heap object-type count returns to baseline after 500 iterations; previously 500+ were retained).WriteBarrier/property edge keeps theWeakhandle alive.Also verified
test/js/node/vm/vm.test.tsand the node-paralleltest-vm-module-dynamic-import/test-vm-module-link/test-vm-module-basic/test-vm-no-dynamic-import-callbacksuites pass.