worker_threads: carry parse error location (file/line/column/stack) to parent SyntaxError - #34333
worker_threads: carry parse error location (file/line/column/stack) to parent SyntaxError#34333robobun wants to merge 4 commits into
Conversation
…o parent SyntaxError A worker whose entry file fails to parse now surfaces a SyntaxError with stack, sourceURL, line and column populated from the parser's Location. Previously on_unhandled_rejection rebuilt the SyntaxError from only msg.data.text, dropping msg.data.location, so err.stack was undefined and no file path reached the parent.
|
Superseded by #38324 (closed). Reproduced with a file-based worker and an eval worker: both yield a CI (build 73898): |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes 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 (2)
Comment |
|
Updated 7:50 AM PT - Jul 16th, 2026
❌ @robobun, your commit 9ab2691 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 34333That installs a local version of the PR into your bun-34333 --bun |
![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 restoration of parse-error location info that a prior refactor dropped.
Checked: Msg::clone() is an inherent method (src/ast/lib.rs:1233); write_format::<false> signature matches; Location.line/column guard values (> 0 / > -1) match the field docs (1-based line, 0-based column). JSValue::put → putDirect (bindings.cpp:3838), so no user JS runs and no exception-check needed on a fresh SyntaxError. ZigString::from_bytes(...).to_js() is the established pattern (e.g. filesystem_router.rs:62). Tests cover both eval and file-based workers and assert exact line/column/sourceURL plus code-frame content.
Extended reasoning...
Overview
The PR touches one function (on_unhandled_rejection in src/jsc/web_worker.rs) and one test file. When a worker's entry point fails to parse, the resulting BuildMessage is converted to a SyntaxError before structured-cloning to the parent. Previously only msg.data.text was carried; this change clones the whole Msg, formats the parser's code frame via Msg::write_format::<false> into stack, and attaches sourceURL/line/column from msg.data.location as own properties — the exact fields the structured-clone Error serializer reads. Per the PR table, this restores what 1.3.14 already reported (plus the correct SyntaxError subtype).
Security risks
None. This is error-metadata surfacing on an internal worker→parent path. No user-controlled input reaches new parsing/allocation logic; the file path in sourceURL was already user-visible in 1.3.14 and in Node.
Level of scrutiny
Low-to-moderate. ~35 net lines in a single error-handling block, no new control flow, no cross-thread state, no allocator/lifetime changes. Verified:
BuildMessage.msgisbun_ast::Msg(src/jsc/BuildMessage.rs:14) with an inherentclone()(src/ast/lib.rs:1233).Location { file: Cow<[u8]>, line: i32, column: i32 }with docs "Line <= 0 means no line/column info" and column "0-based" — theline > 0/column > -1guards are correct.JSC__JSValue__putcallsputDirect(src/jsc/bindings/bindings.cpp:3838), so no JS reentry / exception-check concern on a freshly created error instance.ZigString::from_bytes(...).to_js(global)is used identically elsewhere (filesystem_router.rs:62); bytes are copied into the JS heap before the owningString/Cowdrops.
Other factors
Test coverage is solid: the existing eval-worker test is strengthened back to assert the full "error: ..." prefix and at <file>:1:13, and a new file-based test asserts exact line/column/sourceURL and that stack contains both the file:line:col and the source line. Both are stated to fail on main and the vendored test-worker-syntax-error{,-file}.js still pass. No prior reviewer comments to address; CI is building.
…s in worker_threads.test.ts
There was a problem hiding this comment.
LGTM — focused restoration of parse-error location on the worker→parent SyntaxError path.
Checked: ZigString::from_bytes auto-tags UTF-8 for non-ASCII paths/source lines; .put() targets a freshly-created native error (no user setters, no exception-check needed); location.line > 0 / column > -1 match the Location field docs (1-based line, 0-based column, ≤0 = no info); write_format::<false> into String cannot fail so discarding the result is fine. Tests cover both file and eval workers and fail on main.
Extended reasoning...
Overview
Extends the existing BuildMessage → SyntaxError conversion in on_unhandled_rejection (src/jsc/web_worker.rs) to attach stack (via Msg::write_format::<false>) and sourceURL/line/column from msg.data.location as own properties on the synthesized error, so structured clone carries them to the parent. The only unsafe change is widening the existing (*bm).msg.data.text.clone() to (*bm).msg.clone() under the same SAFETY contract. New test file exercises file-based and eval-based worker parse errors.
Security risks
None. This is an error-reporting path; inputs are the parser's own Msg/Location data, not user-controlled beyond what was already displayed in the message. No auth, crypto, filesystem, or network surface touched.
Level of scrutiny
Moderate — web_worker.rs is threading- and GC-sensitive, but this change is confined to a single already-existing conversion block on the worker (JS) thread inside on_unhandled_rejection. The target of the .put() calls is a native-created SyntaxError (no proxy/setter re-entry), the string data is Rust-owned and lives across the to_js call, and ZigString::from_bytes handles the ASCII/UTF-8 tag automatically. No new locking, no cross-thread state, no new lifetime edges.
Other factors
The PR includes robobun evidence showing both tests fail on main (stack undefined, location fields absent) and pass with the fix under debug+ASAN and release. The finder-raised encoding concern was verified against ZigString::from_bytes (auto-tags UTF-8 on non-ASCII). No CODEOWNERS match for the touched paths and no outstanding human review comments.
There was a problem hiding this comment.
LGTM — focused fix that attaches parser location (stack/sourceURL/line/column) to the synthesized SyntaxError before it's structured-cloned to the parent.
Checked: Msg::clone() deep-dupes the Cow-borrowed file/line_text so the cloned location outlives the BuildMessage cell; the line > 0 / column > -1 guards match the documented Location sentinel semantics in src/ast/lib.rs.
Checked: .put() on a fresh native SyntaxError instance runs no user JS, so no new exception-check obligations vs. the pre-existing path.
Checked: pattern matches BuildMessage.rs:148-161 (ZigString → to_js, put for location fields).
Tests use tempDir, cover file + eval workers, and the PR evidence shows fails-on-main / passes-with-fix under ASAN.
Extended reasoning...
Overview
The PR touches on_unhandled_rejection in src/jsc/web_worker.rs (~40 net lines) and adds a new 53-line test file. When a worker's entry point fails to parse, the existing code already converted the resulting BuildMessage into a SyntaxError (because BuildMessage doesn't survive structured clone), but only carried msg.data.text — dropping the file/line/column/code-frame that 1.3.14 used to report. This change clones the full Msg, formats it via the existing Msg::write_format::<false> into a stack string, and attaches sourceURL/line/column from msg.data.location as own properties on the error before it's serialized to the parent.
Security risks
None. This is error-reporting metadata on a developer-facing SyntaxError. No auth, crypto, permissions, or untrusted-input parsing paths are touched. The added .put() calls target a freshly-created native error object (no proxy traps / user setters), so no new re-entrancy surface.
Level of scrutiny
Low-to-medium. The change is additive to an existing error-conversion branch and uses well-established helpers verified against neighboring code:
Msghas an inherentclone()(src/ast/lib.rs:1233) that deep-copiesLocation'sCow<'static, [u8]>fields, so the cloned data is safe past theBuildMessagecell's lifetime.- The
location.line > 0/location.column > -1guards match the field docs at src/ast/lib.rs:726-733 (line ≤ 0 = no location; column is 0-based). ZigString::from_bytes(...).to_js()+JSValue::put()mirrors the exact pattern inBuildMessage.rs:148-161for the sameLocationfields.write_format::<false>writes toimpl fmt::Write(Stringhere) and itsfmt::Resultis intentionally discarded (Stringwrites never fail).
Other factors
- Tests are placed in a dedicated file per commit da5c8c9 (avoiding pre-existing debug+ASAN timeouts in
worker_threads.test.ts), usetempDirfrom harness, assert exact values, and normalize path separators for Windows. - PR evidence shows both tests fail on main and pass on the PR under debug+ASAN and release;
test-worker-syntax-error{,-file}.jsnode-parallel tests still pass. - Verifiers ruled out: worker cleanup ordering (worker self-exits after parse failure, so
terminate()after assertions is a no-op); comment length (added comments are exactly 3 lines). - No CODEOWNERS entries match the touched paths; no outstanding reviewer comments; no prior claude[bot] reviews on this PR.
|
Closing in favour of #38324, which carries the same fix (the parse error's |
1 participant
A worker whose entry file fails to parse reports a
SyntaxErrorwith the rightnamebut nothing else:err.stackisundefined, and there is nosourceURL/line/column. 1.3.14 reportedname: "Error"(wrong subtype) but carried the full code frame and file path. Node reports both.namekeysstackErrorat /abs/bad.js:2:11SyntaxErrorat /abs/bad.js:2SyntaxErrorundefinedSyntaxErrorat /abs/bad.js:2:11A control worker doing
throw new SyntaxError("x")already arrives with["message","line","column","sourceURL","stack"]on main: structured clone carries these fields fine. The synthesized error simply had none to carry.Cause
on_unhandled_rejectioninsrc/jsc/web_worker.rsconverts aBuildMessage(which doesn't survive structured clone) into aSyntaxError, but built it from onlymsg.data.text("Unexpected ;"), droppingmsg.data.location(file, line, column, line text). The error is created from native code with no JS frames on the stack, so its lazy stack materialization produces nothing.Fix
After building the
SyntaxError, attachstack(the parser's formatted code frame +at <file>:<line>:<column>, viaMsg::write_format) andsourceURL/line/columnfrommsg.data.locationas own properties. The structured-clone serializer reads exactly those own fields.Tests
Strengthened the existing "support worker eval that throws" test, whose assertion had been relaxed from
toInclude("error: Unexpected throw")totoInclude("Unexpected throw")when theSyntaxErrorconversion landed, letting the location loss through. Added a file-based worker test assertingsourceURL/line/columnand thatstackincludes<file>:2:11and the source line.Both tests fail on main (
stackisundefined, location fields absent) and pass with this change.test/js/node/test/parallel/test-worker-syntax-error{,-file}.jsstill pass.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 2
evidence per changed file