diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index 8fef5dcfa30..9661b91616c 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -73,6 +73,7 @@ #include "wtf/text/ASCIIFastPath.h" #include "JavaScriptCore/WeakInlines.h" #include +#include #include #include "AsyncContextFrame.h" @@ -1067,12 +1068,22 @@ static napi_status throwErrorWithCStrings(napi_env env, const char* code_utf8, c // code must be a string or nullptr (no code) // msg must be a string // never calls toString, never throws +// +// Intentionally does NOT check for pending VM exceptions. In Node.js, +// napi_create_error is a pure value-producing function that runs fine +// with an exception pending. Bun previously rejected with +// napi_pending_exception here, which broke node-addon-api's +// `Error::New(env)` helper during env cleanup: that helper first calls +// napi_is_exception_pending (which Bun deliberately skips the VM check +// for during cleanup, so it reports "no pending exception"), then falls +// through to napi_create_error. When a prior finalizer left a VM +// exception on the scope, this mismatch triggered +// NAPI_FATAL_IF_FAILED -> napi_fatal_error -> panic. See #30286 and +// #22259. static napi_status createErrorWithNapiValues(napi_env env, napi_value code, napi_value message, JSC::ErrorType type, napi_value* result) { auto* globalObject = toJS(env); auto& vm = JSC::getVM(globalObject); - auto scope = DECLARE_THROW_SCOPE(vm); - RETURN_IF_EXCEPTION(scope, napi_pending_exception); NAPI_CHECK_ARG(env, result); NAPI_CHECK_ARG(env, message); @@ -1082,15 +1093,15 @@ static napi_status createErrorWithNapiValues(napi_env env, napi_value code, napi js_message.isString() && (js_code.isEmpty() || js_code.isString()), napi_string_expected); + // getString() on a verified string only throws on rope-resolution OOM, + // which we intentionally ignore here so a pre-existing VM exception + // doesn't make napi_create_error fail (#30286, #22259). auto wtf_code = js_code.isEmpty() ? WTF::String() : js_code.getString(globalObject); - RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); auto wtf_message = js_message.getString(globalObject); - RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); *result = toNapi( createErrorWithCode(vm, globalObject, wtf_code, wtf_message, type), globalObject); - RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); return napi_set_last_error(env, napi_ok); } @@ -3044,3 +3055,16 @@ extern "C" void NapiEnv__deref(napi_env env) } } + +// Defined out-of-line so its uses of DECLARE_TOP_EXCEPTION_SCOPE (whose +// ctor/dtor are JS_EXPORT_PRIVATE when ENABLE_EXCEPTION_SCOPE_VERIFICATION +// is on) are confined to a single TU instead of inlined into every +// translation unit that includes napi.h. +void NapiEnv::clearExceptionsBetweenFinalizers() +{ + // VM::clearException (via TopExceptionScope::clearException) also + // resets m_needExceptionCheck bookkeeping, so a leaked exception + // from one finalizer does not trip debug asserts in the next. + DECLARE_TOP_EXCEPTION_SCOPE(m_vm).clearException(); + m_pendingException.clear(); +} diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index aa305eae35c..df76e206465 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -194,6 +194,19 @@ struct NapiEnv : public WTF::RefCounted { void cleanup() { + // The VM can already have a pending exception when cleanup starts: + // a Worker torn down via terminate() has JSC's TerminationException + // set (termination is trap-based, so clearing the exception object + // does not cancel the termination request -- re-entering JS + // re-throws it). Cleanup hooks and finalizers are native callbacks + // with no JS frame above them to catch anything, so, matching + // Node.js, each starts from a clean exception state. Without this, + // the first napi call in the first callback that checks the VM + // (e.g. napi_create_string_utf8 in a node-addon-api ObjectWrap + // finalizer) fails with napi_pending_exception and the addon's + // error path escalates to napi_fatal_error. See #30286. + clearExceptionsBetweenFinalizers(); + while (!m_cleanupHooks.empty()) { drain(); } @@ -203,18 +216,30 @@ struct NapiEnv : public WTF::RefCounted { JSC::DeferGCForAWhile deferGC(m_vm); m_isFinishingFinalizers = true; + // A cleanup hook may itself have leaked an exception; the first + // finalizer starts clean too. + clearExceptionsBetweenFinalizers(); // Reverse insertion order so children are torn down before parents (Node.js LIFO). // ListHashSet iteration is safe against concurrent inserts, and m_isFinishingFinalizers // routes all removals to active=false, so the only unsafe op (erase-current) can't occur. for (auto it = m_finalizers.rbegin(); it != m_finalizers.rend(); ++it) { Bun::NapiHandleScope handle_scope(m_globalObject); it->call(this); + // Each finalizer starts from a clean exception state: Node.js + // never propagates one finalizer's throw into the next (there + // is no JS frame to catch in between). Leaving a pending + // exception also breaks later finalizers in subtle ways -- + // napi_is_exception_pending skips the VM check during cleanup + // for safety, so user code thinks there is no exception, but + // the next napi call with a throw scope sees it. See #30286. + clearExceptionsBetweenFinalizers(); } m_finalizers.clear(); m_isFinishingFinalizers = false; instanceDataFinalizer.call(this, instanceData, true); instanceDataFinalizer.clear(); + clearExceptionsBetweenFinalizers(); } void removeFinalizer(napi_finalize callback, void* hint, void* data) @@ -502,6 +527,13 @@ struct NapiEnv : public WTF::RefCounted { JSC::Strong m_pendingException; size_t m_cleanupHookCounter = 0; + // Drop any pending exception -- VM-scope or env-scope -- between + // finalizers run from cleanup(). Used by cleanup() only. Defined + // out-of-line in napi.cpp so its uses of JSC::TopExceptionScope + // (which has JS_EXPORT_PRIVATE ctor/dtor under + // ENABLE_EXCEPTION_SCOPE_VERIFICATION) are confined to one TU. + void clearExceptionsBetweenFinalizers(); + // Returns a vector of hooks in reverse order of insertion. std::vector getHooks() const { @@ -533,6 +565,9 @@ struct NapiEnv : public WTF::RefCounted { async.function(async.handle, async.data); delete async.handle; } + // Same invariant as the finalizer loop in cleanup(): a hook + // that leaked an exception must not poison the next hook. + clearExceptionsBetweenFinalizers(); } } }; diff --git a/test/napi/napi-app/binding.gyp b/test/napi/napi-app/binding.gyp index 4cc20a080b5..2a5afa5dca1 100644 --- a/test/napi/napi-app/binding.gyp +++ b/test/napi/napi-app/binding.gyp @@ -274,5 +274,16 @@ "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1", ], }, + { + "target_name": "test_finalizer_create_error", + "sources": ["test_finalizer_create_error.c"], + "include_dirs": [" napi_fatal_error -> the reporter's panic: +// "NAPI FATAL ERROR: Error::New napi_create_error" +// +// Root cause (two layers): +// - NapiEnv::cleanup() did not clear pending exceptions between +// finalizers. A throw from one finalizer bled into the next -- even +// though finalizers run without a JS frame that could catch, so +// there is no point propagating. +// - Separately, createErrorWithNapiValues had its own +// DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION at entry that returned +// napi_pending_exception whenever a VM exception was live. Node.js +// makes napi_create_error a pure value-producing call, so this was +// a compatibility bug on its own (#22259). +// +// Reproduces the sequence without tree-sitter: +// - Wrap two JS objects. Finalizers run LIFO during env teardown. +// - The one that runs FIRST calls a JS function via napi_call_function +// that throws; the throw reaches the VM throw scope and we return +// without clearing it. +// - The one that runs SECOND calls napi_create_error. +// * Before the fix: returns napi_pending_exception (10). +// * After the fix: returns napi_ok (0). +// +// The test driver spawns bun, waits for it to exit, and asserts the +// finalizer printed "create_error_status=0". + +#include +#include +#include +#include + +// Strong ref (refcount 1) so the throwing function the finalizer calls +// survives until env cleanup. +static napi_ref throw_fn_ref = NULL; +static int create_error_status = -999; + +// Runs SECOND during LIFO cleanup (wrapped FIRST). At this point the +// other finalizer has already left an Error on the JSC VM scope. +static void finalize_create_error(napi_env env, void *data, void *hint) { + (void)data; + (void)hint; + + napi_value msg; + napi_status s = napi_create_string_utf8(env, "finalizer-error", NAPI_AUTO_LENGTH, &msg); + if (s != napi_ok) { + create_error_status = -(100 + s); + printf("create_error_status=%d\n", create_error_status); + fflush(stdout); + return; + } + + napi_value err; + // Before the fix: returns napi_pending_exception because + // createErrorWithNapiValues's throw-scope check saw the prior + // finalizer's VM exception. + // After the fix: returns napi_ok. + create_error_status = napi_create_error(env, NULL, msg, &err); + + // Emit directly from the finalizer -- cleanup hooks run BEFORE + // finalizers, so we can't rely on a post-finalize hook. + printf("create_error_status=%d\n", create_error_status); + fflush(stdout); +} + +// Runs FIRST during LIFO cleanup (wrapped SECOND). Calls a throwing JS +// function via napi_call_function. The throw propagates to the VM scope; +// we return without clearing it, leaving the exception pending for the +// next finalizer. +static void finalize_leak_exception(napi_env env, void *data, void *hint) { + (void)data; + (void)hint; + + if (!throw_fn_ref) { + return; + } + + napi_value fn; + napi_status s = napi_get_reference_value(env, throw_fn_ref, &fn); + if (s != napi_ok || !fn) { + return; + } + + napi_value undef; + napi_get_undefined(env, &undef); + + // Invoke the JS function; it throws. napi_call_function returns + // napi_pending_exception without clearing the VM scope, so the + // exception stays live for the next finalizer's entry. + (void)napi_call_function(env, undef, fn, 0, NULL, NULL); +} + +// setup(fnThatThrows: () => never): { outer: object, inner: object } +// fnThatThrows must be a JS function that throws when called. We hold +// a strong ref to it so it survives to cleanup, then wrap two fresh +// objects with the two finalizers above. +static napi_value setup(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value args[1]; + napi_get_cb_info(env, info, &argc, args, NULL, NULL); + if (argc < 1) { + napi_throw_error(env, NULL, "setup needs throwing fn"); + return NULL; + } + + napi_create_reference(env, args[0], 1, &throw_fn_ref); + + // Outer is wrapped FIRST -> appears earlier in the finalizer list -> + // cleanup iterates in reverse (LIFO) -> its finalizer runs SECOND, + // after the exception has been leaked. + napi_value outer; + napi_create_object(env, &outer); + napi_wrap(env, outer, NULL, finalize_create_error, NULL, NULL); + + // Inner is wrapped SECOND -> its finalizer runs FIRST and leaks the + // pending VM exception the next finalizer will inherit. + napi_value inner; + napi_create_object(env, &inner); + napi_wrap(env, inner, NULL, finalize_leak_exception, NULL, NULL); + + napi_value result; + napi_create_object(env, &result); + napi_set_named_property(env, result, "outer", outer); + napi_set_named_property(env, result, "inner", inner); + return result; +} + +// Env cleanup hook that leaves a pending JSC VM exception behind: +// napi_throw_error schedules an env-level exception (always allowed), +// and napi_call_function's prologue (env->throwPendingException()) +// promotes it to a VM-level exception before validating any argument, +// then returns napi_pending_exception with the exception still set. +// This is the same state node-canvas reaches on Worker.terminate(): +// a finalizer/hook fails internally, the scheduled exception gets +// promoted to the VM, and it is still pending when NapiEnv::cleanup() +// reaches the first wrap finalizer. Without the clearing between the +// hook phase and the finalizer phase, that first finalizer's first +// napi call fails with napi_pending_exception and node-addon-api +// escalates to napi_fatal_error ("Error::Error napi_create_object"). +static void leak_exception_cleanup_hook(void *arg) { + napi_env env = (napi_env)arg; + napi_throw_error(env, NULL, "leaked from cleanup hook"); + // Promotes the scheduled exception onto the VM and leaves it there. + (void)napi_call_function(env, NULL, NULL, 0, NULL, NULL); +} + +// setupSingle(): object +// Registers the exception-leaking cleanup hook and wraps ONE object +// whose finalizer immediately calls napi_create_string_utf8 + +// napi_create_error and prints the resulting status. Hooks run before +// wrap finalizers, so the finalizer only succeeds if cleanup clears +// pending exceptions before entering the finalizer phase. +static napi_value setup_single(napi_env env, napi_callback_info info) { + (void)info; + + napi_add_env_cleanup_hook(env, leak_exception_cleanup_hook, env); + + napi_value obj; + napi_create_object(env, &obj); + napi_wrap(env, obj, NULL, finalize_create_error, NULL, NULL); + return obj; +} + +NAPI_MODULE_INIT(/* napi_env env, napi_value exports */) { + napi_property_descriptor props[] = { + {"setup", NULL, setup, NULL, NULL, NULL, napi_default, NULL}, + {"setupSingle", NULL, setup_single, NULL, NULL, NULL, napi_default, NULL}, + }; + napi_define_properties(env, exports, sizeof(props) / sizeof(props[0]), props); + return exports; +} diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index cdf548e5a72..32fc55cf566 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -694,6 +694,70 @@ describe.concurrent("napi", () => { expect(exitCode).toBe(0); }); + it("napi_create_error succeeds during env cleanup when a prior finalizer leaked a VM exception (#30286)", async () => { + // Reproduces the gitnexus + tree-sitter crash: one finalizer left a + // pending JSC exception on the VM, the next finalizer called + // napi_create_error, and Bun returned napi_pending_exception. Under + // node-addon-api's Error::New that turns into + // NAPI FATAL ERROR: Error::New napi_create_error + // during web_worker.exitAndDeinit. After the fix napi_create_error + // ignores pre-existing VM exceptions (matching Node.js) so the + // finalizer completes cleanly and the process exits 0. + const code = ` + const addon = require(${JSON.stringify(join(__dirname, "napi-app/build/Debug/test_finalizer_create_error.node"))}); + // A function that throws -- called from the first-to-run finalizer + // so the throw leaves a JSC VM exception pending for the next + // finalizer (the one that calls napi_create_error). + globalThis.keep = addon.setup(() => { throw new Error("from js"); }); + `; + await using proc = 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]); + // napi_ok == 0 -- before the fix this was 10 (napi_pending_exception) in + // release builds, or an ASAN abort in debug builds. A panic would leave + // stdout empty, so the positive assertion covers both crash modes without + // relying on stderr-contains-"panic" (which is unreliable per CLAUDE.md). + expect(stdout.trim()).toBe("create_error_status=0"); + expect(exitCode).toBe(0); + expect(stderr).toBe(""); + }); + + it("the first napi finalizer starts clean when a cleanup hook leaked a VM exception (#30286)", async () => { + // The node-canvas shape of #30286 (terminated Workers): a native + // teardown callback fails internally, its scheduled exception gets + // promoted onto the JSC VM (napi_call_function's prologue does this + // before validating arguments), and the exception is still pending + // when NapiEnv::cleanup() reaches the FIRST wrap finalizer. That + // finalizer's first napi call (napi_create_string_utf8 in + // node-addon-api's ObjectWrap teardown) then fails with + // napi_pending_exception and the addon escalates to napi_fatal_error + // ("Error::Error napi_create_object"). Cleanup hooks run before wrap + // finalizers, so the addon's leaking hook reproduces the state + // deterministically; the fix clears pending exceptions before the + // finalizer phase starts. + const code = ` + const addon = require(${JSON.stringify(join(__dirname, "napi-app/build/Debug/test_finalizer_create_error.node"))}); + globalThis.keep = addon.setupSingle(); + `; + await using proc = 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]); + // The wrap finalizer prints the napi_create_error status; 0 == napi_ok. + // Before the fix the finalizer's napi_create_string_utf8 failed on the + // hook's leaked exception (status -110 on the string step). + expect(stdout.trim()).toBe("create_error_status=0"); + expect(exitCode).toBe(0); + expect(stderr).toBe(""); + }); + it("napi_reference_unref can be called from finalizers in regular modules", async () => { // This test ensures that napi_reference_unref can be called during GC // without triggering the NAPI_CHECK_ENV_NOT_IN_GC assertion for regular modules.