Fix Worker crash when async onmessage handler throws

Fixes #20911

When an async error was thrown in a Worker's onmessage handler, Bun
would crash with "ASSERTION FAILED: !vm().entryScope" instead of
properly handling the error and keeping the process alive.

The issue was introduced in v1.2.14 when promise rejection handling
was added to JSEventListener. When an async function throws, it returns
a rejected promise. The code would attempt to attach a rejection handler
via `.then()`, which could cause VM re-entrancy issues in workers.

This fix:

1. Adds termination exception checks throughout JSEventListener to
   properly handle VM termination (uncomments and adapts the WebKit
   termination handling code)

2. Skips promise rejection handling entirely for WorkerGlobalScope
   contexts, as workers handle unhandled rejections through their
   own error event mechanism (worker.onerror)

3. Adds VM termination request checks after calling `.then()` to
   ensure we exit cleanly if termination was requested during the call

The error is still properly reported through worker.onerror, but we
avoid the VM state corruption that caused crashes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Claude Bot
2025-10-10 11:53:35 +00:00
parent 012a2bab92
commit e98cee25e9
2 changed files with 92 additions and 8 deletions

View File

@@ -206,6 +206,9 @@ void JSEventListener::handleEvent(ScriptExecutionContext& scriptExecutionContext
if (scope.exception()) [[unlikely]] {
auto* exception = scope.exception();
scope.clearException();
// Don't report termination exceptions
if (vm.isTerminationException(exception))
return;
event.target()->uncaughtExceptionInEventHandler();
reportException(lexicalGlobalObject, exception);
return;
@@ -233,14 +236,15 @@ void JSEventListener::handleEvent(ScriptExecutionContext& scriptExecutionContext
// InspectorInstrumentation::didCallFunction(&scriptExecutionContext);
auto handleExceptionIfNeeded = [&](JSC::Exception* exception) -> bool {
// if (is<WorkerGlobalScope>(scriptExecutionContext)) {
// auto* scriptController = downcast<WorkerGlobalScope>(scriptExecutionContext).script();
// bool terminatorCausedException = (exception && vm.isTerminationException(exception));
// if (terminatorCausedException || (scriptController && scriptController->isTerminatingExecution()))
// scriptController->forbidExecution();
// }
if (exception) {
// Check if this is a termination exception (e.g., from process.exit() or worker shutdown)
// Termination exceptions should not be reported as they are expected
bool isTermination = vm.isTerminationException(exception);
if (isTermination) {
// For termination exceptions, just return without reporting
return true;
}
event.target()->uncaughtExceptionInEventHandler();
reportException(lexicalGlobalObject, exception);
return true;
@@ -253,11 +257,16 @@ void JSEventListener::handleEvent(ScriptExecutionContext& scriptExecutionContext
// Node handles promises in the return value and throws an uncaught exception on nextTick if it rejects.
// See event_target.js function addCatch in node
if (retval.isObject()) {
// Skip promise handling for workers to avoid VM entry scope issues when async errors occur (fixes #20911)
// Workers handle unhandled rejections through their own error event mechanism
if (retval.isObject() && !scriptExecutionContext.isWorkerGlobalScope()) {
auto then = retval.get(lexicalGlobalObject, vm.propertyNames->then);
if (scope.exception()) [[unlikely]] {
auto* exception = scope.exception();
scope.clearException();
// Don't report termination exceptions
if (vm.isTerminationException(exception))
return;
event.target()->uncaughtExceptionInEventHandler();
reportException(lexicalGlobalObject, exception);
return;
@@ -267,9 +276,18 @@ void JSEventListener::handleEvent(ScriptExecutionContext& scriptExecutionContext
arglist.append(JSValue(JSC::jsUndefined()));
arglist.append(JSValue(JSC::JSFunction::create(vm, lexicalGlobalObject, 1, String(), jsFunctionEmitUncaughtExceptionNextTick, ImplementationVisibility::Public, NoIntrinsic))); // err => process.nextTick(() => throw err)
JSC::call(lexicalGlobalObject, then, retval, arglist, "Promise.then is not callable"_s);
// Check if VM termination was requested during the call
if (vm.hasTerminationRequest()) {
return;
}
if (scope.exception()) [[unlikely]] {
auto* exception = scope.exception();
scope.clearException();
// Don't report termination exceptions
if (vm.isTerminationException(exception))
return;
event.target()->uncaughtExceptionInEventHandler();
reportException(lexicalGlobalObject, exception);
return;

View File

@@ -0,0 +1,66 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";
// https://github.com/oven-sh/bun/issues/20911
test("Worker async onmessage error should not crash process", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const blob = new Blob(
[
\`
self.onmessage = async () => {
throw new Error('pong')
}
\`,
],
{
type: 'application/typescript',
},
)
const url = URL.createObjectURL(blob)
const worker = new Worker(url)
worker.onerror = (error) => console.error(error)
worker.postMessage('ping')
// keep alive
setInterval(() => {}, 1000)
`,
],
env: bunEnv,
stdin: "ignore",
stdout: "pipe",
stderr: "pipe",
});
// Race: either the process exits (crashes) or we see the ErrorEvent output
const errorEventPromise = (async () => {
const stderr = await proc.stderr.text();
return stderr.includes("ErrorEvent");
})();
const result = await Promise.race([
proc.exited.then(code => ({ type: "exited" as const, code })),
errorEventPromise.then(() => ({ type: "errorEvent" as const })),
Bun.sleep(2000).then(() => ({ type: "timeout" as const })),
]);
// If process exited before we saw ErrorEvent, it crashed
if (result.type === "exited") {
expect(result.code).not.toBe(134); // 134 = SIGABRT (abort/crash)
expect(result.code).not.toBe(139); // 139 = SIGSEGV (segfault)
}
// Terminate the process if it's still running
proc.kill();
const exitCode = await proc.exited;
// Process should exit cleanly when killed (SIGTERM or SIGKILL), not crash with SIGABRT
expect(exitCode).not.toBe(134); // 134 = SIGABRT (abort/crash)
expect(exitCode).not.toBe(139); // 139 = SIGSEGV (segfault)
});
// TODO: Sync handler errors also crash workers, but that's a separate issue
// test("Worker sync onmessage error should work as before", async () => { ... });