From 2701f02961435a210b1acec2aa3094a0820c0b68 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 6 Jul 2026 02:04:55 +0000 Subject: [PATCH] Propagate errors thrown by a runtime plugin's onResolve callback An async onResolve whose promise was already rejected had its rejection swallowed: OnResolve::run flipped the promise's status to Fulfilled and returned the rejection reason as the result object. With no `path` property on it, resolution fell through to the default resolver, so the importer saw a generic "Cannot find package" ResolveMessage. Worse, setFlags() replaces the entire flags word rather than setting a bit, so isHandled was cleared and the plugin's error went on to fire as a process-level unhandledRejection. Mark the promise as handled and throw its rejection reason instead, the same way ModuleLoader.cpp handles a rejected virtual-module promise. A still-pending promise is marked handled too, since nothing will ever observe it once the pending-promise TypeError is thrown. That exposed a crash underneath. moduleLoaderResolve left ErrorableString uninitialized and, on failure, threw res.result.err unconditionally. The resolve hook returns false without writing res whenever a JS exception is already pending, which is exactly what a throwing onResolve does, so the wild value read off the stack segfaulted. Zero-initialize res and check for a pending exception first, as moduleLoaderImportModule already does. --- src/jsc/bindings/BunPlugin.cpp | 8 +- src/jsc/bindings/ZigGlobalObject.cpp | 28 ++--- test/js/bun/plugin/plugins.test.ts | 167 +++++++++++++++++++++++++++ test/regression/issue/22199.test.ts | 6 +- 4 files changed, 192 insertions(+), 17 deletions(-) diff --git a/src/jsc/bindings/BunPlugin.cpp b/src/jsc/bindings/BunPlugin.cpp index 4e16d5246f37..efd4ec90a6b9 100644 --- a/src/jsc/bindings/BunPlugin.cpp +++ b/src/jsc/bindings/BunPlugin.cpp @@ -866,13 +866,15 @@ EncodedJSValue BunPlugin::OnResolve::run(JSC::JSGlobalObject* globalObject, BunS if (auto* promise = dynamicDowncast(result)) { switch (promise->status()) { case JSPromise::Status::Pending: { + // Discarded here, so a later rejection must not surface as unhandled. + promise->markAsHandled(); JSC::throwTypeError(globalObject, scope, "onResolve() doesn't support pending promises yet"_s); return {}; } case JSPromise::Status::Rejected: { - promise->setFlags(static_cast(JSC::JSPromise::Status::Fulfilled)); - result = promise->result(); - return JSValue::encode(result); + promise->markAsHandled(); + JSC::throwException(globalObject, scope, promise->result()); + return {}; } case JSPromise::Status::Fulfilled: { result = promise->result(); diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index b5252a9fbe43..57129a0cf8dd 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -3626,7 +3626,7 @@ JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject Zig::GlobalObject* globalObject = static_cast(jsGlobalObject); ErrorableString res; - res.success = false; + memset(&res, 0, sizeof(res)); BunString keyZ; if (key.isString()) { @@ -3682,26 +3682,28 @@ JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject } BunString queryString = { BunStringTag::Empty, nullptr }; + auto& vm = globalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); Zig__GlobalObject__resolve(&res, globalObject, &keyZ, &referrerZ, &queryString); keyZ.deref(); referrerZ.deref(); - if (res.success) { - if (!queryString.isEmpty()) { - auto result = JSC::Identifier::fromString(globalObject->vm(), makeString(res.result.value.toWTFString(BunString::ZeroCopy), queryString.toWTFString(BunString::ZeroCopy))); - res.result.value.deref(); - queryString.deref(); - return result; - } + // The resolve hook leaves `res` unwritten when it throws (e.g. from an onResolve plugin). + if (!res.success && !scope.exception()) [[unlikely]] { + throwException(scope, res.result.err, globalObject); + } + RETURN_IF_EXCEPTION(scope, vm.propertyNames->emptyIdentifier); - auto result = Identifier::fromString(globalObject->vm(), res.result.value.toWTFString(BunString::ZeroCopy)); + if (!queryString.isEmpty()) { + auto result = JSC::Identifier::fromString(vm, makeString(res.result.value.toWTFString(BunString::ZeroCopy), queryString.toWTFString(BunString::ZeroCopy))); res.result.value.deref(); + queryString.deref(); return result; - } else { - auto scope = DECLARE_THROW_SCOPE(globalObject->vm()); - throwException(scope, res.result.err, globalObject); - return globalObject->vm().propertyNames->emptyIdentifier; } + + auto result = Identifier::fromString(vm, res.result.value.toWTFString(BunString::ZeroCopy)); + res.result.value.deref(); + return result; } JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalObject, diff --git a/test/js/bun/plugin/plugins.test.ts b/test/js/bun/plugin/plugins.test.ts index 5aea1966b6c0..ec7a74dea3de 100644 --- a/test/js/bun/plugin/plugins.test.ts +++ b/test/js/bun/plugin/plugins.test.ts @@ -936,3 +936,170 @@ describe.concurrent("Bun.plugin.clearAll()", () => { }); }); }); + +describe.concurrent("onResolve failures", () => { + // Prints `{ ...result, unhandled }` once the event loop is drained, so an + // unhandled rejection queued by the plugin has already been reported. + const reporter = ` + const unhandled = []; + let result = null; + process.on("unhandledRejection", error => unhandled.push(error?.message ?? String(error))); + process.on("exit", () => console.log(JSON.stringify({ ...result, unhandled }))); + `; + + async function report(source: string) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", reporter + source], + stdout: "pipe", + stderr: "pipe", + env: bunEnv, + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + try { + return { report: JSON.parse(stdout), exitCode }; + } catch { + throw new Error(`expected JSON on stdout, got:\n${stdout}\n--- stderr ---\n${stderr}`); + } + } + + it("rejects the import with the error an async onResolve threw", async () => { + expect( + await report(` + Bun.plugin({ + name: "rejecting resolver", + setup(builder) { + builder.onResolve({ filter: /^boom$/, namespace: "asyncthrow" }, async () => { + throw new Error("config missing"); + }); + }, + }); + const error = await import("asyncthrow:boom").catch(error => error); + result = { name: error.name, message: error.message }; + `), + ).toEqual({ + report: { name: "Error", message: "config missing", unhandled: [] }, + exitCode: 0, + }); + }); + + it("rejects the import with a non-Error thrown by an async onResolve", async () => { + expect( + await report(` + Bun.plugin({ + name: "rejecting resolver", + setup(builder) { + builder.onResolve({ filter: /^boom$/, namespace: "asyncthrowstring" }, async () => { + throw "config missing"; + }); + }, + }); + const error = await import("asyncthrowstring:boom").catch(error => error); + result = { thrown: error }; + `), + ).toEqual({ + report: { thrown: "config missing", unhandled: [] }, + exitCode: 0, + }); + }); + + it("throws the error an async onResolve threw out of require()", async () => { + expect( + await report(` + Bun.plugin({ + name: "rejecting resolver", + setup(builder) { + builder.onResolve({ filter: /^boom$/, namespace: "asyncthrowrequire" }, async () => { + throw new Error("config missing"); + }); + }, + }); + try { + require("asyncthrowrequire:boom"); + result = { name: "(nothing was thrown)", message: "(nothing was thrown)" }; + } catch (error) { + result = { name: error.name, message: error.message }; + } + `), + ).toEqual({ + report: { name: "Error", message: "config missing", unhandled: [] }, + exitCode: 0, + }); + }); + + it("throws the error a sync onResolve threw", async () => { + expect( + await report(` + Bun.plugin({ + name: "throwing resolver", + setup(builder) { + builder.onResolve({ filter: /^boom$/, namespace: "syncthrow" }, () => { + throw new Error("config missing"); + }); + }, + }); + const error = await import("syncthrow:boom").catch(error => error); + result = { name: error.name, message: error.message }; + `), + ).toEqual({ + report: { name: "Error", message: "config missing", unhandled: [] }, + exitCode: 0, + }); + }); + + it("fails the entry point when onResolve throws while resolving it", async () => { + using dir = tempDir("plugin-entry-throw", { + "plugin.js": ` + Bun.plugin({ + name: "throwing resolver", + setup(builder) { + builder.onResolve({ filter: /entry\\.js$/ }, () => { + throw new Error("config missing"); + }); + }, + }); + `, + "entry.js": `console.log("the entry point ran");`, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "--preload", "./plugin.js", "./entry.js"], + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + env: bunEnv, + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toContain("config missing"); + expect(stdout).toBe(""); + expect(exitCode).toBe(1); + }); + + it("does not leak the rejection of an onResolve promise that settles too late", async () => { + expect( + await report(` + Bun.plugin({ + name: "pending resolver", + setup(builder) { + builder.onResolve({ filter: /^boom$/, namespace: "pendingthrow" }, async () => { + await Bun.sleep(1); + throw new Error("config missing"); + }); + }, + }); + const error = await import("pendingthrow:boom").catch(error => error); + result = { name: error.name, message: error.message }; + `), + ).toEqual({ + report: { + name: "TypeError", + message: "onResolve() doesn't support pending promises yet", + unhandled: [], + }, + exitCode: 0, + }); + }); +}); diff --git a/test/regression/issue/22199.test.ts b/test/regression/issue/22199.test.ts index b774b8b7563f..7c4f9d1ff1b3 100644 --- a/test/regression/issue/22199.test.ts +++ b/test/regression/issue/22199.test.ts @@ -101,9 +101,13 @@ test("plugin onResolve with rejected promise should throw error", () => { cmd: [bunExe(), "--preload", "./plugin.js", "./index.js"], env: bunEnv, cwd: String(dir), + stdout: "pipe", stderr: "pipe", }); - expect(result.exitCode).toBe(1); expect(result.stderr.toString()).toContain("Custom plugin error"); + // The error has to fail the resolution, not arrive later as an unhandled + // rejection while index.js runs anyway. + expect(result.stdout.toString()).toBe(""); + expect(result.exitCode).toBe(1); });