diff --git a/src/bun.js/bindings/ZigGlobalObject.cpp b/src/bun.js/bindings/ZigGlobalObject.cpp index 4f50452f4a56fe..51979fa231350b 100644 --- a/src/bun.js/bindings/ZigGlobalObject.cpp +++ b/src/bun.js/bindings/ZigGlobalObject.cpp @@ -4646,8 +4646,8 @@ GlobalObject::PromiseFunctions GlobalObject::promiseHandlerID(Zig::FFIFunction h napi_env GlobalObject::makeNapiEnv(const napi_module& mod) { - m_napiEnvs.append(std::make_unique(this, mod)); - return m_napiEnvs.last().get(); + m_napiEnvs.append(napi_env__::create(this, mod)); + return &m_napiEnvs.last().get(); } napi_env GlobalObject::makeNapiEnvForFFI() diff --git a/src/bun.js/bindings/ZigGlobalObject.h b/src/bun.js/bindings/ZigGlobalObject.h index 60e3940bc6dd8f..1e01d11f6ba851 100644 --- a/src/bun.js/bindings/ZigGlobalObject.h +++ b/src/bun.js/bindings/ZigGlobalObject.h @@ -651,7 +651,7 @@ class GlobalObject : public Bun::GlobalScope { // De-optimization once `require("module").runMain` is written to bool hasOverriddenModuleRunMain = false; - WTF::Vector> m_napiEnvs; + Vector> m_napiEnvs; napi_env makeNapiEnv(const napi_module&); napi_env makeNapiEnvForFFI(); bool hasNapiFinalizers() const; diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index 720d44afd05952..94ef29928a9f22 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -1237,9 +1237,9 @@ extern "C" napi_status napi_add_finalizer(napi_env env, napi_value js_object, *result = toNapi(ref); } else { // Otherwise, it's cheaper to just call .addFinalizer. - vm.heap.addFinalizer(object, [env, finalize_cb, native_object, finalize_hint](JSCell* cell) -> void { + vm.heap.addFinalizer(object, [weak_env = ThreadSafeWeakPtr(*env), finalize_cb, native_object, finalize_hint](JSCell* cell) -> void { NAPI_LOG("finalizer %p", finalize_hint); - env->doFinalizer(finalize_cb, native_object, finalize_hint); + napi_env__::doFinalizer(weak_env.get(), finalize_cb, native_object, finalize_hint); }); } @@ -2024,9 +2024,9 @@ extern "C" napi_status napi_create_external_buffer(napi_env env, size_t length, Zig::GlobalObject* globalObject = toJS(env); - auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(data), length }, createSharedTask([env, finalize_hint, finalize_cb](void* p) { + auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(data), length }, createSharedTask([weak_env = ThreadSafeWeakPtr(*env), finalize_hint, finalize_cb](void* p) { NAPI_LOG("external buffer finalizer"); - env->doFinalizer(finalize_cb, p, finalize_hint); + napi_env__::doFinalizer(weak_env.get(), finalize_cb, p, finalize_hint); })); auto* subclassStructure = globalObject->JSBufferSubclassStructure(); @@ -2045,9 +2045,9 @@ extern "C" napi_status napi_create_external_arraybuffer(napi_env env, void* exte Zig::GlobalObject* globalObject = toJS(env); JSC::VM& vm = JSC::getVM(globalObject); - auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(external_data), byte_length }, createSharedTask([env, finalize_hint, finalize_cb](void* p) { + auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(external_data), byte_length }, createSharedTask([weak_env = ThreadSafeWeakPtr(*env), finalize_hint, finalize_cb](void* p) { NAPI_LOG("external ArrayBuffer finalizer"); - env->doFinalizer(finalize_cb, p, finalize_hint); + napi_env__::doFinalizer(weak_env.get(), finalize_cb, p, finalize_hint); })); auto* buffer = JSC::JSArrayBuffer::create(vm, globalObject->arrayBufferStructure(ArrayBufferSharingMode::Default), WTFMove(arrayBuffer)); diff --git a/src/bun.js/bindings/napi.h b/src/bun.js/bindings/napi.h index 385333740282e8..e1c1a1be677068 100644 --- a/src/bun.js/bindings/napi.h +++ b/src/bun.js/bindings/napi.h @@ -61,13 +61,11 @@ struct napi_async_cleanup_hook_handle__ { } while (0) // Named this way so we can manipulate napi_env values directly (since napi_env is defined as a pointer to struct napi_env__) -struct napi_env__ { +struct napi_env__ : public WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr { public: - napi_env__(Zig::GlobalObject* globalObject, const napi_module& napiModule) - : m_globalObject(globalObject) - , m_napiModule(napiModule) + static Ref create(Zig::GlobalObject* globalObject, const napi_module& napiModule) { - napi_internal_register_cleanup_zig(this); + return adoptRef(*new napi_env__(globalObject, napiModule)); } ~napi_env__() @@ -192,20 +190,28 @@ struct napi_env__ { return JSC::getVM(m_globalObject).hasTerminationRequest(); } - void doFinalizer(napi_finalize finalize_cb, void* data, void* finalize_hint) + static void doFinalizer(RefPtr&& env, napi_finalize finalize_cb, void* data, void* finalize_hint) { if (!finalize_cb) { return; } - if (mustDeferFinalizers() && inGC()) { - napi_internal_enqueue_finalizer(this, finalize_cb, data, finalize_hint); + if (env) { + if (env->mustDeferFinalizers() && env->inGC()) { + napi_internal_enqueue_finalizer(env.get(), finalize_cb, data, finalize_hint); + } else { + finalize_cb(env.get(), data, finalize_hint); + } } else { - finalize_cb(this, data, finalize_hint); + NAPI_LOG("skipping finalizer as env is already freed"); } } - inline Zig::GlobalObject* globalObject() const { return m_globalObject; } + inline Zig::GlobalObject* + globalObject() const + { + return m_globalObject; + } inline const napi_module& napiModule() const { return m_napiModule; } // Returns true if finalizers from this module need to be scheduled for the next tick after garbage collection, instead of running during garbage collection @@ -291,6 +297,13 @@ struct napi_env__ { }; private: + napi_env__(Zig::GlobalObject* globalObject, const napi_module& napiModule) + : m_globalObject(globalObject) + , m_napiModule(napiModule) + { + napi_internal_register_cleanup_zig(this); + } + Zig::GlobalObject* m_globalObject = nullptr; napi_module m_napiModule; // TODO(@heimskr): Use WTF::HashSet diff --git a/test/napi/napi-app/js_test_helpers.cpp b/test/napi/napi-app/js_test_helpers.cpp index 25425473731fea..b4ba17877e7acc 100644 --- a/test/napi/napi-app/js_test_helpers.cpp +++ b/test/napi/napi-app/js_test_helpers.cpp @@ -320,6 +320,23 @@ static napi_value create_weird_bigints(const Napi::CallbackInfo &info) { return array; } +static napi_value add_finalizer_to_object(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + napi_value js_object = info[0]; + int *native_object = new int{123}; + NODE_API_CALL(env, + napi_add_finalizer( + env, js_object, static_cast(native_object), + [](napi_env, void *data, void *) { + auto casted = static_cast(data); + printf("add_finalizer_to_object finalizer data = %d\n", + *casted); + delete casted; + }, + nullptr, nullptr)); + return ok(env); +} + void register_js_test_helpers(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, create_ref_with_finalizer); REGISTER_FUNCTION(env, exports, was_finalize_called); @@ -333,6 +350,7 @@ void register_js_test_helpers(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, try_add_tag); REGISTER_FUNCTION(env, exports, check_tag); REGISTER_FUNCTION(env, exports, create_weird_bigints); + REGISTER_FUNCTION(env, exports, add_finalizer_to_object); } } // namespace napitests diff --git a/test/napi/napi-app/module.js b/test/napi/napi-app/module.js index d332b2ece13324..33e7fbbdd9dd03 100644 --- a/test/napi/napi-app/module.js +++ b/test/napi/napi-app/module.js @@ -2,6 +2,7 @@ const assert = require("node:assert"); const nativeTests = require("./build/Debug/napitests.node"); const secondAddon = require("./build/Debug/second_addon.node"); const asyncFinalizeAddon = require("./build/Debug/async_finalize_addon.node"); +const { Worker } = require("node:worker_threads"); async function gcUntil(fn) { const MAX = 100; @@ -618,4 +619,12 @@ nativeTests.test_get_value_string = () => { } }; +// Should be run with +// BUN_DESTRUCT_VM_ON_EXIT=1 -- makes us tear down the JSC::VM while exiting, so that finalizers run +// BUN_JSC_useGC=0 -- ensures the object's finalizer will be called at exit not during normal GC +nativeTests.test_finalizer_called_during_destruction = () => { + let object = {}; + nativeTests.add_finalizer_to_object(object); +}; + module.exports = nativeTests; diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 62106fb364dc7b..7ef676acf959a1 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -1,6 +1,6 @@ import { spawnSync } from "bun"; import { beforeAll, describe, expect, it } from "bun:test"; -import { bunEnv, bunExe, tempDirWithFiles } from "harness"; +import { bunEnv, bunExe, isBroken, tempDirWithFiles } from "harness"; import { join } from "path"; describe("napi", () => { @@ -453,18 +453,27 @@ describe("napi", () => { it("works when the module register function throws", () => { expect(() => require("./napi-app/build/Debug/throw_addon.node")).toThrow(new Error("oops!")); }); + + describe("napi_add_finalizer", () => { + it.todoIf(isBroken)("does not crash if the finalizer is called during VM shutdown", () => { + checkSameOutput("test_finalizer_called_during_destruction", [], { + BUN_DESTRUCT_VM_ON_EXIT: "1", + BUN_JSC_useGC: "0", + }); + }); + }); }); -function checkSameOutput(test: string, args: any[] | string) { - const nodeResult = runOn("node", test, args).trim(); - let bunResult = runOn(bunExe(), test, args); +function checkSameOutput(test: string, args: any[] | string, env: object = {}) { + const nodeResult = runOn("node", test, args, env).trim(); + let bunResult = runOn(bunExe(), test, args, env); // remove all debug logs bunResult = bunResult.replaceAll(/^\[\w+\].+$/gm, "").trim(); expect(bunResult).toEqual(nodeResult); return nodeResult; } -function runOn(executable: string, test: string, args: any[] | string) { +function runOn(executable: string, test: string, args: any[] | string, env: object) { // when the inspector runs (can be due to VSCode extension), there is // a bug that in debug modes the console logs extra stuff const { BUN_INSPECT_CONNECT_TO: _, ...rest } = bunEnv; @@ -476,7 +485,8 @@ function runOn(executable: string, test: string, args: any[] | string) { test, typeof args == "string" ? args : JSON.stringify(args), ], - env: rest, + env: { ...rest, ...env }, + cwd: join(__dirname, "napi-app"), }); const errs = exec.stderr.toString(); if (errs !== "") {