Repository navigation
Conversation
…s cause The five Promise.any rejection sites in JSPromiseConstructor.cpp and JSMicrotask.cpp called createAggregateError with message = String() and cause = jsUndefined(). ErrorInstance::finishCreation treats a non-empty JSValue cause as an explicit cause and installs it as an own property, so every Promise.any rejection carried an own 'cause: undefined'. Route all five call sites through createAggregateErrorForPromiseAnyRejection, which passes an empty JSValue for cause and (under BUN_JSC_ADDITIONS) V8's "All promises were rejected" message. The async rejection paths run from a native internal microtask with no JS frames on the stack, so the captured trace is empty and materializeErrorInfoIfNeeded's BUN_JSC_ADDITIONS branch (gated on !m_stackTrace->isEmpty()) never ran, leaving .stack undefined. Drop the isEmpty() gate so the embedder hook still produces the "Name: message" header for zero-frame errors; skip line/column/sourceURL when there are no frames to source them from.
|
Warning Review limit reached
Next review available in: 17 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 (4)
Comment |
There was a problem hiding this comment.
I didn't find any bugs, but this changes user-observable error semantics in the core runtime and widens the onComputeErrorInfoJSValue embedder-callback contract to cover zero-frame stack traces, so it's worth a human look.
What was reviewed:
- Verified
JSValue()forcausecorrectly skips the own-property install inErrorInstance::finishCreation(checks!cause.isEmpty()). - Confirmed all five
Promise.anyrejection sites are routed through the new helper; no othercreateAggregateErrorcallers were affected. - Checked that the dropped
!m_stackTrace->isEmpty()gate now sends empty vectors to Bun's callback for all zero-frame errors (not justPromise.any's) — the callback must tolerate that, which lives on the Bun side. - Confirmed
line/column/sourceURLare correctly guarded byhasFramesso they aren't set to garbage/zero values on frameless errors.
Extended reasoning...
Overview
This PR touches four files in the JSC runtime to fix the AggregateError produced by Promise.any when all inputs reject:
AggregateError.h: adds an inline helpercreateAggregateErrorForPromiseAnyRejectionthat passes an emptyJSValue()forcause(so no owncauseproperty is installed) and, underUSE(BUN_JSC_ADDITIONS), sets the message to"All promises were rejected".JSPromiseConstructor.cppandJSMicrotask.cpp: mechanically reroute the five existingcreateAggregateError(..., String(), jsUndefined())call sites through the new helper.ErrorInstance.cpp: in theUSE(BUN_JSC_ADDITIONS)branch ofmaterializeErrorInfoIfNeeded, drops the!m_stackTrace->isEmpty()gate so the Bun embedder hook runs even when the captured stack has zero frames, and wraps theline/column/sourceURLputDirects in a newhasFramesguard.
Security risks
None identified. This is error-metadata plumbing — no parsing, no untrusted input, no auth/crypto surface. The new code paths allocate a small string literal and pass an empty JSValue, both benign.
Level of scrutiny
Medium-high. The call-site rerouting and the cause/message change are straightforward and low-risk. However, the materializeErrorInfoIfNeeded change is a semantic change to core error materialization that applies to every ErrorInstance created with an empty stack trace under Bun, not just Promise.any's AggregateError. Previously such errors fell through to the non-Bun path (which is a no-op for empty traces, leaving m_errorInfoMaterialized false and .stack unmaterialized). Now they enter the Bun path, invoke vm.onComputeErrorInfoJSValue() with an empty vector, set m_errorInfoMaterialized = true, and install a .stack property. This relies on Bun's callback (which lives in the Bun repo, not here) handling an empty frame vector gracefully and producing a sensible header-only string. That's a cross-repo contract change that a human maintainer should confirm.
Other factors
- The
causefix (emptyJSValue()instead ofjsUndefined()) is spec-correct: ECMA-262'sPerformPromiseAnyconstructs theAggregateErrorwith no options object, so no owncauseshould exist. This part applies unconditionally (not gated on Bun) and is a real upstream-observable bug fix. - The message string is Bun-gated, so upstream/non-Bun builds keep the spec-compliant empty message.
- The
hasFramesguard aroundline/column/sourceURLis correct — those out-params are only populated by the callback when there are frames to source them from, so writing them unconditionally would installline: 0/column: 0on frameless errors. - No new tests are included in this PR; verification presumably happens in the Bun repo's test suite.
Given the cross-repo callback-contract implication and the fact that this changes observable .stack behavior for a broader class of errors than the PR title suggests, I'm deferring to a human reviewer rather than auto-approving.
Preview Builds
|
Problem
The
AggregateErrorrejected byPromise.anyin Bun has:err.stack === undefined(no ownstackat all)err.message === ""(V8/Node set"All promises were rejected")cause: undefined(Object.hasOwn(err, "cause")istrue)while a user-constructed
new AggregateError(...)in the same process has a stack and no owncause. Result: the most common real-worldAggregateError(aPromise.anyfan-in where everything rejected) is effectively undebuggable.Cause
The five
Promise.anyrejection sites (promiseConstructorFuncAny,promiseAnySlow,promiseAnyRejectFunction,promiseAnySlowRejectFunction,promiseAnyResolveJob) call:createAggregateError(vm, structure, errors, String(), jsUndefined());ErrorInstance::finishCreationinstallscausewhenever the JSValue is non-empty, sojsUndefined()becomes an owncause: undefined. The nullString()means no ownmessageis set.The async paths (
promiseAnyResolveJob, the reject functions) run from a native internal microtask with no JS frames on the stack, som_stackTraceis an empty vector.materializeErrorInfoIfNeeded'sBUN_JSC_ADDITIONSbranch is gated on!m_stackTrace->isEmpty(), so it never runs and.stackis never materialized.Fix
createAggregateErrorForPromiseAnyRejectionwhich passes an emptyJSValue()forcause(so no own property is installed) and, underUSE(BUN_JSC_ADDITIONS), sets the message to"All promises were rejected"to match V8. Route all five call sites through it.materializeErrorInfoIfNeeded, drop the!m_stackTrace->isEmpty()gate on the Bun branch so the embedder hook still runs for zero-frame errors and produces the"Name: message"header string.line/column/sourceURLare only set when there are frames to source them from.With this change the repro above prints
string "All promises were rejected" ["message","errors","stack"], matching Node.The
causefix applies unconditionally (it is a spec-observable bug in upstream as well); the message string is Bun-only.