JSModuleLoader: decide needsErrorReaction by membership in innerModuleLoading - #396
JSModuleLoader: decide needsErrorReaction by membership in innerModuleLoading#396robobun wants to merge 1 commit into
Conversation
…eLoading A require(esm) nested inside the host load hook drains the synchronous module queue: Bun evaluates CommonJS module bodies during makeModule, and such a body can require an ESM sibling of the graph that is currently loading. The drain then runs pending ModuleLoadStep reactions for other requests of the same referrer module, so module.loadedModules() can grow across the hostLoadImportedModule call without the current request having completed, and can grow by more than one. The size-delta check then misidentifies whether this request still needs a ModuleGraphLoadingError reaction (a missed reaction loses the rejection, leaving the graph load pending forever), and the growth assertion fires on debug builds: ASSERTION FAILED: module->loadedModules().size() <= loadedModulesCountBefore + 1 Decide by membership of the request itself instead.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesModule loading
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 changes reentrancy handling in JSC's module graph loading — a critical path — and is one half of a two-PR fix with a companion in oven-sh/bun, a human look would still be worthwhile.
What was reviewed:
- The new membership check uses the same
ModuleMapKey { request.m_specifier.impl(), request.type() }as the pre-callfind()at 2.d.ii and asfinishLoadingImportedModule, which populates the map — keys line up. - The removed second ASSERT was already asserting that the size-delta test equals the membership test, so in every case the old code accepted, behavior is unchanged; only the reentrant case (which the old code asserted against) differs.
- The extra
contains()per request is a hash lookup on a map already probed just above and is dwarfed byhostLoadImportedModule— not a perf concern.
Extended reasoning...
Overview
This PR changes one decision in JSModuleLoader::innerModuleLoading (JSModuleLoader.cpp): after calling hostLoadImportedModule for a request, it now decides whether to attach a ModuleGraphLoadingError reaction by checking whether this specific request is present in module.[[LoadedModules]], instead of comparing the map's size before/after the call. It also drops two debug assertions that assumed the map could grow by at most one across that call. Net code delta is small (one variable and two ASSERTs removed, one contains() added, plus an explanatory comment).
Security risks
None identified. This is internal module-loading control flow; no parsing, auth, or externally-attacker-controlled data handling changes. The failure mode being fixed is a hung load / debug assertion, not a memory-safety or privilege issue.
Level of scrutiny
High. This is JavaScriptCore's ECMA-262 module loading algorithm — correctness here affects every ESM graph Bun loads, and the scenario involves subtle reentrancy (Bun evaluating CJS bodies inside makeModule, which drains the synchronous module queue and can advance other pending requests of the same referrer during a single hostLoadImportedModule call). The change is small and the argument is tight — the removed second ASSERT literally stated needsErrorReaction != contains(key), i.e. the old size-delta expression was already asserted equivalent to the new membership expression in every case the old code considered valid — so the new code is provably no worse in the non-reentrant case and correct in the reentrant one. Still, module-loader reentrancy is exactly the kind of thing where a second pair of eyes on the invariants is worth having.
Other factors
- The PR description gives a concrete 6-file repro and explains the mechanism precisely; the comment added in-code preserves that reasoning for future readers.
- No test is added here; the repro requires the Bun embedder (CJS-evaluated-in-makeModule +
require(esm)), and the description says a companion oven-sh/bun PR is coming. A reviewer may want to confirm that companion lands together or that a JSC-shell-level regression is infeasible. - The
ModuleMapKeyconstruction matches both the 2.d.ii pre-check andfinishLoadingImportedModule's insert, so there is no key-shape mismatch that could cause a false miss. - No prior reviews or comments on the PR to account for.
Preview Builds
|
1 participant
Symptom
On a mixed ESM/CJS graph where a CommonJS module
require()s an ESM module that an ancestor also imports statically, debug builds of bun abort inside the graph loading step:Repro (6 files), with bun built against this tree:
bun req.cjs ./entry.mjsandBUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1 bun entry.mjsboth hit the assertion 3/3;NODE_COMPILE_CACHE=/tmp/cc bun entry.mjshits it as well.Cause
Bun evaluates CommonJS module bodies during makeModule, so user code runs inside the host load hook while
innerModuleLoadingis iterating a module's requests. When that bodyrequire()s an ESM sibling of the graph being loaded, the synchronous module queue drains and runs pending ModuleLoadStep reactions for other requests of the same referrer module.module.loadedModules()therefore grows across thehostLoadImportedModulecall without the current request having completed, and can grow by more than one.The size-delta check conflates "this request completed synchronously" with "any request completed":
needsErrorReactioncomes out false for a request that is still pending, so its ModuleGraphLoadingError reaction is never attached and a later rejection of that request is lost (the graph load then stays pending forever). On debug builds the two growth assertions abort.Fix
Decide
needsErrorReactionby membership of the request itself inmodule.loadedModules(), which is the property the error-reaction decision actually depends on, and drop the size-based assertions (growth by more than one is legal under this reentrancy).With this change plus a bun-side companion (delivering sync-transpiled source to a registry entry whose async fetch is still in flight, oven-sh/bun PR to follow), all three repro doors complete with the evaluation order Node prints:
e,d,c,b,a,entry.