From 7cb2bad7999529e20e9ce18af8545321720d80d0 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Tue, 10 Sep 2024 18:43:14 -0700 Subject: [PATCH 1/5] Avoid creating a Napi handle scope within a finalizer --- src/bun.js/bindings/napi.h | 4 +++- src/bun.js/bindings/napi_handle_scope.cpp | 17 +++++++++++++++-- src/napi/napi.zig | 23 ++++++++++++++--------- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/bun.js/bindings/napi.h b/src/bun.js/bindings/napi.h index b644bf4d56ab21..e78badd4959456 100644 --- a/src/bun.js/bindings/napi.h +++ b/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(JSC::JSValue::encode(val)); } diff --git a/src/bun.js/bindings/napi_handle_scope.cpp b/src/bun.js/bindings/napi_handle_scope.cpp index 5aa599ced63ae0..0b12a47324b70a 100644 --- a/src/bun.js/bindings/napi_handle_scope.cpp +++ b/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) + 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; } diff --git a/src/napi/napi.zig b/src/napi/napi.zig index 088960a4b4a514..e9d805819c31a5 100644 --- a/src/napi/napi.zig +++ b/src/napi/napi.zig @@ -70,7 +70,7 @@ pub const NapiHandleScope = opaque { 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); } @@ -744,7 +744,7 @@ pub extern fn napi_reference_unref(env: napi_env, ref: *Ref, result: [*c]u32) na pub extern fn napi_get_reference_value(env: napi_env, ref: *Ref, result: *napi_value) napi_status; pub extern fn napi_get_reference_value_internal(ref: *Ref) JSC.JSValue; -pub export fn napi_open_handle_scope(env: napi_env, result_: ?*napi_handle_scope) napi_status { +pub export fn napi_open_handle_scope(env: napi_env, result_: ?*?napi_handle_scope) napi_status { log("napi_open_handle_scope", .{}); const result = result_ orelse { return invalidArg(); @@ -753,9 +753,12 @@ pub export fn napi_open_handle_scope(env: napi_env, result_: ?*napi_handle_scope return .ok; } -pub export fn napi_close_handle_scope(env: napi_env, handle_scope: napi_handle_scope) napi_status { +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(); @@ -829,9 +832,11 @@ pub export fn napi_open_escapable_handle_scope(env: napi_env, result_: ?*napi_es result.* = NapiHandleScope.push(env, true); return .ok; } -pub export fn napi_close_escapable_handle_scope(env: napi_env, scope: napi_escapable_handle_scope) napi_status { +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 { @@ -1195,7 +1200,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 +1582,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); }, } From 1efbe7bb179c95f4f7941b8fc94c9fb2a7b6c465 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Tue, 10 Sep 2024 18:47:53 -0700 Subject: [PATCH 2/5] Update napi_handle_scope.cpp --- src/bun.js/bindings/napi_handle_scope.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bun.js/bindings/napi_handle_scope.cpp b/src/bun.js/bindings/napi_handle_scope.cpp index 0b12a47324b70a..b23d8bdfdece25 100644 --- a/src/bun.js/bindings/napi_handle_scope.cpp +++ b/src/bun.js/bindings/napi_handle_scope.cpp @@ -88,7 +88,7 @@ NapiHandleScopeImpl* NapiHandleScope::push(Zig::GlobalObject* globalObject, bool // 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) + // 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; } From 7193fd698a81319ac8bd244cbf31cb30cbf36513 Mon Sep 17 00:00:00 2001 From: Ben Grant Date: Wed, 11 Sep 2024 11:58:24 -0700 Subject: [PATCH 3/5] Fix napi.zig function signatures to reflect optional handle scopes --- src/napi/napi.zig | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/napi/napi.zig b/src/napi/napi.zig index e9d805819c31a5..2ee777f7ebd809 100644 --- a/src/napi/napi.zig +++ b/src/napi/napi.zig @@ -65,8 +65,8 @@ 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; @@ -74,7 +74,7 @@ pub const NapiHandleScope = opaque { 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; @@ -744,7 +744,7 @@ pub extern fn napi_reference_unref(env: napi_env, ref: *Ref, result: [*c]u32) na pub extern fn napi_get_reference_value(env: napi_env, ref: *Ref, result: *napi_value) napi_status; pub extern fn napi_get_reference_value_internal(ref: *Ref) JSC.JSValue; -pub export fn napi_open_handle_scope(env: napi_env, result_: ?*?napi_handle_scope) napi_status { +pub export fn napi_open_handle_scope(env: napi_env, result_: ?*napi_handle_scope) napi_status { log("napi_open_handle_scope", .{}); const result = result_ orelse { return invalidArg(); @@ -753,7 +753,7 @@ pub export fn napi_open_handle_scope(env: napi_env, result_: ?*?napi_handle_scop return .ok; } -pub export fn napi_close_handle_scope(env: napi_env, handle_scope: ?napi_handle_scope) napi_status { +pub export fn napi_close_handle_scope(env: napi_env, handle_scope: napi_handle_scope) napi_status { log("napi_close_handle_scope", .{}); if (handle_scope) |scope| { scope.pop(env); @@ -832,18 +832,21 @@ pub export fn napi_open_escapable_handle_scope(env: napi_env, result_: ?*?napi_e result.* = NapiHandleScope.push(env, true); return .ok; } -pub export fn napi_close_escapable_handle_scope(env: napi_env, scope: ?napi_escapable_handle_scope) napi_status { +pub export fn napi_close_escapable_handle_scope(env: napi_env, scope: napi_escapable_handle_scope) napi_status { log("napi_close_escapable_handle_scope", .{}); 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; From f16fa6bff14b643af7b7e1331d758f941cb5b430 Mon Sep 17 00:00:00 2001 From: Ben Grant Date: Wed, 11 Sep 2024 12:16:07 -0700 Subject: [PATCH 4/5] Add test for creating a handle scope in a finalizer --- test/napi/napi-app/main.cpp | 33 +++++++++++++++++++++++++++++++++ test/napi/napi-app/module.js | 17 +++++++++++++++++ test/napi/napi.test.ts | 11 ++++++++++- 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/test/napi/napi-app/main.cpp b/test/napi/napi-app/main.cpp index 0cc3ff2207de4c..f95a8df9e693d1 100644 --- a/test/napi/napi-app/main.cpp +++ b/test/napi/napi-app/main.cpp @@ -450,6 +450,35 @@ 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) { + napi_handle_scope hs; + assert(napi_open_handle_scope(env, &hs) == napi_ok); + assert(napi_close_handle_scope(env, hs) == napi_ok); + + finalize_called = true; +} + +napi_value create_ref_with_finalizer(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + + napi_value object; + assert(napi_create_object(env, &object) == napi_ok); + napi_ref ref; + + assert(napi_wrap(env, object, nullptr, finalize_cb, nullptr, &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(); @@ -492,6 +521,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; } diff --git a/test/napi/napi-app/module.js b/test/napi/napi-app/module.js index 0ab3896980da79..203caa1dc0e9dd 100644 --- a/test/napi/napi-app/module.js +++ b/test/napi/napi-app/module.js @@ -6,4 +6,21 @@ 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 + nativeTests.create_ref_with_finalizer(); + + // 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; diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 876c7cbc77534a..2d9d9a6d19ae5e 100644 --- a/test/napi/napi.test.ts +++ b/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(); From 91310bfde666ecaf67c6aba993c407c1231a17a7 Mon Sep 17 00:00:00 2001 From: Ben Grant Date: Wed, 11 Sep 2024 17:03:59 -0700 Subject: [PATCH 5/5] Avoid creating a handle scope in a finalizer under Node.js --- test/napi/napi-app/main.cpp | 22 ++++++++++++++++------ test/napi/napi-app/module.js | 3 ++- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/test/napi/napi-app/main.cpp b/test/napi/napi-app/main.cpp index f95a8df9e693d1..acebd690a008ec 100644 --- a/test/napi/napi-app/main.cpp +++ b/test/napi/napi-app/main.cpp @@ -453,22 +453,32 @@ napi_value test_napi_ref(const Napi::CallbackInfo &info) { static bool finalize_called = false; void finalize_cb(napi_env env, void *finalize_data, void *finalize_hint) { - napi_handle_scope hs; - assert(napi_open_handle_scope(env, &hs) == napi_ok); - assert(napi_close_handle_scope(env, hs) == napi_ok); - + // only do this in bun + bool &create_handle_scope = *reinterpret_cast(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, nullptr, &ref) == - napi_ok); + assert(napi_wrap(env, object, nullptr, finalize_cb, + reinterpret_cast(finalize_hint), &ref) == napi_ok); return ok(env); } diff --git a/test/napi/napi-app/module.js b/test/napi/napi-app/module.js index 203caa1dc0e9dd..a0353c7fce385b 100644 --- a/test/napi/napi-app/module.js +++ b/test/napi/napi-app/module.js @@ -8,7 +8,8 @@ nativeTests.test_napi_class_constructor_handle_scope = () => { nativeTests.test_napi_handle_scope_finalizer = async () => { // Create a weak reference, which will be collected eventually - nativeTests.create_ref_with_finalizer(); + // 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()) {