JSModuleRecord::evaluate: resume suspended TLA body when a cycle sibling throws#281
JSModuleRecord::evaluate: resume suspended TLA body when a cycle sibling throws#281robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Companion bun PR with tests: oven-sh/bun#33961 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches core JSC module-evaluation semantics (TLA resume vs. graph-level evaluationError in cycles), so it's worth a human look to confirm the spec reasoning.
What was reviewed:
- The
State::Initgate:Init = 0, suspended-at-await states are positive resume labels, so the check correctly distinguishes "body not yet started" from "suspended mid-body". - Confirmed
asyncExecutionFulfilled/Rejectedstep 1 already no-ops whenstatus == Evaluated, so letting the body run to completion doesn't double-resolve the graph. - Ruled out re-execution of a sync-thrown body via this gate — after completion the executable is cleared and the top-of-function null-check returns first.
Extended reasoning...
Overview
This PR changes one guard in JSModuleRecord::evaluate() (JSModuleRecord.cpp). Previously, any call into evaluate() with a non-empty evaluationError() would re-throw and bail. Now, that early-return only fires when the generator State field is still Init (0) — i.e., the module body has not yet begun executing. A body that has already started and is suspended at an await (positive state = resume-point label) will proceed into executeModuleProgram and run to completion, even if a cycle sibling stamped an evaluationError onto this record while it was suspended on the InnerModuleEvaluation stack.
Security risks
None identified. This is a spec-compliance behavioral change in module evaluation ordering; no new inputs, parsing, or trust boundaries are introduced.
Level of scrutiny
High. JSModuleRecord::evaluate sits on the hot path for every ES module and directly implements TC39 Evaluate() / AsyncBlockStart semantics. The interaction between the generator-level State (Init/Executing/Completed/resume-label) and the graph-level Status + evaluationError is subtle and spec-driven. A regression here could skip or double-run module bodies. This is not a mechanical change; it encodes a specific reading of spec steps (Evaluate step 9, AsyncModuleExecutionFulfilled/Rejected step 1) that a JSC-familiar reviewer should confirm.
Other factors
- Verified the
Stateenum inJSGenerator.h:Init = 0,Completed = -1,Executing = -2; suspended states are positive integers, soasInt32AsAnyInt() == Initis the right "never started" test and mirrors the existingstate.isNumber() && ... == Executingidiom used a few lines below and inasyncModuleExecutionResume(JSMicrotask.cpp:878). - Verified
CyclicModuleRecord::asyncExecutionFulfilled/Rejectedboth early-return whenstatus() == Evaluated(step 1), so resuming the body after the graph has already been marked failed does not corrupt graph state or double-reject the top-level capability — matching the PR description's claim. - The candidate concern "State::Init gate may allow re-execution of a synchronously-thrown module body" was examined and ruled out: once a body reaches
State::Executingand the executable is cleared, the!m_moduleProgramExecutableguard at the top ofevaluate()returns before the new check is reached. - Tests exist but live in a companion bun repo PR (oven-sh/bun#33961), not here, so this change lands without in-tree test coverage.
Given the critical code path and the spec-interpretation nature of the fix, deferring to a human reviewer rather than auto-approving.
Preview Builds
|
…ing throws When a top-level-await module is part of an import cycle and suspends at its first await, it is still on the InnerModuleEvaluation stack with status Evaluating. If a sibling in the same Evaluate() pass then throws, step 9 of Evaluate() stamps that error onto every stack member as evaluationError and transitions them to Evaluated. When the suspended body's resume microtask later fires, it re-enters JSModuleRecord::evaluate(), which would see evaluationError() set and throw it instead of resuming the body. Everything after the await (finally blocks, export initialisers, resource registration) is then silently skipped while the process continues. Node and the spec resume the body: AsyncBlockStart runs it as an independent async context, and AsyncModuleExecutionFulfilled/Rejected already handle the "status is already Evaluated" case afterwards. Gate the evaluationError() early-return on the generator State still being Init (body not yet started). A suspended body (State > 0) proceeds to executeModuleProgram and runs to completion.
0e491da to
3a40c5b
Compare
A top-level-await module that is suspended at its await inside an import
cycle was never resumed if a sibling module threw during the same
Evaluate() pass: everything after the await (finally blocks, export
initialisers) was silently skipped while the process continued. Node and
the spec run the continuation.
The resume microtask re-enters JSModuleRecord::evaluate(), which took the
evaluationError() early-return once the cycle root had stamped the
sibling's error onto the suspended module. The WebKit change gates that
early-return on the generator State still being Init; a suspended body
proceeds to executeModuleProgram and runs to completion, and
asyncExecution{Fulfilled,Rejected} already no-op when status is Evaluated.
WebKit change: oven-sh/WebKit#281 (autobuild-preview-pr-281-0e491dad)
| JSValue state = internalField(Field::State).get(); | ||
| if (state.isNumber() && state.asInt32AsAnyInt() == std::to_underlying(State::Init)) { |
There was a problem hiding this comment.
🟡 nit: The new function-scope state local (line 118) is shadowed by the pre-existing if-init JSValue state = ... on line 130. Both reads are correct — line 130 intentionally re-reads after executeModuleProgram may mutate the field — but renaming this one to something like initialState (or scoping it in its own if-init) would make it clearer that it's the pre-execution snapshot.
Extended reasoning...
What
The PR introduces JSValue state = internalField(Field::State).get(); at function scope on line 118 of JSModuleRecord::evaluate. A few lines below, the pre-existing code on line 130 has:
if (JSValue state = internalField(Field::State).get(); !state.isNumber() || ...)which declares a second state in the C++17 if-init scope, shadowing the outer one added by this PR.
Why it's only a nit
Both reads are semantically correct and intentionally distinct:
- Line 118 reads the state before
executeModuleProgramruns, to decide whether theevaluationError()early-return applies (only when the body is still inState::Init). - Line 130 re-reads the state after
executeModuleProgramreturns, because the interpreter mutatesField::Stateduring execution (e.g. toExecutingon completion, or a suspend index onawait). Reusing the outerstatehere would be wrong — it holds a stale pre-execution value — so the re-read is required regardless of naming.
There is no build impact: JavaScriptCore does not compile with -Wshadow (that flag only appears under Source/ThirdParty for gtest/opus/abseil/libwebrtc, and OptionsIOS.cmake explicitly passes -Wno-shadow). So this cannot cause a warning or failure on any supported configuration.
Step-by-step
evaluate()is entered; line 118 binds outerstateto, say,jsNumber(0)(Init) or a suspend index.- The Init check on line 119 uses outer
state— correct. executeModuleProgramruns and writes a new value intointernalField(Field::State).- Line 130's if-init declares a new
statebound to the fresh field value and uses it in the condition. The outerstateis shadowed but never referenced again, so behavior is unchanged.
Addressing the refutation
One verifier argued this shouldn't be reported at all because scoped if-init reuse is idiomatic in this file (e.g. execute() uses the same pattern) and JSC doesn't build with -Wshadow. That's fair — this is purely a readability suggestion, not a defect, and it should not block the PR. It's flagged only because the two reads are deliberately different snapshots of the same field within ~12 lines, and a distinct name makes that intent self-documenting.
Suggested fix
Rename the new variable to initialState (or move it into its own if-init so both declarations are equally scoped):
if (JSValue initialState = internalField(Field::State).get();
initialState.isNumber() && initialState.asInt32AsAnyInt() == std::to_underlying(State::Init)) {
if (JSValue error = evaluationError()) { ... }
}Entirely optional.
1 participant
Repro
When a top-level-await module is part of an import cycle and suspends at its first
await, it is still on theInnerModuleEvaluationstack with statusEvaluating. If a sibling in the sameEvaluate()pass then throws, step 9 ofEvaluate()stamps that error onto every stack member asevaluationErrorand transitions them toEvaluated.When the suspended body's resume microtask (
AsyncModuleExecutionResume) later fires, it re-entersJSModuleRecord::evaluate(), which would seeevaluationError()set and re-throw it instead of resuming the body. Everything after theawait(finally blocks, export initialisers, resource registration) is then silently skipped while the process continues.Node and the spec resume the body:
AsyncBlockStartruns it as an independent async context, andAsyncModuleExecutionFulfilled/AsyncModuleExecutionRejectedalready handle the "status is alreadyEvaluated" case afterwards (step 1 of each).Fix
Gate the
evaluationError()early-return on the generatorStatestill beingInit(body not yet started). A suspended body (State > 0) proceeds toexecuteModuleProgramand runs to completion.Verification
Tests live in the companion bun PR (
test/js/bun/resolve/dynamic-import-tla-cycle.test.ts, 3 new cases). All 9 tests in that file pass with this change applied viabun run build:local; the 3 new tests fail on stockmain.BUN_JSC_validateExceptionChecks=1passes.no test proof · iteration 0 · docs-only change; test-proof not applicable