DOMException: record source-mapped positions and a Bun-format stack on internally created exceptions - #39320
DOMException: record source-mapped positions and a Bun-format stack on internally created exceptions#39320robobun wants to merge 4 commits into
Conversation
…n internally created exceptions createDOMException() attached line/column/sourceURL/stack with JSC's addErrorInfo(), so they held positions in Bun's transpiled output and a JSC-format stack string, unlike Errors, whose positions go through the source map when they are materialized. Replace it with Bun::addErrorInfoWithSourceMap(), which formats the captured frames with Bun::formatStackTrace() and stores the mapped position (plus the originalLine/originalColumn an Error gets) as DontEnum properties. The error printer fallback for objects that are not ErrorInstances now also reads column and treats a present originalLine as meaning the position is already mapped, instead of substituting the unmapped line, so uncaught DOMExceptions print the line they were created on.
|
Warning Review limit reached
Next review available in: 8 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 12:49 AM PT - Aug 16th, 2026
✅ @robobun, your commit 03d96c67fad7a2f582908623ebc93c3f461f458f passed in 🧪 To try this PR locally: bunx bun-pr 39320That installs a local version of the PR into your bun-39320 --bun |
|
Status: reproduced and fixed.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes user-visible .stack formatting on internally-created DOMExceptions and interacts with several open PRs (#39311, #32898, #35723), a maintainer look at the sequencing would still be worthwhile.
What was reviewed:
addErrorInfoWithSourceMapreuses the existingformatStackTracepath that Errors already go through; theRef<DOMException>local keeps the C++ object alive across thename()/message()reads.- The
exceptionFromStringchange mirrors theoriginalLine→remappedhandling infromErrorInstance; the pinned user-object case instack.test.ts(at http://example.com/test.js:42, nooriginalLine, nocolumn) is unaffected. - No new GC hazard from holding the captured
Vector<StackFrame>locally — the frames' cells are still on the live VM stack, same as theJSC::addErrorInfothis replaces.
Extended reasoning...
Overview
Replaces JSC's raw addErrorInfo() with a Bun-side addErrorInfoWithSourceMap() in createDOMException(), so DOMExceptions Bun creates get source-mapped line/column/sourceURL and a Bun-format stack, matching what ErrorInstances get via the computeErrorInfo hook. Also fixes exceptionFromString() to read column and treat an originalLine property as the already-remapped signal (mirroring fromErrorInstance()), rather than overwriting line with the unmapped position. Adds a thorough describe.concurrent block to the existing DOMException test file covering five creation sites, a frameless case, and the uncaught/unhandled printer output.
Security risks
None. This is error-formatting/diagnostics code; no auth, crypto, parsing of untrusted input, or filesystem writes.
Level of scrutiny
Medium-high. C++ JSC bindings touching stack capture and property attachment is an area where GC rooting, exception-scope discipline, and finalizer safety matter. However, the new function is a thin wrapper around the existing, well-trodden formatStackTrace() path (same one computeErrorInfoWithoutPrepareStackTrace uses), and the ZigException.cpp change removes a TODO: don't sourcemap it twice by aligning with the sibling fromErrorInstance path. The captured frames are used synchronously while the corresponding JS frames are still live on the VM stack, so no new rooting is needed — same as the JSC::addErrorInfo being replaced.
Other factors
- Tests are comprehensive: they compute expected line numbers from the fixture source (not hardcoded), compare against an
Erroron the same line, assert non-enumerability, cover the no-JS-on-stack case, and verify both the unhandled-rejection and uncaught-exception printer paths (frame line + code excerpt). Tests usetempDir+bunEnv+ concurrent subprocess spawning with pipes drained viaPromise.all, per repo conventions. - The PR description explicitly verified the change under
BUN_JSC_validateExceptionChecks=1and re-ran the abort/structuredClone/stack/capture-stack-trace/inspect-error suites. - The user-visible
.stackformat on DOMExceptions changes from JSC'smodule code@...to Bun'sName: message\n at .... This is a correctness fix (matches Node and Bun's own Error format) but is observable, and the PR notes it interacts with three other open PRs — a maintainer should confirm the landing order.
rejectWithCause() called makeCause(), whose ML import variant declares a ThrowScope, and then createDOMException() without checking the scope. createDOMException() now declares exception scopes of its own while formatting the stack, so the exception-check validator flagged every ML-DSA / ML-KEM import failure. Check after makeCause(), and cover the promise rejection creation paths in the DOMException position tests.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds a new C++ helper that runs source-map remapping and putDirect from inside createDOMException()'s many callers, changes the exceptionFromString() fallback semantics, and alters user-visible stack format / property enumerability on DOMExceptions, a human look would still be worthwhile.
What was reviewed
addErrorInfoWithSourceMap():getStackTracereturning null (stackTraceLimit deleted) short-circuits;line == beforeFirst()guards the frameless case;formatStackTrace()putsoriginalLine/originalColumnon the passed object, matching the Error path.exceptionFromString(): previously swappedoriginalLinein forline; now keepsline+columnand setsframe.remapped, which mirrorsfromErrorInstance()'s handling and is whatremap_zig_exceptionexpects.stack.test.ts's user-object case (line: 42, nooriginalLine) still yieldsremapped=false→ unchanged.rejectWithCause(): the addedRETURN_IF_EXCEPTIONreturns{}intorejectWithCallback, whose caller handles an empty JSValue.- Comment-cop lint fired on earlier revisions; the latest commit reduced both to one-liners.
Extended reasoning...
Overview
This PR replaces JSC's addErrorInfo() in createDOMException() with a new Bun::addErrorInfoWithSourceMap() that captures the stack via JSC::getStackTrace, runs it through Bun's existing formatStackTrace() (which source-maps positions and records originalLine/originalColumn), and putDirects DontEnum line/column/sourceURL/stack on the DOMException wrapper. It also updates exceptionFromString() to read column and treat originalLine as an already-mapped marker (setting frame.remapped) rather than substituting it for line, and adds a ThrowScope + RETURN_IF_EXCEPTION in SubtleCrypto's rejectWithCause() because the new path opens exception scopes inside createDOMException(). A ~200-line test describe covers six creation sites, the frameless case, the rejection-with-cause path, and the uncaught/unhandled printer output.
Security risks
None identified. The change is confined to error-position reporting and stack formatting; no auth, crypto correctness, or untrusted-input parsing is touched (the SubtleCrypto.cpp edit only adds an exception check).
Level of scrutiny
Medium-high. This is C++ in the JSC bindings layer where exception-scope discipline, GC safety, and source-map interaction are all in play. createDOMException() is called from many sites (abort signals, atob, structuredClone, WebCrypto rejections, propagateException, throwDataCloneError, DeferredPromise::reject), so a latent unchecked-exception path or a wrong putDirect would surface widely. The exceptionFromString() semantic change also affects any thrown non-Error object with a line property. The PR description enumerates the caller audit and the validated-exception-check run, and the tests are thorough, but the surface area and the interaction with three other open PRs (#39311, #32898, #35723) warrant a maintainer's eye.
Other factors
- The CI failure on e6faff5 (unchecked exception in
rejectWithCause) was addressed by a4fe2e6; the robobun status comment has not yet updated for the two later commits. - The comment-cop bot fired three times on earlier revisions; commits 0ec6735 and 03d96c6 shortened the flagged comments to one-liners, so those appear resolved.
- User-visible behavior changes: DOMException
.stackformat switches from JSC'smodule code@...to Bun'sName: message\n at ..., the properties become DontEnum, andconsole.logobject dumps now includeoriginalLine/originalColumn. These are documented in the description as intentional and covered by tests, but are the kind of visible-output change a maintainer should sign off on.
Problem
AbortSignal.abort().reason,AbortController#abort(),AbortSignal.timeout(),atob()'s InvalidCharacterError,structuredClone()'s DataCloneError, everything else that goes throughWebCore::createDOMException) carryline,column,sourceURLandstackvalues that point into Bun's transpiled output, not into the user's file. With 10 comment lines aboveconst r = AbortSignal.abort().reason;,r.lineis1andr.stackismodule code@/x/f.js:1:30; anew Error()on the same line reports line 11.at /x/f.js:1, with the code excerpt to match. This is the wrong-line half of DOMException (AbortError) prints raw error object and an incorrect stack frame #37419 (the property dump half is not touched here).createDOMException()(src/jsc/bindings/JSDOMExceptionHandling.cpp:187) attaches the properties with JSC'saddErrorInfo(), which stores the raw JSC position andInterpreter::stackTraceAsString(). Errors get theirs from Bun'scomputeErrorInfohook, which runs them through the source map.ErrorInstances (exceptionFromString,src/jsc/bindings/ZigException.cpp:751) readslinebut nevercolumn, so the printer's own source map lookup (which needs both) never matches, and when anoriginalLineis present it substitutes it forlineinstead of treating the position as already mapped.Fix
Bun::addErrorInfoWithSourceMap()(new,FormatStackTraceForJS.cpp) captures the frames the same way as before (JSC::getStackTrace, soError.stackTraceLimitis honored as before), formats them withBun::formatStackTrace(), and puts the mappedline/column/sourceURLand the Bun-formatstackon the object as DontEnum properties.formatStackTrace()also records the unmapped position as DontEnumoriginalLine/originalColumn, as it does for Errors.createDOMException()calls it with the DOMException's name and message, sostackisAbortError: The operation was aborted.followed by frames in the usual format.util.inspect()of these now printsDOMException [AbortError]: ...plus frames, as node does, instead of[@/x/f.js:1:30] { line: 1, ... }.exceptionFromString()readscolumnas well and marks the frameremappedwhenoriginalLineis present, which is whatfromErrorInstance()already does for Errors whose stack has been materialized. The printer then shows the recorded position and takes the excerpt from the source file.ErrorInstance::materializeErrorInfoIfNeededgives an Error in Bun (the same four properties, attributes and format, and theline/originalLineconvention thattest/js/bun/test/stack.test.tspins), and the printer reads both kinds of object the same way. Two details fall out of using the Error path: a DOMException created with no JS frames (AbortSignal.timeout()firing from the timer) gets the header line as itsstackinstead of""(node's behavior for a frameless error, and what error: give errors created with no JS frames a .stack property #38074 does for Errors), and withError.stackTraceLimitdeleted nothing is attached, as for an Error.Error.prepareStackTraceis not consulted, as before; DOMException: inherit from ErrorInstance for [[ErrorData]] and .stack #32898 is the change that would make these stacks lazy.JSDOMExceptionanErrorInstance) would delete this call together with the old one; until it lands this is the fix for the positions. One visible consequence: Bun'sconsole.logobject dump of these DOMExceptions, which prints non-enumerable own properties, now also listsoriginalLineandoriginalColumn; console, error printer: render instanceof-Error objects as errors, not object dumps #35723 and DOMException: inherit from ErrorInstance for [[ErrorData]] and .stack #32898 replace that dump with error-style output.SubtleCrypto.cpprejectWithCause()(ML-DSA / ML-KEM import failures and the oversized ML-DSA context rejection) calledmakeCause(), whose import variant declares aThrowScope, and thencreateDOMException()without checking for an exception. That was invisible whilecreateDOMException()declared no exception scopes of its own; formatting the stack now does, so the exception-check validator (the ASAN CI lane runs every test withBUN_JSC_validateExceptionChecks=1) abortedtest-webcrypto-export-import-ml-dsa.jsand-ml-kem.js. The lambda now checks aftermakeCause(); that was the only site the validated suite found.DeferredPromise::reject(),propagateException()andthrowDataCloneError(), which cover the other callers, already check or open a scope right before the call.test/js/node/domexception-node.test.js(new describe): positions and frames of six creation sites (abort reasons,atob,structuredClone, acrypto.subtle.importKey()rejection, one created inside a function) checked against the line numbers of the fixture source and against an Error created on the same line, enumerability, the ML-DSA rejection-with-cause path compared against an Error created on the same line, the header-only case, and the unhandled rejection and uncaught exception printer output (frame line and excerpt). The five new tests fail on the unfixed binary (line 16 instead of 21, JSC-formatstack, printerat <dir>/uncaught.js:1) and pass with the fix, also underBUN_JSC_validateExceptionChecks=1(where the rejection-with-cause test aborts without theSubtleCrypto.cppchange).test/js/web/abort/,structured-clone.test.ts,stack.test.ts(pins the user-object fallback,at http://example.com/test.js:42, unchanged),capture-stack-trace.test.js,inspect-error.test.js(its two minified-file failures are the pre-existing debug-onlyat requireframe noted in error printer: remap frames when the original source is unavailable, and not twice after error.stack #38296),timers.promises,web-globals,globals,reportError, the node:stream: ensure stack traces are good #23022 regression test, node'stest-domexception-cause,test-global-domexceptionandtest-structuredClone-domexception, and the abort-related fetch tests.controller.abort()in a debug build: 551us/op before, 511 to 526us/op after; theabortsignal-leak-fixturetimings are unchanged.Background
createDOMException()is where a WebCoreExceptionCodebecomes a JS value. For the codes that are DOMExceptions it creates aJSDOMException, which is a plain DOM wrapper object and not aJSC::ErrorInstance, so none of the Error machinery below applies to it and its error properties have to be put on it explicitly.onComputeErrorInfohook (FormatStackTraceForJS.cpp); whenstack,lineorcolumnis first read,formatStackTrace()runs the frames through the source maps (Bun__remapStackFramePositions), the mapped position becomesline/column, and the unmapped one is kept as DontEnumoriginalLine/originalColumn.console.error(err)) converts the thrown value into aZigExceptioninZigException.cpp, thenremap_zig_exception(src/jsc/VirtualMachine.rs) maps each frame through the source maps and fetches the source for the excerpt. A frame'sremappedflag means its position is already a source position, so the mapping step is skipped for it.fromErrorInstance()handlesErrorInstances;exceptionFromString()is the fallback for every other thrown object, DOMExceptions included, and builds a single frame from the object'ssourceURL/lineproperties.Repro
{ for i in $(seq 10); do echo "// c$i"; done echo 'const r = AbortSignal.abort().reason;' echo 'console.log(r.line, JSON.stringify(r.stack));' echo 'Promise.reject(r);'; } > f.js && bun f.jsBefore (1.4.0):
After:
Column 29 comes from the same source map lookup Error frames go through (the nearest mapping at or before the JSC position).
Refs #37419