Skip to content

DOMException: inherit from ErrorInstance for [[ErrorData]] and .stack - #32898

Open
robobun wants to merge 12 commits into
mainfrom
farm/c00f6fd2/domexception-error-instance
Open

DOMException: inherit from ErrorInstance for [[ErrorData]] and .stack#32898
robobun wants to merge 12 commits into
mainfrom
farm/c00f6fd2/domexception-error-instance

Conversation

@robobun

@robobun robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

What

DOMException now derives from JSC::ErrorInstance, so it carries the [[ErrorData]] internal slot and a captured stack trace like native Error types. WebIDL §3.14.1 specifies this, and Node.js, Chrome and Firefox all do it.

Repro

const d = new DOMException("boom", "AbortError");
console.log(Error.isError(d));                        // was false, now true
console.log(typeof d.stack);                          // was undefined, now string
console.log(typeof AbortSignal.abort().reason.stack); // was undefined, now string
console.log(typeof structuredClone(d).stack);         // was undefined, now string
console.log(d);  // was a 25-line legacy-constant object dump, now formats as an error

Bun was also internally inconsistent: AbortController#abort() produced a reason with a .stack (because createDOMException called addErrorInfo afterwards), while AbortSignal.abort() and user-constructed DOMExceptions had none.

Cause

JSDOMException was a plain JSDOMWrapper<DOMException> with JSType::ObjectType. Error.isError checks type() == ErrorInstanceType, and nothing on the wrapper-creation path captured a stack.

Fix

JSDOMException now inherits from JSC::ErrorInstance while keeping its Ref<DOMException> wrapped impl and the existing prototype (accessor-based name/message/code, legacy constants):

  • createStructure uses ErrorInstanceType.
  • finishCreation calls ErrorInstance::finishCreation with a null message/cause so a stack trace is captured but name/message stay as prototype accessors backed by the wrapped DOMException.
  • visitChildren keeps the captured stack frames' callee/codeBlock alive. ErrorInstance normally relies on Heap's finalizeUnconditionally sweep over errorInstanceSpace, which our DOM subspace is not part of, so without this a lazy .stack read after GC would touch freed CodeBlocks.
  • New IsoHeapCellType in JSHeapData so the larger cell destructs its Ref<DOMException> and ErrorInstance state correctly.
  • When the captured trace is empty (a native entry with no JS frames, e.g. AbortSignal.timeout() firing from a native timer), ErrorInstance never materializes a stack, so finishCreation eagerly puts the name: message header. A DOMException .stack is now always a string. This improves No stack trace on AbortSignal.timeout error #25182 and Random TimeoutError is sometimes thrown without a stack trace #21900 (no longer undefined / "") but does not close them: Node shows real async frames there and Bun's native timer has none to capture.

Callers that branched on ErrorInstance before JSDOMException now check the DOMException case first: the .stack header in FormatStackTraceForJS, the except.name in ZigException, and the terminal-dump path in SerializedScriptValue, so structuredClone keeps emitting DOMExceptionTag instead of falling through to ErrorInstanceTag. The redundant addErrorInfo in createDOMException is removed now that the wrapper captures a Bun-formatted stack on construction; internally-thrown DOMExceptions now have the same stack format as regular errors instead of JSC's native foo@file format.

Latent exception-check bug this exposed

The BUN_JSC_validateExceptionChecks CI lane caught an unchecked exception in getNonObservable (ZigException.cpp):

This scope can throw a JS exception: getNonIndexPropertySlot @ JSObjectInlines.h:251
But the exception was unchecked as of this scope: get @ JSDOMAttribute.h:83

getNonObservable called a throwable getNonIndexPropertySlot with no exception check, and its slot.isAccessor() guard only filters JS getter/setter pairs, not native custom accessors, so slot.getValue() would invoke them. That was unreachable while fromErrorInstance only saw plain ErrorInstances, whose prototypes have no custom accessors. With JSDOMException now an ErrorInstance, the lookup for code lands on DOMException.prototype.code and invokes the getter. Fixed by adding a ThrowScope with RETURN_IF_EXCEPTION (the caller already clears after each call) and restricting the helper to plain data properties, which is what its name promises. This also stops the DOM legacy code (20) from being misread as a node system_code.

structuredClone round-trips the stack (wire format version 15)

test/js/node/test/parallel/test-structuredClone-domexception.js asserts clone.stack === e.stack. That passed vacuously before (both undefined); once DOMExceptions had stacks, the clone captured a fresh one at the structuredClone call site. Node serializes the stack, so DOMExceptionTag now carries it: the serializer reads it through [[Get]] (a throwing prepareStackTrace propagates out of the clone, as in the ErrorInstanceTag branch) and the deserializer installs it on the new wrapper, drops the frames the wrapper captured at the clone site (so GC does not keep rooting them and .line cannot materialize the clone site's position over the copy), and pins the stack property. CurrentVersion is bumped to 15 and the read is gated on it; older payloads still deserialize.

Header stack is put after the subclass structure swap

The header-only .stack fallback (empty trace, e.g. Error.stackTraceLimit = 0 or a native timer firing with no JS frames) was put from finishCreation. JSDOMException has no inline slots, so that allocated a butterfly, and setSubclassStructureIfNeeded then swapped class Sub extends DOMException instances onto a capacity-0 structure, tripping the setStructure butterfly assertion on debug builds. finishCreation now puts nothing; the toJSNewlyCreated free function (every internal creation path) puts the header after createWrapper, and the constructor builds the wrapper directly so the swap happens before the header and cause are put.

Verification

test/js/node/domexception-node.test.js adds coverage for Error.isError, .stack capture (including the AbortSignal.abort(), AbortSignal.timeout(), and structuredClone paths), stack equality through two structuredClones, the subclass + stackTraceLimit = 0 case, prototype-accessor invariants, util.inspect formatting, and a GC stress loop over lazy stack materialization. test-structuredClone-domexception.js, structured-clone.test.ts (including its version 13 payload and cross-process cases) and bun-jsc.test.ts pass. The existing .failing test for .stack is un-failed. fetch.test.ts and abort.test.ts pass under BUN_JSC_validateExceptionChecks=1.

Fixes #15821
Fixes #17877
Fixes #37419
Refs #25182
Refs #21900


[review] gate passed · iteration 1 · 10 files touched

fails on main (without fix)
ASAN without fix: 14 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/domexception-node.test.js
bun test v1.4.0 (d8661f2c1)

test/js/node/domexception-node.test.js:
(pass) DOMException in Node.js environment > exists globally [5.14ms]
(pass) DOMException in Node.js environment > creates instance with message and name [2.99ms]
(pass) DOMException in Node.js environment > uses default name when only message is provided [1.97ms]
(pass) DOMException in Node.js environment > creates instance with options object [2.48ms]
(pass) DOMException in Node.js environment > has standard error constants [12.29ms]
59 |   });
60 | 
61 |   it("inherits prototype properties from Error", () => {
62 |     const error = new DOMException("Test error");
63 |     expect(error.toString()).toBe("Error: Test error");
64 |     expect(error.stack).toBeDefined();
                             ^
error: expect(received).toBeDefined()

Received: undefined

      at <anonymous> (/workspace/bun/test/js/node/domexception-node.test.js:64:25)
(fail) DOMException in Node.js environment > inherits prototype properties from Error [6.2
... (truncated)

release without fix: 14 FAILED
bun test v1.4.0-canary.1 (9008ae7ab)

test/js/node/domexception-node.test.js:
(pass) DOMException in Node.js environment > exists globally [0.05ms]
(pass) DOMException in Node.js environment > creates instance with message and name [0.04ms]
(pass) DOMException in Node.js environment > uses default name when only message is provided [0.02ms]
(pass) DOMException in Node.js environment > creates instance with options object [0.03ms]
(pass) DOMException in Node.js environment > has standard error constants [0.09ms]
59 |   });
60 | 
61 |   it("inherits prototype properties from Error", () => {
62 |     const error = new DOMException("Test error");
63 |     expect(error.toString()).toBe("Error: Test error");
64 |     expect(error.stack).toBeDefined();
                             ^
error: expect(received).toBeDefined()

Received: undefined

      at <anonymous> (/workspace/bun/test/js/node/domexception-node.test.js:64:25)
(fail) DOMException in Node.js environment > inherits prototype properties from Error [0.22ms]
64 |     expect(error.stack).toBeDefined();
65 |   });
66 | 
67 |   it("has [[ErrorData]] internal slot", () => {
68 |     const error = new DOMException("boom
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/domexception-node.test.js
bun test v1.4.0 (d8661f2c1)

test/js/node/domexception-node.test.js:
(pass) DOMException in Node.js environment > exists globally [5.89ms]
(pass) DOMException in Node.js environment > creates instance with message and name [5.23ms]
(pass) DOMException in Node.js environment > uses default name when only message is provided [3.14ms]
(pass) DOMException in Node.js environment > creates instance with options object [5.01ms]
(pass) DOMException in Node.js environment > has standard error constants [15.38ms]
(pass) DOMException in Node.js environment > inherits prototype properties from Error [5.43ms]
(pass) DOMException in Node.js environment > has [[ErrorData]] internal slot [4.84ms]
(pass) DOMException in Node.js environment > captures a stack trace [7.83ms]
(pass) DOMException in Node.js environment > keeps name/message/code as prototype accessors [8.57ms]
(pass) DOMException in Node.js environment > AbortSignal.abort().reason is a DOMException with a stack [7.24ms]
(pass) DOMException in Node.js e
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 740ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/127] gen generated_host_exports.rs
generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited
[2/127] gen cpp.rs (cppbind)
[3/127] gen JS modules (bundle-modules)
Preprocess modules (9568ms)
Bundle modules (69ms)
Postprocesss modules (26ms)
Bundle Functions (637ms)
Generate Code (31ms)

[10.35s] Bundled "src/js" for production
  2610 kb
  197 internal modules
  13 native modules
  91 internal functions across 17 files
[3/126] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_parsers v0.0.0 (/workspace/bun/src/parsers)
�[1m�[92m   Compiling�[0m bun_http v0.0.0 (/workspace/bun/src/http)
�[1m�[92m   Compiling�[0m bun_resolver v0.0.0 (/workspace/bun/src/resolver)
�[1m�[92m   Compiling�[0m bun_sourcemap v0.0.0 (/workspace/bun/src/sourcemap)
�[1m�[92m   Compiling�[0m bun_ini v0.0.0 (/workspace/bun/src/ini)
�[1m�[92
... (truncated)
diff hotspot
src/jsc/bindings/BunClientData.cpp                 |   2 +
 src/jsc/bindings/BunClientData.h                   |   1 +
 src/jsc/bindings/FormatStackTraceForJS.cpp         |  25 ++-
 src/jsc/bindings/JSDOMExceptionHandling.cpp        |   5 +-
 src/jsc/bindings/JSDOMWrapperCache.h               |  12 +-
 src/jsc/bindings/ZigException.cpp                  |  28 +--
 src/jsc/bindings/webcore/JSDOMException.cpp        | 104 ++++++++++--
 src/jsc/bindings/webcore/JSDOMException.h          |  35 +++-
 src/jsc/bindings/webcore/SerializedScriptValue.cpp |  46 ++---
 test/js/node/domexception-node.test.js             | 188 ++++++++++++++++++++-
 10 files changed, 376 insertions(+), 70 deletions(-)

gate history · 3 passed · 0 rejected · iteration 1

evidence per changed file
file                                                reads  edits  tests
src/jsc/bindings/BunClientData.cpp                      1      2     18
src/jsc/bindings/BunClientData.h                        1      1     18
src/jsc/bindings/FormatStackTraceForJS.cpp              4      8     18
src/jsc/bindings/JSDOMExceptionHandling.cpp             5      7     18
src/jsc/bindings/JSDOMWrapperCache.h                    1      2     18
src/jsc/bindings/ZigException.cpp                       4      6     18
src/jsc/bindings/webcore/JSDOMException.cpp            10     18     18
src/jsc/bindings/webcore/JSDOMException.h               5      5     18
src/jsc/bindings/webcore/SerializedScriptValue.cpp      5      7     18
test/js/node/domexception-node.test.js                 13     18     18

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

JSDOMException now inherits from JSC::ErrorInstance, preserves stack data, supports custom heap-cell handling, and integrates with error formatting and structured cloning. Tests cover Error branding, stack output, abort reasons, cloning, inspection, uncaught reporting, and garbage collection.

Changes

JSDOMException ErrorInstance Refactor

Layer / File(s) Summary
JSDOMException class and heap integration
src/jsc/bindings/webcore/JSDOMException.h, src/jsc/bindings/webcore/JSDOMException.cpp, src/jsc/bindings/BunClientData.*
JSDOMException now inherits from JSC::ErrorInstance, stores the wrapped DOMException, materializes stack headers, visits stack frames, and uses custom heap-cell storage.
Error and stack trace handling
src/jsc/bindings/FormatStackTraceForJS.cpp, src/jsc/bindings/JSDOMExceptionHandling.cpp, src/jsc/bindings/ZigException.cpp
Error formatting uses DOMException display values. Stack updates preserve live frames. DOMException creation no longer adds separate error metadata. Property lookup handles thrown lookups and excludes custom accessors.
Serialization, wrapper integration, and behavior coverage
src/jsc/bindings/webcore/SerializedScriptValue.cpp, src/jsc/bindings/JSDOMWrapperCache.h, test/js/node/domexception-node.test.js
Structured cloning preserves nullable DOMException stacks with backward compatibility. Wrapper cache overloads use JSC::JSObject*. Tests cover Error semantics, stacks, abort reasons, cloning, inspection, uncaught reporting, and GC retention.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address Error.isError support, DOMException stack traces, error formatting, and correct abort stack frames for issues [#15821], [#17877], and [#37419].
Out of Scope Changes check ✅ Passed The implementation, serialization updates, lifetime handling, exception checks, and tests directly support the linked issue objectives.
Title check ✅ Passed The title clearly and concisely identifies the main change: making DOMException inherit from ErrorInstance for ErrorData and stack support.
Description check ✅ Passed The description explains the change, cause, implementation, verification, tests, and linked issues, despite using headings different from the template.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:35 AM PT - Aug 11th, 2026

@autofix-ci[bot], your commit d8661f2c152a959069d5c6a35ad247f199fd493e passed in Build #92035! 🎉


🧪   To try this PR locally:

bunx bun-pr 32898

That installs a local version of the PR into your bun-32898 executable, so you can run:

bun-32898 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 4 issues this PR may fix:

  1. Error.isError and DOMException #15821 - Error.isError(new DOMException()) returns false; this PR gives DOMException the [[ErrorData]] internal slot by inheriting from ErrorInstance
  2. No stack trace when calling throwIfAborted() on an AbortSignal #17877 - throwIfAborted() produces a DOMException with no stack trace; this PR captures .stack on DOMException construction
  3. No stack trace on AbortSignal.timeout error #25182 - AbortSignal.timeout() error has no stack trace; this PR ensures all DOMException creation paths capture stack traces
  4. Random TimeoutError is sometimes thrown without a stack trace #21900 - TimeoutError (DOMException) sometimes thrown without a stack trace; same root cause fixed by inheriting from ErrorInstance

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #15821
Fixes #17877
Fixes #25182
Fixes #21900

🤖 Generated with Claude Code

Comment thread src/jsc/bindings/webcore/JSDOMException.cpp
Comment thread test/js/node/domexception-node.test.js Outdated
Comment thread src/jsc/bindings/JSDOMExceptionHandling.cpp Outdated
@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review in 5ab3d98.

  • retrieveErrorMessage now guards the empty-message case so it does not emit a trailing ": ".
  • On the visitChildren locking concern: every m_stackTrace mutation in ErrorInstance.cpp already takes the cell lock, but two Bun helpers bypassed the locked API through the raw stackTrace() pointer, and since visitChildren is now a genuine concurrent reader of m_stackTrace I routed both through setStackFrames. Details on the thread.
  • Added the Fixes lines for the four linked issues.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/jsc/bindings/webcore/JSDOMException.cpp`:
- Around line 251-254: The comment in JSDOMException’s stack-handling block is
too long; shorten the explanatory note around the ErrorInstance behavior in
JSDOMException.cpp to three lines max. Keep the essential point about
native-entry stack traces and the `name: message` header, but trim redundant
wording while preserving clarity near the stack formatting logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 621506c1-0816-417b-bb28-8783f550aa3c

📥 Commits

Reviewing files that changed from the base of the PR and between 8706328 and 5ab3d98.

📒 Files selected for processing (10)
  • src/jsc/bindings/BunClientData.cpp
  • src/jsc/bindings/BunClientData.h
  • src/jsc/bindings/FormatStackTraceForJS.cpp
  • src/jsc/bindings/JSDOMExceptionHandling.cpp
  • src/jsc/bindings/JSDOMWrapperCache.h
  • src/jsc/bindings/ZigException.cpp
  • src/jsc/bindings/webcore/JSDOMException.cpp
  • src/jsc/bindings/webcore/JSDOMException.h
  • src/jsc/bindings/webcore/SerializedScriptValue.cpp
  • test/js/node/domexception-node.test.js

Comment thread src/jsc/bindings/webcore/JSDOMException.cpp Outdated
Comment thread test/js/node/domexception-node.test.js
@robobun

robobun commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator Author

3c600b9 fixes the test/js/node/domexception-node.test.js timeout that hit all three Windows test lanes in builds 65757 and 65785.

Root cause was the shape of one test, not the native change. The AbortSignal.timeout() test attached an abort listener and then awaited only that event; on Windows that wedges the process (the job logs show exactly 10 reporter dots and then 180 seconds of silence, 4/4 attempts, so bun test's own per-test timeout never fired either). On the same Windows binary in the same build, test/js/web/abort/abort.test.ts runs the identical native path (AbortSignal.timeout(0) through the zig timer into createDOMException into the new JSDOMException::finishCreation) and passes in 318ms; the only difference is it reads signal.reason after a ref'd Bun.sleep instead of awaiting the abort event.

The test is now split in two:

  • The AbortSignal.timeout() coverage uses the shape proven on Windows: a bounded ref'd-sleep poll of signal.aborted, no abort listener, so a missed abort fails the assertion instead of hanging.
  • The property it was named for, a header-only .stack when no JS frames are captured, is covered directly and synchronously: Error.stackTraceLimit = undefined gives a null trace and = 0 an empty one, so a subprocess test hits both arms plus the empty-message header with no timers involved.

19/19 pass against the debug build; 12 fail against stock bun, including both rewritten tests (in 11ms, no hang).

Comment thread test/js/node/domexception-node.test.js
@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Verified this also fixes #37419 (uncaught DOMException printed as a raw property dump of the legacy constants, with a single unremapped stack line). On this branch the issue's repro now prints:

12 |   controller.abort();
                  ^
AbortError: The operation was aborted.
      at main (/tmp/repro37419/exact.ts:12:14)

which matches Node's position (12:14) and drops the object dump. Added Fixes #37419 to the description. The last CI run (build 65887) failed with every build job expired, which looks like infra rather than the diff; a rebase and re-push should get a clean run.

WebIDL requires DOMException to carry the [[ErrorData]] internal slot
like native Error types. Bun's JSDOMException was a plain JSDOMWrapper,
so Error.isError(new DOMException()) returned false, .stack was
undefined for user-constructed DOMExceptions and for
AbortSignal.abort().reason / structuredClone results, and console.log
printed a 25-line legacy-constant dump instead of an error.

JSDOMException now derives from JSC::ErrorInstance while keeping its
Ref<DOMException> wrapped impl:
- createStructure uses ErrorInstanceType so Error.isError / util.types
  .isNativeError recognize it and error formatting paths trigger.
- finishCreation calls ErrorInstance::finishCreation with a null
  message/cause so a stack trace is captured but name/message/code
  stay as prototype accessors backed by the wrapped DOMException.
- visitChildren keeps the captured stack frames' callee/codeBlock
  alive; ErrorInstance normally relies on Heap's errorInstanceSpace
  finalizeUnconditionally sweep which our DOM subspace does not get.
- new IsoHeapCellType in JSHeapData so the larger cell destructs its
  Ref<DOMException> and ErrorInstance state.

Callers that previously branched on ErrorInstance before JSDOMException
now check the DOMException case first (ZigException name, stack header
in FormatStackTraceForJS, SerializedScriptValue terminal dump). The
redundant addErrorInfo in createDOMException is removed since the
wrapper now captures a Bun-formatted stack on construction.
…ck fallback

getNonObservable in ZigException.cpp did a throwable getNonIndexPropertySlot
with no exception check and then called slot.getValue(), which invokes native
custom getters (slot.isAccessor() only filters JS getter/setter pairs). This
was unreachable while fromErrorInstance only saw plain ErrorInstances; now
that JSDOMException is an ErrorInstance, the lookup for 'code' lands on the
DOMException prototype's custom accessor and the BUN_JSC_validateExceptionChecks
lane aborts at JSDOMAttribute.h:83. Add a ThrowScope with RETURN_IF_EXCEPTION
(the caller already clears after each call) and restrict the helper to plain
data properties, which is what its name promises.

Also: ErrorInstance never materializes a .stack from an empty stack trace,
which happens when a DOMException is created from a native entry with no JS
frames (AbortSignal.timeout fires from a Zig timer). Put the 'name: message'
header eagerly in that case so DOMException.stack is always a string.

Move the node:util import in the test to module scope.
…tter through the locked API

JSDOMException::visitChildren iterates m_stackTrace from the GC marker under
the cell lock. Every m_stackTrace mutation inside ErrorInstance.cpp already
pairs with that lock (setStackFrames, captureStackTrace, both finishCreation
overloads, computeErrorInfo, materializeErrorInfoIfNeeded), but two Bun
helpers bypassed the locked API through the raw stackTrace() pointer, which
a concurrent reader turns into a use-after-free:

- errorConstructorFuncAppendStackTrace reallocated the destination's vector
  and cleared the source's in place. Build the combined vector locally and
  install it with setStackFrames on both.
- errorInstanceLazyStackCustomGetter move-constructed the live buffer out of
  m_stackTrace before the locked clear. Copy the frames instead; the
  setStackFrames(vm, {}) that follows still releases the originals.

Also guard the JSDOMException branch of retrieveErrorMessage against an
empty message so it does not emit a trailing ': ', matching the header
fallback in JSDOMException::finishCreation.

Tests: Error.captureStackTrace on a DOMException (drives the lazy stack
getter end to end) and the empty-message stack header.
test/js/node/domexception-node.test.js hung for 180s on every Windows
test lane. The AbortSignal.timeout test added the abort listener and
then awaited only the abort event; on Windows that wedges the event
loop (bun:test's own per-test timeout timer never fires either), so
the file never progressed past it.

The same native path (AbortSignal.timeout -> zig timer ->
createDOMException -> JSDOMException::finishCreation with an empty
frame list) passes on the same Windows binary in
test/js/web/abort/abort.test.ts, which reads signal.reason after a
ref'd Bun.sleep instead of awaiting the abort event. Use that shape
here: drive the loop with a bounded ref'd-sleep poll of signal.aborted
and never attach an abort listener, so a missed abort fails loudly
instead of hanging.

The property that test was actually named for (a header-only .stack
when no JS frames are captured) does not need the native timer at all:
Error.stackTraceLimit = undefined yields a null trace and
Error.stackTraceLimit = 0 an empty one, both synchronously. Cover both
arms, plus the empty-message header, in a subprocess so the global
stackTraceLimit mutation is isolated.

Also shorten an over-long comment in JSDOMException::finishCreation.
@robobun
robobun force-pushed the farm/c00f6fd2/domexception-error-instance branch from 3c600b9 to 0b390eb Compare August 11, 2026 05:12
Comment thread src/jsc/bindings/FormatStackTraceForJS.cpp Outdated
Comment thread src/jsc/bindings/FormatStackTraceForJS.cpp Outdated
Comment thread src/jsc/bindings/ZigException.cpp Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.cpp Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.cpp Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.cpp Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.h Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.cpp Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.cpp Outdated
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (1058 commits, including the WebKit bump in #34373 and the AbortSignal.abort() change in #36574). Now at 3d7b807.

Three conflicts, all mechanical: upstream had removed a neighboring subspace in BunClientData.cpp, deleted the dead retrieveErrorMessageWithoutName in JSDOMExceptionHandling.cpp (so this PR no longer touches it), and stripped dead code around the later JSDOMException branch in SerializedScriptValue.cpp (this PR still removes that branch in favor of the earlier one). Nothing else on main touched JSDOMException; it is still a plain JSDOMWrapper there, so this is still the fix.

The ErrorInstance constructor and finishCreation overload this subclasses are unchanged by the WebKit bump. #36574 makes AbortSignal.abort() build its default reason through the same createDOMException path as AbortController, so both now get their stack from JSDOMException::finishCreation.

Re-verified on the rebased build: domexception-node.test.js 19/19 (12 still fail on the released bun), web/abort/, structured-clone.test.ts and capture-stack-trace.test.js 293/293, and the DOMException, abort and fetch AbortError tests are clean under BUN_JSC_validateExceptionChecks=1. The two commits after the rebase only shorten comments.

For the record, the earlier red on build 65757's successor (65887) was a fleet outage: all 33 build jobs expired unstarted, as did a dozen other PRs' builds in the same window. No code from this PR ran in it.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/node/domexception-node.test.js`:
- Around line 121-142: Update the combined assertion in the “gets a header-only
stack when no frames are captured” test to include stderr: "" alongside stdout
and exitCode, preserving the existing concurrent stderr drain and expected
stdout output.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8857fb98-cf41-48fb-9730-5816b096dc22

📥 Commits

Reviewing files that changed from the base of the PR and between 9fcdea8 and 3d7b807.

📒 Files selected for processing (10)
  • src/jsc/bindings/BunClientData.cpp
  • src/jsc/bindings/BunClientData.h
  • src/jsc/bindings/FormatStackTraceForJS.cpp
  • src/jsc/bindings/JSDOMExceptionHandling.cpp
  • src/jsc/bindings/JSDOMWrapperCache.h
  • src/jsc/bindings/ZigException.cpp
  • src/jsc/bindings/webcore/JSDOMException.cpp
  • src/jsc/bindings/webcore/JSDOMException.h
  • src/jsc/bindings/webcore/SerializedScriptValue.cpp
  • test/js/node/domexception-node.test.js

Comment thread test/js/node/domexception-node.test.js
Comment thread src/jsc/bindings/webcore/SerializedScriptValue.cpp Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.cpp Outdated
Comment thread test/js/node/domexception-node.test.js
…ack after the subclass structure swap

test/js/node/test/parallel/test-structuredClone-domexception.js asserts
clone.stack === e.stack. That passed vacuously before this branch, when both
were undefined; now the clone captured a fresh trace at the structuredClone
call site. Node serializes the stack, so DOMExceptionTag now carries it too:
the serializer reads .stack through [[Get]] (a throwing prepareStackTrace
propagates out of the clone, matching the ErrorInstance branch) and the
deserializer installs it on the new wrapper and marks the stack property
materialized so the wrapper's own trace never overwrites it. This is a wire
format change, so CurrentVersion goes to 15 and the read is gated on it;
FirstVersionWithPooledTerminals is unaffected.

The header-only .stack fallback used to be put from finishCreation. For
`class Sub extends DOMException` that first put allocates a butterfly (a
JSNonFinalObject has no inline slots), and setSubclassStructureIfNeeded then
swaps to a capacity-0 structure, tripping the setStructure butterfly
assertion in debug builds and leaving the release object inconsistent.
finishCreation now puts nothing; toJSNewlyCreated (every internal creation
path) puts the header after createWrapper, and the constructor builds the
wrapper directly so it can put the header and cause only after the swap.

Tests: the clone test now double round-trips and asserts stack equality, and
the stackTraceLimit subprocess test adds the subclass case.
Comment thread src/jsc/bindings/FormatStackTraceForJS.cpp
The .stack header, the uncaught exception report and retrieveErrorMessage
read name and message straight off the wrapped impl, so an own data
property defined on the instance was ignored, where a plain Error (via
sanitizedNameString) and Node honor it. Reading through sanitizedNameString
is not an option because DOMException.prototype's attributes are custom
accessors, which it skips before falling back to "Error"; that is why these
sites special-case DOMException at all.

Add displayName/displayMessage on JSDOMException: an own string property
wins, otherwise the impl's value. getDirect never runs JS, and a rope falls
back rather than resolving, since the stack header can be computed inside a
finalizer. All three sites and the header-stack fallback go through it.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/node/domexception-node.test.js`:
- Around line 199-214: Update the DOMException subprocess test around Bun.spawn
to pipe stdout, read it concurrently with proc.stderr.text() and proc.exited,
and include the drained stdout in the combined assertion while preserving the
existing stderr content checks and exitCode assertion.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: eaaf9e0a-f065-45e8-bec3-95659086a2b9

📥 Commits

Reviewing files that changed from the base of the PR and between 3d7b807 and ec536d7.

📒 Files selected for processing (7)
  • src/jsc/bindings/FormatStackTraceForJS.cpp
  • src/jsc/bindings/JSDOMExceptionHandling.cpp
  • src/jsc/bindings/ZigException.cpp
  • src/jsc/bindings/webcore/JSDOMException.cpp
  • src/jsc/bindings/webcore/JSDOMException.h
  • src/jsc/bindings/webcore/SerializedScriptValue.cpp
  • test/js/node/domexception-node.test.js

Comment thread test/js/node/domexception-node.test.js Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.cpp Outdated
Comment thread src/jsc/bindings/webcore/JSDOMException.cpp
robobun and others added 2 commits August 11, 2026 07:14
…ames once a stack string is installed

The header-only .stack and retrieveErrorMessage guarded an empty message
but not an empty name, so new DOMException("boom", "") produced ": boom"
where the framed path (formatStackTrace) and Error.prototype.toString give
"boom". Both now go through displayHeader, which applies the same rule as
formatStackTrace.

setStackString left the frames captured at the deserialize site in
m_stackTrace. Nothing would ever format them (the stack property is pinned),
but visitChildren kept rooting their callees and CodeBlocks for as long as
the clone lived, and reading .line on a clone would have materialized the
clone site's position over the copy. Clear the frames through setStackFrames,
which takes the cell lock visitChildren reads under.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and the bug-hunting pass found no new issues; all earlier inline findings have been addressed in e6f7940. Because this swaps JSDOMException's base class to ErrorInstance with a custom IsoSubspace, hand-rolled visitChildren/cellLock coordination for stack-frame rooting, and a SerializedScriptValue wire-format bump, a human look is still worthwhile.

What was reviewed:

  • GC safety: visitChildrenImpl roots frames under cellLock(); mutator-side writes go through setStackFrames which takes the same lock — the move-to-copy in errorInstanceLazyStackCustomGetter and vector rebuild in errorConstructorFuncAppendStackTrace were checked for that race.
  • Ordering: JSDOMException branches precede ErrorInstance at all three sibling sites (SerializedScriptValue, ZigException, retrieveErrorMessage) so structuredClone still emits DOMExceptionTag.
  • readDOMException's version-gated readNullableString and setStackString's frame drop after deserialize.
  • The BunClientData.h member declaration order vs the .cpp initializer-list order differ (Bake/NapiHandleScope swapped), but that predates this PR.
Extended reasoning...

Overview

This PR changes JSDOMException from a plain JSDOMWrapper<DOMException> to a JSC::ErrorInstance subclass so DOMExceptions carry [[ErrorData]] and a captured .stack, matching WebIDL §3.14.1 and Node/Chrome/Firefox. It touches 10 files: the class itself (JSDOMException.{h,cpp}), a new IsoHeapCellType in BunClientData, three call sites that branch on ErrorInstance vs JSDOMException (FormatStackTraceForJS, ZigException, JSDOMExceptionHandling), the structured-clone wire format (SerializedScriptValue, version 14→15), a signature widening in JSDOMWrapperCache.h so createWrapper compiles for a non-JSDOMObject wrapper, and ~180 lines of new tests.

Security risks

None identified. No untrusted-input parsing beyond the existing structured-clone reader, whose only new read is a version-gated readNullableString. No auth/crypto/permissions.

Level of scrutiny

High. This is GC-sensitive C++ in the JSC bindings: a base-class swap on a widely-thrown error type, a custom heap subspace that is not part of vm.errorInstanceSpace() (so ErrorInstance::finalizeUnconditionally never sweeps it — the PR compensates with a visitChildrenImpl that roots StackFrame callees/CodeBlocks under cellLock()), and mutator-side stack-frame mutations rewritten to go through setStackFrames for lock safety. The structured-clone wire-format bump is cross-process/worker-visible. Per the repo's review guidance, native memory safety and GC rooting are the most-blocked category and warrant a maintainer's eyes.

Other factors

The PR has been through five rounds of bot review; every inline finding (subclass-structure butterfly assertion, own-property name/message in the stack header, empty-name header joining, deserialize-site frame retention, subprocess test assertions) was confirmed and fixed with tests. The mechgate evidence shows the test file failing on main (ASAN and release) and passing on the PR. Test coverage is thorough — GC stress loop, subclass + stackTraceLimit=0, structuredClone double round-trip with stack equality, AbortSignal.timeout() native-timer path, uncaught-report subprocess. No outstanding unresolved review threads. The complexity of the GC/subspace interaction is the sole reason for deferring rather than approving.

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Ready for review. CI is green on the current head (d8661f2, build 92035, 190/190), and every review thread is resolved.

For whoever picks this up, the parts that deserve the closest look are the ones the description calls out: the base class swap to ErrorInstance with its own IsoSubspace (so visitChildren roots the captured frames itself, under the cell lock that setStackFrames takes), and the structured clone wire format bump to 15 so a DOMException's stack survives structuredClone and postMessage. Everything else is call sites learning that a DOMException is now an ErrorInstance.

Verification is in the description; the short version is test/js/node/domexception-node.test.js (21 tests, 12 of which fail on the released bun), the Node test-structuredClone-domexception.js, the structured clone and abort suites, and a clean run under BUN_JSC_validateExceptionChecks=1.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this revision and the bug hunting pass found no issues; every prior inline finding (one-sided cellLock, subclass butterfly assertion, clone stack round-trip, empty-name header, retained clone-site frames) has been addressed with a targeted fix and a test. Because this re-parents JSDOMException onto ErrorInstance with a custom IsoSubspace, hand-written visitChildren, and a SerializedScriptValue wire-format bump, a maintainer sign-off is still worth having.

What was reviewed

  • visitChildren/cellLock pairing: mutator sites now go through setStackFrames (append, clear, lazy getter copies instead of moving) so the concurrent visitor's lock is not one-sided.
  • setStackString drops captured frames before pinning, so clones don't root deserialize-site CodeBlocks and .line can't materialize the clone site over the copy.
  • DOMExceptionTag v15: read is version-gated, dumpIfTerminal checks JSDOMException before ErrorInstance so the tag is unchanged, and a throwing prepareStackTrace propagates like the Error branch.
  • getNonObservable's isValue() guard keeps DOMException.prototype.code from being invoked and misread as a Node system_code.
Extended reasoning...

Overview

The PR re-parents JSDOMException from JSDOMWrapper<DOMException> to JSC::ErrorInstance so DOMExceptions carry [[ErrorData]] and a captured stack (WebIDL §3.14.1, matching Node/Chrome/Firefox). Ten files touched: the class itself and its header, a new IsoHeapCellType in BunClientData, JSDOMWrapperCache.h widened to JSC::JSObject* so the non-JSDOMObject wrapper still flows through createWrapper/cacheWrapper, SerializedScriptValue bumped to version 15 to carry the stack string, and three call sites (FormatStackTraceForJS, ZigException, JSDOMExceptionHandling) that now branch on JSDOMException before the generic ErrorInstance path. ~180 lines of new tests cover Error.isError, stack capture from every creation path, structuredClone equality, subclassing under stackTraceLimit=0, own-property overrides, util.inspect, and a GC stress loop.

Security risks

None identified. No untrusted-input parsing was added; the serializer's new stack read goes through [[Get]] (which can run prepareStackTrace) with a RETURN_IF_EXCEPTION, mirroring the existing ErrorInstanceTag branch. The deserializer's version gate keeps older payloads valid. getNonObservable's tightening to slot.isValue() is a strict narrowing that stops native getters from being invoked where they previously could be — a hardening, not a widening.

Level of scrutiny

High. Changing a wrapper's base class to ErrorInstance moves it out of vm.errorInstanceSpace()'s finalizeUnconditionally sweep, which is why the PR adds a custom IsoHeapCellType and a hand-written visitChildren that roots StackFrame callees/CodeBlocks under the cellLock. That is exactly the class of change REVIEW.md flags as most-blocked (GC rooting, concurrent visitor races, cell destruction). The PR history shows several real defects found and fixed across iterations — a debug-build butterfly assertion on subclassing, a one-sided cellLock, clone-site frames retained past setStackString — which is evidence the surface is subtle enough to merit a human look even though the current revision reads correct.

Other factors

The design choices here — visitChildren rooting instead of joining errorInstanceSpace's finalize sweep, putting the header stack post-createWrapper at both entry points rather than in finishCreation, widening the wrapper-cache overload set to JSC::JSObject*, and the displayName/displayMessage/displayHeader helper trio — are all reasonable and well-commented, but they are architectural calls a maintainer should ratify. Test coverage is thorough and the PR's own gate confirms the file fails on main and passes on both debug-ASAN and release. No outstanding reviewer comments remain; all inline threads are resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant