Avoid creating a Napi handle scope within a finalizer

src/bun.js/bindings/napi.h

@@ -39,7 +39,9 @@ static inline Zig::GlobalObject* toJS(napi_env val)
 static inline napi_value toNapi(JSC::JSValue val, Zig::GlobalObject* globalObject)
 {
     if (val.isCell()) {
-        globalObject->m_currentNapiHandleScopeImpl.get()->append(val);
+        if (auto* scope = globalObject->m_currentNapiHandleScopeImpl.get()) {
+            scope->append(val);
+        }
     }
     return reinterpret_cast<napi_value>(JSC::JSValue::encode(val));
 }

src/bun.js/bindings/napi_handle_scope.cpp

@@ -80,11 +80,24 @@ NapiHandleScopeImpl::Slot* NapiHandleScopeImpl::reserveSlot()
 
 NapiHandleScopeImpl* NapiHandleScope::push(Zig::GlobalObject* globalObject, bool escapable)
 {
-    auto* impl = NapiHandleScopeImpl::create(globalObject->vm(),
+    auto& vm = globalObject->vm();
+    // Do not create a new handle scope while a finalizer is in progress
+    // This state is possible because we call napi finalizers immediately
+    // so a finalizer can be called while an allocation is in progress.
+    // An example where this happens:
+    // 1. Use the `sqlite3` package
+    // 2. Do an allocation in a hot code path
+    // 3. the napi_ref finalizer is called while the constructor is running
+    // 4. The finalizer creates a new handle scope (yes, it should not do that. No, we can't change that.)
+    if (vm.heap.mutatorState() == JSC::MutatorState::Sweeping) {
+        return nullptr;
+    }
+
+    auto* impl = NapiHandleScopeImpl::create(vm,
         globalObject->NapiHandleScopeImplStructure(),
         globalObject->m_currentNapiHandleScopeImpl.get(),
         escapable);
-    globalObject->m_currentNapiHandleScopeImpl.set(globalObject->vm(), globalObject, impl);
+    globalObject->m_currentNapiHandleScopeImpl.set(vm, globalObject, impl);
     return impl;
 }
 

src/napi/napi.zig

@@ -65,16 +65,16 @@ pub const Ref = opaque {
     extern fn napi_set_ref(ref: *Ref, value: JSC.JSValue) void;
 };
 pub const NapiHandleScope = opaque {
-    extern fn NapiHandleScope__push(globalObject: *JSC.JSGlobalObject, escapable: bool) *NapiHandleScope;
-    extern fn NapiHandleScope__pop(globalObject: *JSC.JSGlobalObject, current: *NapiHandleScope) void;
+    extern fn NapiHandleScope__push(globalObject: *JSC.JSGlobalObject, escapable: bool) ?*NapiHandleScope;
+    extern fn NapiHandleScope__pop(globalObject: *JSC.JSGlobalObject, current: ?*NapiHandleScope) void;
     extern fn NapiHandleScope__append(globalObject: *JSC.JSGlobalObject, value: JSC.JSValueReprInt) void;
     extern fn NapiHandleScope__escape(handleScope: *NapiHandleScope, value: JSC.JSValueReprInt) bool;
 
-    pub fn push(env: napi_env, escapable: bool) *NapiHandleScope {
+    pub fn push(env: napi_env, escapable: bool) ?*NapiHandleScope {
         return NapiHandleScope__push(env, escapable);
     }
 
-    pub fn pop(self: *NapiHandleScope, env: napi_env) void {
+    pub fn pop(self: ?*NapiHandleScope, env: napi_env) void {
         NapiHandleScope__pop(env, self);
     }
 
@@ -89,8 +89,8 @@ pub const NapiHandleScope = opaque {
     }
 };
 
-pub const napi_handle_scope = *NapiHandleScope;
-pub const napi_escapable_handle_scope = *NapiHandleScope;
+pub const napi_handle_scope = ?*NapiHandleScope;
+pub const napi_escapable_handle_scope = ?*NapiHandleScope;
 pub const napi_callback_info = *JSC.CallFrame;
 pub const napi_deferred = *JSC.JSPromise.Strong;
 
@@ -755,7 +755,10 @@ pub export fn napi_open_handle_scope(env: napi_env, result_: ?*napi_handle_scope
 
 pub export fn napi_close_handle_scope(env: napi_env, handle_scope: napi_handle_scope) napi_status {
     log("napi_close_handle_scope", .{});
-    handle_scope.pop(env);
+    if (handle_scope) |scope| {
+        scope.pop(env);
+    }
+
     return .ok;
 }
 
@@ -821,7 +824,7 @@ fn notImplementedYet(comptime name: []const u8) void {
     );
 }
 
-pub export fn napi_open_escapable_handle_scope(env: napi_env, result_: ?*napi_escapable_handle_scope) napi_status {
+pub export fn napi_open_escapable_handle_scope(env: napi_env, result_: ?*?napi_escapable_handle_scope) napi_status {
     log("napi_open_escapable_handle_scope", .{});
     const result = result_ orelse {
         return invalidArg();
@@ -831,14 +834,19 @@ pub export fn napi_open_escapable_handle_scope(env: napi_env, result_: ?*napi_es
 }
 pub export fn napi_close_escapable_handle_scope(env: napi_env, scope: napi_escapable_handle_scope) napi_status {
     log("napi_close_escapable_handle_scope", .{});
-    scope.pop(env);
+    if (scope) |s| {
+        s.pop(env);
+    }
     return .ok;
 }
-pub export fn napi_escape_handle(_: napi_env, scope: napi_escapable_handle_scope, escapee: napi_value, result_: ?*napi_value) napi_status {
+pub export fn napi_escape_handle(_: napi_env, scope_: napi_escapable_handle_scope, escapee: napi_value, result_: ?*napi_value) napi_status {
     log("napi_escape_handle", .{});
     const result = result_ orelse {
         return invalidArg();
     };
+    const scope = scope_ orelse {
+        return invalidArg();
+    };
     scope.escape(escapee.get()) catch return .escape_called_twice;
     result.* = escapee;
     return .ok;
@@ -1195,7 +1203,7 @@ pub const napi_async_work = struct {
 
     pub fn runFromJS(this: *napi_async_work) void {
         const handle_scope = NapiHandleScope.push(this.global, false);
-        defer handle_scope.pop(this.global);
+        defer if (handle_scope) |scope| scope.pop(this.global);
         this.complete.?(
             this.global,
             if (this.status.load(.seq_cst) == @intFromEnum(Status.cancelled))
@@ -1577,7 +1585,7 @@ pub const ThreadSafeFunction = struct {
                 }
 
                 const handle_scope = NapiHandleScope.push(globalObject, false);
-                defer handle_scope.pop(globalObject);
+                defer if (handle_scope) |scope| scope.pop(globalObject);
                 cb.napi_threadsafe_function_call_js(globalObject, napi_value.create(globalObject, cb.js), this.ctx, task);
             },
         }

test/napi/napi-app/main.cpp

@@ -450,6 +450,45 @@ napi_value test_napi_ref(const Napi::CallbackInfo &info) {
   return ok(env);
 }
 
+static bool finalize_called = false;
+
+void finalize_cb(napi_env env, void *finalize_data, void *finalize_hint) {
+  // only do this in bun
+  bool &create_handle_scope = *reinterpret_cast<bool *>(finalize_hint);
+  if (create_handle_scope) {
+    napi_handle_scope hs;
+    assert(napi_open_handle_scope(env, &hs) == napi_ok);
+    assert(napi_close_handle_scope(env, hs) == napi_ok);
+  }
+  delete &create_handle_scope;
+  finalize_called = true;
+}
+
+napi_value create_ref_with_finalizer(const Napi::CallbackInfo &info) {
+  napi_env env = info.Env();
+  napi_value create_handle_scope_in_finalizer = info[0];
+
+  napi_value object;
+  assert(napi_create_object(env, &object) == napi_ok);
+
+  bool *finalize_hint = new bool;
+  assert(napi_get_value_bool(env, create_handle_scope_in_finalizer,
+                             finalize_hint) == napi_ok);
+
+  napi_ref ref;
+
+  assert(napi_wrap(env, object, nullptr, finalize_cb,
+                   reinterpret_cast<bool *>(finalize_hint), &ref) == napi_ok);
+
+  return ok(env);
+}
+
+napi_value was_finalize_called(const Napi::CallbackInfo &info) {
+  napi_value ret;
+  assert(napi_get_boolean(info.Env(), finalize_called, &ret) == napi_ok);
+  return ret;
+}
+
 Napi::Value RunCallback(const Napi::CallbackInfo &info) {
   Napi::Env env = info.Env();
   Napi::Function cb = info[0].As<Napi::Function>();
@@ -492,6 +531,10 @@ Napi::Object InitAll(Napi::Env env, Napi::Object exports1) {
               Napi::Function::New(env, get_class_with_constructor));
   exports.Set("create_promise", Napi::Function::New(env, create_promise));
   exports.Set("test_napi_ref", Napi::Function::New(env, test_napi_ref));
+  exports.Set("create_ref_with_finalizer",
+              Napi::Function::New(env, create_ref_with_finalizer));
+  exports.Set("was_finalize_called",
+              Napi::Function::New(env, was_finalize_called));
 
   return exports;
 }

test/napi/napi-app/module.js

@@ -6,4 +6,22 @@ nativeTests.test_napi_class_constructor_handle_scope = () => {
   console.log("x.foo =", x.foo);
 };
 
+nativeTests.test_napi_handle_scope_finalizer = async () => {
+  // Create a weak reference, which will be collected eventually
+  // Pass false in Node.js so it does not create a handle scope
+  nativeTests.create_ref_with_finalizer(Boolean(process.isBun));
+
+  // Wait until it actually has been collected by ticking the event loop and forcing GC
+  while (!nativeTests.was_finalize_called()) {
+    await new Promise(resolve => {
+      setTimeout(() => resolve(), 0);
+    });
+    if (process.isBun) {
+      Bun.gc(true);
+    } else if (global.gc) {
+      global.gc();
+    }
+  }
+};
+
 module.exports = nativeTests;

test/napi/napi.test.ts

@@ -121,6 +121,9 @@ describe("napi", () => {
     it("can recover the value from a weak ref", () => {
       checkSameOutput("test_napi_ref", []);
     });
+    it("allows creating a handle scope in the finalizer", () => {
+      checkSameOutput("test_napi_handle_scope_finalizer", []);
+    });
   });
 });
 
@@ -135,7 +138,13 @@ function checkSameOutput(test: string, args: any[] | string) {
 
 function runOn(executable: string, test: string, args: any[] | string) {
   const exec = spawnSync({
-    cmd: [executable, join(__dirname, "napi-app/main.js"), test, typeof args == "string" ? args : JSON.stringify(args)],
+    cmd: [
+      executable,
+      "--expose-gc",
+      join(__dirname, "napi-app/main.js"),
+      test,
+      typeof args == "string" ? args : JSON.stringify(args),
+    ],
     env: bunEnv,
   });
   const errs = exec.stderr.toString();