Repository navigation
[JSC] removeEntry() and clearAll() keep the realm's [[LoadedModules]] in step with the registry - #748
Conversation
…tes only to its own registry entry
|
Preview build of 25dd940: |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. WalkthroughImported-module lookup now uses the registry-key path for symbol specifiers and realm referrers. Successful results are stored in ChangesModule loading cache handling
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No concrete merge-blocking issue is established. Confidence is limited because registry-key consistency for realm-referrer imports could not be verified; normal build and module-loading checks remain necessary. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the bug, root cause, fix, affected code paths, and verification. However, it does not follow the repository template because it omits the Bugzilla bug title and link, the reviewed-by line, and the required changed-file and function summary.
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.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the entryOfLoad fallback (context with no entry still writes the error to whatever entry now sits under the key) and the referrer overload's lookup at JSModuleLoader.cpp:930 by the unresolved m_specifier (an entry registered under a different resolved key is never adopted) — both are pre-existing upstream behavior this PR narrows rather than introduces, so not flagged separately.
Extended reasoning...
The change is 37 lines across JSMicrotask.cpp, JSModuleLoader.cpp and ModuleLoadingContext.h, all under USE(BUN_JSC_ADDITIONS), pinning an in-flight top-level module load to its own registry entry and gating the [[LoadedModules]] cache write on the record still being registered. It touches no injection, auth or data-exposure surface. A confirmed inline finding on the moduleLoadTopSettled re-registration path plus further verified findings that are not posted mean a human should review; there are no in-repo tests since removeEntry() is not reachable from the jsc shell.
…ger looks it up by key
dylan-conway
changed the title
There was a problem hiding this comment.
Beyond the inline finding, I also checked the unconditional context->entry() dereferences in the Bun branches of moduleLoadTopSettled and moduleLoadTopRejected: those two continuations are only attached from the by-name loadModule() (JSModuleLoader.cpp:449/453), where entryOfLoad is always non-null under USE(BUN_JSC_ADDITIONS), and the record-based loadModule() path that can seed a null entry via getRegisteredMayBeNull only reaches moduleLoadStoreError, which keeps its if (!entry) return guard. The upstream #else branches pass a null entry to the new defaulted parameter, so non-Bun behavior is unchanged.
Extended reasoning...
The change touches JSC's module loader (JSModuleLoader.cpp/.h, JSMicrotask.cpp, ModuleLoadingContext.h) so an in-flight load under USE(BUN_JSC_ADDITIONS) holds its own ModuleRegistryEntry instead of re-looking it up by key; no security-sensitive surface. A confirmed finding is being posted inline, so this body only records the null-dereference and upstream-behavior concerns that were examined and ruled out.
…odule() opens, loadEntry() loads, and the realm's [[LoadedModules]] is gone
dylan-conway
changed the title
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
…FetchError(), for every kind of load
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
…ules]]: hostLoadImportedModule() asks the registry
dylan-conway
changed the title
There was a problem hiding this comment.
I reviewed the current version of this PR (now only the removal of the realm's [[LoadedModules]] table, after the last commit put the rest of the loader back as on main) and found no bugs; a human look is still worthwhile because this is JSC's module loader, CI runs it without assertions, and the PR description still describes the larger refactor (loadEntry(), entry-carrying ModuleLoadingContext, removal in setFetchError()) that the diff no longer contains, so it is worth confirming the reduced scope is meant to fix only the [[LoadedModules]] crash and not the other four issues the description lists.
What was reviewed:
- The widened
specifier.isSymbol() || referrer.isRealm()lookup inhostLoadImportedModule(): every realm-referrer caller (moduleLoadTopSettledafterprovideFetch(),loadAndEvaluateModule(SourceCode)) passes the key the entry was just registered under, andresolve()still runs when no entry exists, so behavior only changes where the old code already requiredresolve(key) == key. finishLoadingImportedModule(): the realm branch now records nothing; the deleted fast path was the only reader of that table, and theCyclicModuleRecordbranch is unchanged.visitChildrenImpl,removeEntry(),clearAll(): no remaining reference tom_loadedModulesanywhere in Source/JavaScriptCore, so no unvisitedWriteBarrieris left behind.
Extended reasoning...
The diff touches two files in Source/JavaScriptCore/runtime (JSModuleLoader.cpp/.h, +8/-46) and removes the realm-level [[LoadedModules]] table, its GC visit, its two purges, and the hostLoadImportedModule fast path that read it, replacing that with a direct registry lookup for realm-referrer loads. It touches no auth, injection, or data-exposure surface. It was not approved because the module loader is a core engine path owned by jsc-reviewers in CODEOWNERS, the fork's CI lanes run without assertions, no test accompanies the change, and the PR description is stale relative to the code after the final commit reverted the larger refactor. The earlier inline findings from prior runs were on code that the final commit removed from the diff.
… module to it that the registry no longer holds
dylan-conway
changed the title
…er whatever specifier it was asked for
dylan-conway
changed the title
There was a problem hiding this comment.
I reviewed the latest push (the removeEntry() purge-by-record line) and found no bugs; the final version nets out to ~12 lines, but since it changes the invariant between the registry and the realm's [[LoadedModules]] in a path that is only exercised by Bun's removal routes, a human look would still be worthwhile.
What was reviewed:
- The new gate in finishLoadingImportedModule mirrors the exact lookup the fast path in hostLoadImportedModule asserts on (record->moduleKey() with the request's type), so a skipped write can only cost a resolve() on the next import, never a wrong record.
- The unconditional m_module dereference in removeEntry: the loader's m_loadedModules is populated only from finishLoadingImportedModule with a non-null record, and removeIf visits live entries only, so it cannot be null.
- Both edits sit inside USE(BUN_JSC_ADDITIONS) (the header block spans lines 193-263), and the #else arm keeps upstream's line unchanged; module (non-realm) referrers take the original path.
Extended reasoning...
The change touches Source/JavaScriptCore/runtime/JSModuleLoader.cpp and JSModuleLoader.h only, adding a registry-consistency guard before the realm's [[LoadedModules]] insert and widening removeEntry() to also purge by the record's module key. It touches no auth, injection, or data-exposure surface. The last commit (25dd940) addresses the pre-existing specifier-vs-key mismatch flagged inline in the prior run, and no new findings surfaced this run. Approval is withheld because .github/CODEOWNERS covers /Source/JavaScriptCore, and the module-loader lifecycle under concurrent removal is subtle enough (the branch went through several reverted designs) that a maintainer should confirm the invariant choice; CI also runs release-only, so the ASSERTs in this path are not exercised.
…44281) ### What does this PR do? Fixes a segfault at address `0x10` in `JSC::JSModuleLoader::loadModule` ← `moduleLoadTopSettled` (Sentry BUN-4NFS, BUN-41VH), on every platform. The fix is in oven-sh/WebKit#748 (merged), which also explains why it belongs there. This PR bumps WebKit to it and adds the tests. Two things ride along: - **The WebKit bump goes to `fb1167ebf2cb`, the current tip.** Besides the fix (`0e8e9c238e91`) that brings in one more commit, oven-sh/WebKit#749: on Linux, OS randomness is read with `getrandom(2)`, and `/dev/urandom` is opened only when that fails. - **A type error on main.** `src/js/bun/sql.ts(892,26): error TS2339: Property 'command' does not exist on type '{}'` came in with #35119 and fails "Lint JavaScript" for every PR that includes it. `unsafeQueryFromTransaction()` now says what its query resolves to (`SQLResultArray`). It is a type argument only, so the code that runs is the same. **Cause** The module loader has two tables of what is loaded: the registry, and a shortcut that lets a repeated `import()` skip resolving. Removing a module clears both. But an `import()` of that module that is still in flight adds it to the shortcut afterwards, when its dependencies have loaded. The next `import()` finds the old module there, pairs it with a new registry entry that has no load promise yet, and dereferences null. ```ts const inFlight = import("./a"); // a.ts is registered, its dependencies are still loading mock.module("./a", () => ({ a: "mocked-a" })); await inFlight; await import("./a"); // segfault ``` Everything that removes a module while its `import()` is in flight gets there: | route | 1.4.2 | canary `11c41c645` | with the fix | |---|---|---|---| | `delete require.cache[path]` | segfault | ok (#40123) | ok | | `mock.module()` | segfault | segfault | ok | | `build.module()` in a plugin | segfault | segfault | ok | | a `bun --hot` reload | segfault | segfault | ok | | a plugin's `onResolve` redirecting a → b → c → d, `delete require.cache[d]`, `import(a)` again | segfault | segfault | ok | The last one needs nothing in flight: the shortcut is keyed by the name that was asked for, and removing a module cleared it only by the name it resolved to. It was found in review, and nothing in the crash reports points at it. Of the 481 reports, 273 are from `bun test`, 203 from `bun run` or `bun <file>`, and 240 had an HTTP server running. **Fix** (oven-sh/WebKit#748) The place that adds a module to the shortcut first checks that the registry still holds it, and skips the write if not. And removing a module clears it from the shortcut under whatever name it was asked for. It is eleven lines in Bun's part of the loader. For a program that removes no module nothing changes. ### How did you verify your code works? Four new tests, one for each route that still crashes. All four fail on 1.4.2 and on canary `11c41c645`, and pass against oven-sh/WebKit#748 on debug and release builds (Linux x64). Without the WebKit bump, CI had all three segfault at `0x10` on every platform: macOS, Linux glibc and musl, and Windows, x64 and arm64. The hot reload test passed 10 runs of 10. | test | file | |---|---| | mock.module() of a module whose import() is still loading its dependencies | `mock-module.test.ts` | | build.module() of a module whose import() is still loading its dependencies | `plugins.test.ts` | | should import a module again after a hot reload while its import() was still loading its dependencies | `hot.test.ts` | | import() after delete require.cache of a module that onResolve redirected a resolved path to | `plugins.test.ts` | The WebKit PR has the evidence that nothing changes for a program that removes no module, the comparison of behavior with main, WebKit's own module tests and the cost.
1 participant
Problem
A null dereference in
JSModuleLoader::loadModule←moduleLoadTopSettled, fault address0x10(Bun crash reports BUN-4NFS, BUN-41VH). A debug build stops earlier, atASSERTION FAILED: loadedEntry->record() == loadedinhostLoadImportedModule().The loader has two tables of what is loaded:
m_moduleMap[[LoadedModules]],m_loadedModulesimport()finds its record here and skipsresolve()The fast path in
hostLoadImportedModule()takes the record it finds in the second table, looks its entry up in the registry, and returns that entry's load promise. It counts on the two tables agreeing, and asserts it.removeEntry()andclearAll()clear both, and leave them out of step in two ways.1. A write that comes after the purge. A load that is in flight at that moment writes its record into the second table afterwards, when its dependencies have loaded:
import("./a")starts.ais registered, its dependencies are loading.ais removed. Both tables are cleared of it, and it is not in the second one yet.ato the second table.import("./a")registers a new entry, finds the old record in the second table, pairs it with the new entry, which has no load promise yet, and dereferences null.If a new load of
ahas started by step 3 there is no crash: later imports silently get the old module.2. An entry the purge misses. The second table is keyed by the specifier that was asked for, the registry by the key
resolve()made of it.removeEntry(key)purges the second table by specifier only. With aresolve()that turns a key it produced into yet another key, the table holdsc → record of d,removeEntry(d)leaves that behind, and the nextimport()finds a record whose entry is gone. Nothing has to be in flight.The routes in Bun:
11c41c645delete require.cache[path]mock.module()build.module()in a pluginbun --hotreloadbun --hotreload, the old load finishing while a new one is in flightonResolveredirecting a → b → c → d,delete require.cache[d],import(a)againThe first four are what the crash reports show. Nothing in them points at the last one, which was found in review: it takes an unusual plugin.
Why the fix is here and not in Bun
Upstream is not at fault. It writes a record into the second table only once the module and all its dependencies have loaded, and it removes an entry from the registry only when its fetch failed. The two never overlap, so the tables cannot disagree.
removeEntry()andclearAll()are this fork's. They remove any entry at any time. They do purge the second table, but a purge cannot catch what has not been written yet.Bun cannot avoid the state from outside. oven-sh/bun#40123 does for
require.cache, by not removing a module that is still loading.mock.module()could wait for the load instead. A hot reload cannot do either: it clears the whole registry at once, and the loader keeps no list of the loads in flight for it to wait for.Fix
1.
finishLoadingImportedModule()is the only place that writes to the second table. Before it adds a record to the realm's table, it checks that the registry still holds that record under its key. If not, the write is skipped. The load itself goes on, so whoever awaits it still gets its module. +10 −0, in one function, inside#if USE(BUN_JSC_ADDITIONS), with upstream's line kept in the#else.2.
removeEntry()purges the second table by the key of the record as well as by the specifier. One line, in a function of this fork.A module's own
[[LoadedModules]]is not affected by either.For a program that removes no module nothing changes. The condition is what the fast path already asserts about the two tables (
loadedEntryexists,loadedEntry->record() == loaded), checked at the write instead of the read. An entry has its record fromfetchComplete()on, which is before any of the calls that reach this write, so without a removal the condition holds. The second change is inremoveEntry(), which such a program never calls.An earlier version of this PR removed the second table instead. That saved a
resolve()per load by name for every program, which is a change in behavior a Bun test pins, and it deleted upstream code. This version does neither.Verification
The numbers below were taken with the first change alone. The second was added afterwards.
The condition is never false without a removal. A build that logs every evaluation of the condition, with the number of removals its loader had seen, ran 615 of Bun's test files (compiled executables, hot reload and watch,
bun run, the test runner, plugins, mocks,Bun.ModuleGraph, resolution,node:module,node:vm, workers, and the 394 regression tests):No assertion failed. 20 of the files fail or time out on that debug build. They do on a debug build of unmodified main too, with the same tests failing by name.
Same behavior as main. Two release builds of Bun from one tree, differing in these ten lines: 23 scripts print the same. They cover numbers of
onLoadandonResolvecalls, identity of error objects, order against a promise chain, retries after a failure to resolve, build, link or evaluate, which of two concurrent imports finishes first, andrequire()while animport()of the same module is in flight.test/bundler/bundler_compile_prelinked.test.ts, which pins the number ofresolve,fetch,evaluateandimporthook calls of a compiled executable, passes 79 of 79.The bug. The six routes above, on a debug build with assertions.
JSTests/modules: 149 of 149 with a debugjscwith assertions and ASAN.Cost: one hash lookup when a load by name finishes. The same two release builds, runs interleaved and pinned, with a copy of the baseline as an A/A control:
import()simport()s, whole processTimes overlap: the best warm
import()of an absolute path is 1.795 µs and 1.763 µs for the two baselines and 1.716 µs with the change, on a busy machine.Tests.
removeEntry()is not reachable from thejscshell, so the tests of the crash are in oven-sh/bun#44281: one each formock.module(),build.module(), a hot reload and the redirect chain. Without a fix, Bun's CI had the first three segfault at0x10on macOS, Linux (glibc and musl) and Windows, x64 and arm64.