JSModuleLoader: a removed registry entry's in-flight load must not re-cache its record - #472
JSModuleLoader: a removed registry entry's in-flight load must not re-cache its record#472robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 21 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between cb61607 and daf9f5f9bc466567e32ad6d0648a3a33b4583b97. 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesModule Cache Validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is comprehensive and directly related to the change. It explains the problem, root cause, fix, scope, tests, performance impact, and known limitations. It does not include the Bugzilla URL or reviewer line required by the repository template. Warning Git: Failed to clone repository. Please run the Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The author's reply on the cellLock() question checks out (callers hold it; now documented in removeEntry()), and that thread is resolved. Because this changes control flow in hostLoadImportedModule / finishLoadingImportedModule — spec-implementing module-loader hot paths — a human look would still be worthwhile.
What was reviewed:
isCacheableLoadedModule()mirrors exactly the three assertions the fast path relies on; both new call sites are gated to the realm-level cache only (per-module[[LoadedModules]]untouched).- The
hostLoadImportedModuleeviction takescellLock()like the neighboring insertion, and resetsitertoend()so the fall-through path is the pre-existing resolve-and-load flow. - The
finishLoadingImportedModuleskip still runscontinueModuleLoading/continueDynamicImporton the payload, so the in-flight import still settles; only the realm-cache write is suppressed. - Non-Bun builds: both hunks are under
USE(BUN_JSC_ADDITIONS)and the originalifline is preserved verbatim in the#else.
Extended reasoning...
Overview
Two-file change to JSModuleLoader.{cpp,h} fixing Sentry BUN-443N: a segfault when Bun's fork-only removeEntry() drops a registry entry while a load of it is in flight (or when the realm-cache key differs from the registry key). The fix adds a private helper isCacheableLoadedModule() that re-checks the fast-path invariant (registry entry exists, holds this record, has a loadPromise), and consults it at both the cache-read side (hostLoadImportedModule) and the cache-write side (finishLoadingImportedModule), realm-level only, all under USE(BUN_JSC_ADDITIONS).
Security risks
None identified. No new user-controlled input surface; the change narrows when an internal cache is trusted. No auth, crypto, or memory-lifetime changes beyond taking cellLock() for the new remove(iter), matching the existing pattern for concurrent-GC safety.
Level of scrutiny
High. hostLoadImportedModule and finishLoadingImportedModule implement ECMA-262 §16.2 module-loading abstract operations and sit on every dynamic-import path in the engine. Mistakes here manifest as subtle spec deviations, cache incoherence, or crashes across the whole runtime. The change is small and well-argued, but the surrounding machinery (microtask-driven ModuleLoadingContext steps, detached entries, per-module vs realm-level [[LoadedModules]], the require(esm) synchronous-queue overlay) is intricate enough that a maintainer with module-loader ownership should confirm the invariant reasoning — particularly that isCacheableLoadedModule() always holds on the normal (non-removeEntry) path so repeat imports still skip resolve().
Other factors
- The PR description is unusually thorough: repro shape, why the invariant holds in the normal flow, debug-assert vs release-segfault behavior, 3000-iteration perf parity, and five tests in oven-sh/bun#39674 pinned to this preview build.
- My earlier pre-existing note about
removeEntry()/clearAll()locking was answered (Bun callers holdcellLock();JSCellLockis non-reentrant so taking it inside would deadlock) and the contract is now documented in the header comment via commita092ae2. That thread is resolved and not a blocker. - No outstanding reviewer comments remain. Given the critical path, I'm deferring rather than approving.
a092ae2 to
86b6c4d
Compare
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
There was a problem hiding this comment.
I reviewed this PR and found no bugs. The earlier cellLock() question is settled — callers hold the lock, and 86b6c4d now documents that invariant on removeEntry(). Because this touches the module loader's cache/registry consistency on a hot path, a human look would still be worthwhile.
What was reviewed:
isCacheableLoadedModule()mirrors exactly what the fast-path hit asserts and dereferences (entry,entry->record() == record,entry->loadPromise()), so both guarded sites can no longer reach the null-deref.- The
finishLoadingImportedModuleskip is realm-referrer only; a module's own[[LoadedModules]]is still populated, sogetImportedModule()and linking are unaffected. - The new
loadedModules.remove(iter)is undercellLock()matchingvisitChildrenImpl(); the iterator obtained before the lock stays valid because only the GC marker (read-only) contends. - Both changes are
#if USE(BUN_JSC_ADDITIONS); the upstream path compiles unchanged.
Extended reasoning...
Overview
The PR adds a guard predicate isCacheableLoadedModule() and applies it at two points in JSModuleLoader: the realm-level [[LoadedModules]] fast-path hit in hostLoadImportedModule(), and the realm-level cache insertion in finishLoadingImportedModule(). It also adds a header declaration and documents on removeEntry() that callers hold cellLock() and that in-flight detached loads are handled by the new predicate. All logic changes are behind USE(BUN_JSC_ADDITIONS).
Security risks
None identified. This is internal cache-consistency bookkeeping in the ES module loader; there is no new user-controlled input surface, no auth/crypto/permissions code, and no new allocation or pointer arithmetic. The change strictly narrows when a cached record is trusted, which is a hardening direction relative to the previous behavior (which dereferenced a possibly-null loadPromise() / loadedEntry).
Level of scrutiny
High. JSModuleLoader is on the critical path for every import() and require(esm) in Bun, and correctness here depends on subtle ordering invariants between fetchComplete(), setLoadPromise(), moduleLoadStep microtasks, and the detached-entry lifetime after removeEntry(). The PR description is unusually thorough (Sentry ref, repro, debug-assert vs release-segfault behavior, perf check, and paired tests in oven-sh/bun#39674), and the change is small and well-scoped, but the invariants it relies on are not something I can fully verify from the diff alone — e.g., that isCacheableLoadedModule() always holds in the non-removeEntry() flow so repeat imports still hit the cache. That reasoning is stated and spot-checked by the author, but a maintainer familiar with the loader's state machine should confirm it.
Other factors
- My prior review flagged
removeEntry()/clearAll()mutating maps withoutcellLock(). The author (via robobun) explained that all Bun call sites hold the lock externally andJSCellLockis non-reentrant; commit86b6c4dnow records "Callers hold cellLock()" in the function comment. That thread is resolved and was pre-existing anyway. - The iterator used for
loadedModules.remove(iter)is obtained viafind()beforecellLock()is taken. This is safe: the only concurrent user of the map isvisitChildrenImpl()on GC marker threads, which iterates read-only, so the iterator cannot be invalidated betweenfind()andremove(). The lock is there to keep the marker from reading a mid-mutation table, not to protect the mutator's own iterator. - The
cacheRecord = falsepath infinishLoadingImportedModule()skips step 1 entirely but still runs step 2/3 (continueModuleLoading/continueDynamicImport), so the in-flightimport()still resolves — matches the PR's stated intent. - Tests exist in the paired Bun PR and the author reports passing the relevant Bun test suites against a patched engine, but there are no in-tree JSC tests added here (typical for this fork; verification is via Bun's test suite).
Given the subtlety of the loader state machine and the fact that this fixes a production crash, deferring to a human reviewer rather than auto-approving.
…f217b4 The previous preview was built on 0f966e81 and predates oven-sh/WebKit#477, which the ffi.test.js case from #39829 needs on Windows. This preview is b7f217b4 (what main pins) plus the #472 change.
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
…f217b4 The previous preview was built on 0f966e81 and predates oven-sh/WebKit#477, which the ffi.test.js case from #39829 needs on Windows. This preview is b7f217b4 (what main pins) plus the #472 change.
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
…f217b4 The previous preview was built on 0f966e81 and predates oven-sh/WebKit#477, which the ffi.test.js case from #39829 needs on Windows. This preview is b7f217b4 (what main pins) plus the #472 change.
86b6c4d to
50b13c0
Compare
There was a problem hiding this comment.
I re-reviewed after the latest push and didn't find any bugs. My earlier note about removeEntry()/clearAll() locking is resolved — callers hold cellLock(), and that contract is now documented in the header. Because this touches the core module-loading fast path and the registry/cache coherency invariants it relies on are subtle, a human look would still be worthwhile.
What was reviewed:
isCacheableLoadedModule()mirrors exactly what the fast-path hit asserts (entry && entry->record() == record && entry->loadPromise()), so the guard and the assertions stay in lockstep.- The new
loadedModules.remove(iter)takescellLock()like the existing insertion; thefind()before it is a read on the mutator thread, consistent with other lookups in this file. - The
finishLoadingImportedModuleskip is realm-referrer only, so per-module[[LoadedModules]]still fills in and step 2/3 (continueModuleLoading/continueDynamicImport) still run — the in-flight import still resolves. - Both guarded blocks are under
USE(BUN_JSC_ADDITIONS); the non-Bun build path is byte-identical.
Extended reasoning...
Overview
This PR fixes Sentry crashes BUN-443N / BUN-4NFS: a segfault in JSModuleLoader::loadModule where the realm-level [[LoadedModules]] fast path in hostLoadImportedModule returns a null loadPromise() (or dereferences a null loadedEntry) after Bun's removeEntry() drops a registry entry while a load of it is still in flight, or when the cache key and registry key differ. Two files change: JSModuleLoader.cpp adds a cacheability guard at both the cache-hit and cache-insert sites plus a small isCacheableLoadedModule() helper; JSModuleLoader.h declares the helper and adds documenting comments to removeEntry().
Since my previous comment, commit 50b13c0652 added the "Callers hold cellLock()" note to removeEntry() and expanded the explanatory comment in hostLoadImportedModule about how a cache line can outlive its entry — addressing the locking concern I raised (which the author correctly pointed out is handled at Bun's call sites, and taking the non-reentrant lock inside would deadlock them).
Security risks
None identified. This is crash-avoidance in the module loader; no auth, crypto, permissions, or untrusted-input parsing is touched. The change narrows what gets cached rather than broadening any capability.
Level of scrutiny
High. hostLoadImportedModule and finishLoadingImportedModule sit on the hot path of every dynamic import() and module graph load in Bun. The fix depends on an invariant (registry entry holds the record and a load promise by the time finishLoadingImportedModule runs for a live entry) that the PR description argues for carefully but that is not mechanically obvious from the diff alone. Getting the realm-vs-module referrer distinction wrong, or the interaction with removeEntry()'s timing, would either regress the crash or break repeat-import caching. This is exactly the kind of change where a maintainer familiar with the C++ module loader rewrite and with #258 should sign off.
Other factors
- The change is well-tested end-to-end via oven-sh/bun#39674 (five tests, four failing without the patch), plus regression suites listed in the notes, and a 3000-iteration perf check showing no slowdown.
- All new logic is gated behind
USE(BUN_JSC_ADDITIONS), so upstream/non-Bun builds are unaffected. - The iterator obtained before taking
cellLock()in the new eviction branch is safe: only the mutator thread mutatesm_loadedModules, andvisitChildrenImplon marker threads only reads it under the same lock, so the iterator cannot be invalidated betweenfind()andremove(iter). - The PR description explicitly scopes out a related pre-existing issue (error paths re-registering via
ensureRegistered) as overlapping with #258 — reasonable, but another reason a human familiar with that PR should look.
Given the criticality of the code path and the subtlety of the invariants, I'm deferring to human review rather than approving.
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
50b13c0 to
22dfd70
Compare
There was a problem hiding this comment.
I re-ran the automated review and found no bugs. The earlier note about removeEntry()/clearAll() locking was answered (callers hold cellLock(), now documented in the header) and that thread is resolved. Because this touches JSC's module-loader fast path and the registry/[[LoadedModules]] consistency invariants, a human look is still worthwhile.
What was reviewed:
isCacheableLoadedModule()mirrors the three fast-path assertions; both guard sites check it only for the realm-level cache, per-module[[LoadedModules]]still fills in.- The new
loadedModules.remove(iter)takescellLock(); the iterator from the unlockedfind()stays valid because concurrent GC only reads the map. cacheRecord = falseskips step 1's cache insert but still runsContinueModuleLoading/ContinueDynamicImport, so the in-flight payload resolves.- Both files compile with
USE(BUN_JSC_ADDITIONS)off — the non-Bun path is byte-identical.
Extended reasoning...
Overview
This PR fixes a production segfault (Sentry BUN-443N / BUN-4NFS) in Bun's ES module loader. It adds a Bun-only helper isCacheableLoadedModule() that mirrors the assertions in hostLoadImportedModule's [[LoadedModules]] fast path, and guards both the cache-hit read and the cache insert in finishLoadingImportedModule with it. When removeEntry() drops a registry entry while a load is in flight (or when a resolve hook makes the cache key differ from the registry key), the stale cache line is evicted / not written, so the fast path never dereferences a null loadPromise() or a null loadedEntry. All new logic is under #if USE(BUN_JSC_ADDITIONS); the upstream path is unchanged.
Security risks
None identified. This is internal cache-coherency logic in the module loader; no parsing, auth, or externally-controlled data handling changes. The change strictly adds checks before dereferencing.
Level of scrutiny
High. JSModuleLoader::hostLoadImportedModule and finishLoadingImportedModule are on the hot path of every dynamic import() in Bun, and the correctness argument relies on subtle invariants: when fetchComplete()/setLoadPromise() run relative to moduleLoadStep, that ModuleLoadingContext keeps a detached entry alive, that per-module [[LoadedModules]] is consulted by innerModuleLoading before reaching this fast path, and that the mutator-side unlocked find() → locked remove(iter) is safe because the GC marker only iterates. The PR description walks through each of these and the paired Bun PR adds five regression tests, but this is exactly the kind of engine-core change a maintainer should sign off on.
Other factors
- My earlier pre-existing-issue note (unlocked
removeEntry()) was answered: Bun's callers holdcellLock(),JSCellLockisn't reentrant, and a follow-up commit documented the contract in the header. That thread is resolved and was never a blocker for this PR. - The change is well-scoped (~40 effective lines, all guarded by the Bun feature flag), thoroughly explained, and locally verified against a debug-asan build with the paired test suite in oven-sh/bun#39674.
- I'm deferring rather than approving because the guidelines call for human review on complex changes to critical engine paths, not because of any specific concern with the diff.
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
22dfd70 to
daf9f5f
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix looks correct and is thoroughly documented and tested, but since it changes core JSC module-loader state handling (registry/cache coherence, promise chains, GC cellLock() interaction), a human review is still warranted.
Checked that isCacheableLoadedModule() mirrors exactly the three conditions the fast path asserts and returns, so the guard and the assert cannot disagree.
Checked the !record gate on the eviction — per-module [[LoadedModules]] is untouched, and the realm-level branch takes cellLock() for the removal like the insertion does.
Checked that skipping step 1 in finishLoadingImportedModule still falls through to step 2/3 so the payload continues and the in-flight import resolves.
The earlier cellLock() concern on removeEntry() was resolved — callers hold the lock, now documented in the comment.
Extended reasoning...
Overview
The PR guards two points in JSModuleLoader against a stale realm-level [[LoadedModules]] cache after Bun's fork-only removeEntry() drops a registry entry mid-load: (1) hostLoadImportedModule now validates a realm-level cache hit with isCacheableLoadedModule() and evicts it under cellLock() if stale, and (2) finishLoadingImportedModule skips inserting a record into the realm-level cache when the same predicate fails. A new private helper isCacheableLoadedModule() encapsulates the exact condition the fast path asserts (entry && entry->record() == record && entry->loadPromise()). All new logic is under USE(BUN_JSC_ADDITIONS); the header adds the declaration and a clarifying comment on removeEntry().
Security risks
None identified. This is internal engine cache/registry coherence; no untrusted input parsing, auth, or data-exposure surface is touched. The change strictly narrows when a cache is populated/consulted and otherwise falls through to the existing load path.
Level of scrutiny
High. JSModuleLoader is a critical runtime path in JavaScriptCore: it drives dynamic import, module graph loading, and interacts with concurrent GC via cellLock() and with microtask ordering via ModuleLoadingContext. The fix depends on subtle invariants (when loadPromise() / record() are set relative to moduleLoadStep, that innerModuleLoading consults the per-module cache before reaching hostLoadImportedModule, that the payload continuation is independent of the step-1 cache write). The PR description argues each of these convincingly and cites passing tests in oven-sh/bun#39674 plus regression suites, but engine module-loader changes of this kind should be signed off by a human familiar with the loader's state machine.
Other factors
The one prior inline finding from this bot (missing cellLock() in removeEntry()/clearAll()) was answered — Bun call sites hold the lock and JSCellLock is non-reentrant — and the PR now documents that in the removeEntry() comment; the thread is resolved. The iterator used for loadedModules.remove(iter) is obtained immediately before, with no intervening mutation, and isCacheableLoadedModule() only reads m_moduleMap (not m_loadedModules), so the iterator stays valid. The finishLoadingImportedModule change only gates the cache write; continueModuleLoading/continueDynamicImport still run, so the detached load's promise settles. Given the criticality of the code path and the multi-rebase history, deferring to human review rather than auto-approving.
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
18461ab to
3580958
Compare
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
030d206 to
d3deb97
Compare
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
d3deb97 to
f7cb3cb
Compare
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
f7cb3cb to
6a17c6a
Compare
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
6a17c6a to
cfccea9
Compare
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
…-cache its record Bun's removeEntry() (mock.module(), delete require.cache[key], plugin virtual modules) drops a registry entry, and its m_loadedModules entry, while a load of that entry can still be in flight. When that load completed, finishLoadingImportedModule() put its record back into m_loadedModules. The next top-level import of the key then took the [[LoadedModules]] fast path in hostLoadImportedModule(), looked up the registry entry that now exists for the key (a fresh one created by provideFetch(), or none), and returned its loadPromise(), which is null. JSModuleLoader::loadModule() dereferenced it in performPromiseThenWithInternalMicrotask(). finishLoadingImportedModule() now only caches a record in the realm-level map while the registry still holds that record through a loaded entry. The in-flight load still continues its payload, and a module's own [[LoadedModules]] is still filled in. hostLoadImportedModule() checks the same condition on a realm-level cache hit, and otherwise forgets the record and loads what the registry holds.
…oveEntry() callers hold the cell lock
cfccea9 to
eaacac2
Compare
…ader's import cache delete require.cache[path] and mock.module() remove the ES module registry entry of a module. When an import() of it is still loading, or when the path import() was resolved to differs from the path the registry ended up with (an onResolve chain), the loader's import cache kept pointing at the removed entry and the next import() crashed in JSModuleLoader::loadModule (null loadPromise) or in hostLoadImportedModule (null entry), or returned the stale module. The fix is in JavaScriptCore (oven-sh/WebKit#472). The tests cover the delete require.cache[] shape with the load held in flight by an async onLoad plugin, the shape where a replacement load finishes before the removed one, the onResolve chain shape with and without a replacement, and the mock.module() shape.
A removed registry entry's in-flight load no longer puts its record back into the realm-level [[LoadedModules]] cache, and the cache fast path verifies the registry entry it answers with. Fixes the null loadPromise crash in JSModuleLoader::loadModule after delete require.cache[] or mock.module() of a module whose import() was still loading (Sentry BUN-443N), and the null entry crash and stale module when the cached key and the registry key differ. Preview tag; it has to move to the merged main sha before this lands.
1 participant
Problem
JSModuleLoader::loadModule(JSModuleLoader.cpp:813):performPromiseThenWithInternalMicrotaskreadsstatus()of a null promise (segfault at0x10, undermoduleLoadTopSettled). Sentry BUN-443N (canary builds, macOS and Linux) and BUN-4NFS (the same stack on the Bun 1.4.0 release build).[[LoadedModules]]fast path inhostLoadImportedModule(JSModuleLoader.cpp:624-650). It returnsloadPromise()of the registry entry for a cached record, and only asserts that this entry exists, holds that record, and has a load promise. Bun'sremoveEntry()(mock.module(),delete require.cache[key], plugin virtual modules) drops an entry, and its cache line, while a load of it is still in flight. That load then puts its record back into the cache infinishLoadingImportedModule. The nextimport()of the key gets a fresh entry fromprovideFetch, hits the fast path, and returns its nullloadPromise().resolve()hook that is not idempotent on its own output):removeEntry()of the registry key leaves the cache line behind, and the next import crashes on the nullloadedEntry(address0x38) or, if the key was re-registered, resolves with the stale record.Fix
finishLoadingImportedModule: with the realm as referrer, cache the record only ifisCacheableLoadedModule()holds, which is the condition the fast path asserts: the registry holds this record through an entry that has a load promise or wasmarkLoaded(). The load still continues its payload, so the in-flightimport()resolves with the module it started to load, and a module's own[[LoadedModules]]is still filled in, since that module links against the record it requested.hostLoadImportedModule: check the same condition on a realm-level cache hit. Otherwise drop the cache line (undercellLock(), like the insertion) and fall through to the normal path, which loads what the registry holds now. The upstream body of the hit is unchanged. Per-module caches are not affected byremoveEntry(), andinnerModuleLoadingconsults them before it calls here.USE(BUN_JSC_ADDITIONS): onlyremoveEntry()andclearAll()can make the cache disagree with the registry. In the normal flow the condition holds at both places, so repeat imports are still served from the cache (checked, see notes).Background
m_moduleMap) maps a module key to aModuleRegistryEntry: its promises, plus the record once fetched.removeEntry()is a fork addition. Upstream only removes entries whose fetch failed (removeFailedFetchEntry()), which never reach the cache, so upstream can assert here.m_loadedModulesisRealmRecord.[[LoadedModules]]: a cache from a top-level import specifier to its record. The fast path uses it to skip the hostresolve()hook on repeat imports. It answers with the registry entry'sloadPromise, so it is only valid while it agrees with the registry.moduleLoadStepmicrotasks whoseModuleLoadingContextholds the entry they started from. AfterremoveEntry()they complete against that detached entry.f5deafe090, the commit Bun pins (rebased each time Bun moved its pin:0f966e81b7,b7f217b4a6,aea1f010b6,c148a12dd8,cb61607f1a,1cb96a7b0e,76882271d7,2da33d53e3,7259739917,0bb01ed526), so the preview build is that commit plus this change. That base lets an embedderprovideModule()+markLoaded()an entry without aloadPromise, and the fast path answers such a hit withloadedPromise();isCacheableLoadedModule()accepts those entries too (loadPromise() || isLoaded()), so their records stay cacheable.Notes
Repro shape (Bun 1.4.0-canary crashes every time): import a module whose dependency is held back by an async
onLoadplugin,delete require.cache[entry]while it waits, release it, await the import, thenimport()the entry again. A debug build failsASSERT(loadedEntry->record() == loaded)at the fast path instead. If nothing re-imports the key before the crash path,loadedEntryitself is null.Why
isCacheableLoadedModule()holds in the normal flow:fetchComplete()stores the record in the entry before the module promise settles, andsetLoadPromise()runs before the firstmoduleLoadStepis queued, so by the timefinishLoadingImportedModuleruns for a live entry, the entry holds the record and has a load promise. Repeat imports of one key produce the samedumpModuleLoadingStatetrace as before this change: the secondresolve()is still skipped.The skip in
finishLoadingImportedModulecovers step 1.a as well as 1.b: when a replacement load finishes first and caches its own record, the removed load's completion used to trip the step 1.a assertion (JSModuleLoader.cpp:955) in debug builds.Local verification used the pinned
0f966e81b7linux debug-asan tarball with theUnifiedSource-runtime-26bundle (the one that holdsJSModuleLoader.cpp) recompiled from this branch and swapped intolibJavaScriptCore.a. A debug Bun linked against it passes the three new tests: thedelete require.cache[]shape, the shape where the replacement load finishes before the removed one, and themock.module()shape. The same Bun linked against the unmodified tarball fails all three: the first and third on the fast path assertion, the second on the step 1.a assertion. The release Bun segfaults at0x10on the first and third and passes the second, since release ignores the assertion.3000 iterations of
delete require.cache[]+import()take the same time with and without the change (about 12.5 s each on the debug-asan build). Both files also compile withUSE_BUN_JSC_ADDITIONSoff. Bun'stest/js/bun/test/mock/,test/js/node/module/,test/js/bun/plugin/plugins.test.tsand the dynamic import and require(esm) files oftest/js/bun/resolve/pass against the patched engine.Found in review, covered by oven-sh/bun#39674's
onResolvechain tests: with a resolve hook that maps a -> b -> c -> d, Bun's import path resolves the specifier more than once, so the realm-level cache line is keyedcwhile the registry entry and record are keyedd.delete require.cache[d]then leaves thecline behind. Unpatched, the nextimport("./a")segfaults at0x38(loadedEntrynull), or resolves with the stale record ifdwas imported again in between; with this change the hit-side check evicts the line and the import loads (or returns) what the registry holds. This is the case that exercises thehostLoadImportedModulehalf; the in-flight tests are carried by thefinishLoadingImportedModulehalf.Pre-existing and out of scope here: when the removed in-flight load fails,
moduleLoadTopSettled(fetch rejected),moduleLoadTopRejectedandmoduleLoadStoreErrorinJSMicrotask.cppstore the error throughensureRegistered(key), which creates a fresh entry for the key, so the next import rejects with the removed load's error instead of loading again. No crash; fixing it means threading the entry through the top-levelModuleLoadingContext, which overlaps with #258, so it is left for a follow-up.