Conversation
A SyntheticSourceProvider generator is a closure. A generator that exports a JS value captured that value, and a closure cannot be traced, so the embedder had to root the value and decide when to release the root. The loader runs a generator every time it makes a module from the source: never, once, or more than once. No release point is right for all three. JSSourceCode::createWithPayload() makes the source of a synthetic module together with the value it exports. The cell traces the value, and JSModuleLoader::makeModule() passes it to the generator, which is a plain function (PayloadSyntheticSourceGenerator) and captures nothing. The three generator forms share one slot in SyntheticSourceProvider, and the payload takes the bytes that rounding up to the cell size left unused in JSSourceCode, so neither object grows.
|
Preview build of 47ef27f: |
A module that exports a JS value (a plugin's loader "object", a mock.module() result, a JSON, TOML or YAML file, a text file) had a generator that captured the value as a raw pointer. The value was protected when the generator was made and unprotected inside the generator's first run. The loader runs a generator each time it makes a module from the source, which is never, once, or more than once: - A second run read a freed value: a crash, or a namespace with the properties of another object. - A source that never became a module, or whose generator threw, kept the value protected forever, and the value kept its realm alive. The value is now the payload of the JSSourceCode (JSSourceCode::createWithPayload, oven-sh/WebKit#758). The cell traces it and the loader passes it to a generator that captures nothing. WEBKIT_VERSION is the preview build of oven-sh/WebKit#758.
A module that exports a JS value (a plugin's loader "object", a mock.module() result, a JSON, TOML or YAML file, a text file) had a generator that captured the value as a raw pointer. The value was protected when the generator was made and unprotected inside the generator's first run. The loader runs a generator each time it makes a module from the source, which is never, once, or more than once: - A second run read a freed value: a crash, or a namespace with the properties of another object. - A source that never became a module, or whose generator threw, kept the value protected forever, and the value kept its realm alive. The value is now the payload of the JSSourceCode (JSSourceCode::createWithPayload, oven-sh/WebKit#758). The cell traces it and the loader passes it to a generator that captures nothing. WEBKIT_VERSION is the preview build of oven-sh/WebKit#758.
![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 a new GC-traced field and visitChildren to a core JSC cell and can only be exercised from Bun (no JSTests coverage), a human look would still be worthwhile.
What was reviewed:
- Every
generate()caller and everySyntheticSourceProviderconstructor site was updated;makeModule()'spayload()call sits inside the existingUSE(BUN_JSC_ADDITIONS)block, so the non-Bun build is unaffected. m_payloadis default-empty on thecreate()paths and appended unconditionally invisitChildrenImpl(empty barrier is fine);createWithPayloadsets it via the write barrier afterfinishCreation, andJSCellInlines.hpulls inSlotVisitorInlines.hforDEFINE_VISIT_CHILDREN.- The
sizeof(JSSourceCode) <= 32assert checks out: 8 (JSCell) + 16 (SourceCode: RefPtr + two ints) + 8 (payload). - Candidate issues ruled out: a generator that throws keeps the payload alive for the registry entry's life (by design, the cell owns it); the replay path reads
payload()only after the result is already treated as aJSSourceCode; a null function pointer crashes at make time rather than creation (embedder contract, same as the existing forms).
Extended reasoning...
The change touches five JavaScriptCore files: SourceProvider.h collapses the two synthetic generator members into a Variant and adds a captureless function-pointer form that receives a JSValue payload; JSSourceCode gains a WriteBarrier m_payload, a visitChildren, and a createWithPayload factory under USE(BUN_JSC_ADDITIONS); JSModuleLoader::makeModule and SyntheticModuleRecord::runDeferredGenerator pass the payload (or an empty value with an assert) through. It touches no injection, auth, or data-exposure surface, but it does touch GC tracing of a core cell, which is memory-safety-sensitive. Deferring rather than approving because Source/JavaScriptCore is covered by a CODEOWNERS entry, the new tracing path has no in-repo test (only downstream Bun tests), and the new ASSERTs are not exercised by any CI lane.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)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 (5)
Included review availability: This review used your included allowance. Your plan provides up to 5 included reviews per hour; 1 remain after this review. WalkthroughSynthetic source providers now support eager, lazy, and payload-aware generators. With BUN_JSC_ADDITIONS enabled, JSSourceCode stores and traces a payload. JSModuleLoader passes that payload to synthetic module generation, while deferred generation passes an empty value. ChangesSynthetic module payload flow
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established. The payload reuse path remains unverified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
1 participant
Needed by oven-sh/bun#44466.
Problem
SyntheticSourceProvidergenerator is a closure, which the collector cannot trace. A generator that exports a JS value holds a raw pointer and a root, and Bun drops the root in the first run.makeModule()runs the generator each time a module is made from the source: never, once, or more than once. In Bun a second run reads a freed object (ASSERTION FAILED: decontaminate(),StructureID.h(76)), and a root that stays keeps its realm alive.Fix
JSSourceCode::createWithPayload()makes a synthetic source with the value it exports. The cell traces the value, andmakeModule()passes it to the generator, a plain function.jscshell cannot make a synthetic source.Background
JSSourceCode.removeEntry(),clearAll()or the synchronous replay inhostLoadImportedModule(), which this fork added.JSC::Strongin the closure. The value reaches that root through its global, so its realm is never collected.Downsides
JSSourceCode: +6 instructions per module source per full collection.JSSourceCodecosts one more store when it is made. It grows from 24 to 32 bytes inside the same 32-byte cell.Notes
visitChildrencall per liveJSSourceCodeper full collection, fast path 35 -> 41 instructions for a source with no payload (objdump, release).sizeof(SyntheticSourceProvider)stays 208 bytes on debug and 200 on release, because the three generator forms share one slot. Per object or data module fetch:Heap::protect1 -> 0,Heap::unprotect1 -> 0, malloc calls 3 -> 2.Strong: value, Structure, global object, loader, registry entry, fetch promise,JSSourceCode, provider, root. With the protect root that Bun has today, 20 ShadowRealms that each start twoimport()of one.jsonfile leave 21 live global objects after a full collection. Without a root, 2.moduleLoadTopSettledstill gives the source of a removed entry to a new entry by key (the thread at [JSC] removeEntry() and clearAll() keep the realm's [[LoadedModules]] in step with the registry #748 (comment)). After this change that second module is made from a live value.jscshell has no way to make aSyntheticSourceProvider, so there is no test inJSTests. CI here builds every lane. Bun's current sources and the Bun change both build and pass against the preview.