From e98cee25e9249dde2f93a06019774b7e7cd153f7 Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Fri, 10 Oct 2025 11:53:35 +0000 Subject: [PATCH] Fix Worker crash when async onmessage handler throws MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../bindings/webcore/JSEventListener.cpp | 34 +++++++--- test/regression/issue/20911.test.ts | 66 +++++++++++++++++++ 2 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 test/regression/issue/20911.test.ts diff --git a/src/bun.js/bindings/webcore/JSEventListener.cpp b/src/bun.js/bindings/webcore/JSEventListener.cpp index fe36e1dfef..b748a5ca80 100644 --- a/src/bun.js/bindings/webcore/JSEventListener.cpp +++ b/src/bun.js/bindings/webcore/JSEventListener.cpp @@ -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(scriptExecutionContext)) { - // auto* scriptController = downcast(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; diff --git a/test/regression/issue/20911.test.ts b/test/regression/issue/20911.test.ts new file mode 100644 index 0000000000..80098a298d --- /dev/null +++ b/test/regression/issue/20911.test.ts @@ -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 () => { ... });