Repository navigation
Conversation
The 'error' event of a worker had an empty filename and 0 for lineno and colno, in the worker and on the Worker object. Take the three from the throw's stack when the JSC::Exception is still there, else from the position the thrown Error or DOMException carries.
|
Status How I reproduced it: the script from #32342 on a release build of main (116dd64) prints
Pull request: #45429 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughWorker error reporting derives script positions from thrown values and exception stacks, then includes filename, line, and column in worker error events. The change adds script-frame and source-URL classification, source-map remapping support, and tests for error paths and URL forms. ChangesWorker Error Event Locations
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: populating worker error events with filename, line number, and column number. |
| Description check | |
The description clearly explains the problem, implementation, scope, limitations, related work, and verification results. It does not use the exact template headings, but it provides the required info… |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Updated 7:05 PM PT - Oct 15th, 2026
❌ @robobun, your commit 2f7d6e7 has some failures in 🧪 To try this PR locally: bunx bun-pr 45429That installs a local version of the PR into your bun-45429 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked: the remapLineColumn extraction keeps the empty-URL guard in computeLineColumnWithSourcemap and every new direct caller is gated by isScriptFrame/isScriptSourceURL, so no behavior change for existing .stack formatting; the new filename is isolatedCopy()'d into the cross-thread task and both the Web and Node postErrorToWorkerObject arms pass the three fields; scriptPositionOfThrownValue only uses getDirect/tryGetValue/cell accessors, so it cannot re-enter JS from the error hook.
Extended reasoning...
The change adds position extraction for worker uncaught errors across the C++ error-hook path (JSDOMExceptionHandling, Worker.cpp, WorkerMessagingProxy), a stack-frame predicate in ErrorStackTrace, and a Rust change passing the JSC::Exception through; no security-sensitive surface. Inline findings already cover the compiled-sourcemap filename shape, the BuildMessage path dropping the exception, and the inspect-hook error substitution, so this note only records the thread-safety, refactor-equivalence and re-entrancy checks that were ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/web_worker.rs— A worker whose callback doesrequire("./file-with-syntax-error.js")inside a timer, listener or onmessage still getsfilename "",lineno 0,colno 0, even though the JSC::Exception carrying the script's require call site arrived. src/jsc/web_worker.rs:1163-1167 replaces the BuildMessage value with a synthetic SyntaxError, so error_instance != thrown_value at web_worker.rs:1210 and the original exception is dropped. The synthetic error has no script frames. Fix: pass the original JSC::Exception for the position while still dispatching the synthetic SyntaxError as the event'serror, e.g. by splitting the position lookup from the value in WebWorker__dispatchError.Why this was flagged
A worker's setTimeout or onmessage handler requires a module with a parse error, or imports one whose resolution fails. The hook receives a JSC::Exception whose value is a BuildMessage (web_worker.rs:1142). Line 1163 matches it and line 1167 builds a fresh SyntaxError via to_syntax_error_instance, created from native code with no JS frames. At web_worker.rs:1210
error_instance == thrown_valueis false, sothrownis the synthetic error and the JSC::Exception with the script's frame is discarded. scriptPositionOfThrownValue at src/jsc/bindings/JSDOMExceptionHandling.cpp:146 gets exception == nullptr; the ErrorInstance arm finds line()==0 and either no frames or a native-only frame and returns {}. The PR's own table promises 'where it was thrown' for any value thrown in a timer or listener; this input gets "",0,0 instead. The base also reported "",0,0, but the fix is one line: pass error_instance_or_exception for the position and the synthetic error for init.error.Verification: nit (pre-existing route: the base also reports "", 0, 0 here). Trigger: a worker callback under a timer/listener/onmessage calls
require()on a file with a parse error. src/jsc/web_worker.rs:1163-1167 replaceserror_instancewith a SyntaxError, so line 1210if error_instance == thrown_valueis false and the JSC::Exception is dropped, givingfilename "",lineno 0,colno 0.
… throws The position now comes from what was thrown, also when the event's error is a SyntaxError made for a module that did not parse. The test cases run concurrently.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Part of #23473
Problem
Workerobject, theerrorevent of an uncaught error hasfilename "",lineno 0andcolno 0. Worker data: URL errors do not populate ErrorEvent fields (filename, lineno, colno) in Bun #32342 has the repro.WebWorker__dispatchError(src/jsc/bindings/webcore/Worker.cpp) builds both events and set none of the three.on_unhandled_rejection(src/jsc/web_worker.rs) also dropped theJSC::Exception, which holds the throw's stack.Fix
on_unhandled_rejectionpasses theJSC::Exceptionon.WebWorker__dispatchErrorfills the fields with the newscriptPositionOfThrownValue(JSDOMExceptionHandling.cpp).JSC::Exceptionit is the position that a native Error orDOMExceptioncarries.messagedoes not change.erroris the thrown value, also when its console rendering throws.test/js/web/workers/worker-error-event.test.ts(32 of 36 fail before). Self-reviewed: 45 concerns raised, 2 left (Notes).Background
JSC::Exceptionholds a thrown value and the stack at the throw. Timers and listeners pass it to the hook. A rejected promise or a failed entry passes only the value.Zig::isScriptFrame).message, as in browsers. A listener'smessageis today the only place that shows the stack, so that waits for worker: report a Worker error that no listener took as the parent's uncaught error #35829.Downsides
JSC::Exception, a top-levelthrow 5or[].reduce(cb)still reports"", 0, 0..textgrows 3,072 bytes.Notes
What the two events say. The worker's own event and the
Workerobject's event carry the samefilename,lineno,colno.reportError(error)onmessage, a microtask,nextTick[].reduce(cb),emitter.emit("error", x),new WebSocket("x")fromws)require()structuredClone()DOMExceptionthat native code threw (structuredClone(() => {}))colno 0DOMException, an object that is not a native Error, each thrown at the top level"", 0, 0"", 0, 0"", 0, 0 (as before)filenamenames the frame asstackdoes. An absolute path is written as afile:URL, so a module on disk gives itsimport.meta.url. Ablob:ordata:worker gives that URL. The query and the fragment of the URL given tonew Workerare not part of it.stack: a.tsworker reports the line of its source. In abun build --compileexecutable the frame is in the bundle (file:///$bunfs/root/worker.js). With--sourcemapit is the path that the map names (src/lib/thrower.ts), which is not a URL. An Error whosestackwas read keeps only that name, so the event gives it in both states.stackgives that frame. Chrome reports the start of the statement.return JSON.parse(text)reports the caller of that function.Why an Error that starts inside the runtime says nothing without its
JSC::Exception. Whenstackis read, or when a collection finds one of the frames dead, JavaScriptCore replaces the frames with the position of the first frame that has one.test/js/bun/jsc/webkit-upgrade-7b485a76e9.test.tspins thatlineandcolumnname that frame, also when it is a builtin. The event reports that frame or nothing, so that the same Error gives the same answer before and after. An earlier draft of this PR changed which frame is stamped. That is a change for every program and is not in this PR.Not changed.
messageis still the console report. Today a listener'smessageis the only place where the stack shows: nothing prints when an error listener exists, and nothing prints when none exists. A one-linemessageneeds the default report of worker: report a Worker error that no listener took as the parent's uncaught error #35829 or a maintainer's decision first. The work is on the branchrobobun/592381ed/worker-error-event-message.erroron theWorkerobject's event staysnull. HTML sets it to null for that event and WPTworkers/Worker_ErrorEvent_error.htmasserts it. Web-style worker error event always has a null error property #23473 asks for the thrown value there, so this PR is only part of it.node:worker_threadsparents get what they got. On the path where the error cannot be cloned,err.cause(the event) now has the three fields too.filenameandlineno. The column is 11, not 7, andmessagehas noUncaught (in promise)prefix.An abort that goes away. When the console printer threw (an inspect hook that throws), the hook replaced the event's
errorwith what the printer threw, as aJSC::Exceptioncell. A listener in the worker that touched it ended the process (exit 134). The thrown value now stays the event'serror, and the position comes from what was thrown. Anode:worker_threadsparent then gets the clone of that value, where it gotnull. The testa value whose console rendering throwscovers it. #38560 has the same change inon_unhandled_rejection. A typedtake_exceptionwould fix the class and is not in this PR.Related pull requests. This PR does not depend on any of them.
postMessageErrorToWorkerObject. This PR adds three parameters to it.thrown_valueline sits before the BuildMessage block that worker errors: carry the parse error's location to the parent; let parse/resolve diagnostics cross structured clone #38324 removes.is_builtinflag for the error printer, from the sameisBuiltinFunction()thatZig::isScriptFramereads. The two can share one predicate after either lands.JSC::Exception: the top-levelthrow 5here.errorevent at all. worker: fire 'error' for the VM log only when the worker fails #45203 and worker: take a listener's termination at the native boundary, end the tick pass on a stopped VM #45298 touch the same two functions.addEventListenerfromnode:worker_threadsworkers. This PR adds no test that uses it.line()andcolumn()of a formatted Error, as the stamped arm does here.Self-review. The review asked for this narrower cut: no change to which frame
stackstamps, no second mapping for parse errors,messageas on main, builtin code detected from the code block and not from a URL prefix. Two things are left.console.log(event)printstype,messageanderror, and not the three new fields. The location is still inmessage, so that goes with themessagechange.test/js/third_party/wpt-testharness-shim.ts. The test file hasworkers/support/ErrorEvent.jsunchanged, the checks ofWorker_ErrorEvent_filename.htm,_lineno.htmand_message.htm, and atest.todoforWorkerGlobalScope_ErrorEvent_*.htm.Left for later.
test.todorows in the test file: a position for a value that carries none and arrives without itsJSC::Exception, andself.onerrorcalled with five arguments. It gets the event alone.node:fs,node:netandBun.servecallbacks do not pass theJSC::Exceptionto the hook. Athrow 5there, orif (err) throw errwith an Error that the runtime made, reports"", 0, 0. An Error made in its throw statement reports its line.stackwas read under a userError.prepareStackTracehas no stamped position.WebCore::reportExceptionstill reports"", 0, 0. It can use the same function.Measurements. Release builds of main 116dd64 and of this branch at cff528d. gdb on
bun-profile, JIT off, three runs with identical counts.strace,valgrind,perfandbloatyare not available in the container..text: +3,072 bytes (size -A)throw 5from a timer 8 -> 18, Web script-made DOMException 37 -> 37, Node cloneable Error 50 -> 52, Nodethrow port1105 -> 105 (counters on 18 mimalloc entry points, GC cells not counted)static_asserton the lambda, hit counts onpostTaskTo)flush_logsthe same 115 instructions,formatStackTracethe same 7,120 bytescomputeLineColumnWithSourcemap(CPU profilers,v8::Functionpositions): its body moved toremapLineColumn. The hook is now 10 instructions and a tail call, and no longer takes and drops a reference on the URL (11 instructions, two of them atomic).Suites run on the debug build of this branch.
worker-error-event.test.ts: 36 pass, 2 skip, 5 todo. Also withBUN_JSC_validateExceptionChecks=1. The 2 skipped tests build an executable. They pass on the release build (38 pass). On a release build of main, 34 of the 38 fail.test/js/web/workers:worker.test.ts42,worker_blob6,worker-entry-point2,message-event10,structured-clone244.test/js/node/worker_threads/worker_threads.test.ts: 153 pass in three of four runs. One run failedterminate(): nothing of the worker's runs after the request, which passes alone six times of six.test/cli/run/cpu-prof.test.ts12 (it uses the source-map hook).webkit-upgrade-7b485a76e922,webkit-upgrade-9b02218df64,capture-stack-trace68,error-gc-test4,inspect-error39,inspect.test.js96,reportError2,internal-sourcemap6,internal-sourcemap-roundtrip22,vm.test.ts309,console-log5.Questions for a maintainer.
messagebecome one line (TypeError: Hello, world!, WebKit's form, or Chrome's withUncaught), and does that wait for worker: report a Worker error that no listener took as the parent's uncaught error #35829?line/column/sourceURLof an Error name its first frame in the program and not a builtin on top? That would give the top-level cases above a position.