[JSC] A live ScriptExecutable makes its source's ScriptFetcher an opaque root - #712
Conversation
…que root A ScriptFetcher is ref-counted and reached from code through SourceCode -> SourceProvider -> SourceOrigin -> RefPtr<ScriptFetcher>, which the GC cannot see. A fetcher that holds a JS value for its code could only root it with a Strong, which also roots everything the value reaches for as long as any of that code is unswept, or hold it weakly and rely on some other cell, which cannot follow code that outlives that cell: code built by eval or the Function constructor copies its caller's SourceOrigin and has no GC edge back to anything the embedder controls. Function, program, eval and module executables now add their source's fetcher as an opaque root when visited, so the fetcher can hold the value in a Weak whose WeakHandleOwner answers containsOpaqueRoot(fetcher): the value is alive exactly while some executable from that source is.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughWhen ChangesSource fetcher GC traversal
Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains what changed, why it changed, the GC behavior, cost, and consumer issue. It does not follow the required template because it omits the bug title, Bugzilla URL, review line, and changed-file details.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds work to visitChildren on JSC's concurrent marking threads and changes what every script-backed executable roots, a human look from someone who knows the GC would still be worthwhile.
What was reviewed:
- The new
visitSourceFetcherpath:addOpaqueRootdrops null, so sources without a fetcher cost one null check;GlobalExecutablecovers program, eval and module executables viaBase::visitChildren. - Whether
m_sourcecan change under a concurrent marker: the only post-construction write isFunctionExecutable::overrideInfo, reached only withJSC_functionOverrides(a debugging option), so ordinary runs read a stable provider. - Opaque roots keyed on a raw
ScriptFetcher*: a Weak whose owner answerscontainsOpaqueRoot(fetcher)could see a stale address until the next full GC, which is the usual opaque-root caveat rather than a defect in this diff. - Cost: one hash-set lookup per visited executable with a fetcher on each full GC; no tests or consumer are in this repo (the consumer is the linked Bun PR).
Extended reasoning...
The change is 10 lines across Source/JavaScriptCore/runtime/ScriptExecutable.h, FunctionExecutable.cpp and GlobalExecutable.cpp, all under USE(BUN_JSC_ADDITIONS), making a ScriptExecutable's source ScriptFetcher an opaque root during marking so an embedder can hold a JS value in a Weak tied to the fetcher's lifetime. It touches no injection, auth or data-exposure surface, but it does run on concurrent GC marking threads and reads m_source.provider()->sourceOrigin().fetcher() without synchronization. It was not approved outright because it is a GC-semantics design choice affecting every executable, /Source/JavaScriptCore is covered by the repository's (upstream-inherited) CODEOWNERS, and no tests or consumer of the new root are in this diff; the bug hunt exited on dry_streak with three candidates ruled out and no findings.
This review covers commit b9b1237, which is no longer the latest commit on this pull request; later commits are not covered by it.
|
Preview build of 723034c: |
oven-sh/WebKit#712 merged as 299c5323879e79af282d7bb7bac8b8446a0be3f3; its autobuild release has all platform tarballs. Replaces the preview-tag pin.
…mport() (#43724) ### What does this PR do? `NodeVMScriptFetcher` held the `importModuleDynamically` callback of every `vm.Script`, `vm.compileFunction` and `vm.SourceTextModule` in a `JSC::Strong`. The fetcher is ref-counted and held by the code's `SourceProvider`, which the GC cannot see, so the `Strong` rooted the callback until all code from that source had been swept. A callback that can reach its context (a function compiled inside it, or a host closure over the sandbox) therefore kept the context, its code, the fetcher and itself alive for the life of the process: callback -> context -> function -> executable -> SourceProvider -> SourceOrigin -> RefPtr<fetcher> -> Strong -> callback Node.js keeps the callback, and the referrer it is called with, alive exactly as long as `import()` can still be initiated from the code ([`registerModule`](https://github.com/nodejs/node/blob/v26.3.0/lib/internal/modules/esm/utils.js#L151-L185)). This PR does the same. The fetcher holds both in a `JSC::Weak` whose `WeakHandleOwner` answers `visitor.containsOpaqueRoot(fetcher)`, and every cell that holds a source using the fetcher adds it as an opaque root when it is visited: `NodeVMScript`, `NodeVMSourceTextModule`, and JSC's function / program / eval / module executables (oven-sh/WebKit#712, picked up by the `WEBKIT_VERSION` bump here). That follows the code, not the `Script` object or the context, so it also covers code that outlives both: a function the script built with another realm's `Function` constructor or indirect `eval` carries the script's source origin and still reaches the callback, as in Node.js. Also fixed, because they are the same few lines: - `new vm.Script(source, "filename")` left the callback as the empty `JSValue`, and `import()` from that script crashed (segfault at `0x5`). It now rejects with `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`. - The referrer passed to the callback (the `Script`, the function, or the `SourceTextModule`) was `undefined` once nothing else referenced it. Node.js keeps it alive with the code; so does this. ### How did you verify your code works? `test/js/node/vm/vm-import-callback-lifetime-fixture.js` runs unchanged under Node.js (`--expose-gc --experimental-vm-modules --no-compilation-cache`; V8's script cache otherwise keeps host-defined options alive for a while). Output per scenario: | scenario | Node.js v26.3.0 | Bun 1.4.2 | this PR | | --- | --- | --- | --- | | `leak-script` | `{"alive":0}` | `{"alive":20}` | same as Node.js | | `leak-compileFunction` | `{"alive":0}` | `{"alive":20}` | same as Node.js | | `leak-module` | `{"alive":0}` | `{"alive":20}` | same as Node.js | | `alive-hostFunction` | `{"contextCollected":true,"result":"hooked","referrer":"Script"}` | `...,"referrer":"undefined"}` | same as Node.js | | `alive-compiledBeforeRun` | `{"result":"hooked","referrer":"Script"}` | same | same as Node.js | | `alive-module` | `{"result":"hooked","referrer":"SourceTextModule"}` | `...,"referrer":"undefined"}` | same as Node.js | | `freed-perScriptClosures` | `{"alive":0}` | `{"alive":20}` | same as Node.js | | `stringFilename` | `{"result":"ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING"}` | segfault at `0x5` | same as Node.js | ### Before merging - `WEBKIT_VERSION` is pinned to oven-sh/WebKit#712's preview build (`autobuild-preview-pr-712-723034c4` = main's WebKit + that PR) so CI tests the whole change; swap it to the merged SHA before merging.
2 participants
What
FunctionExecutable::visitChildrenandGlobalExecutable::visitChildren(program, eval and moduleexecutables) call
visitor.addOpaqueRoot(sourceOrigin().fetcher()). BehindUSE(BUN_JSC_ADDITIONS).Why
A
ScriptFetcheris ref-counted and reached from code throughSourceCode -> SourceProvider -> SourceOrigin -> RefPtr<ScriptFetcher>, which the GC cannot see. A fetcherthat needs to hold a JS value for the code it belongs to (Bun's
NodeVMScriptFetcherholds node:vm'simportModuleDynamicallycallback) has had two choices: aStrong, which roots the value and everything itreaches for as long as any of that code is unswept, or a
Weakkept alive by some other cell, which cannotfollow code that outlives that cell. Code built by
evalor theFunctionconstructor copies its caller'sSourceOrigin, so it carries the same fetcher with no GC edge back to anything the embedder controls.With the fetcher as an opaque root, the embedder can hold the value in a
WeakwhoseWeakHandleOwner::isReachableFromOpaqueRootsanswersvisitor.containsOpaqueRoot(fetcher): the value isthen alive exactly while some executable from that source is, which is what V8 gives Node.js through a
script's host-defined options.
Cost: one
addOpaqueRootper visited executable; it returns on the null check for every source without afetcher.
The consumer is oven-sh/bun#43724, whose
tests fail without this:
import()from code that outlives itsvm.Scriptand its context loses thecallback.