diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index a7f5b8143bd8..b084de1bb46f 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -1166,26 +1166,23 @@ extern "C" napi_status napi_add_finalizer(napi_env env, napi_value js_object, NAPI_CHECK_ENV_NOT_IN_GC(env); NAPI_CHECK_ARG(env, js_object); NAPI_CHECK_ARG(env, finalize_cb); - Zig::GlobalObject* globalObject = toJS(env); - JSC::VM& vm = JSC::getVM(globalObject); JSC::JSValue objectValue = toJS(js_object); JSC::JSObject* object = objectValue.getObject(); NAPI_RETURN_EARLY_IF_FALSE(env, object, napi_object_expected); + // Mirror napi_wrap: create a NapiRef and register it with the env so the + // finalizer also runs at env teardown for objects still alive at exit. + auto* ref = new NapiRef(*env, 0, Bun::NapiFinalizer { finalize_cb, finalize_hint }); + const auto& bound_cleanup = env->addFinalizer(wrap_cleanup, native_object, ref); + ref->boundCleanup = &bound_cleanup; + ref->nativeObject = native_object; + if (result) { - // If they're expecting a Ref, use the ref. - auto* ref = new NapiRef(*env, 0, Bun::NapiFinalizer { finalize_cb, finalize_hint }); - // TODO(@heimskr): consider detecting whether the value can't be weak, as we do in napi_create_reference. - ref->setValueInitial(object, true); - ref->nativeObject = native_object; + ref->weakValueRef.set(objectValue, Napi::NapiRefWeakHandleOwner::weakValueHandleOwner(), ref); *result = toNapi(ref); } else { - // Otherwise, it's cheaper to just call .addFinalizer. - vm.heap.addFinalizer(object, [env = WTF::Ref(*env), finalize_cb, native_object, finalize_hint](JSCell* cell) -> void { - NAPI_LOG("finalizer %p", finalize_hint); - env->doFinalizer(finalize_cb, native_object, finalize_hint); - }); + ref->weakValueRef.set(objectValue, Napi::NapiRefSelfDeletingWeakHandleOwner::weakValueHandleOwner(), ref); } NAPI_RETURN_SUCCESS(env); @@ -2452,6 +2449,18 @@ extern "C" napi_status napi_create_object(napi_env env, napi_value* result) NAPI_RETURN_SUCCESS(env); } +static void external_cleanup(napi_env env, void* data, void* hint) +{ + auto* external = reinterpret_cast(data); + ASSERT(external->m_boundCleanup != nullptr); + external->m_boundCleanup->deactivate(*env); + external->m_boundCleanup = nullptr; + // The user callback may do anything, so detach all state from the external first. + Bun::NapiFinalizer finalizer = external->m_finalizer; + external->m_finalizer.clear(); + finalizer.call(env, external->m_value, true); +} + extern "C" napi_status napi_create_external(napi_env env, void* data, napi_finalize finalize_cb, void* finalize_hint, @@ -2464,9 +2473,16 @@ extern "C" napi_status napi_create_external(napi_env env, void* data, JSC::VM& vm = JSC::getVM(globalObject); auto* structure = globalObject->NapiExternalStructure(); - JSValue value = Bun::NapiExternal::create(vm, structure, data, finalize_hint, finalize_cb, env); - JSC::EnsureStillAliveScope ensureStillAlive(value); - *result = toNapi(value, globalObject); + Bun::NapiExternal* external = Bun::NapiExternal::create(vm, structure, data, finalize_hint, finalize_cb, env); + JSC::EnsureStillAliveScope ensureStillAlive(external); + if (finalize_cb) { + // Register with the env so the finalizer also runs at env teardown for + // externals still alive at exit. The external's destructor deactivates + // this entry if GC runs first. + const auto& bound_cleanup = env->addFinalizer(external_cleanup, nullptr, external); + external->m_boundCleanup = &bound_cleanup; + } + *result = toNapi(JSValue(external), globalObject); NAPI_RETURN_SUCCESS(env); } diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 73f2f639eedd..04910a944df6 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -222,18 +222,35 @@ struct NapiEnv : public WTF::RefCounted { // 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(); - } + // + // A finalizer may register new finalizers, appended past where this pass already is. + // Repeat until a pass runs nothing new: clear() after one pass would free entries a + // still-live NapiRef or NapiExternal points at through boundCleanup. + bool ranAny; + do { + ranAny = false; + for (auto it = m_finalizers.rbegin(); it != m_finalizers.rend(); ++it) { + if (!it->active) { + continue; + } + // Deactivate before calling so an entry can never run on a later pass; + // the callback's own deactivate() is then an idempotent no-op. + it->active = false; + ranAny = true; + Bun::NapiHandleScope handle_scope(m_globalObject); + if (it->callback) { + it->callback(this, it->data, it->hint); + } + // 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(); + } + } while (ranAny); m_finalizers.clear(); m_isFinishingFinalizers = false; @@ -256,7 +273,12 @@ struct NapiEnv : public WTF::RefCounted { const auto& addFinalizer(napi_finalize callback, void* hint, void* data) { - return *m_finalizers.add({ callback, hint, data }).iterator; + // add() dedups on (callback, hint, data), so during cleanup() a new registration + // whose freed-and-reused address matches a tombstoned entry gets that entry back. + // Reactivate it so the drain runs it before clear() frees it under its new owner. + const auto& bound = *m_finalizers.add({ callback, hint, data }).iterator; + bound.active = true; + return bound; } bool hasFinalizers() const @@ -479,13 +501,6 @@ struct NapiEnv : public WTF::RefCounted { { } - void call(napi_env env) const - { - if (callback && active) { - callback(env, data, hint); - } - } - void deactivate(NapiEnv& env) const { if (env.isFinishingFinalizers()) { diff --git a/src/jsc/bindings/napi_external.cpp b/src/jsc/bindings/napi_external.cpp index 239ba8c2fe2b..96f8a42e5be6 100644 --- a/src/jsc/bindings/napi_external.cpp +++ b/src/jsc/bindings/napi_external.cpp @@ -6,6 +6,10 @@ namespace Bun { NapiExternal::~NapiExternal() { auto* env = m_env.get(); + if (m_boundCleanup) { + m_boundCleanup->deactivate(*env); + m_boundCleanup = nullptr; + } m_finalizer.call(env, m_value, env && !env->mustDeferFinalizers()); } diff --git a/src/jsc/bindings/napi_external.h b/src/jsc/bindings/napi_external.h index 2d104fceb81b..4004493ef891 100644 --- a/src/jsc/bindings/napi_external.h +++ b/src/jsc/bindings/napi_external.h @@ -96,6 +96,7 @@ class NapiExternal : public JSC::JSDestructibleObject { void* m_value; NapiFinalizer m_finalizer; WTF::RefPtr m_env; + const NapiEnv::BoundFinalizer* m_boundCleanup = nullptr; #if ASSERT_ENABLED String sourceOriginURL = String(); diff --git a/test/napi/napi-app/binding.gyp b/test/napi/napi-app/binding.gyp index 320e90558694..9e59bdce6b69 100644 --- a/test/napi/napi-app/binding.gyp +++ b/test/napi/napi-app/binding.gyp @@ -275,5 +275,16 @@ "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1", ], }, + { + "target_name": "test_teardown_finalizers", + "sources": ["test_teardown_finalizers.c"], + "include_dirs": ["" to stdout (a child's stderr is unreliable on CI). + +#include +#include +#include +#include + +static int finalize_count = 0; + +static void finalize(napi_env env, void* data, void* hint) { + (void)env; + (void)data; + printf("finalize: %s\n", (const char*)hint); + fflush(stdout); + free(hint); + finalize_count++; +} + +// Copies the string in `arg` into a malloc'd buffer that finalize() frees. +static char* dup_name_arg(napi_env env, napi_value arg) { + size_t len = 0; + napi_get_value_string_utf8(env, arg, NULL, 0, &len); + char* name = (char*)malloc(len + 1); + napi_get_value_string_utf8(env, arg, name, len + 1, &len); + return name; +} + +// wrap(obj, name) +static napi_value wrap(napi_env env, napi_callback_info info) { + size_t argc = 2; + napi_value argv[2]; + napi_get_cb_info(env, info, &argc, argv, NULL, NULL); + napi_wrap(env, argv[0], NULL, finalize, dup_name_arg(env, argv[1]), NULL); + return argv[0]; +} + +// addFinalizer(obj, name, wantRef): wantRef=true exercises the napi_ref-returning overload. +static napi_value add_finalizer(napi_env env, napi_callback_info info) { + size_t argc = 3; + napi_value argv[3]; + napi_get_cb_info(env, info, &argc, argv, NULL, NULL); + bool want_ref = false; + napi_get_value_bool(env, argv[2], &want_ref); + // Leaked on purpose: this test never releases the ref, matching addons + // that hold a weak ref for the object's whole lifetime. + napi_ref* ref = want_ref ? (napi_ref*)malloc(sizeof(napi_ref)) : NULL; + napi_add_finalizer(env, argv[0], NULL, finalize, dup_name_arg(env, argv[1]), ref); + return argv[0]; +} + +// createExternal(name) -> external value +static napi_value create_external(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value argv[1]; + napi_get_cb_info(env, info, &argc, argv, NULL, NULL); + napi_value external; + napi_create_external(env, NULL, finalize, dup_name_arg(env, argv[0]), &external); + return external; +} + +// finalizeCount() -> number of finalizers that have already run (and flushed). +static napi_value get_finalize_count(napi_env env, napi_callback_info info) { + (void)info; + napi_value out; + napi_create_int32(env, finalize_count, &out); + return out; +} + +typedef struct { + char* outer_name; + char* nested_external_name; + char* nested_add_finalizer_name; +} NestingContext; + +// Runs as a teardown finalizer and registers two new finalizers while the env +// is already draining its finalizer list. Bun accepts these calls here (Node +// rejects them with napi_pending_exception), so it must drain them safely. +static void nesting_finalize(napi_env env, void* data, void* hint) { + (void)hint; + NestingContext* ctx = (NestingContext*)data; + printf("finalize: %s\n", ctx->outer_name); + fflush(stdout); + finalize_count++; + napi_value external; + napi_create_external(env, NULL, finalize, ctx->nested_external_name, &external); + napi_add_finalizer(env, external, NULL, finalize, ctx->nested_add_finalizer_name, NULL); + free(ctx->outer_name); + free(ctx); +} + +// wrapNesting(obj, outerName, nestedExternalName, nestedAddFinalizerName) +static napi_value wrap_nesting(napi_env env, napi_callback_info info) { + size_t argc = 4; + napi_value argv[4]; + napi_get_cb_info(env, info, &argc, argv, NULL, NULL); + NestingContext* ctx = (NestingContext*)malloc(sizeof(NestingContext)); + ctx->outer_name = dup_name_arg(env, argv[1]); + ctx->nested_external_name = dup_name_arg(env, argv[2]); + ctx->nested_add_finalizer_name = dup_name_arg(env, argv[3]); + napi_wrap(env, argv[0], ctx, nesting_finalize, NULL, NULL); + return argv[0]; +} + +static napi_ref saved_ref = NULL; + +// addFinalizerSaveRef(obj, name): napi_add_finalizer keeping the returned ref +// so a later teardown finalizer can napi_delete_reference it. +static napi_value add_finalizer_save_ref(napi_env env, napi_callback_info info) { + size_t argc = 2; + napi_value argv[2]; + napi_get_cb_info(env, info, &argc, argv, NULL, NULL); + napi_add_finalizer(env, argv[0], NULL, finalize, dup_name_arg(env, argv[1]), &saved_ref); + return argv[0]; +} + +typedef struct { + char* outer_name; + char* recycled_name; +} RecycleContext; + +// Runs as a teardown finalizer: deletes the ref from addFinalizerSaveRef, then +// registers a new finalizer. The allocator commonly hands back the just-freed +// NapiRef address; the new finalizer must still run. +static void recycle_finalize(napi_env env, void* data, void* hint) { + (void)hint; + RecycleContext* ctx = (RecycleContext*)data; + printf("finalize: %s\n", ctx->outer_name); + fflush(stdout); + finalize_count++; + void* old_ref = (void*)saved_ref; + napi_delete_reference(env, saved_ref); + napi_value obj; + napi_create_object(env, &obj); + napi_ref new_ref = NULL; + napi_add_finalizer(env, obj, NULL, finalize, ctx->recycled_name, &new_ref); + // Diagnostic only (not asserted): says whether the address-reuse collision + // this exercise targets actually happened on this platform and allocator. + printf("recycled-address: %s\n", (void*)new_ref == old_ref ? "yes" : "no"); + fflush(stdout); + free(ctx->outer_name); + free(ctx); +} + +// wrapRecycling(obj, outerName, recycledName) +static napi_value wrap_recycling(napi_env env, napi_callback_info info) { + size_t argc = 3; + napi_value argv[3]; + napi_get_cb_info(env, info, &argc, argv, NULL, NULL); + RecycleContext* ctx = (RecycleContext*)malloc(sizeof(RecycleContext)); + ctx->outer_name = dup_name_arg(env, argv[1]); + ctx->recycled_name = dup_name_arg(env, argv[2]); + napi_wrap(env, argv[0], ctx, recycle_finalize, NULL, NULL); + return argv[0]; +} + +static napi_value init(napi_env env, napi_value exports) { + napi_property_descriptor properties[] = { + { "wrap", 0, wrap, 0, 0, 0, napi_default, 0 }, + { "addFinalizer", 0, add_finalizer, 0, 0, 0, napi_default, 0 }, + { "createExternal", 0, create_external, 0, 0, 0, napi_default, 0 }, + { "finalizeCount", 0, get_finalize_count, 0, 0, 0, napi_default, 0 }, + { "wrapNesting", 0, wrap_nesting, 0, 0, 0, napi_default, 0 }, + { "addFinalizerSaveRef", 0, add_finalizer_save_ref, 0, 0, 0, napi_default, 0 }, + { "wrapRecycling", 0, wrap_recycling, 0, 0, 0, napi_default, 0 }, + }; + napi_define_properties(env, exports, sizeof(properties) / sizeof(properties[0]), properties); + return exports; +} + +NAPI_MODULE(NODE_GYP_MODULE_NAME, init) diff --git a/test/napi/napi-teardown-finalizers.test.ts b/test/napi/napi-teardown-finalizers.test.ts new file mode 100644 index 000000000000..5b3ea6c80e7d --- /dev/null +++ b/test/napi/napi-teardown-finalizers.test.ts @@ -0,0 +1,152 @@ +import { spawn, spawnSync } from "bun"; +import { beforeAll, expect, it } from "bun:test"; +import { existsSync, statSync } from "fs"; +import { bunEnv, bunExe, canBuildNodeAddons } from "harness"; +import { join } from "path"; + +const addonDir = join(__dirname, "napi-app"); +const addonSource = join(addonDir, "test_teardown_finalizers.c"); +const addonPath = join(addonDir, "build/Debug/test_teardown_finalizers.node"); + +// Printed by the fixture after the gc_* finalizers are observed so the test can +// prove they ran during Bun.gc(), not only at env teardown. The addon writes to +// stdout (a spawned child's stderr is unreliable on some CI lanes), so this does too. +const GC_BARRIER = "--gc-barrier--"; + +beforeAll(() => { + if (!canBuildNodeAddons()) return; + // Build the napi-app addons only if the one this test needs is missing or older + // than its inputs. It doesn't link against bun, so an existing binary stays valid + // across bun builds, and skipping the slow node-gyp rebuild avoids flakes. + if (existsSync(addonPath)) { + const built = statSync(addonPath).mtimeMs; + const inputs = [addonSource, join(addonDir, "binding.gyp")]; + if (inputs.every(f => statSync(f).mtimeMs <= built)) { + return; + } + } + for (let attempt = 0; ; attempt++) { + const install = spawnSync({ + cmd: [bunExe(), "install", "--verbose"], + cwd: addonDir, + stderr: "inherit", + env: bunEnv, + stdout: "inherit", + stdin: "inherit", + }); + if (install.success && existsSync(addonPath)) { + return; + } + if (attempt >= 1) { + throw new Error("building napi-app addons failed"); + } + } +}, 300_000); + +async function runFixture(code: string) { + await using proc = spawn({ + cmd: [bunExe(), "-e", `const addon = require(${JSON.stringify(addonPath)});\n${code}`], + // Strip JSC exception-scope validation if a CI agent has it set (like the + // JSC_useJIT strip in harness.ts): Node-API has no place for an exception check + // between two napi calls, so any addon callback making two aborts under it. + env: { ...bunEnv, BUN_JSC_validateExceptionChecks: undefined, BUN_JSC_dumpSimulatedThrows: undefined }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // On Windows the C runtime writes \r\n, so split on either line ending. + const lines = stdout.split(/\r?\n/).filter(l => l.startsWith("finalize: ") || l === GC_BARRIER); + const names = (ls: string[]) => ls.map(l => l.slice("finalize: ".length)).sort(); + const finalized = names(lines.filter(l => l !== GC_BARRIER)); + // If nothing fired, the child never got that far (crashed, require failed, or + // the finalizers regressed); surface its raw output in the failure diff. + const childOutputIfNothingFinalized = finalized.length === 0 ? { stdout, stderr } : null; + return { stdout, stderr, exitCode, lines, names, finalized, childOutputIfNothingFinalized }; +} + +it.skipIf(!canBuildNodeAddons())("finalizers from every registration API run at env teardown", async () => { + // Every object is kept strongly reachable until exit so GC never collects any + // of them: env teardown is the finalizers' only chance to run. Node runs all four. + const { exitCode, finalized, childOutputIfNothingFinalized } = await runFixture(` + const o1 = {}, o2 = {}, o3 = {}; + addon.wrap(o1, "wrap"); + addon.addFinalizer(o2, "add_finalizer", false); + addon.addFinalizer(o3, "add_finalizer_ref", true); + globalThis.__keep = [o1, o2, o3, addon.createExternal("external")]; + `); + expect({ exitCode, finalized, childOutputIfNothingFinalized }).toEqual({ + exitCode: 0, + finalized: ["add_finalizer", "add_finalizer_ref", "external", "wrap"], + childOutputIfNothingFinalized: null, + }); +}); + +it.skipIf(!canBuildNodeAddons())("a finalizer already run by GC does not run again at env teardown", async () => { + // The fixture forces GC until the gc_* finalizers have observably run, prints + // the barrier, then roots the exit_* objects until teardown. Each group landing + // on its own side of the barrier exactly once proves no finalizer runs twice. + const { exitCode, lines, names, childOutputIfNothingFinalized } = await runFixture(` + function makeGarbage() { + addon.addFinalizer({}, "gc_add_finalizer", false); + addon.createExternal("gc_external"); + } + makeGarbage(); + for (let i = 0; addon.finalizeCount() < 2; i++) { + if (i > 500) throw new Error("gc-time finalizers never ran; count=" + addon.finalizeCount()); + Bun.gc(true); + await new Promise(resolve => setImmediate(resolve)); + } + console.log(${JSON.stringify(GC_BARRIER)}); + const kept = {}; + addon.addFinalizer(kept, "exit_add_finalizer", false); + globalThis.__keep = [kept, addon.createExternal("exit_external")]; + `); + const barrier = lines.indexOf(GC_BARRIER); + expect({ + exitCode, + beforeGcBarrier: barrier === -1 ? "missing barrier" : names(lines.slice(0, barrier)), + afterGcBarrier: barrier === -1 ? "missing barrier" : names(lines.slice(barrier + 1)), + childOutputIfNothingFinalized, + }).toEqual({ + exitCode: 0, + beforeGcBarrier: ["gc_add_finalizer", "gc_external"], + afterGcBarrier: ["exit_add_finalizer", "exit_external"], + childOutputIfNothingFinalized: null, + }); +}); + +it.skipIf(!canBuildNodeAddons())( + "finalizers registered by a teardown finalizer also run in the same teardown", + async () => { + // nesting_finalize registers two more finalizers (napi_create_external and + // napi_add_finalizer) while the env is already draining its finalizer list. + // Both must still run before the list is freed out from under their owners. + const { exitCode, finalized, childOutputIfNothingFinalized } = await runFixture(` + const o = {}; + addon.wrapNesting(o, "outer", "nested_external", "nested_add_finalizer"); + globalThis.__keep = [o]; + `); + expect({ exitCode, finalized, childOutputIfNothingFinalized }).toEqual({ + exitCode: 0, + finalized: ["nested_add_finalizer", "nested_external", "outer"], + childOutputIfNothingFinalized: null, + }); + }, +); + +it.skipIf(!canBuildNodeAddons())("a finalizer registered after deleting a ref during teardown still runs", async () => { + // recycle_finalize deletes the "saved" ref during teardown, then registers a + // new finalizer; the allocator commonly hands back the just-freed NapiRef + // address. "recycled" must still run, and the deleted "saved" must not. + const { exitCode, finalized, childOutputIfNothingFinalized } = await runFixture(` + const o1 = {}, o2 = {}; + addon.addFinalizerSaveRef(o2, "saved"); + addon.wrapRecycling(o1, "outer", "recycled"); + globalThis.__keep = [o1, o2]; + `); + expect({ exitCode, finalized, childOutputIfNothingFinalized }).toEqual({ + exitCode: 0, + finalized: ["outer", "recycled"], + childOutputIfNothingFinalized: null, + }); +});