Conversation
WebWorker::create stored the stack address of temp_log in the transpiler's log pointer, then moved temp_log into a scopeguard tuple. The transpiler wrote the ModuleNotFound message through the stale pointer; the guard's temp_log stayed empty, so log.to_js() returned undefined and the thrown TypeError's message was the string 'undefined'. Move temp_log into the guard first, then call set_log with its final address.
|
Updated 7:48 PM PT - Jun 15th, 2026
✅ @robobun, your commit ef918a1773552aca151d6c704ca3a29dcaee4f56 passed in 🧪 To try this PR locally: bunx bun-pr 32369That installs a local version of the PR into your bun-32369 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesWebWorker preload error propagation fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — correct reordering so set_log targets the post-move address inside the scopeguard, with a regression test.
Extended reasoning...
Overview
This PR fixes a dangling-pointer bug in WebWorker::create (src/jsc/web_worker.rs) where set_log(&raw mut temp_log) was called before temp_log was moved into a scopeguard tuple, leaving the transpiler holding a stale stack address. The fix reorders the operations: construct the guard first (moving the Log in), then call set_log on the address obtained via &mut *restore. A regression test in worker.test.ts asserts the error message for an unresolvable preload module is not "undefined" and contains the specifier.
Security risks
None. This only affects the content of an error message on the Worker-construction failure path. No auth, crypto, permissions, or untrusted-input parsing is involved.
Level of scrutiny
Low-to-medium. The src change is ~5 lines of reordering with a clarifying comment, confined to an error-reporting path. The root cause (raw *mut Log stored in the transpiler, then the pointee moved) is well-explained and the fix is the obvious correct one — restore is a stack local that is never moved after the set_log call, so the address obtained through DerefMut stays valid for the guard's lifetime. The closure still restores prev_log and drops the temp log on every return path, exactly as before.
Other factors
- The PR description includes a clear repro, root-cause analysis (including why
log.to_js()on an empty log yieldsundefined), and verification that the new test fails without the fix and passes with it. - The test is added under the existing
preloaddescribe block and follows the surrounding test patterns. - No outstanding reviewer comments; CI build was triggered.
The previous commit fixed the address (temp_log moved into the guard before set_log), which worked on debug builds. On release, the separate &mut Log parameter to resolve_entry_point_specifier carries noalias, so the optimizer could not see resolve_entry_point's write through transpiler.log into the same allocation, and to_js() still read an empty log. Drop the log parameter; read (*parent).transpiler.log directly after the resolve call so the read and write share one provenance path. Move spin()'s vm_log binding into the None arm so it is not held as &mut across the call either. Move the regression test to its own file to avoid pre-existing 1000ms-timeout flakes in worker.test.ts on slow ASAN runners.
The in-process new Worker() call on the leaksan lane reported the BuildMessage's external-string backing buffer as leaked (it is JS-heap-owned and freed by GC, which does not run before process exit). worker.test.ts is already on the no-validate-leaksan list for the same class of report; spawning a child with bunEnv avoids leak detection without adding another exclusion.
The error path creates a BuildMessage whose to_string_fn hands an external-string backing buffer to JSC; it is freed at GC, which does not run before the subprocess exits. worker.test.ts, worker_blob.test.ts and message-channel.test.ts are already on this list for the same class of JS-heap-lifetime report.
If the child aborts, stdout is empty and JSON.parse throws an
opaque EOF error. Assert the combined { stdout, stderr, exitCode }
object first so the CI failure diff shows the actual diagnostic.
The subprocess wrapper was added to avoid adding a leaksan exclusion, but bunEnv spreads process.env so the child inherited detect_leaks=1 anyway. With the file now on no-validate-leaksan.txt (same class as worker.test.ts), an in-process try/catch has identical LSan behavior and drops ~25 lines of scaffolding.
|
Closing: this branch conflicts with main and was never rebased. #41496 fixes the same root cause on current main, with a preload specific message. |
1 participant
What does this PR do?
Fixes
new Worker(entry, { preload: ["./missing.js"] })throwingTypeError: undefinedinstead of the actual resolve error.Repro:
bun -e 'try { new Worker("./entry.js", { preload: ["./does-not-exist.js"] }); } catch (e) { console.log(e.constructor.name + ":", JSON.stringify(e.message)); }'Before:
TypeError: "undefined"After:
TypeError: "BuildMessage: ModuleNotFound resolving \"./does-not-exist.js\" (entry point)"Cause: two compounding issues in
WebWorker::create/resolve_entry_point_specifier(src/jsc/web_worker.rs):set_log(&raw mut temp_log)stored the stack address oftemp_login the transpiler, andtemp_logwas then moved into ascopeguardtuple. The transpiler's raw*mut Logpointed at moved-from stack.resolve_entry_point_specifiertooklog: &mut Logas a parameter that aliased(*parent).transpiler.log.resolve_entry_pointwrites the error throughtranspiler.log; under release optimization the&mutparameter'snoaliaslets the compiler assumelog.msgsis still empty whenlog.to_js()reads it, so it returnsJSValue::UNDEFINED(LogJsc::to_js, count == 0) andto_bun_string()yields"undefined".The Zig reference (web_worker.zig:288) avoids both:
&temp_logwith no move, and the log is passed as a raw*Log(nonoalias).Fix:
temp_loginto the guard first, then callset_logwith its post-move address.logparameter fromresolve_entry_point_specifier; read(*parent).transpiler.logdirectly after the resolve so the read and write share one provenance path.spin()'svm_logbinding into theNonearm so it is not held as&mutacross the resolve call (same aliasing class).How did you verify your code works?
New test
test/js/web/workers/worker-preload-resolve-error.test.ts: constructs a Worker with a nonexistent preload module and asserts the caught error message is not"undefined"and contains the module specifier.bun bd testandbun run build:release testwith main's src/: fails (message is"undefined").bun bd testandbun run build:release testwith the fix: passes.worker.test.tson release: 23 pass, 1 pre-existing todo.The test lives in its own file because
worker.test.tshas two pre-existing 1000ms-timeout tests ("worker with event listeners doesn't close event loop") that flake on slow debug+ASAN runners independently of this change (also noted in #31951).