diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 05b95066da16..fec6ca165724 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -387,9 +387,10 @@ struct NapiEnv : public WTF::RefCounted { delete handle; } + // isShuttingDown(): ~VM (Heap::lastChanceToFinalize) runs finalizers without the Sweeping mutator state. bool inGC() const { - return this->vm().isCollectorBusyOnCurrentThread(); + return this->vm().isCollectorBusyOnCurrentThread() || this->vm().heap.isShuttingDown(); } void checkGC() const diff --git a/src/jsc/bindings/napi_handle_scope.cpp b/src/jsc/bindings/napi_handle_scope.cpp index 7c4be7ec19a6..9fc6ff7dd171 100644 --- a/src/jsc/bindings/napi_handle_scope.cpp +++ b/src/jsc/bindings/napi_handle_scope.cpp @@ -103,7 +103,8 @@ NapiHandleScopeImpl* NapiHandleScope::open(Zig::GlobalObject* globalObject, bool // 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) { + // isShuttingDown(): same thing from ~VM's finalizers, which do not set Sweeping (see NapiEnv::inGC()). + if (vm.heap.mutatorState() == JSC::MutatorState::Sweeping || vm.heap.isShuttingDown()) { return nullptr; } diff --git a/test/napi/napi-app/binding.gyp b/test/napi/napi-app/binding.gyp index d0daf9c422a9..e11241f19cd6 100644 --- a/test/napi/napi-app/binding.gyp +++ b/test/napi/napi-app/binding.gyp @@ -297,5 +297,28 @@ "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1", ], }, + { + "target_name": "test_vm_teardown_finalizers", + "sources": ["test_vm_teardown_finalizers.c"], + "include_dirs": [" +#include +#include +#include +#include + +#define NODE_API_CALL(env, call) \ + do { \ + if ((call) != napi_ok) { \ + napi_throw_error((env), NULL, #call " failed"); \ + return NULL; \ + } \ + } while (0) + +static int env_torn_down = 0; +static int pending_finalizers = 0; +static int instance_data; + +static void instance_data_finalizer(napi_env env, void *data, void *hint) { + (void)env; + (void)data; + (void)hint; + env_torn_down = 1; + printf("env teardown: %d finalizer(s) still pending\n", pending_finalizers); + fflush(stdout); +} + +// Registered as the finalize callback of every kind; `hint` is the kind name. +static void finalizer(napi_env env, void *data, void *hint) { + const char *kind = hint; + (void)data; + pending_finalizers--; + + if (!env_torn_down) { + printf("finalizer: %s\n", kind); + fflush(stdout); + return; + } + + // Only VM destruction gets here. +#ifdef TEST_EXPERIMENTAL + // Bun has to refuse this (it aborts, as during a collection); returning from + // it means the call went through. + napi_value undefined; + napi_status status = napi_get_undefined(env, &undefined); + fprintf(stderr, + "FAIL: %s finalizer called napi_get_undefined during VM destruction " + "and got status %d\n", + kind, (int)status); +#else + fprintf(stderr, "FAIL: %s finalizer ran after env teardown\n", kind); + // What node-addon-api's finalizer wrapper does before anything else; here it + // allocates a JSC cell in the heap being destroyed. + napi_handle_scope scope; + if (napi_open_handle_scope(env, &scope) == napi_ok) { + napi_close_handle_scope(env, scope); + } +#endif +} + +static char *dup_kind(const char *kind) { + // Leaked on purpose: it is the finalize hint, read whenever the finalizer + // runs, including from VM destruction. + char *copy = malloc(strlen(kind) + 1); + strcpy(copy, kind); + return copy; +} + +static napi_value setup(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value argv[1]; + char kind[64]; + NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); + if (argc < 1) { + napi_throw_error(env, NULL, "setup(kind) needs a kind"); + return NULL; + } + NODE_API_CALL(env, napi_get_value_string_utf8(env, argv[0], kind, + sizeof(kind), NULL)); + + napi_value pinned; + if (strcmp(kind, "add_finalizer") == 0) { + // No napi_ref asked for: registered straight on the JSC heap + // (Heap::addFinalizer). + NODE_API_CALL(env, napi_create_object(env, &pinned)); + NODE_API_CALL(env, napi_add_finalizer(env, pinned, NULL, finalizer, + dup_kind(kind), NULL)); + } else if (strcmp(kind, "add_finalizer_ref") == 0) { + // Registered through a weak NapiRef (the napi_ref variant). + napi_ref weak; + NODE_API_CALL(env, napi_create_object(env, &pinned)); + NODE_API_CALL(env, napi_add_finalizer(env, pinned, NULL, finalizer, + dup_kind(kind), &weak)); + } else if (strcmp(kind, "external") == 0) { + // Runs from the NapiExternal cell's destructor. + NODE_API_CALL(env, napi_create_external(env, NULL, finalizer, + dup_kind(kind), &pinned)); + } else if (strcmp(kind, "empty_external_buffer") == 0) { + // A zero-length external buffer has no contents to hang the finalizer + // on, so it too is registered straight on the JSC heap. + NODE_API_CALL(env, napi_create_external_buffer(env, 0, NULL, finalizer, + dup_kind(kind), &pinned)); + } else { + napi_throw_error(env, NULL, "unknown kind"); + return NULL; + } + pending_finalizers++; + + // Never deleted: the object stays reachable until the VM is destroyed. + napi_ref pin; + NODE_API_CALL(env, napi_create_reference(env, pinned, 1, &pin)); + + napi_value undefined; + NODE_API_CALL(env, napi_get_undefined(env, &undefined)); + return undefined; +} + +static napi_value init(napi_env env, napi_value exports) { + NODE_API_CALL(env, napi_set_instance_data(env, &instance_data, + instance_data_finalizer, NULL)); + napi_value setup_fn; + NODE_API_CALL(env, napi_create_function(env, "setup", NAPI_AUTO_LENGTH, + setup, NULL, &setup_fn)); + NODE_API_CALL(env, napi_set_named_property(env, exports, "setup", setup_fn)); + return exports; +} + +NAPI_MODULE(test_vm_teardown_finalizers, init) diff --git a/test/napi/napi-app/vm-teardown-finalizers.js b/test/napi/napi-app/vm-teardown-finalizers.js new file mode 100644 index 000000000000..cfd3145cdb3f --- /dev/null +++ b/test/napi/napi-app/vm-teardown-finalizers.js @@ -0,0 +1,27 @@ +// Usage: bun vm-teardown-finalizers.js [,...] [--main-thread] +// +// Loads build/Debug/.node (see test_vm_teardown_finalizers.c) +// and registers one pinned finalizer per kind, in a worker_threads Worker +// that then exits, or on the main thread with --main-thread (run under +// BUN_DESTRUCT_VM_ON_EXIT=1 so that the main thread's VM is destroyed too). +// Any other argument is ignored. +const { Worker, isMainThread, workerData } = require("node:worker_threads"); +const path = require("node:path"); + +function setup({ addon, kinds }) { + const { setup } = require(path.join(__dirname, "build/Debug", addon + ".node")); + for (const kind of kinds) setup(kind); + console.log("registered:", kinds.join(",")); +} + +if (!isMainThread) { + setup(workerData); +} else { + const [addon, kindList] = process.argv.slice(2); + const args = { addon, kinds: kindList.split(",") }; + if (process.argv.includes("--main-thread")) { + setup(args); + } else { + new Worker(__filename, { workerData: args }).on("exit", code => console.log("worker exited:", code)); + } +} diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 7846b80ebeec..493fa8548e5f 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -1126,12 +1126,13 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { await checkSameOutput("bigint_to_i64", testsString); await checkSameOutput("bigint_to_u64", testsString); }); + // Three serial node + bun runs: over 5s on a debug ASAN build. it("returns the right error code", async () => { const badTypes = '[null, undefined, 5, "123", "abc"]'; await checkSameOutput("bigint_to_i64", badTypes); await checkSameOutput("bigint_to_u64", badTypes); await checkSameOutput("bigint_to_64_null", []); - }); + }, 10_000); }); describe("create_bigint_words", () => { @@ -1445,6 +1446,94 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { }, 25_000, ); + + // Destroying a JSC VM (a Worker exiting; the main thread under + // BUN_DESTRUCT_VM_ON_EXIT) fires every finalizer still registered. That + // happens after env teardown and without the "sweeping" mutator state a + // collection has, which Bun used to mistake for an ordinary non-GC context: + // it ran the addon's callbacks inline, inside the heap being destroyed. The + // fixture pins its objects with strong refs, so VM destruction is the only + // thing that can reach these finalizers; the "still pending" count shows they + // were all still registered when env teardown finished. + describe("finalizers still registered when the VM is destroyed", () => { + const fixture = join(__dirname, "napi-app/vm-teardown-finalizers.js"); + // The registration kinds env teardown (NapiEnv::cleanup) leaves registered, + // unlike napi_wrap and non-empty external buffers. If one of them starts + // being run at env teardown too, the "still pending" count below drops and + // the kind belongs in that path's tests instead of this list. + const kinds = ["add_finalizer", "add_finalizer_ref", "external", "empty_external_buffer"]; + + // The ASAN lanes export BUN_DESTRUCT_VM_ON_EXIT and LSan settings that the + // children would inherit. The variants below choose which VM is destroyed + // themselves, and the fixture's pins are never deleted (nothing of the + // addon's runs after the point they have to survive), so they keep its + // NapiEnv and the VM handle ref it holds alive, which LSan reports once the + // Worker is gone. + const { BUN_DESTRUCT_VM_ON_EXIT: _destruct, BUN_INSPECT_CONNECT_TO: _inspect, ...inheritedEnv } = bunEnv; + const fixtureEnv = (...asanOptions: string[]) => ({ + ...inheritedEnv, + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=0", ...asanOptions].filter(Boolean).join(":"), + }); + + const vms = [ + { vm: "a worker_threads Worker", flags: [] as string[], extraEnv: {} as Record }, + { vm: "the main thread", flags: ["--main-thread"], extraEnv: { BUN_DESTRUCT_VM_ON_EXIT: "1" } }, + ]; + it.each(vms)( + "a regular module's finalizers are not invoked from the dying heap of $vm", + async ({ flags, extraEnv }) => { + await using proc = spawn({ + cmd: [bunExe(), fixture, "test_vm_teardown_finalizers", kinds.join(","), ...flags], + env: { ...fixtureEnv(), ...extraEnv }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Before the fix: one "FAIL: finalizer ran after env teardown" line + // per kind, then (debug builds) an assertion failure in JSCell::JSCell + // when the finalizer's handle scope allocates in the heap being destroyed. + expect(stderr).toBe(""); + expect(stdout).toContain(`env teardown: ${kinds.length} finalizer(s) still pending`); + if (flags.length === 0) expect(stdout).toContain("worker exited: 0"); + expect(exitCode).toBe(0); + }, + ); + + // Experimental modules have their finalizers run synchronously by whatever + // frees the object, VM destruction included; what has to hold is that a + // GC-affecting call made from there is refused the same way it is during a + // collection. One kind per path JSC fires them from: the fixture's + // NapiExternal is destroyed as an individually allocated cell + // (PreciseAllocation::sweep), a napi_add_finalizer callback is a weak-handle + // finalizer (WeakBlock::lastChanceToFinalize). The first finalizer to run + // aborts the process, so each kind gets its own run. + it.each(["external", "add_finalizer"])( + "an experimental module's %s finalizer is told it is running from GC", + async kind => { + await using proc = spawn({ + // The trailing flag makes a debug build's crash handler skip its slow + // symbolized backtrace; the fixture ignores it. + cmd: [ + bunExe(), + fixture, + "test_vm_teardown_finalizers_experimental", + kind, + "--debug-crash-handler-use-trace-string", + ], + env: { ...fixtureEnv("disable_coredump=1", "symbolize=0"), BUN_INTERNAL_SUPPRESS_CRASH_ON_NAPI_ABORT: "1" }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toContain("env teardown: 1 finalizer(s) still pending"); + // Before the fix: "FAIL: finalizer called napi_get_undefined + // during VM destruction and got status 0" and a clean exit. + expect(stderr).not.toContain("FAIL:"); + expect(stderr).toContain("FATAL ERROR: Finalizer is calling a function that may affect GC state."); + expect(exitCode).not.toBe(0); + }, + ); + }); }); // Kept outside describe.concurrent("napi") so RSS measurement isn't skewed by