ErrorInstance: record that addErrorInfo() attached a parser location (hasParseLocation) - #407
ErrorInstance: record that addErrorInfo() attached a parser location (hasParseLocation)#407robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped ordering fix guarded by USE(BUN_JSC_ADDITIONS).
What was reviewed:
createSyntaxErroralways returns anErrorInstance(viaErrorInstance::create), so thedowncast<ErrorInstance>(*syntaxError)is safe — same pattern as the existing unconditional call after the switch.- Confirmed
addErrorInfo(VM&, JSObject*, int, const SourceCode&)callsmaterializeErrorInfoIfNeeded()after setting line/sourceURL (Error.cpp:254-267), so the ordering claim holds. - Non-Bun builds: the only change is hoisting
createSyntaxError(...)into a local before passing it toaddErrorInfo— semantically identical.
Extended reasoning...
Overview
This PR touches a single header, Source/JavaScriptCore/parser/ParserError.h, and changes only the SyntaxError case of ParserError::toErrorObject(). It hoists the createSyntaxError(...) call into a local variable and, under #if USE(BUN_JSC_ADDITIONS), calls setParseError() on it before handing it to addErrorInfo(). The existing unconditional setParseError() after the switch is left in place, so for the SyntaxError case the second call is a harmless idempotent write of m_parseError = true.
Security risks
None. This only reorders when a boolean flag on an ErrorInstance is set relative to stack-string materialization. No new inputs, no allocation changes, no control-flow changes visible outside the Bun host formatter hook.
Level of scrutiny
Low. The diff is ~10 lines, guarded by the Bun-specific feature macro, and mirrors an existing pattern in the same function (downcast<ErrorInstance>(*error).setParseError()). I verified in Error.cpp that createSyntaxError(JSGlobalObject*, const String&) returns ErrorInstance::create(...) (never null), so dereferencing for the downcast is as safe as the pre-existing dereference at the bottom of the function. I also confirmed that addErrorInfo(VM&, JSObject*, int, const SourceCode&) does call materializeErrorInfoIfNeeded() (Error.cpp:267) after setting line/sourceURL, which validates the PR's motivation.
Other factors
For non-USE(BUN_JSC_ADDITIONS) builds the only observable change is that the createSyntaxError result is stored in a named local before being passed to addErrorInfo — no behavioral difference. The header already includes <JavaScriptCore/ErrorInstance.h>, so downcast<ErrorInstance> compiles without new includes. The bug hunting system found no issues.
Preview Builds
|
…(hasParseLocation) Bun's stack formatter renders a synthetic "at <parse> (url:line)" frame for SyntaxErrors whose line/sourceURL were recorded by the parser via addErrorInfo(). Until now it recognized those errors by the sourceURL being set, which is also true of structured-cloned errors (created with their original's sourceURL) and of errors whose frames the GC finalizer already flushed to a string (computeErrorInfo records the first frame's URL), so giving either of those new frames with Error.captureStackTrace() produced a bogus <parse> frame. isParseError() cannot serve as the discriminator: it is also set for eval-code syntax errors, which never go through addErrorInfo() and so record no location, and toErrorObject() sets it only after addErrorInfo() has already materialized the stack through the host hook. Add a Bun-only bit that addErrorInfo() sets right where it records the location, before materializing, and expose it as hasParseLocation().
deb6bcb to
f6d8e2a
Compare
robobun
changed the title
There was a problem hiding this comment.
LGTM — small, self-contained Bun-only flag addition that follows the existing m_parseError/m_nativeGetterTypeError pattern.
What was reviewed:
- Confirmed the 4-arg
addErrorInfo(VM&, JSObject*, int, const SourceCode&)overload is called only fromParserError::toErrorObject()'sSyntaxErrorbranch, so the bit is set exactly where claimed. - Checked ordering:
setHasParseLocation()runs beforematerializeErrorInfoIfNeeded(), so the host formatter sees it when building.stack. - Bitfield count: 6 base bits + wasm bit + this bit = 8, still one byte; constructor initializer order matches declaration order; all additions are behind
USE(BUN_JSC_ADDITIONS).
Extended reasoning...
Overview
This PR adds a single Bun-only boolean bitfield m_hasParseLocation to ErrorInstance, with a getter/setter pair, and sets it inside the existing USE(BUN_JSC_ADDITIONS) block of the 4-argument addErrorInfo() overload in Error.cpp — right after the line/sourceURL are recorded and immediately before materializeErrorInfoIfNeeded(). The constructor in ErrorInstance.cpp initializes it to false. Three files touched, ~15 lines added, zero lines removed.
The purpose is to give Bun's stack-trace formatter a precise signal that an error's line()/sourceURL() came from the parser (via ParserError::toErrorObject()), as opposed to structured clone or GC-driven computeErrorInfo(). I verified via grep that ParserError.h:101 is the sole caller of this addErrorInfo overload, and only in its SyntaxError case — the EvalError, StackOverflow, and OutOfMemory branches don't call it, so the bit's semantics match the PR description exactly.
Security risks
None. This is an internal, read-only metadata flag on a C++ object. It is not exposed to JavaScript, does not affect parsing, JIT, GC, or memory layout in any exploitable way, and is entirely guarded by USE(BUN_JSC_ADDITIONS).
Level of scrutiny
Low. The change is mechanical and mirrors half a dozen existing 1-bit flags on the same class (m_parseError, m_stackOverflowError, m_nativeGetterTypeError, etc.). It adds no control flow to JSC itself — the only in-tree write is the one setHasParseLocation() call, and there are no in-tree readers (the reader lives in oven-sh/bun#37432). Non-Bun builds compile identically.
Other factors
- Bitfield packing: 6 unconditional bits +
m_catchableFromWasm+m_hasParseLocation= 8 bits max, fitting the same byte as before, sosizeof(ErrorInstance)is unchanged as the description states. - Constructor initializer list order matches member declaration order (both appended after the
ENABLE(WEBASSEMBLY)block), so no-Wreorder. - The set happens before
materializeErrorInfoIfNeeded(), which under Bun calls the hostonComputeErrorInfoJSValuehook — so the formatter can read the flag at the right moment, addressing the ordering problem the description calls out forisParseError(). - Preview build succeeded; the paired Bun PR carries the actual tests for the clone/worker/GC-flush/eval cases.
isParseError() is also set for syntax errors in eval code, which go through toErrorObject() without addErrorInfo() and so keep live frames; once GC flushed those frames and recorded the first frame's URL on the error, Error.captureStackTrace() still produced the fabricated frame for them. The reworked oven-sh/WebKit#407 instead has addErrorInfo() set a dedicated bit right where it records the parser's line and sourceURL, which is exactly what the frame renders, so the error type check is no longer needed either. Adds the eval() and indirect eval rows to the GC test; a caught error's frames stay alive while it is the VM's last exception, so the test throws something else before collecting.
1 participant
Problem
Bun's stack formatter (
formatStackTraceinsrc/jsc/bindings/FormatStackTraceForJS.cpp) renders a syntheticat <parse> (url:line)frame for SyntaxErrors whose line and sourceURL were recorded by the parser, i.e. by theaddErrorInfo(VM&, JSObject*, int line, const SourceCode&)overload thatParserError::toErrorObject()calls. It currently recognizes those errors bysourceURL()being set. That is also true of two other kinds of error:ErrorInstance::create(JSGlobalObject*, String&& message, ErrorType, LineColumn, String&& sourceURL, String&& stackString, ...), which structured clone uses, so every clone carries the file its original was created in;ErrorInstance::finalizeUnconditionally()already flushed to a string:computeErrorInfo()records the first frame's URL inm_sourceURL.Give either of those frames afterwards (
Error.captureStackTrace()from another file; the realistic shape is an error posted from a worker) and the formatter emits a bogusat <parse> (creating-file.js:1)line ahead of the real frames.isParseError()is not usable as the discriminator, for two reasons:toErrorObject()callsaddErrorInfo()first, and underUSE(BUN_JSC_ADDITIONS)that materializes.stackon the spot through the host hook, so the flag is still false at the one moment the host formats a parser error's stack.ParserError::EvalError(a syntax error while parsing eval code), which is built withoutaddErrorInfo()and so records no location. Such an error keeps live frames, the GC flush above then records the first frame's URL on it, and it ends up with a fabricated frame too. (The first revision of this PR only fixed the ordering, which left exactly that case open.)Fix
Add a Bun-only bit to
ErrorInstance, set by the 4-argaddErrorInfo()in its existingUSE(BUN_JSC_ADDITIONS)block right where it records the line and sourceURL, beforematerializeErrorInfoIfNeeded(), and exposed ashasParseLocation(). It is set exactly for the errors whose location came from the parser; clones, GC-flushed errors, eval parse errors, and the stack overflow / OOM kinds thattoErrorObject()also flags never get it. Upstream'sm_parseErrorkeeps its meaning andParserError.his untouched.The bit fits in the existing bitfield byte, so
sizeof(ErrorInstance)does not change. Builds without the additions are unaffected.Verification
The modified
ErrorInstance.hcompiles in Bun's TUs that use it (NodeVMScript.cpp,FormatStackTraceForJS.cpp,-fsyntax-onlyagainst the current prebuilt's headers with this header swapped in).The consumer change is oven-sh/bun#37432, which switches the
<parse>condition tohasParseLocation()and adds tests for the clone, worker-postMessage, GC-flush and eval-code shapes, alongside the existing guards that parser errors (vm.Script,vm.compileFunctionwith a filename,import()of modules JSC rejects) keep the frame. It is pinned to this PR's preview build.