napi: clear pending exceptions between finalizers during env cleanup

src/jsc/bindings/napi.cpp

@@ -73,6 +73,7 @@
 #include "wtf/text/ASCIIFastPath.h"
 #include "JavaScriptCore/WeakInlines.h"
 #include <JavaScriptCore/BuiltinNames.h>
+#include <JavaScriptCore/TopExceptionScope.h>
 #include <wtf/TZoneMallocInlines.h>
 #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();
+}

src/jsc/bindings/napi.h

@@ -194,6 +194,19 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
 
     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<NapiEnv> {
         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<NapiEnv> {
     JSC::Strong<JSC::Unknown> 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<Napi::EitherCleanupHook> getHooks() const
     {
@@ -533,6 +565,9 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
                 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();
         }
     }
 };

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": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+            ],
+        },
     ]
 }

test/napi/napi-app/test_finalizer_create_error.c

@@ -0,0 +1,178 @@
+// Regression test for oven-sh/bun#30286.
+//
+// Previously, if a NAPI finalizer left a pending JSC-VM exception behind
+// (which happens in the wild when a finalizer calls a napi function
+// that throws -- tree-sitter's FinalizeNode path is one example), the
+// NEXT finalizer in the LIFO chain that called napi_create_error would
+// fail with napi_pending_exception. When the caller was node-addon-api's
+// Napi::Error::New(env) helper, that failure status fed
+// NAPI_FATAL_IF_FAILED -> 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 <node_api.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+// 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;
+}