DOMException: make line/column/sourceURL on internally created exceptions non-enumerable - #39311
DOMException: make line/column/sourceURL on internally created exceptions non-enumerable#39311robobun wants to merge 3 commits into
Conversation
…ions non-enumerable createDOMException() attached the creation site to the new JSDOMException with JSC::addErrorInfo(). JSDOMException is not an ErrorInstance, so that helper used plain putDirect() and line, column and sourceURL ended up as own enumerable properties, unlike user-constructed DOMExceptions, Bun's own Error objects (DontEnum) and Node (no such properties). Object.keys, JSON.stringify, spread and deep equality of AbortSignal reasons, DataCloneErrors and every other DOMException Bun creates exposed them, so two abort reasons created on different lines were not deep equal. Put the same properties with DontEnum instead. The values are unchanged: the uncaught error printer still reads them to point at the creation site. abort.test.ts no longer needs to strip these keys before comparing a signal's reason with a constructed DOMException.
|
Warning Review limit reached
Next review available in: 2 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 (3)
Comment |
|
Status: reproduced on Bun 1.4.0 and on main (83d65fa) with |
There was a problem hiding this comment.
LGTM — focused compat fix that only flips property enumerability, with thorough coverage.
What was reviewed:
addDOMExceptionErrorInfo()mirrors JSC'saddErrorInfopath (null/empty stackTrace branches,getLineColumnAndSource,stackTraceAsString) withDontEnumon all four puts; no user-JS re-entry, so no exception scope needed beyond what the replaced call had.- New
it.eachcovers constructed, cloned, and six internally-created DOMException paths; the spawned-fixture test provesline/sourceURLremain readable by the unhandled-rejection printer. abort.test.tsworkaround removal turns two existing tests into regression tests;WTF::moveand helper placement match local convention.
Extended reasoning...
Overview
The PR changes one call site in src/jsc/bindings/JSDOMExceptionHandling.cpp: the default arm of createDOMException() now calls a new file-local addDOMExceptionErrorInfo() instead of JSC::addErrorInfo(). The helper reproduces the upstream addErrorInfo(VM&, Vector<StackFrame>*, JSObject*) body — getStackTrace → null return / empty→stack:"" / otherwise getLineColumnAndSource + four putDirects — but passes DontEnum for line, column, and sourceURL (upstream only marks stack DontEnum on non-ErrorInstance objects). Two test files are updated: a new describe block in test/js/node/domexception-node.test.js and a one-line workaround removal in test/js/web/abort/abort.test.ts.
Security risks
None. The change alters property attributes on a freshly created DOM wrapper; no untrusted input, no auth/crypto, no memory ownership changes. putDirect on a brand-new JSDOMException cannot re-enter user JS, and the helper makes the same non-throwing JSC calls the replaced function made (verified per the description under BUN_JSC_validateExceptionChecks=1).
Level of scrutiny
Low-to-medium. This is a Node/WebIDL compat fix in a shared C++ path that every internally-created DOMException flows through, so I checked that the helper is a faithful structural copy of addErrorInfo (including the !stackTrace early return and the sourceURL.isEmpty() guard) and that nothing but enumerability changes. The values, the empty-trace fallback, and the stack string format are all identical. WTF::move matches the ~1500 existing uses in src/jsc/bindings.
Other factors
Test coverage is strong for the size of the change: an eight-case it.each (constructed, structured-cloned, AbortSignal.abort, AbortController#abort, AbortSignal.timeout, DataCloneError, atob, WebCrypto rejection) asserts Object.keys/JSON.stringify/spread are empty; a deep-equality test covers util.isDeepStrictEqual and toEqual; a spawned subprocess asserts the unhandled-rejection printer still emits at <dir>/reject.js:1, proving the retained non-enumerable line/sourceURL are still consumed by ZigException.cpp. The abort.test.ts fmt() helper no longer skips these keys, converting two existing assertions into regression tests. Tests follow harness conventions (tempDir, bunEnv, concurrent pipe drain, await using, event-awaited timeoutReason with no sleep). No CODEOWNERS covers the touched paths and there are no outstanding human review comments.
… Windows The sourceURL JSC records for a module spells the drive letter in lowercase on Windows, so the printed location does not match the tempDir path byte for byte there.
There was a problem hiding this comment.
LGTM — targeted attribute change with thorough coverage.
What was reviewed:
addDOMExceptionErrorInfo()mirrors JSC'saddErrorInfo()internals (getStackTrace→getLineColumnAndSource→putDirect, empty-trace →stack: "") with only theDontEnumflag added; no new throwing calls,WTF::movematches file convention.- New tests cover 8 DOMException creation paths,
isDeepStrictEqual/toEqualacross call sites, and a spawned fixture confirming the unhandled-rejection printer still reads backsourceURL/line. - The
abort.test.tsfmt()workaround removal tightens the existing assertion; comment-cop feedback was addressed in 69587b7.
Extended reasoning...
Overview
Replaces the addErrorInfo(lexicalGlobalObject, asObject(errorObject), true) call in the default arm of createDOMException() with a local addDOMExceptionErrorInfo() helper that stores the same four properties (line, column, sourceURL, stack) via putDirect() but with PropertyAttribute::DontEnum. Adds a DOMException own properties describe block to test/js/node/domexception-node.test.js covering constructed, structured-cloned, AbortSignal.abort(), AbortController#abort(), AbortSignal.timeout(), DataCloneError, atob(), and WebCrypto rejection paths, plus deep-equality and a spawned printer-output regression guard. Removes the column/line/sourceURL skip from abort.test.ts's fmt() helper.
Security risks
None. This changes only the enumerability attribute of diagnostic properties on internally-created DOMException wrappers. No user input parsing, no auth/crypto logic, no new allocation or lifetime paths.
Level of scrutiny
Medium — the code path runs for every internally-created DOMException, but the change is a near-verbatim inline of JSC::addErrorInfo()'s non-ErrorInstance branch with only the attribute flag changed. The helper makes the same non-throwing calls (getStackTrace, getLineColumnAndSource, Interpreter::stackTraceAsString, putDirect on a fresh wrapper), so no new exception-scope handling is needed; the PR confirms it passes under BUN_JSC_validateExceptionChecks=1. The empty-stack-trace and empty-sourceURL branches match the original's behavior. WTF::move is already the idiom used elsewhere in this file.
Other factors
Tests follow harness conventions (tempDir, bunEnv/bunExe, await using, concurrent pipe drain, Windows drive-letter case normalization) and the PR description documents USE_SYSTEM_BUN=1 failure on the five internally-created paths and the deep-equality test. The removed fmt() skip in abort.test.ts turns two existing tests into regression guards for this change. The github-actions comment-cop feedback about the long comment was addressed in 69587b7 (trimmed to one line) and the thread is resolved. No CODEOWNERS entry covers this file and there are no outstanding human reviewer comments.
|
Heads up on an overlap: #39320 replaces the same |
Problem
line,columnandsourceURLproperties:Object.keys(AbortSignal.abort().reason)is["line", "column", "sourceURL"](Node:[]), and the same holds forAbortController#abort()reasons,structuredClone()DataCloneErrors,atob()InvalidCharacterErrors,WebSocket#close()InvalidAccessErrors and WebCrypto rejections.JSON.stringify(reason)is{"line":1,"column":32,"sourceURL":"/app/index.js"}and{ ...reason }copies the three properties.util.isDeepStrictEqual,assert.deepStrictEqualandexpect().toEqual()compare those properties as ordinary own enumerable properties, so two abort reasons created on different lines (or against anew DOMException(...)with the same name and message) are not deep equal. They are in Node.test/js/web/abort/abort.test.tswas stripping these keys before comparing a reason with a constructed DOMException.createDOMException()(src/jsc/bindings/JSDOMExceptionHandling.cpp, default arm) records the creation site on the new wrapper withJSC::addErrorInfo().JSDOMExceptionis a plain DOM wrapper, not aJSC::ErrorInstance, soaddErrorInfo()storesline,columnandsourceURLwithputDirect()and default (enumerable) attributes (vendor/WebKit/Source/JavaScriptCore/runtime/Error.cpp,addErrorInfo(VM&, Vector<StackFrame>*, JSObject*)); onlystackis storedDontEnumthere. User-constructed and structured-cloned DOMExceptions do not go through this arm and have no own properties at all.Fix
addErrorInfo()call with a localaddDOMExceptionErrorInfo()that stores the same four properties (line,column,sourceURL,stack; juststack: ""when no JS frame is on the stack, as forAbortSignal.timeout()) withDontEnum. Values,stackcontents and the empty-trace case are unchanged; only the attribute differs.sourceURLandlineback off the object to print theat file:linelocation of an internally created DOMException (ZigException.cpp,exceptionFromString()), since a rejection reason was never thrown and has noJSC::Exceptionstack. Keeping themDontEnumkeeps that output andreason.linereads working while removing them from everything enumeration-based. This is also how Bun's ownErrorobjects already carryline/column/sourceURL(own, non-enumerable), and it gives the shape Node and WebIDL define for a DOMException: no own enumerable properties.console.log(reason)output is unchanged (Bun.inspect lists a DOM wrapper's own properties regardless of enumerability, and it already listed the non-enumerablestack).abort.test.ts: removed the line that skippedcolumn/line/sourceURLin itsfmt()helper. The two tests using it now fail without this change and pass with it.test/js/node/domexception-node.test.js: newDOMException own propertiesblock. Anit.eachover constructed, structured-cloned,AbortSignal.abort(),AbortController#abort(),AbortSignal.timeout(),structuredClone()DataCloneError,atob()and a WebCrypto rejection assertsObject.keys/JSON.stringify/ spread are empty; a deep-equality test coversutil.isDeepStrictEqualandtoEqual()across call sites and against a constructed DOMException; a spawned fixture asserts the printer still printsat <fixture path>:1for an unhandled abort reason (compared case-insensitively on Windows, where the sourceURL JSC records spells the drive letter in lowercase; that is pre-existing and unchanged here). On the released binary (USE_SYSTEM_BUN=1, checked on Linux and Windows x64) the five internally created paths and the deep-equality test fail; everything passes with the fix. Every assertion in the block also holds under Node v26.3.0 (output below).test/js/web/abort/abort.test.ts(16 pass;.signal.reason should be a DOMExceptionfails on the released binary), plusglobals.test.js,web-globals.test.js,abort-controller-gc-reason.test.ts,websocket-close-code.test.ts,structured-clone.test.ts,timers.promises.test.ts, the denoencoding.test.ts, and the upstreamtest-global-domexception.js,test-domexception-cause.js,test-structuredClone-domexception.js.BUN_JSC_validateExceptionChecks=1(the helper makes the same non-throwing callsaddErrorInfo()made).JSDOMExceptionontoJSC::ErrorInstanceand deletes this call altogether; this change is the small fix for the shape problem independent of that refactor.Background
DOMExceptionin Bun is a WebCore DOM wrapper (JSDOMException):name,messageandcodeare getters on the prototype that read the wrapped C++ object, so an instance normally has no own properties.JSC::ErrorInstanceis the class behindnew Error(); it stores line/column/sourceURL/stack in internal fields and materializes them as non-enumerable own properties, which is why the same data is invisible toObject.keyson a regular Error.JSC::addErrorInfo(globalObject, object, useCurrentFrame)captures the current JS stack and writesline,column,sourceURLandstackonto an arbitrary object. Upstream WebCore calls it fromcreateDOMException();createDOMException()is the single place all of Bun's internally created DOMExceptions come from (abort reasons viatoJS(CommonAbortReason),throwDataCloneError(),propagateException()for WebIDLExceptionCodes, andDeferredPromiserejections).DontEnumis JSC's name forenumerable: false.Object.keys,JSON.stringify, spread,for...inand Bun's and Node's deep-equality walks only look at enumerable own properties;[[Get]]-based reads such as the error printer's still see aDontEnumproperty.Node v26.3.0 on the same scenarios
Bun 1.4.0 on the same script reports
"keys":["line","column","sourceURL"]for the last five creation paths andfalsefor both deep-equality lines; this branch matches the Node output above (Node's own property set differs only in the non-enumerablestack/ creation-site properties, which none of these operations observe).Before / after for the printer and for util.inspect
The missing
AbortError: ...header in thatutil.inspectoutput comes from the JSC-formatstackstring, which this change does not touch; #32898 replaces it with a Bun-formatted stack.