diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index 25f447a738..5ec94d96ae 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -87,6 +87,11 @@ using namespace Zig; #define NAPI_PREAMBLE(_env) \ NAPI_LOG_CURRENT_FUNCTION; \ NAPI_CHECK_ARG(_env, _env); \ + /* Check for pending NAPI exceptions before proceeding */ \ + if ((_env)->hasNapiException()) [[unlikely]] { \ + fprintf(stderr, "[DEBUG] NAPI_PREAMBLE: pending exception, returning\n"); \ + return napi_set_last_error(_env, napi_pending_exception); \ + } \ /* You should not use this throw scope directly -- if you need */ \ /* to throw or clear exceptions, make your own scope */ \ auto napi_preamble_throw_scope__ = DECLARE_THROW_SCOPE(_env->vm()); \ @@ -100,6 +105,11 @@ using namespace Zig; #define NAPI_PREAMBLE(_env) \ NAPI_LOG_CURRENT_FUNCTION; \ NAPI_CHECK_ARG(_env, _env); \ + /* Check for pending NAPI exceptions before proceeding */ \ + if ((_env)->hasNapiException()) [[unlikely]] { \ + fprintf(stderr, "[DEBUG] NAPI_PREAMBLE: pending exception, returning\n"); \ + return napi_set_last_error(_env, napi_pending_exception); \ + } \ /* You should not use this throw scope directly -- if you need */ \ /* to throw or clear exceptions, make your own scope */ \ auto napi_preamble_throw_scope__ = DECLARE_THROW_SCOPE(_env->vm()); \ @@ -136,11 +146,16 @@ using namespace Zig; } while (0) // Return an error code if an exception was thrown after NAPI_PREAMBLE -#define NAPI_RETURN_IF_EXCEPTION(_env) RETURN_IF_EXCEPTION(napi_preamble_throw_scope__, napi_set_last_error(_env, napi_pending_exception)) +#define NAPI_RETURN_IF_EXCEPTION(_env) \ + do { \ + /* Check JSC exceptions - NAPI exceptions are already thrown to JSC */ \ + RETURN_IF_EXCEPTION(napi_preamble_throw_scope__, napi_set_last_error(_env, napi_pending_exception)); \ + } while (0) // Return indicating that no error occurred in a NAPI function, and an exception is not expected #define NAPI_RETURN_SUCCESS(_env) \ do { \ + /* NAPI exceptions are already thrown to JSC */ \ napi_preamble_throw_scope__.assertNoException(); \ return napi_set_last_error(_env, napi_ok); \ } while (0) @@ -1003,7 +1018,6 @@ static napi_status throwErrorWithCStrings(napi_env env, const char* code_utf8, c { auto* globalObject = toJS(env); auto& vm = JSC::getVM(globalObject); - auto scope = DECLARE_THROW_SCOPE(vm); if (!msg_utf8) { return napi_set_last_error(env, napi_invalid_arg); @@ -1012,7 +1026,18 @@ static napi_status throwErrorWithCStrings(napi_env env, const char* code_utf8, c WTF::String code = code_utf8 ? WTF::String::fromUTF8(code_utf8) : WTF::String(); WTF::String message = WTF::String::fromUTF8(msg_utf8); + // Check if there's already a pending NAPI exception + if (env->hasNapiException()) { + // Ignore subsequent throws - matches Node.js behavior + return napi_set_last_error(env, napi_ok); + } + auto* error = createErrorWithCode(vm, globalObject, code, message, type); + + // Set the NAPI exception flag and also throw to JSC + env->setNapiException(); + + auto scope = DECLARE_THROW_SCOPE(vm); scope.throwException(globalObject, error); return napi_set_last_error(env, napi_ok); } @@ -1254,9 +1279,10 @@ extern "C" napi_status napi_is_exception_pending(napi_env env, bool* result) NAPI_CHECK_ENV_NOT_IN_GC(env); NAPI_CHECK_ARG(env, result); + // Check both NAPI-level flag and JSC-level exceptions auto globalObject = toJS(env); auto scope = DECLARE_THROW_SCOPE(JSC::getVM(globalObject)); - *result = scope.exception() != nullptr; + *result = env->hasNapiException() || (scope.exception() != nullptr); // skip macros as they assume we made a throw scope in the preamble return napi_set_last_error(env, napi_ok); } @@ -1273,12 +1299,17 @@ extern "C" napi_status napi_get_and_clear_last_exception(napi_env env, auto globalObject = toJS(env); auto scope = DECLARE_CATCH_SCOPE(JSC::getVM(globalObject)); + if (scope.exception()) [[unlikely]] { *result = toNapi(JSValue(scope.exception()->value()), globalObject); + scope.clearException(); + // Clear the NAPI exception flag when clearing JSC exception + env->clearNapiException(); } else { *result = toNapi(JSC::jsUndefined(), globalObject); + // Still clear the flag in case it was set + env->clearNapiException(); } - scope.clearException(); return napi_set_last_error(env, napi_ok); } @@ -1302,13 +1333,25 @@ extern "C" napi_status napi_throw(napi_env env, napi_value error) { NAPI_PREAMBLE_NO_THROW_SCOPE(env); NAPI_CHECK_ARG(env, error); + + // Check if there's already a pending NAPI exception + if (env->hasNapiException()) { + // Ignore subsequent throws - matches Node.js behavior + fprintf(stderr, "[DEBUG] napi_throw: ignoring throw, already have pending exception\n"); + return napi_set_last_error(env, napi_ok); + } + + // Set the NAPI exception flag and also throw to JSC + // This way the exception is available immediately but we track the state + env->setNapiException(); + auto globalObject = toJS(env); - JSC::VM& vm = JSC::getVM(globalObject); - auto throwScope = DECLARE_THROW_SCOPE(vm); - + auto scope = DECLARE_THROW_SCOPE(JSC::getVM(globalObject)); JSValue value = toJS(error); - JSC::throwException(globalObject, throwScope, value); - + + fprintf(stderr, "[DEBUG] napi_throw: throwing exception to JSC\n"); + scope.throwException(globalObject, value); + return napi_set_last_error(env, napi_ok); } diff --git a/src/bun.js/bindings/napi.h b/src/bun.js/bindings/napi.h index 9cbf4ac3a7..de5d3b8042 100644 --- a/src/bun.js/bindings/napi.h +++ b/src/bun.js/bindings/napi.h @@ -228,6 +228,23 @@ public: inline bool isFinishingFinalizers() const { return m_isFinishingFinalizers; } + // NAPI exception management methods (simple flag-based approach) + inline bool hasNapiException() const { + return m_hasPendingNapiException; + } + + inline void setNapiException() { + // Only set the flag if there isn't already a pending exception + // This matches Node.js behavior where subsequent throws are ignored + if (!m_hasPendingNapiException) { + m_hasPendingNapiException = true; + } + } + + inline void clearNapiException() { + m_hasPendingNapiException = false; + } + // Almost all NAPI functions should set error_code to the status they're returning right before // they return it napi_extended_error_info m_lastNapiErrorInfo = { @@ -243,6 +260,10 @@ public: Bun::NapiFinalizer instanceDataFinalizer; char* filename = nullptr; + // Track NAPI-level pending exceptions with a simple flag + // This avoids GC issues with storing JSC values in non-JSC objects + bool m_hasPendingNapiException = false; + struct BoundFinalizer { napi_finalize callback = nullptr; void* hint = nullptr; diff --git a/test/napi/multiple-exceptions.test.js b/test/napi/multiple-exceptions.test.js new file mode 100644 index 0000000000..cffa671e11 --- /dev/null +++ b/test/napi/multiple-exceptions.test.js @@ -0,0 +1,42 @@ +import { test, expect } from "bun:test"; +import { join } from "path"; + +// This test reproduces the bug where multiple ThrowAsJavaScriptException calls +// in the same NAPI function cause Bun to crash with an assertion failure + +const addonPath = join(import.meta.dir, "napi-app", "build", "Debug", "multiple_exceptions_addon.node"); + +// Build the addon first if needed +import { spawnSync } from "child_process"; +const buildResult = spawnSync("node-gyp", ["build"], { + cwd: join(import.meta.dir, "napi-app"), + stdio: "inherit" +}); + +let addon; +try { + addon = require(addonPath); +} catch (error) { + console.warn("Could not load multiple_exceptions_addon.node - skipping tests"); + console.warn("Run 'cd test/napi/napi-app && node-gyp build' to build the addon"); + addon = null; +} + +test.skipIf(!addon)("throwing after catching exception should not crash Bun", () => { + expect(() => { + addon.throwAfterCatch(); + }).toThrow("Second exception after catch"); +}); + +test.skipIf(!addon)("multiple exceptions should not crash - second overwrites first", () => { + expect(() => { + addon.throwMultiple(); + }).toThrow("Second exception"); +}); + +test.skipIf(!addon)("exception pending check should work", () => { + expect(() => { + const result = addon.checkExceptionPending(); + // This should throw before returning the result + }).toThrow("Test exception"); +}); \ No newline at end of file diff --git a/test/napi/napi-app/binding.gyp b/test/napi/napi-app/binding.gyp index 78c9554535..118b605725 100644 --- a/test/napi/napi-app/binding.gyp +++ b/test/napi/napi-app/binding.gyp @@ -121,5 +121,27 @@ "NAPI_DISABLE_CPP_EXCEPTIONS", ], }, + { + "target_name": "multiple_exceptions_addon", + "sources": ["multiple_exceptions_addon.cpp"], + "include_dirs": [" +#include + +// Test function that throws multiple exceptions in sequence +// This should cause Bun to crash with an assertion failure currently +napi_value ThrowAfterCatch(napi_env env, napi_callback_info info) { + // First exception - throw it + napi_value error1; + napi_create_error(env, nullptr, + nullptr, // we'll create the message below + &error1); + + napi_value message1; + napi_create_string_utf8(env, "First exception", NAPI_AUTO_LENGTH, &message1); + napi_set_named_property(env, error1, "message", message1); + + napi_throw(env, error1); + + // Check if exception is pending after first throw + bool pending; + napi_is_exception_pending(env, &pending); + + // Second exception - this should cause assertion failure in Bun + // but should work fine in Node.js (should be ignored since first is pending) + napi_value error2; + napi_create_error(env, nullptr, + nullptr, + &error2); + + napi_value message2; + napi_create_string_utf8(env, "Second exception after first", NAPI_AUTO_LENGTH, &message2); + napi_set_named_property(env, error2, "message", message2); + + napi_throw(env, error2); + + napi_value result; + napi_get_null(env, &result); + return result; +} + +// Test function that throws multiple exceptions without any catching +napi_value ThrowMultiple(napi_env env, napi_callback_info info) { + // First exception + napi_value error1; + napi_create_error(env, nullptr, nullptr, &error1); + + napi_value message1; + napi_create_string_utf8(env, "First exception", NAPI_AUTO_LENGTH, &message1); + napi_set_named_property(env, error1, "message", message1); + napi_throw(env, error1); + + // Second exception - this should be ignored/overwrite the first in Node.js + napi_value error2; + napi_create_error(env, nullptr, nullptr, &error2); + + napi_value message2; + napi_create_string_utf8(env, "Second exception", NAPI_AUTO_LENGTH, &message2); + napi_set_named_property(env, error2, "message", message2); + napi_throw(env, error2); + + napi_value result; + napi_get_null(env, &result); + return result; +} + +// Test function that checks if an exception is pending +napi_value CheckExceptionPending(napi_env env, napi_callback_info info) { + // Throw an exception + napi_value error; + napi_create_error(env, nullptr, nullptr, &error); + + napi_value message; + napi_create_string_utf8(env, "Test exception", NAPI_AUTO_LENGTH, &message); + napi_set_named_property(env, error, "message", message); + napi_throw(env, error); + + // Check if exception is pending + bool isPending; + napi_is_exception_pending(env, &isPending); + + napi_value result; + napi_get_boolean(env, isPending, &result); + return result; +} + +napi_value Init(napi_env env, napi_value exports) { + napi_value fn1, fn2, fn3; + + napi_create_function(env, nullptr, 0, ThrowAfterCatch, nullptr, &fn1); + napi_set_named_property(env, exports, "throwAfterCatch", fn1); + + napi_create_function(env, nullptr, 0, ThrowMultiple, nullptr, &fn2); + napi_set_named_property(env, exports, "throwMultiple", fn2); + + napi_create_function(env, nullptr, 0, CheckExceptionPending, nullptr, &fn3); + napi_set_named_property(env, exports, "checkExceptionPending", fn3); + + return exports; +} + +NAPI_MODULE(multiple_exceptions_addon, Init) \ No newline at end of file diff --git a/test/napi/napi-app/simple_test_addon.c b/test/napi/napi-app/simple_test_addon.c new file mode 100644 index 0000000000..a0342a2c6d --- /dev/null +++ b/test/napi/napi-app/simple_test_addon.c @@ -0,0 +1,33 @@ +#include + +// Simple test function that throws two exceptions in a row +napi_value test_double_throw(napi_env env, napi_callback_info info) { + // First throw + napi_value error1; + napi_create_error(env, NULL, NULL, &error1); + napi_value message1; + napi_create_string_utf8(env, "First error", NAPI_AUTO_LENGTH, &message1); + napi_set_named_property(env, error1, "message", message1); + napi_throw(env, error1); + + // Second throw - this should be ignored in Node.js but crashes in Bun + napi_value error2; + napi_create_error(env, NULL, NULL, &error2); + napi_value message2; + napi_create_string_utf8(env, "Second error", NAPI_AUTO_LENGTH, &message2); + napi_set_named_property(env, error2, "message", message2); + napi_throw(env, error2); + + napi_value result; + napi_get_null(env, &result); + return result; +} + +napi_value Init(napi_env env, napi_value exports) { + napi_value fn; + napi_create_function(env, NULL, 0, test_double_throw, NULL, &fn); + napi_set_named_property(env, exports, "testDoubleThrow", fn); + return exports; +} + +NAPI_MODULE(simple_test_addon, Init) \ No newline at end of file diff --git a/test/napi/simple-c-test.js b/test/napi/simple-c-test.js new file mode 100644 index 0000000000..58b2919c4c --- /dev/null +++ b/test/napi/simple-c-test.js @@ -0,0 +1,22 @@ +import { test, expect } from "bun:test"; +import { join } from "path"; + +const addonPath = join(import.meta.dir, "napi-app", "build", "Debug", "simple_test_addon.node"); + +let addon; +try { + addon = require(addonPath); + console.log("Simple C addon loaded successfully"); +} catch (error) { + console.warn("Could not load addon:", error.message); + addon = null; +} + +// Test the simple C function that throws two exceptions +test.skipIf(!addon)("C API double throw test", () => { + console.log("About to call testDoubleThrow..."); + expect(() => { + const result = addon.testDoubleThrow(); + console.log("testDoubleThrow returned:", result); + }).toThrow(); +}); \ No newline at end of file diff --git a/test/napi/simple-double-throw.test.js b/test/napi/simple-double-throw.test.js new file mode 100644 index 0000000000..108a98b0e0 --- /dev/null +++ b/test/napi/simple-double-throw.test.js @@ -0,0 +1,22 @@ +import { test, expect } from "bun:test"; +import { join } from "path"; + +const addonPath = join(import.meta.dir, "napi-app", "build", "Debug", "multiple_exceptions_addon.node"); + +let addon; +try { + addon = require(addonPath); + console.log("Addon loaded successfully"); +} catch (error) { + console.warn("Could not load addon:", error.message); + addon = null; +} + +// Test just one simple function at a time +test.skipIf(!addon)("simple exception test", () => { + console.log("About to call throwMultiple..."); + expect(() => { + const result = addon.throwMultiple(); + console.log("throwMultiple returned:", result); + }).toThrow(); +}); \ No newline at end of file