innerModuleEvaluation 12.b.v: replace watermark heuristic with referrer asyncEvaluationOrder identity#254
Conversation
…er asyncEvaluationOrder identity The previous skip used three local discriminators (depWasAlreadyEvaluatingAsync, order < asyncOrderWatermark, pendingAsyncDependencies == 0) to guess whether an EvaluatingAsync dep was the dynamic-import referrer (self-deadlock — must skip) or an unrelated TLA sibling (must wait). The same predicate matched the @lexical/react shape (oven-sh/bun#30634): two Promise.all'd dynamic imports sharing a TLA wrapper, where skipping the wait runs the second consumer with the wrapper's post-await exports still in TDZ. Replace the heuristic with an exact identity check. The host hook captures the referrer's asyncEvaluationOrder() (a VM-unique int64, set at step 12.b before ExecuteAsyncModule) and threads it through ModuleLoadingContext -> ModuleLoaderPayload -> continueDynamicImport -> dynamicImportLoadSettled -> evaluate() -> innerModuleEvaluation. At 12.b.v: skip iff cyclic->asyncEvaluationOrder().order() == referrerAsyncOrder. -1 (no TLA referrer / static entry) never matches. Adds JSModuleLoader::asyncEvaluationOrderForKey() so the embedder can do the registry lookup without coupling it to the resolve() referrer. Adds referrerAsyncOrder param to JSC::importModule / requestImportModule / loadModule (first overload). dynamicImportLoadSettled now receives (module, payload) so the order survives the microtask hop without a new GC field. Drops asyncOrderWatermark, depWasAlreadyEvaluatingAsync, and VM::moduleAsyncEvaluationCount() (only consumer was the removed heuristic). Scope vs the old heuristic: matches only when import() is lexically inside the TLA referrer. import() from a helper file called by the TLA continuation, and transitive A->import(B)->import(C)->A chains, now follow spec/Node behaviour (unsettled TLA). Registry-key misses (query string, non-JS type) degrade to -1 (spec behaviour); they cannot reintroduce the TDZ crash.
WalkthroughUnder ChangesreferrerAsyncOrder propagation through JSC module pipeline
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp (1)
1309-1332:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve the original dependency order before rewriting to the cycle root.
Line 1310 replaces
cyclicwithrequiredModule->cycleRoot()before Line 1332 compares async orders. Ifimport()runs before the referrer’s SCC is popped, the captured value can be the referrer module’s own order, not the later cycle-root order; this check then misses the exact referrer and still appends an async parent, leaving the dynamic import waiting on its own TLA chain.Proposed adjustment
// 12.b.i. Assert: requiredModule.[[Status]] is one of EVALUATING, EVALUATING-ASYNC, or EVALUATED. ASSERT(cyclic->status() == Status::Evaluating || cyclic->status() == Status::EvaluatingAsync || cyclic->status() == Status::Evaluated); +#if USE(BUN_JSC_ADDITIONS) + bool dependencyMatchesReferrerAsyncOrder = cyclic->asyncEvaluationOrder().hasOrder() + && cyclic->asyncEvaluationOrder().order() == referrerAsyncOrder; +#endif // 12.b.ii. Assert: requiredModule.[[Status]] is EVALUATING if and only if stack contains requiredModule. ASSERT(stack.contains(requiredModule) == (cyclic->status() == Status::Evaluating)); @@ // happen to be EvaluatingAsync (`#30259`, `#30634`) never match. - if (cyclic->asyncEvaluationOrder().order() != referrerAsyncOrder) { + if (!dependencyMatchesReferrerAsyncOrder && cyclic->asyncEvaluationOrder().order() != referrerAsyncOrder) { `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp` around lines 1309 - 1332, The comparison of asyncEvaluationOrder with referrerAsyncOrder at line 1332 should use the original requiredModule's async order, not the cycle root's order, because referrerAsyncOrder was captured at the import() call site based on the original referrer module. Currently, the code overwrites cyclic with requiredModule->cycleRoot() at line 1310 before the comparison, causing a mismatch when the cycleRoot differs from the original module. Preserve the original requiredModule's asyncEvaluationOrder value before assigning cyclic to requiredModule->cycleRoot(), then use the preserved original order value in the comparison on line 1332 instead of cyclic->asyncEvaluationOrder().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/runtime/Completion.cpp`:
- Around line 283-290: The importModule function currently exposes the
referrerAsyncOrder parameter publicly without any Bun-specific guards, even
though this parameter is only consumed by Bun-gated evaluation paths. Guard the
function declaration or create separate overloads using USE(BUN_JSC_ADDITIONS)
preprocessor conditionals to ensure the version with the referrerAsyncOrder
parameter is only exposed in Bun builds, preventing non-Bun JSC API/ABI from
carrying Bun-specific state. This ensures consistency with the coding guideline
that Bun-specific features in Source/JavaScriptCore must be guarded with
USE(BUN_JSC_ADDITIONS).
In `@Source/JavaScriptCore/runtime/JSModuleLoader.cpp`:
- Around line 1099-1103: The helper in this code block returns the order of the
leaf referrer record without normalizing it to the async SCC cycle root, causing
order comparisons in innerModuleEvaluation to fail when non-root TLA modules
resume. After the dynamicDowncast<CyclicModuleRecord> succeeds, normalize the
cyclic module to its cycle root (by calling cycleRoot() on it) before retrieving
the asyncEvaluationOrder, ensuring the returned order matches what will be
compared against requiredModule->cycleRoot() downstream.
---
Outside diff comments:
In `@Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp`:
- Around line 1309-1332: The comparison of asyncEvaluationOrder with
referrerAsyncOrder at line 1332 should use the original requiredModule's async
order, not the cycle root's order, because referrerAsyncOrder was captured at
the import() call site based on the original referrer module. Currently, the
code overwrites cyclic with requiredModule->cycleRoot() at line 1310 before the
comparison, causing a mismatch when the cycleRoot differs from the original
module. Preserve the original requiredModule's asyncEvaluationOrder value before
assigning cyclic to requiredModule->cycleRoot(), then use the preserved original
order value in the comparison on line 1332 instead of
cyclic->asyncEvaluationOrder().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28ffffe5-755d-4a0c-84b0-8be8f6c40501
📒 Files selected for processing (14)
Source/JavaScriptCore/runtime/AbstractModuleRecord.cppSource/JavaScriptCore/runtime/AbstractModuleRecord.hSource/JavaScriptCore/runtime/Completion.cppSource/JavaScriptCore/runtime/Completion.hSource/JavaScriptCore/runtime/CyclicModuleRecord.cppSource/JavaScriptCore/runtime/CyclicModuleRecord.hSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSModuleLoader.cppSource/JavaScriptCore/runtime/JSModuleLoader.hSource/JavaScriptCore/runtime/ModuleLoaderPayload.cppSource/JavaScriptCore/runtime/ModuleLoaderPayload.hSource/JavaScriptCore/runtime/ModuleLoadingContext.cppSource/JavaScriptCore/runtime/ModuleLoadingContext.hSource/JavaScriptCore/runtime/VM.h
💤 Files with no reviewable changes (1)
- Source/JavaScriptCore/runtime/VM.h
| JSPromise* importModule(JSGlobalObject* globalObject, const Identifier& moduleName, const Identifier& referrer, RefPtr<ScriptFetchParameters> parameters, RefPtr<ScriptFetcher> scriptFetcher, bool deferred, int64_t referrerAsyncOrder) | ||
| { | ||
| VM& vm = globalObject->vm(); | ||
| JSLockHolder lock(vm); | ||
| RELEASE_ASSERT(vm.atomStringTable() == Thread::currentSingleton().atomStringTable()); | ||
| RELEASE_ASSERT(!vm.isCollectorBusyOnCurrentThread()); | ||
|
|
||
| return globalObject->moduleLoader()->requestImportModule(globalObject, moduleName, referrer, WTF::move(parameters), WTF::move(scriptFetcher), deferred); | ||
| return globalObject->moduleLoader()->requestImportModule(globalObject, moduleName, referrer, WTF::move(parameters), WTF::move(scriptFetcher), deferred, referrerAsyncOrder); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Keep the Bun-only async-order parameter behind the Bun flag.
referrerAsyncOrder is only consumed by the Bun-gated evaluation path, but this public JSC::importModule wrapper now exposes and forwards it for every build. Please keep the extra parameter/overload under USE(BUN_JSC_ADDITIONS) consistently through the loader API so non-Bun JSC API/ABI does not carry Bun-specific state.
As per coding guidelines, "Source/JavaScriptCore/**/*.{cpp,h}: Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/runtime/Completion.cpp` around lines 283 - 290, The
importModule function currently exposes the referrerAsyncOrder parameter
publicly without any Bun-specific guards, even though this parameter is only
consumed by Bun-gated evaluation paths. Guard the function declaration or create
separate overloads using USE(BUN_JSC_ADDITIONS) preprocessor conditionals to
ensure the version with the referrerAsyncOrder parameter is only exposed in Bun
builds, preventing non-Bun JSC API/ABI from carrying Bun-specific state. This
ensures consistency with the coding guideline that Bun-specific features in
Source/JavaScriptCore must be guarded with USE(BUN_JSC_ADDITIONS).
Source: Coding guidelines
| auto* cyclic = dynamicDowncast<CyclicModuleRecord>(entry->record()); | ||
| if (!cyclic) | ||
| return -1; | ||
| auto order = cyclic->asyncEvaluationOrder(); | ||
| return order.hasOrder() ? order.order() : -1; |
There was a problem hiding this comment.
Normalize resumed referrers to the async SCC cycle root.
innerModuleEvaluation compares the order after 12.b.iv rewrites an EvaluatingAsync dependency to requiredModule->cycleRoot(), but this helper returns the leaf referrer record’s order. A non-root TLA module that resumes and calls import() can therefore pass an order that never matches the later cycle-root check, so the dynamic import may still wait on its own async SCC.
Proposed adjustment
auto* cyclic = dynamicDowncast<CyclicModuleRecord>(entry->record());
if (!cyclic)
return -1;
+ if (auto* cycleRoot = cyclic->cycleRoot(); cycleRoot && cycleRoot->asyncEvaluationOrder().hasOrder())
+ cyclic = cycleRoot;
auto order = cyclic->asyncEvaluationOrder();
return order.hasOrder() ? order.order() : -1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/runtime/JSModuleLoader.cpp` around lines 1099 - 1103,
The helper in this code block returns the order of the leaf referrer record
without normalizing it to the async SCC cycle root, causing order comparisons in
innerModuleEvaluation to fail when non-root TLA modules resume. After the
dynamicDowncast<CyclicModuleRecord> succeeds, normalize the cyclic module to its
cycle root (by calling cycleRoot() on it) before retrieving the
asyncEvaluationOrder, ensuring the returned order matches what will be compared
against requiredModule->cycleRoot() downstream.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this redesigns the TLA self-deadlock heuristic with an acknowledged scope tradeoff (helper-file / transitive chains now hang per spec instead of being caught), threads new state through the whole dynamic-import pipeline, and reorders the DynamicImportLoadSettled microtask args in a way that affects the non-Bun build too — worth a human pass on the design call and the upstream-shared changes.
Extended reasoning...
Overview
Replaces the three-predicate watermark heuristic at innerModuleEvaluation 12.b.v with an exact identity check against the dynamic-import referrer's asyncEvaluationOrder(). To get that identity to the comparison site, an int64_t referrerAsyncOrder is threaded through JSC::importModule → requestImportModule → loadModule → ModuleLoadingContext → ModuleLoaderPayload → continueDynamicImport → dynamicImportLoadSettled → evaluate() → innerModuleEvaluation. continueDynamicImport now takes the ModuleLoaderPayload* directly (instead of unpacked promise+deferred), and the DynamicImport{,Defer}LoadSettled microtask argument layout is swapped so arguments[0] is the module record and arguments[2] is the payload. Adds the exported helper asyncEvaluationOrderForKey() and drops VM::moduleAsyncEvaluationCount().
Security risks
None apparent. The added state is a plain int64_t; no new GC edges, no user-controlled input reaching the comparison. Registry-key misses degrade to -1 (spec behaviour). Nothing touches auth/crypto/permissions.
Level of scrutiny
High. This is core ECMA-262 module evaluation machinery and an intentional spec deviation whose semantics are being narrowed. The PR description explicitly calls out that two patterns the old heuristic caught (import() from a helper called by the TLA continuation; transitive A→import(B)→import(C)→A) now follow spec/Node behaviour, i.e. unsettled TLA. That is a deliberate behavior change a human should sign off on. The microtask argument reordering and the continueDynamicImport signature change are not behind USE(BUN_JSC_ADDITIONS), so they affect the upstream build path as well and need to stay in lockstep with both DynamicImportLoadSettled and DynamicImportDeferLoadSettled consumers (they do, from what I checked, but it's the kind of thing a maintainer should eyeball).
Other factors
Supersedes #228; tests live in a companion Bun branch rather than in this repo. 14 files changed across the loader pipeline, plus an exported-API signature addition — well outside the "simple/mechanical" bar for auto-approval.
Preview Builds
|
1 participant
Supersedes #228.
The previous skip used three local discriminators (
depWasAlreadyEvaluatingAsync,order < asyncOrderWatermark,pendingAsyncDependencies == 0) to guess whether an EvaluatingAsync dep was the dynamic-import referrer (self-deadlock — must skip) or an unrelated TLA sibling (must wait). The same predicate matched the @lexical/react shape (oven-sh/bun#30634): twoPromise.all'd dynamic imports sharing a TLA wrapper, where skipping the wait runs the second consumer with the wrapper's post-await exports still in TDZ.What
Replace the heuristic with an exact identity check. The host hook captures the referrer's
asyncEvaluationOrder()(a VM-uniqueint64_t, set at step 12.b beforeExecuteAsyncModule) and threads it throughModuleLoadingContext→ModuleLoaderPayload→continueDynamicImport→dynamicImportLoadSettled→evaluate()→innerModuleEvaluation. At 12.b.v: skip iffcyclic->asyncEvaluationOrder().order() == referrerAsyncOrder.-1(no TLA referrer / static entry) never matches. No new GC edge — the int64 is the only added state; the target module rides the existing microtaskcellslot.Adds
JSModuleLoader::asyncEvaluationOrderForKey()so the embedder can do the registry lookup without coupling it to theresolve()referrer. Addsint64_t referrerAsyncOrder = -1param toJSC::importModule/requestImportModule/ firstloadModuleoverload.Drops
asyncOrderWatermark,depWasAlreadyEvaluatingAsync, andVM::moduleAsyncEvaluationCount()(only consumer was the removed heuristic).Scope vs the old heuristic
Matches only when
import()is lexically inside the TLA referrer.import()from a helper file called by the TLA continuation, and transitive A→import(B)→import(C)→A chains, now follow spec/Node behaviour (unsettled TLA) — these were caught by the old heuristic but are indistinguishable from WebKit#30634 without the referrer identity. Registry-key misses (referrer with query string, non-JS type) degrade to-1(spec behaviour); they cannot reintroduce the TDZ crash.Tests (Bun-side, oven-sh/bun
claude/tla-referrer-async-order)test/js/bun/resolve/dynamic-import-tla-cycle.test.ts— all 6 pass: