Repository navigation
DFG/FTL: bail out of the PerformPromiseThenOneHandler fast path when an async context is active - #278
Conversation
…an async context is active The PerformPromiseThenOneHandler fast path stores the handler directly on the promise as an inline reaction. Inline reactions cannot carry Bun's async context (performPromiseThen already falls back to a full reaction in that case), so once the calling code tiered up to the DFG, AsyncLocalStorage.getStore() returned undefined inside .then() continuations. Check globalObject->m_asyncContextData and take the slow path, which captures the context, when one is active. Fixes oven-sh/bun#33806.
There was a problem hiding this comment.
I didn't find any issues and the JIT check faithfully mirrors the C++ path in JSPromise.cpp, but hand-written DFG/FTL codegen for the promise-then fast path is worth a human pass.
Extended reasoning...
Overview
This PR adds a runtime bailout to the PerformPromiseThenOneHandler fast path in both the DFG (DFGSpeculativeJIT.cpp) and FTL (FTLLowerDFGToB3.cpp) tiers: when JSGlobalObject::m_asyncContextData holds a non-undefined internal field 0, the JIT takes the existing operationPerformPromiseThenOneHandler slow path so performPromiseThen can capture Bun's async context into a full reaction. A trivial offsetOfAsyncContextData() accessor is added to JSGlobalObject.h. All new code is guarded by USE(BUN_JSC_ADDITIONS).
Security risks
None apparent. The check only adds a load/compare on VM-internal state (a WriteBarrier<InternalFieldTuple>) and diverts to an already-existing slow path. No user-controlled data influences the new offsets or loads, and the null-pointer case is guarded before dereferencing internal field 0. The JSInternalFieldObjectImpl<>::offsetOfInternalField(0) and m_heaps.JSInternalFieldObjectImpl_internalFields[0] accessors match established patterns elsewhere in the DFG/FTL.
Level of scrutiny
High. This is hand-written codegen in both optimizing JIT tiers on the Promise.prototype.then hot path. The DFG side introduces a scoped GPRTemporary and converts a single Jump into a JumpList; the FTL side inserts two new basic blocks between the packed-mask check and the fast path and moves lastNext. Mistakes here manifest as miscompilation rather than compile errors, so a reviewer familiar with SpeculativeJIT register lifetimes and B3 block linking should confirm the control flow (in particular that packed still dominates fastPath in the FTL lowering, which it does since it's loaded in the entry block before any branch).
Other factors
The logic exactly mirrors the C++ guard in JSPromise::performPromiseThen (JSPromise.cpp:350-357), the PR description documents end-to-end verification against the repro across DFG-only and FTL configurations plus the async_hooks and promise test suites, and the bug hunter found nothing. I'm deferring rather than approving purely because JIT lowering changes fall outside the "simple/mechanical" bar for auto-approval.
WalkthroughThis change adds async context detection to Promise.then compilation across DFG and FTL JIT tiers, gated by ChangesAsync context aware promise-then compilation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
Preview Builds
|
Points at autobuild-preview-pr-278-45bbb77c so the new regression test runs against the fixed JSC. Re-pin to the merge commit's autobuild when the WebKit PR lands.
Allocating a GPRTemporary can emit a spill store; doing so after the first slowCases branch means that store only runs on the fall-through path and can desync the register state assumed by addSlowPathGenerator. Hoist the allocation next to the packed temporary and drop the now unnecessary block scope.
There was a problem hiding this comment.
I didn't find any issues, but this touches DFG/FTL codegen with register-allocation ordering constraints (which already needed one fixup), so it's worth a final human sign-off.
Extended reasoning...
Overview
This PR adds an async-context bailout to the PerformPromiseThenOneHandler fast path in both the DFG (DFGSpeculativeJIT.cpp) and FTL (FTLLowerDFGToB3.cpp) tiers, plus a new offsetOfAsyncContextData() accessor on JSGlobalObject. When m_asyncContextData's internal field 0 is not undefined, the JIT now routes to the existing slow path so performPromiseThen can capture the context into a full reaction — mirroring the check already present in the C++ runtime path.
Security risks
None apparent. The change only adds an extra runtime guard that diverts to an existing, already-exercised slow path; it introduces no new attacker-controlled inputs, no memory layout changes beyond a compile-time offset accessor, and is gated behind USE(BUN_JSC_ADDITIONS).
Level of scrutiny
High. This is hand-written JIT code generation in two optimizing tiers. Correctness here depends on subtle invariants — register/temporary allocation ordering relative to branches, JumpList linkage into addSlowPathGenerator, and B3 basic-block threading (appendTo / lastNext). The reviewer already caught one such ordering bug (GPRTemporary allocated after a slow-path branch), which was fixed in d95b137. That's exactly the class of issue that argues for human eyes on the final revision rather than bot approval.
Other factors
- A domain expert has already reviewed and requested a change; that change was applied and the thread resolved, but no explicit approval has been posted yet.
- Verification was done against a local Bun build (repro + async_hooks/promise test suites), which is reassuring but not something I can independently confirm.
- The diff is small and focused, and the non-
BUN_JSC_ADDITIONSbuild path is unchanged, but the affected code is on a very hot path (Promise.prototype.then).
Given the JIT-correctness sensitivity and the in-flight human review, deferring is the right call.
oven-sh/WebKit#280 was merged with fork main to pick up oven-sh/WebKit#278 (the DFG/FTL PerformPromiseThenOneHandler async-context bailout for #33806); the preview release is published with 43 artifacts.
oven-sh/WebKit#280 was merged with fork main to pick up oven-sh/WebKit#278 (the DFG/FTL PerformPromiseThenOneHandler async-context bailout for #33806); the preview release is published with 43 artifacts.
oven-sh/WebKit#280 was merged with fork main to pick up oven-sh/WebKit#278 (the DFG/FTL PerformPromiseThenOneHandler async-context bailout for #33806); the preview release is published with 43 artifacts.
Repro (oven-sh/bun#33806)
On Bun 1.4.0 this prints
FAIL 7538/8000, first bad iteration 462: every chain loses its store oncechaintiers up to the DFG.BUN_JSC_useDFGJIT=0makes it pass;BUN_JSC_useFTLJIT=0does not, so the break is in the DFG tier (FTL inherits it). Regression against Bun 1.3.14, introduced with the C++ promise rewrite that added thePromisePrototypeThenIntrinsic/PerformPromiseThenOneHandlerfolding.Cause
Promise.prototype.thenwith one callable handler is folded to aPerformPromiseThenOneHandlernode, and its DFG/FTL fast path stores the handler directly on the promise as an inline reaction. Inline reactions cannot carry Bun's async context: the C++performPromiseThenexplicitly skips the inline-reaction path whenglobalObject->m_asyncContextDataholds an active context (JSPromise.cpp), but the JIT fast path has no such check, so the reaction is enqueued without the context and the continuation runs withgetStore() === undefined.Fix
Mirror the C++ check in both JIT tiers: load
m_asyncContextData's internal field 0 and take the existing slow path (operationPerformPromiseThenOneHandler->performPromiseThen, which captures the context into a full reaction) when it is not undefined. The fast path stays a two-load compare when no AsyncLocalStorage scope is active, and the check is compiled only underUSE(BUN_JSC_ADDITIONS).Verification
With a
bun run build:localdebug build of oven-sh/bun at b05b4fab0 plus this change:PASS(default tiers,BUN_JSC_useFTLJIT=0, and lowered FTL thresholds all pass)PASS: all 3200 scoped renders kept their contexttest/js/node/async_hooks/(111 tests) andtest/js/node/promise/passCompanion Bun PR with the regression test to follow.