Skip to content
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/jsc/VM.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ impl VM {
crate::cpp::JSC__VM__clearHasTerminationRequest(self)
}

pub fn has_termination_request(&self) -> bool {
crate::cpp::JSC__VM__hasTerminationRequest(self)
}

#[track_caller]
pub fn throw_error(&self, global_object: &JSGlobalObject, value: JSValue) -> JsError {
crate::validation_scope!(scope, global_object);
Expand Down
6 changes: 3 additions & 3 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1074,7 +1074,7 @@ impl VirtualMachine {
}

pub fn handled_promise(&self, global_object: &JSGlobalObject, promise: JSValue) -> bool {
if self.is_shutting_down() {
if self.script_execution_status() != crate::ScriptExecutionStatus::Running {
return true;
}
Bun__emitHandledPromiseEvent(global_object, promise)
Expand Down Expand Up @@ -1348,7 +1348,7 @@ impl VirtualMachine {
err: JSValue,
is_rejection: bool,
) -> bool {
if self.is_shutting_down() {
if self.script_execution_status() != crate::ScriptExecutionStatus::Running {
return true;
}

Expand Down Expand Up @@ -3287,7 +3287,7 @@ impl VirtualMachine {
) {
use bun_options_types::schema::api::UnhandledRejections as Mode;

if self.is_shutting_down() {
if self.script_execution_status() != crate::ScriptExecutionStatus::Running {
Comment thread
robobun marked this conversation as resolved.
bun_core::debug_warn!("unhandledRejection during shutdown.");
return;
}
Expand Down
11 changes: 10 additions & 1 deletion src/jsc/bindings/BunProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1212,9 +1212,14 @@ extern "C" int Bun__handleUncaughtException(JSC::JSGlobalObject* lexicalGlobalOb
if (!lexicalGlobalObject->inherits(Zig::GlobalObject::info()))
return false;
auto* globalObject = uncheckedDowncast<Zig::GlobalObject>(lexicalGlobalObject);
auto& vm = JSC::getVM(globalObject);

// Stopped == a worker with has_requested_terminate(); the process->get / emit / call walk below asserts under a terminate() race.
if (Zig::GlobalObject::scriptExecutionStatus(globalObject, globalObject) != JSC::ScriptExecutionStatus::Running) [[unlikely]]
return true;
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

auto* process = globalObject->processObject();
auto& wrapped = process->wrapped();
auto& vm = JSC::getVM(globalObject);

// node parity (exitWithUndefinedFatalException): the internal fatal-exception
// handler is monkey-patchable as process._fatalException. If user code
Expand Down Expand Up @@ -1346,6 +1351,8 @@ extern "C" int Bun__handleUnhandledRejection(JSC::JSGlobalObject* lexicalGlobalO
if (!lexicalGlobalObject->inherits(Zig::GlobalObject::info()))
return false;
auto* globalObject = uncheckedDowncast<Zig::GlobalObject>(lexicalGlobalObject);
if (Zig::GlobalObject::scriptExecutionStatus(globalObject, globalObject) != JSC::ScriptExecutionStatus::Running) [[unlikely]]
return false;
Comment thread
claude[bot] marked this conversation as resolved.
auto* process = globalObject->processObject();

auto eventType = Identifier::fromString(JSC::getVM(globalObject), "unhandledRejection"_s);
Expand All @@ -1369,6 +1376,8 @@ extern "C" bool Bun__emitHandledPromiseEvent(JSC::JSGlobalObject* lexicalGlobalO
if (!lexicalGlobalObject->inherits(Zig::GlobalObject::info()))
return false;
auto* globalObject = uncheckedDowncast<Zig::GlobalObject>(lexicalGlobalObject);
if (Zig::GlobalObject::scriptExecutionStatus(globalObject, globalObject) != JSC::ScriptExecutionStatus::Running) [[unlikely]]
return false;
auto* process = globalObject->processObject();

auto eventType = Identifier::fromString(JSC::getVM(globalObject), "rejectionHandled"_s);
Expand Down
43 changes: 29 additions & 14 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,11 @@
}
}

// terminate() may have landed during entrySettled / the status block above; skip dispatchOnline/tick().
if self.has_requested_terminate() && !self.exit_called.load(Ordering::Relaxed) {
return self.shutdown();
}

self.flush_logs(vm);
log!("[{}] event loop start", self.execution_context_id);
// dispatchOnline fires the parent-side 'open' event and flips the C++
Expand Down Expand Up @@ -1459,7 +1464,28 @@
if vm_log.msgs.is_empty() {
return;
}
// Keyed on the JSC request so the same-thread self-signals that only set the atomic
// (start_vm()'s configure_defines failure: "vm.log carries the error for flushLogs")
// still dispatch.
Comment thread
robobun marked this conversation as resolved.
Outdated
if self.has_requested_terminate() && vm.jsc_vm().has_termination_request() {
return;
}
let global = vm.global();
let report_thrown = |e: JsError| {
if self.has_requested_terminate() {
return;
}

Check warning on line 1477 in src/jsc/web_worker.rs

View check run for this annotation

Claude / Claude Code Review

report_thrown's terminate check is less precise than flush_logs' outer guard

The `report_thrown` closure gates on `self.has_requested_terminate()` alone, while the outer guard at :1470 was deliberately narrowed to `has_requested_terminate() && vm.jsc_vm().has_termination_request()` so the configure_defines self-signal (which sets only the Rust atomic, not the JSC trap) still dispatches. In that path the atomic is already true, so a genuine `JsError::Thrown` from `to_js`/`to_bun_string`/`dispatchError` would be silently dropped without `take_exception`. Consider matching
Comment thread
robobun marked this conversation as resolved.
// `take_exception` on a `JsError` always returns an Exception
// cell; None is unreachable. Do not silently drop the error.
Comment thread
robobun marked this conversation as resolved.
Outdated
let exc = global
.take_exception(e)
.as_exception(global.vm().as_mut_ptr())
.expect("takeException returned non-Exception");
let _ = jsc::js_global_object::report_uncaught_exception(
global,
jsc::Exception::opaque_ref(exc),
);
};
let result: jsc::JsResult<(JSValue, BunString)> = (|| {
let err = vm_log.to_js(global, "Error in worker")?;
let str = err.to_bun_string(global)?;
Expand All @@ -1468,27 +1494,16 @@
let (err, str) = match result {
Ok(pair) => pair,
Err(JsError::OutOfMemory) => bun_core::out_of_memory(),
Err(JsError::Thrown | JsError::Terminated) => panic!("unhandled exception"),
Err(JsError::Terminated) => return,
Err(e @ JsError::Thrown) => return report_thrown(e),
};
let mut str = bun_core::OwnedString::new(str);
let dispatch = jsc::host_fn::from_js_host_call_generic(global, || {
// `cpp_worker` is the opaque C++-owned handle; `str` reffed for the call.
WebWorker__dispatchError(global, self.cpp_worker, &mut str, err)
});
if let Err(e) = dispatch {
// `take_exception` on a `JsError` always returns an Exception
// cell; None is unreachable. Do not silently drop the error.
let exc = global
.take_exception(e)
.as_exception(global.vm().as_mut_ptr())
.expect("takeException returned non-Exception");
// `Exception` is an `opaque_ffi!` ZST handle; `opaque_ref` is the
// centralised non-null-ZST deref proof (`exc` is non-null per the
// `expect` above).
let _ = jsc::js_global_object::report_uncaught_exception(
global,
jsc::Exception::opaque_ref(exc),
);
report_thrown(e);
}
}
}
Expand Down
49 changes: 49 additions & 0 deletions test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,55 @@ test(
timeout,
);

// Regression: a worker whose module fails to load reports its uncaught
// MODULE_NOT_FOUND via flush_logs → report_uncaught_exception →
// Bun__handleUncaughtException. When worker.terminate() + process.exit() land
// mid-dispatch, flush_logs panicked on JsError::Terminated, and the handler
// would still lazily create `process` and do `process->get("_fatalException")`
// on a VM already asked to terminate, which tripped
// ASSERT(object->structure() == this) in Structure::storedPrototype. The race
// is non-deterministic (~2/14 on release-asan-cov), so loop it.
test.skipIf(!isASAN)(
"terminate() + process.exit() while workers are reporting load errors does not crash",
async () => {
// Each subprocess run spawns workers whose module load fails, then main
// terminates them and exits. The race window is between a worker's error
// dispatch and terminate_all_and_wait arming TerminationException. Two
// levels only (no middle worker) to avoid also tripping the unrelated
// nested-worker parent-VM UAF that #31951 addresses.
const code = `
const { Worker } = require("node:worker_threads");
const bad = require("node:path").join(process.cwd(), "does-not-exist-xyzzy.mjs");
const workers = [];
for (let j = 0; j < 4; j++) {
const w = new Worker(bad);
w.on("error", () => {});
workers.push(w);
}
for (const w of workers) w.terminate();
console.log("ok");
process.exit(0);
`;

for (let i = 0; i < 20; i++) {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stderr, stdout, exitCode, signalCode: proc.signalCode }).toEqual({
stderr: "",
stdout: "ok\n",
exitCode: 0,
signalCode: null,
});
}
},
timeout * 2,
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Regression: the per-VM c-ares channel was destroyed in deinit_runtime_state
// (RuntimeState drop) AFTER JSC teardown and RareData.file_polls drop.
// ares_destroy() synchronously fires EDESTRUCTION query callbacks and socket-
Expand Down
Loading