DOMException: inherit from ErrorInstance for [[ErrorData]] and .stack - #32898
Open
robobun wants to merge 12 commits into
Open
DOMException: inherit from ErrorInstance for [[ErrorData]] and .stack#32898robobun wants to merge 12 commits into
robobun wants to merge 12 commits into
Claude / Claude Code Review
completed
Aug 11, 2026 in 29m 21s
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/bindings/webcore/SerializedScriptValue.cpp:1186-1189 |
structuredClone(DOMException) no longer preserves .stack — regresses Node parallel test |
| 🟡 Nit | src/jsc/bindings/webcore/JSDOMException.cpp:253-261 |
Header-only .stack fallback is discarded for DOMException subclasses |
| 🟡 Nit | test/js/node/domexception-node.test.js:137-141 |
Subprocess test drains stderr but omits it from the assertion |
Annotations
Check failure on line 1189 in src/jsc/bindings/webcore/SerializedScriptValue.cpp
claude / Claude Code Review
structuredClone(DOMException) no longer preserves .stack — regresses Node parallel test
Moving the `JSDOMException` check here keeps `DOMExceptionTag` in use, but `dumpDOMException`/`readDOMException` still only round-trip `message`+`name` — so the deserialized wrapper goes through the new `JSDOMException::finishCreation` and captures a *fresh* stack at the `structuredClone` call site. That regresses `test/js/node/test/parallel/test-structuredClone-domexception.js`, which asserts `strictEqual(clone.stack, e.stack)` (previously both were `undefined` so it passed vacuously); that fil
Check warning on line 261 in src/jsc/bindings/webcore/JSDOMException.cpp
claude / Claude Code Review
Header-only .stack fallback is discarded for DOMException subclasses
The header-only `.stack` fallback doesn't survive subclass construction: `finishCreation` runs `putDirect(stack, ...)` on the cached base structure, then `construct()` calls `setSubclassStructureIfNeeded`, which swaps in a fresh property-less structure derived from that same cached base — dropping the `+stack` transition. With `m_errorInfoMaterialized` already set, the lazy path won't repopulate it, so `class Foo extends DOMException {}; Error.stackTraceLimit = 0; new Foo('m','AbortError').stack
Check warning on line 141 in test/js/node/domexception-node.test.js
claude / Claude Code Review
Subprocess test drains stderr but omits it from the assertion
The test destructures `stderr` from the drained pipes but asserts only `{ stdout, exitCode }`, leaving `stderr` a dead variable. Per REVIEW.md's subprocess-test rule ("assert a combined `{ stdout, stderr, exitCode }` object"), add `stderr: ""` to the expected object so a stray warning on the child's stderr fails with a useful diff instead of passing silently.
Loading