error: gate the <parse> stack frame on the parser having recorded a location, not on sourceURL - #37432
error: gate the <parse> stack frame on the parser having recorded a location, not on sourceURL#37432robobun wants to merge 7 commits into
Conversation
… location formatStackTrace appended a synthetic "at <parse> (url:line)" frame to every ErrorInstance of type SyntaxError whose first frame did not come from err->sourceURL(). Only errors created by JSC's parser have a sourceURL recorded (addErrorInfo), so for a SyntaxError constructed by user code, JSON.parse or RegExp the comparison was always true and the frame was rendered from empty data as "at <parse> (:0)". Once .stack had been read, the error printer re-parsed that line and lost the real location of the error entirely. Require a recorded sourceURL before emitting the frame. Parser errors for named sources (vm.Script, imported modules) keep it; everything else, including parser errors for URL-less sources such as new Function(), formats as header plus real frames like node.
…ompileFunction in the URL-less test
new Function("{") materializes .stack from inside JSC's own parse-error
path, which never checks for an exception from the stack hook, so the
test aborted under BUN_JSC_validateExceptionChecks on the debug CI lane.
node:vm's compile path checks. Also drain stdout in the uncaught-error
test.
A SyntaxError can record a sourceURL without coming out of JSC's parser: structured clone recreates an error with its original's line, column and sourceURL, and the GC finalizer that flushes an error's frames to a string records the first frame's URL on it. If such an error is later given frames by Error.captureStackTrace() from a different file, formatStackTrace() rendered that URL as a fabricated "at <parse> (creating-file.js:1)" line ahead of the real frames. Require the parser's own flag instead. It was not usable before because ParserError::toErrorObject() set it only after addErrorInfo() had already materialized the stack through our hook; the WebKit build pinned here (preview of oven-sh/WebKit#407) sets it first. The SyntaxError type check stays because parse-time stack overflow and out-of-memory errors carry the flag too, and the sourceURL check stays so parser errors for URL-less sources still format without the frame.
WalkthroughChangesWebKit preview pin
SyntaxError stack formatting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on bun 1.4.0 and on #37386 with the two-file Current revision (4315f53) gates on a dedicated Blocked only on oven-sh/WebKit#407 (pinned here as preview build |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it pins a preview WebKit build (autobuild-preview-pr-407-deb6bcb2) that the description says must be replaced once oven-sh/WebKit#407 lands, and because that bump also pulls in oven-sh/WebKit#405 relative to the current pin, a human should confirm the WebKit coordination before this merges.
What was reviewed
- The
isParseError()gate informatStackTrace()— strictly narrows the existing condition; the redundant nested!sourceURLForFrame.isEmpty()guards it removes are provably dead now that!err->sourceURL().isEmpty()is checked up front. - New tests: structured-clone / worker-postMessage / GC-flush cases each assert the real first frame (not just absence of
<parse>), and the parser-error guards confirm the frame is still emitted where it should be. - Worker test wires
onerrorto reject and terminates infinally; subprocess tests drain stdout/stderr concurrently.
Extended reasoning...
Overview
This PR tightens the condition under which .stack formatting emits a synthetic at <parse> (url:line) frame for SyntaxErrors. It is stacked on #37386 (whose commits are included in the diff) and paired with oven-sh/WebKit#407, which reorders ParserError::toErrorObject() so isParseError() is set before Bun's stack hook runs. The Bun-side change is one added conjunct (err->isParseError()) plus removal of a now-redundant nested isEmpty() check, a three-line comment, and three new tests covering the structured-clone, worker-postMessage, and GC-flush paths that previously produced a spurious <parse> frame. WEBKIT_VERSION is pinned to a preview build tag.
Security risks
None identified. This is display-only formatting of error stack strings; no untrusted input reaches an allocation size, path, or command. The change strictly narrows an existing branch.
Level of scrutiny
Medium-high. The C++ change itself is small and well-argued, but it is coupled to an unlanded WebKit PR and ships a preview dependency pin that the author explicitly flags as temporary ("Replace with the main sha once it lands"). The bump also carries oven-sh/WebKit#405 as a rider. Merging with a preview tag in WEBKIT_VERSION would leave main depending on a non-mainline WebKit build, which is a coordination decision a maintainer should make.
Other factors
- The PR is stacked on #37386; a human should confirm merge order.
- Test coverage is thorough: positive guards (parser errors keep the frame via
vm.Script,vm.compileFunctionwith filename, andimport()of JSC-rejected modules) and negative guards (clone, worker, GC) both assert exact frame content, not justnot.toContain. - The
prebuiltDestDir/prebuiltUrlhelpers inwebkit.tsalready handleautobuild-prefixed tags, so the preview pin will resolve correctly — but it is still a preview. - No prior reviews from me on this PR.
|
On the WebKit coordination: this is not meant to merge with the preview tag. The order is oven-sh/WebKit#407 first, then I swap |
|
Updated 4:36 AM PT - Aug 11th, 2026
✅ @robobun, your commit 4315f536775a07bd3372e2b79c0b5d057e4950b6 passed in 🧪 To try this PR locally: bunx bun-pr 37432That installs a local version of the PR into your bun-37432 --bun |
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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@scripts/build/deps/webkit.ts`:
- Around line 6-8: Update the WEBKIT_VERSION constant from the temporary preview
tag to 44855720a693f3afd2e217bc986ab75a9ae0f43e, preserving the existing version
format and related reproducibility behavior.
🪄 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 Plus
Run ID: 9ffbd0cf-be6d-4f62-a441-ef4c4ac2b8fb
📒 Files selected for processing (3)
scripts/build/deps/webkit.tssrc/jsc/bindings/FormatStackTraceForJS.cpptest/js/bun/test/stack.test.ts
|
Revised since the first round of review (32117b8, 4315f53): gating on |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it depends on an unmerged engine change (oven-sh/WebKit#407, pinned here as a preview build that must be swapped for a main sha before merge) and is stacked on #37386, a human should coordinate the merge order.
What was reviewed:
- The
hasParseLocation()gate inFormatStackTraceForJS.cpp— the removed inner!sourceURLForFrame.isEmpty()check is now subsumed by the outer condition, so the remap block is still guarded. - The new tests cover clone, worker postMessage, GC-flushed frames (including the eval case that motivated moving off
isParseError()), and confirm real parser errors keep the frame. - Checked that the
globalObjectnull-guard on the remap call is preserved.
Extended reasoning...
Overview
This PR changes one condition in src/jsc/bindings/FormatStackTraceForJS.cpp: the synthetic at <parse> (...) frame is now emitted only when ErrorInstance::hasParseLocation() is set and sourceURL() is non-empty, replacing the prior errorType() == ErrorType::SyntaxError check (from stacked #37386). It also collapses a redundant nested !sourceURLForFrame.isEmpty() check now that emptiness is tested in the outer condition. scripts/build/deps/webkit.ts pins WebKit to a preview build of oven-sh/WebKit#407 (which introduces hasParseLocation()), and test/js/bun/test/stack.test.ts gains a substantial describe("SyntaxError .stack") block covering user-constructed errors, structured clone, worker postMessage, GC-flushed frames with eval/indirect-eval variants, and positive cases where parser errors keep the frame.
Security risks
None identified. This is stack-trace formatting; no auth, crypto, permissions, or untrusted-input parsing is touched. The changed condition narrows when a synthetic frame is rendered.
Level of scrutiny
High, for process reasons rather than code complexity. The C++ diff is small and the mechanism is well-argued in the description, but the change (a) calls a new engine API that only exists in an unmerged WebKit PR, (b) pins WEBKIT_VERSION to a preview autobuild tag that the author explicitly says must be replaced with the merged main sha before this lands, and (c) is stacked on #37386. Merging this out of order would either fail to compile (wrong WebKit) or ship a preview pin. That coordination is a human decision.
Other factors
- The bug hunting system found no issues.
- The removed inner
if (!sourceURLForFrame.isEmpty())was dead once the outer condition gained the same check; theglobalObjectguard aroundBun__remapStackFramePositionsis preserved. - Tests look solid: they use
tempDir, drain subprocess pipes concurrently, wireworker.onerrorto reject, and include both negative (no<parse>) and positive (parser errors keep it) assertions. The GC test's 4-round loop with a comment about conservative-scan stale pointers and the last-exception displacement is deliberate. - All prior review threads on the timeline (comment-cop on the long comment, CodeRabbit on the preview pin) are resolved; the current revision reflects the
isParseError()→hasParseLocation()change made after review turned up the eval case.
1 participant
Stacked on #37386 (its four commits come first; the change here is the last commit). Pairs with oven-sh/WebKit#407, pinned here as a preview build; once that lands I will replace
WEBKIT_VERSIONwith the main sha.What does this PR do?
#37386 stops
.stackfrom giving every SyntaxError a fabricatedat <parse> (:0)line by only emitting the<parse>frame when the error records asourceURL, which is what JSC'saddErrorInfo()leaves on a parser error. AsourceURLis not unique to parser errors though. A SyntaxError that has one for another reason still gets the frame as soon as it is given frames from a different file:The realistic shape is an error
postMessaged by a worker that the parent thencaptureStackTraces. Two ways an error that never had a parser location ends up with asourceURL:structuredClone,postMessage) recreates the error through theErrorInstance::createoverload that takes the original's line/column/sourceURL (SerializedScriptValue.cpp), so every clone records the file its original was created in.ErrorInstance::finalizeUnconditionallyflushes the frames to a string and records the first frame's URL on the error. Anew SyntaxError()created inside a function that has since been collected shows the same<parse>line when captured afterwards, and so does a syntax error thrown byeval()(which JSC does flag as a parse error, but records no location for), no cloning involved:at <parse> (gc-me.js:1)in the new tests.A plain
.stackread on a clone was already fine (it returns the serialized stack string) and still is. Only errors that get frames installed after the fact are affected, and those all go through this one formatter.Fix
The thing the frame renders is the location
addErrorInfo()recorded, so that is what the formatter should test for. oven-sh/WebKit#407 adds a Bun-only bit toErrorInstance, set byaddErrorInfo()right where it records the line and sourceURL (before it materializes the stack through our hook), exposed ashasParseLocation(). Clones, GC-flushed errors and eval parse errors never get it; every parser error for a named source does.formatStackTrace()now checks that instead of the error type. ThesourceURLcheck stays so parser errors for URL-less sources (new Function,vm.compileFunctionwithout a filename) keep formatting without a frame, as #37386 made them.JSC's existing
isParseError()does not work for this, which is why the engine change is needed:ParserError::toErrorObject()sets it only afteraddErrorInfo()has already formatted the stack (#37386 verified that gating on it dropped the frame for every parser error), and it is also set for eval-code syntax errors, which have no location to show and would still pick up the GC-recorded one. The first revision of this PR gated on it with the ordering fixed and review turned up exactly that eval case, so the discriminator is now a dedicated bit.Parser errors are unaffected: the existing guards in
stack.test.ts(vm.Scriptandvm.compileFunctionwith a filename,import()of modules JSC rejects with and without captured frames) pass against the new build, and animport()of an ESM module JSC rejects still printsat <parse> (bad.mjs:2).The clone deserializer is deliberately left alone: a clone carrying its original's
line/column/sourceURLis correct and observable (structuredClone(err).sourceURL). The formatter was reading the wrong signal.Tests
Added to the
SyntaxError .stackblock intest/js/bun/test/stack.test.ts:structuredClone()of a SyntaxError, thenError.captureStackTrace()run from avm.Scriptwith a different filenameWorker, thencaptureStackTraced by the parentnew SyntaxError(),eval("{")and(0, eval)("{")inside avm.Scriptfunction thatBun.gc(true)collects, thencaptureStackTraced (the eval rows need the VM's last-thrown exception displaced first, since it keeps a caught error's frames alive)Each fails with the
<parse>line in place of the real first frame on the released build (USE_SYSTEM_BUN=1 bun test test/js/bun/test/stack.test.ts) and on a debug build with the preview WebKit butsrc/at #37386's state; the two eval rows also fail against the first revision of this PR. With this change the file passes (bun bd test test/js/bun/test/stack.test.ts), including underBUN_JSC_validateExceptionChecks=1.Also run against the debug build:
test/js/node/v8/capture-stack-trace.test.js,test/js/node/vm/vm.test.ts,test/js/node/vm/vm-sourceUrl.test.ts,test/js/web/workers/structured-clone.test.ts,test/js/node/worker_threads/worker_threads.test.ts, all passing.test/js/bun/util/inspect-error.test.jshas two minified-file snapshot failures that reproduce on a pristinemaindebug build (an extra builtinrequireframe, tracked separately) and are unrelated.Notes for the reviewer
WEBKIT_VERSIONpoints atautobuild-preview-pr-407-f6d8e2a0. The preview is branched from WebKitmain(723cea6c), so relative to the current pin (447082ab) it also contains Treat BunTranspiledModule as Module in debugger parse data, cached types and completion WebKit#405, the same delta Bump WebKit to 3997b59485da #37352 bumps to. Not meant to merge with the preview tag; the order is WebKit#407, pin swap here, then error: don't add a synthetic <parse> frame to SyntaxErrors that aren't parser errors #37386 and this.