From 5ca6b155d9b717b7fb2252c632157906b3a9037a Mon Sep 17 00:00:00 2001 From: robobun Date: Tue, 5 May 2026 18:30:03 +0000 Subject: [PATCH 01/14] napi: clear pending exceptions between finalizers during env cleanup Tree-sitter inside node:worker_threads deterministically panicked at worker exit once ~15 finalizers had run: panic: NAPI FATAL ERROR: Error::New napi_create_error at NapiEnv::cleanup -> NapiRef::callFinalizer -> NapiFinalizer::call Two things were broken. First, NapiEnv::cleanup ran each finalizer without clearing any pending exception after it returned, so a throw from one finalizer leaked into the next. Node.js doesn't do that -- finalizers run with no JS frame above them to catch, so propagating the exception is pointless and only confuses later finalizers. Second, createErrorWithNapiValues refused to produce an Error value while a VM-level exception was pending (DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION at the top). Node.js's napi_create_error has no such check -- it is a pure value-producing function. Because napi_is_exception_pending deliberately skips the VM-scope check during cleanup for safety, node-addon-api's Error::New(env) saw 'no pending exception', fell through to napi_create_error, got back napi_pending_exception, and tripped NAPI_FATAL_IF_FAILED -> napi_fatal_error. Fix both layers: clear env/VM exceptions after each finalizer in NapiEnv::cleanup, and drop the pending-exception check from createErrorWithNapiValues. Matches Node.js on both counts. Regression test: a standalone addon wraps two JS objects; the first finalizer calls a throwing JS function via napi_call_function (leaving the JSC VM exception pending without clearing it), the second calls napi_create_error and prints the returned status. Before the fix the status is 10 (napi_pending_exception) and the debug ASAN build aborts on the leaked exception first; after the fix the status is 0 (napi_ok) and the process exits cleanly. Closes #30286. Related: #22259, #28248, #29785. --- src/jsc/bindings/napi.cpp | 19 ++- src/jsc/bindings/napi.h | 23 +++ test/napi/napi-app/binding.gyp | 11 ++ .../napi-app/test_finalizer_create_error.c | 141 ++++++++++++++++++ test/napi/napi.test.ts | 30 ++++ 5 files changed, 219 insertions(+), 5 deletions(-) create mode 100644 test/napi/napi-app/test_finalizer_create_error.c diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index 6e7749b54e1..82317f49edf 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -1067,12 +1067,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 +1092,14 @@ 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 value we've already verified to be a string does + // not throw, so we do not need a throw scope here. 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); } diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index aa305eae35c..c45091ebaa8 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -3,6 +3,7 @@ #include "root.h" #include #include +#include #include #include "headers-handwritten.h" @@ -209,12 +210,21 @@ struct NapiEnv : public WTF::RefCounted { 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 +512,19 @@ 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. + // + // TopExceptionScope::clearException() wraps VM::clearException(), + // which also resets the EXCEPTION_SCOPE_VERIFICATION bookkeeping + // (m_needExceptionCheck). Leaving that unreset would trip debug + // asserts in the next finalizer's napi calls. + void clearExceptionsBetweenFinalizers() + { + DECLARE_TOP_EXCEPTION_SCOPE(m_vm).clearException(); + m_pendingException.clear(); + } + // Returns a vector of hooks in reverse order of insertion. std::vector getHooks() const { 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; +} + +NAPI_MODULE_INIT(/* napi_env env, napi_value exports */) { + napi_property_descriptor props[] = { + {"setup", NULL, setup, 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..0b61ab4fbbf 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -694,6 +694,36 @@ 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]); + expect(stderr).not.toContain("panic"); + expect(stderr).not.toContain("NAPI FATAL"); + // napi_ok == 0 -- before the fix this was 10 (napi_pending_exception). + expect(stdout.trim()).toBe("create_error_status=0"); + expect(exitCode).toBe(0); + }); + 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. From a2d6053b25154801b5bb623c95f6069b3d301f02 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 5 May 2026 19:06:57 +0000 Subject: [PATCH 02/14] test: drop stderr-panic checks in favor of positive assertions Per CLAUDE.md: 'Never write tests that check for no panic or uncaught exception in the test output. These tests will never fail in CI.' The positive assertions (stdout == 'create_error_status=0' and exitCode == 0) already fail decisively if the finalizer panics -- stdout is empty on a crash. Also added stderr == '' as a positive absence-of-diagnostic check. Addresses CodeRabbit review on #30291. --- test/napi/napi.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 0b61ab4fbbf..702431bc26d 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -717,11 +717,13 @@ describe.concurrent("napi", () => { stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stderr).not.toContain("panic"); - expect(stderr).not.toContain("NAPI FATAL"); - // napi_ok == 0 -- before the fix this was 10 (napi_pending_exception). + // 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("napi_reference_unref can be called from finalizers in regular modules", async () => { From 0229b3ba956b1f72d6b508fa455dfa0f08165df1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 5 May 2026 21:09:22 +0000 Subject: [PATCH 03/14] retry CI: buildkite agent expiration wave (build 51854) From 27e34f31a5b351bb3dac5be471aaf763e252c052 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 5 May 2026 21:24:23 +0000 Subject: [PATCH 04/14] napi: move clearExceptionsBetweenFinalizers out of the header Declared inline in napi.h it was getting instantiated in every TU that pulled in napi_external.h. On Windows aarch64 (build 51887's only real failure), the TopExceptionScope ctor/dtor are JS_EXPORT_PRIVATE under ENABLE_EXCEPTION_SCOPE_VERIFICATION; that broke build-cpp. Confining the body to napi.cpp means TopExceptionScope is touched from exactly one TU, matching how every other caller (ErrorStackTrace.cpp, NodeVMModule.cpp, MessagePort.cpp, ...) uses it today. --- src/jsc/bindings/napi.cpp | 14 ++++++++++++++ src/jsc/bindings/napi.h | 17 +++++------------ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index 82317f49edf..b9be76c75bf 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" @@ -3053,3 +3054,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 c45091ebaa8..4cde28a22eb 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -3,7 +3,6 @@ #include "root.h" #include #include -#include #include #include "headers-handwritten.h" @@ -513,17 +512,11 @@ struct NapiEnv : public WTF::RefCounted { size_t m_cleanupHookCounter = 0; // Drop any pending exception -- VM-scope or env-scope -- between - // finalizers run from cleanup(). Used by cleanup() only. - // - // TopExceptionScope::clearException() wraps VM::clearException(), - // which also resets the EXCEPTION_SCOPE_VERIFICATION bookkeeping - // (m_needExceptionCheck). Leaving that unreset would trip debug - // asserts in the next finalizer's napi calls. - void clearExceptionsBetweenFinalizers() - { - DECLARE_TOP_EXCEPTION_SCOPE(m_vm).clearException(); - m_pendingException.clear(); - } + // 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 From 40351ce447d7451248f635871e82324fe993cfe0 Mon Sep 17 00:00:00 2001 From: robobun Date: Tue, 5 May 2026 23:08:41 +0000 Subject: [PATCH 05/14] retry CI: darwin agent fleet expiration (build 51894) From ce118aab1f4fcfbdf42a83ebb1d456499c521f56 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 6 May 2026 00:14:58 +0000 Subject: [PATCH 06/14] retry CI: 1-shard asan flake (build 51921) From 8fa4c6cf01a89ca9ff172a308a3b377ec1d8adb5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 6 May 2026 00:48:30 +0000 Subject: [PATCH 07/14] retry CI: shard-level flake on debian-13-x64-asan (build 51937) From 9027c8672bc073a8729f0f75408dffd0b7785fce Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 6 May 2026 01:15:34 +0000 Subject: [PATCH 08/14] retry CI: one-shard asan flake (build 51947) From 1288d1e67b7f00de0db9b394629879d7cf70a45d Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 6 May 2026 05:08:42 +0000 Subject: [PATCH 09/14] retry CI: darwin aarch64 agent expiration wave (build 51956) From 720f0849077212e6f1ec4f9958d3390c69dc2b01 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 6 May 2026 05:26:30 +0000 Subject: [PATCH 10/14] retry CI: darwin-aarch64 link step exit 128 (build 52007) From 7dcc2f1947c4fd1169f9ef7a1e304c217a78aa09 Mon Sep 17 00:00:00 2001 From: robobun Date: Mon, 8 Jun 2026 00:07:04 +0000 Subject: [PATCH 11/14] napi: clear pending exceptions before the finalizer phase of cleanup too A native teardown callback can leave a pending VM exception before the first wrap finalizer runs: a terminated Worker's teardown promotes a scheduled exception onto the VM (napi_call_function's prologue calls throwPendingException before validating arguments), and nothing cleared it between the cleanup-hook phase and the finalizer phase. The first finalizer's first napi call then failed with napi_pending_exception and node-addon-api escalated to napi_fatal_error -- the node-canvas shape of #30286: panic: NAPI FATAL ERROR: Error::Error napi_create_object reported with canvas@3.2.1 + worker.terminate(). Clear pending exceptions at the start of cleanup and again after the cleanup-hook drain so every native teardown callback starts from a clean exception state, completing the invariant the between-finalizer clearing already established. Deterministic regression test: the addon registers a cleanup hook that schedules an exception via napi_throw_error and promotes it with napi_call_function, then a wrap finalizer probes napi_create_error. Fails without this commit (status -110 / abort), passes with it. Verified the canvas@3.2.1 worker repro from the issue thread exits 0. --- src/jsc/bindings/napi.h | 16 ++++++++ .../napi-app/test_finalizer_create_error.c | 37 +++++++++++++++++++ test/napi/napi.test.ts | 32 ++++++++++++++++ 3 files changed, 85 insertions(+) diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 4cde28a22eb..90286dfe51c 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,6 +216,9 @@ 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. diff --git a/test/napi/napi-app/test_finalizer_create_error.c b/test/napi/napi-app/test_finalizer_create_error.c index 7528ee0cb46..5348443e810 100644 --- a/test/napi/napi-app/test_finalizer_create_error.c +++ b/test/napi/napi-app/test_finalizer_create_error.c @@ -132,9 +132,46 @@ static napi_value setup(napi_env env, napi_callback_info info) { 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 702431bc26d..32fc55cf566 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -726,6 +726,38 @@ describe.concurrent("napi", () => { 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. From 48fcf3fb97c214743a62b945ad0c53340604f9b1 Mon Sep 17 00:00:00 2001 From: robobun Date: Mon, 8 Jun 2026 00:08:34 +0000 Subject: [PATCH 12/14] napi: soften getString comment to note the rope-OOM caveat is intentional --- src/jsc/bindings/napi.cpp | 5 +++-- test/napi/napi-app/bun.lock | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index b9be76c75bf..fee0f379ca6 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -1093,8 +1093,9 @@ 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 value we've already verified to be a string does - // not throw, so we do not need a throw scope here. + // 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); auto wtf_message = js_message.getString(globalObject); diff --git a/test/napi/napi-app/bun.lock b/test/napi/napi-app/bun.lock index 605f6d47777..099875ae95c 100644 --- a/test/napi/napi-app/bun.lock +++ b/test/napi/napi-app/bun.lock @@ -1,5 +1,6 @@ { "lockfileVersion": 1, + "configVersion": 0, "workspaces": { "": { "name": "napi-buffer-bug", From 1ded3d62d67d1b9f0d8f9334749d0e8db87a8828 Mon Sep 17 00:00:00 2001 From: robobun Date: Mon, 8 Jun 2026 00:08:55 +0000 Subject: [PATCH 13/14] revert stray configVersion churn in napi-app bun.lock --- test/napi/napi-app/bun.lock | 1 - 1 file changed, 1 deletion(-) diff --git a/test/napi/napi-app/bun.lock b/test/napi/napi-app/bun.lock index 099875ae95c..605f6d47777 100644 --- a/test/napi/napi-app/bun.lock +++ b/test/napi/napi-app/bun.lock @@ -1,6 +1,5 @@ { "lockfileVersion": 1, - "configVersion": 0, "workspaces": { "": { "name": "napi-buffer-bug", From 97fee1c15219742d855dcd70cb11ab8254570c27 Mon Sep 17 00:00:00 2001 From: robobun Date: Mon, 8 Jun 2026 00:27:23 +0000 Subject: [PATCH 14/14] napi: clear leaked exceptions between cleanup hooks too drain() ran hooks back-to-back, so a hook that leaked a VM exception poisoned the next hook's napi calls. Apply the same per-callback clear the finalizer loop uses, making cleanup()'s documented invariant (every native teardown callback starts from a clean exception state) hold for hooks as well. --- src/jsc/bindings/napi.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 90286dfe51c..df76e206465 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -565,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(); } } };