getAsyncStackTrace: unwrap InternalFieldTuple in reaction context - #347
Conversation
Under USE(BUN_JSC_ADDITIONS), JSPromise::resolveWithInternalMicrotaskForAsyncAwait wraps the awaiting generator in InternalFieldTuple(generator, asyncContext) when an AsyncLocalStorage store is active. getParentGenerator reads that context and casts it with dynamicDowncast<JSAsyncFunctionGenerator>, which fails on the tuple, so every at-async frame is dropped from error.stack. Unwrap the tuple in getContextValueFromPromise so the direct-await, combinator, and Promise.race branches all see the underlying context.
|
Warning Review limit reached
Next review available in: 7 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, guarded diagnostic fix.
What was reviewed:
- Verified
AsyncContextSwapScope::wrapWithCurrentstores[userContext, asyncContext], sogetInternalField(0)reads the correct slot. dynamicDowncast<InternalFieldTuple>(JSValue)matches the existing pattern ingetParentGeneratorand is null-safe under theif (context)guard.- Change is under
AssertNoGC;getInternalFieldis a plain WriteBarrier read with no allocation. - Only affects async stack-trace output, not execution semantics; entirely gated by
USE(BUN_JSC_ADDITIONS).
Extended reasoning...
Overview
This PR adds ~10 lines to Interpreter::getAsyncStackTrace's getContextValueFromPromise lambda in Interpreter.cpp, plus a conditional #include "InternalFieldTuple.h". When the promise reaction context is an InternalFieldTuple (which AsyncContextSwapScope::wrapWithCurrent creates to bundle the awaiting generator with the captured ALS context), the code now unwraps slot 0 to recover the original generator instead of failing the subsequent dynamicDowncast<JSAsyncFunctionGenerator> and dropping the async frame.
Security risks
None. The change is read-only, uses dynamicDowncast (type-checked, returns nullptr on mismatch), and only influences the contents of error.stack. It runs inside an AssertNoGC region and getInternalField(0) is a simple WriteBarrier<Unknown>::get() with no allocation or side effects. No auth, crypto, or memory-safety-sensitive paths are touched.
Level of scrutiny
Low-to-moderate. While this is JSC interpreter code, the change is (a) fully gated behind #if USE(BUN_JSC_ADDITIONS), (b) confined to a diagnostic path that builds stack-trace strings, and (c) defensive — if the context is not an InternalFieldTuple, behavior is byte-for-byte identical to before. I cross-checked AsyncContextSwapScope.h and confirmed the tuple layout is [userContext, asyncContext], so index 0 is correct. InternalFieldTuple is documented as used exclusively for ALS bookkeeping, so there is no ambiguity about other tuple producers colliding here; even if one did, the downstream dynamicDowncast<JSAsyncFunctionGenerator> would simply return nullptr (same as pre-patch behavior).
Other factors
Placing the unwrap in getContextValueFromPromise is the right spot — it covers all three call sites (direct await, JSPromiseCombinatorsGlobalContext, and the Promise.race chain) without duplication. A Bun-side regression test (test/regression/issue/24003.test.ts in oven-sh/bun#35772) exercises the fix. No outstanding reviewer comments.
The hook is now registered the first time AsyncLocalStorage turns tracking on, so code that never touches ALS pays nothing at error-capture time. Also point at oven-sh/WebKit#347, which unwraps the tuple inside getAsyncStackTrace and will make this hook unnecessary once WEBKIT_VERSION moves past it.
The hook is now registered the first time AsyncLocalStorage turns tracking on, so code that never touches ALS pays nothing at error-capture time. Also point at oven-sh/WebKit#347, which unwraps the tuple inside getAsyncStackTrace and will make this hook unnecessary once WEBKIT_VERSION moves past it.
The hook is now registered the first time AsyncLocalStorage turns tracking on, so code that never touches ALS pays nothing at error-capture time. Also point at oven-sh/WebKit#347, which unwraps the tuple inside getAsyncStackTrace and will make this hook unnecessary once WEBKIT_VERSION moves past it.
Preview Builds
|
2 participants
Under
USE(BUN_JSC_ADDITIONS),JSPromise::resolveWithInternalMicrotaskForAsyncAwaitwraps the awaiting generator inInternalFieldTuple(generator, asyncContext)when anAsyncLocalStoragestore is active, so the context can be restored when the microtask resumes.Interpreter::getAsyncStackTrace'sgetParentGeneratorreads that context viaasyncStackTraceContext()and casts it withdynamicDowncast<JSAsyncFunctionGenerator>, which fails on the tuple, so everyat asyncframe is dropped fromerror.stackunder ALS. Unwrapping ingetContextValueFromPromisecovers the direct-await,JSPromiseCombinatorsGlobalContext, andPromise.racebranches that all flow through it.Bun-side test: oven-sh/bun#35772 (
test/regression/issue/24003.test.ts), which bumpsWEBKIT_VERSIONto this branch's preview autobuild.