Allow napi_delete_reference to be called from finalizers during GC#31684
Conversation
Node.js declares napi_delete_reference with node_api_basic_env and does not perform the GC-access check: deleting the reference returned by napi_wrap from its finalize callback is the documented cleanup pattern, and node-addon-api's ObjectWrap destructor relies on it. Bun applied NAPI_CHECK_ENV_NOT_IN_GC here, so modules built with the experimental Node-API version (whose finalizers run synchronously while the collector sweeps) aborted as soon as a finalizer deleted its own reference. Also replace the hardcoded "napi_reference_unref" abort message in NapiEnv::checkGC with an accurate one; it previously blamed that function no matter which Node-API call tripped the check.
|
Warning Review limit reached
More reviews will be available in 41 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 2:05 AM PT - Jun 2nd, 2026
❌ @robobun, your commit 8b5a509 has 6 failures in
🧪 To try this PR locally: bunx bun-pr 31684That installs a local version of the PR into your bun-31684 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
#22259 is a different bug and is not fixed by this PR. Its panic is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The node-gyp rebuild re-runs on every bun version change and is occasionally flaky under resource pressure; the addon binary doesn't link against bun, so reuse it when present and retry a failed install once before giving up.
There was a problem hiding this comment.
I didn't find correctness issues, but this relaxes a GC-safety guard in a core NAPI path (deleting a NapiRef while the collector is mid-sweep) and changes pending-exception semantics for napi_delete_reference, so it's worth a human pass — and the x64-asan run for the new test was red on the prior commit.
Extended reasoning...
Overview
The PR changes napi_delete_reference in src/jsc/bindings/napi.cpp to drop NAPI_CHECK_ENV_NOT_IN_GC and the throw-scope preamble (so it no longer aborts when called from a finalizer running synchronously inside the JSC sweep, and no longer short-circuits on a pending exception), and rewords the checkGC() abort message in napi.h. It adds a new NAPI_EXPERIMENTAL C addon plus a standalone test driver and a binding.gyp target. The macro-level diff is small, but the behavioral change is in a hot, memory-safety-sensitive path used by every node-addon-api ObjectWrap.
Security risks
No direct security surface — no parsing, auth, or untrusted input. The risk profile is memory safety / UAF rather than security: delete napiRef now runs while WeakBlock::finalize is on the stack holding the same WeakImpl. The PR's argument (the WeakImpl is already Finalized, so Weak::clear() only flips it to Deallocated, mirroring NapiRefSelfDeletingWeakHandleOwner) is sound on inspection, and Node.js does declare this function with node_api_basic_env, but this is exactly the class of reasoning that benefits from a second pair of eyes familiar with JSC's heap.
Level of scrutiny
High. This is production native code in the NAPI bindings that interacts with the garbage collector's sweeping phase. A mistake here is a heap corruption or UAF that would be hard to reproduce and diagnose. The change also subtly alters semantics: napi_delete_reference will now succeed even with a pending JS exception (previously it returned napi_pending_exception). That matches Node, but it's a behavior change worth a maintainer ack.
Other factors
- CI on commit
0b9e468showed the newnapi-finalizer-delete-ref.test.tsfailing onx64-asan; a follow-up commit (5d7015f5, skip rebuild when addon exists) was pushed, but I don't see a green run reported against it yet. The musl LTO link errors look like infra flakes unrelated to this diff. - The existing sibling test (
napi_reference_unref is blocked from finalizers in experimental modules) still asserts the negative case, so the guard wasn't removed wholesale — only fornapi_delete_reference. - My earlier nit about test-file placement was answered with a reasonable rationale and the thread is resolved; not a blocker.
- No CODEOWNERS entry covers
src/jsc/bindings/napi*.
Loading any napi-app addon under BUN_JSC_validateExceptionChecks trips a pre-existing unchecked-exception report in the module registration path (napi_create_function -> napi_set_named_property), which is why the other NAPI addon tests are already listed here. The new test spawns bun with an addon and inherits the runner's env on the ASAN lane, so it needs the same exemption.
There was a problem hiding this comment.
No bugs found, but this relaxes a GC-safety guard in native NAPI code (delete NapiRef mid-sweep) — worth a human pass on the JSC weak-handle lifecycle reasoning, and CI for 8b5a509 (the no-validate-exceptions exemption) hasn't reported yet.
Extended reasoning...
Overview
The PR removes NAPI_CHECK_ENV_NOT_IN_GC from napi_delete_reference in src/jsc/bindings/napi.cpp and switches it to NAPI_PREAMBLE_NO_THROW_SCOPE + napi_clear_last_error, so that experimental-version NAPI modules can delete their own napi_wrap/napi_add_finalizer reference from inside a finalizer that runs synchronously during the JSC heap sweep. It also replaces the misleading hardcoded napi_reference_unref abort message in NapiEnv::checkGC(). Supporting changes add a new C addon, a standalone test driver, a binding.gyp target, and a no-validate-exceptions.txt entry.
Security risks
None identified. This is a crash-fix in the native addon runtime; no auth, network, filesystem, or untrusted-input parsing is involved. The change loosens a guard rather than tightening one, but only to match Node.js's documented contract for node_api_basic_env functions.
Level of scrutiny
High. The functional change is small (~10 lines in napi.cpp plus a string in napi.h), but it sits at the intersection of JSC garbage collection, weak-handle finalization, and native heap deletion. Correctness rests on the claim that delete napiRef is safe while WeakBlock::finalize is on the stack — i.e., that the WeakImpl is already Finalized so Weak::clear() only flips it to Deallocated, and that ~NapiRef (which also touches strongRef, boundCleanup->deactivate(), and the env's finalizer set) does nothing that perturbs the collector. The argument in the PR description is well-reasoned and mirrors what NapiRefSelfDeletingWeakHandleOwner already does, but it's exactly the kind of invariant a maintainer familiar with the JSC heap should sign off on rather than a bot.
Other factors
- The new test failed on
x64-asanat commit 5d7015f; the follow-up commit 8b5a509 adds it totest/no-validate-exceptions.txt(the validateExceptionChecks opt-out list alongside other NAPI tests), but robobun hasn't posted updated CI results for that commit yet. - My earlier inline nit about test-file placement was answered and resolved by the author; nothing further on that.
- No CODEOWNERS entry covers
src/jsc/bindings/napi.cpp. - The bug-hunting system found no issues.
2 participants
What does this PR do?
Fixes the
napi_reference_unrefpanic (Sentry BUN-39FR, 11 crashes/14d on 1.4.0-canary, also present on 1.3.x stable) that fires when a NAPI addon's finalize callback callsnapi_delete_referenceon its own reference while the garbage collector is sweeping.Reproduction
A module built with the experimental Node-API version runs its finalizers synchronously from the weak-handle sweep (
MarkedBlock::Handle::sweep→WeakSet::sweep→WeakBlock::finalize→NapiRefWeakHandleOwner::finalize). If such a finalizer callsnapi_delete_reference— e.g. node-addon-api'sObjectWrapdestructor deleting the ref returned bynapi_wrap, which is exactly the two addon frames in the crash stacks — Bun aborted:Reproduced on Linux with
Bun.gc(true)(→collectNow(Sync)→sweepSynchronously()), which enters the sameSweepingScopeas the incremental sweeper timer seen in the wild — the all-Windows telemetry is population skew, not a platform difference; the guard is shared C++.Cause
napi_delete_referenceusedNAPI_CHECK_ENV_NOT_IN_GC. Node.js deliberately does not: it declares the function withnode_api_basic_envand plainCHECK_ENV, because deleting thenapi_wrapreference from its finalize callback is the documented cleanup pattern ("For a napi_reference returned fromnapi_wrap, this must be called in the finalizer." — node/src/js_native_api_v8.cc).NapiEnv::checkGC()was hardcoded tonapi_reference_unrefregardless of which API tripped the check, which is why the crash reports blame the wrong function.Fix
napi_delete_referenceno longer performs the GC-access check, and usesNAPI_PREAMBLE_NO_THROW_SCOPE+napi_clear_last_errorto match Node's "omit NAPI_PREAMBLE, calls here cannot throw" semantics (it must also work with a pending exception). Deleting theNapiRefmid-sweep is safe: itsWeakImplis already in theFinalizedstate, soWeak::clear()only marks itDeallocated— the same patternNapiRefSelfDeletingWeakHandleOwneralready uses.checkGC()aborts with an accurate message.napi_reference_unref(and other non-basic functions) remain blocked from in-GC finalizers, matching Node — the existingnapi_reference_unref is blocked from finalizers in experimental modulestest still covers that.Verification
New addon
test/napi/napi-app/test_delete_ref_in_finalizer_experimental.c(NAPI_EXPERIMENTAL) wraps objects withnapi_wrap/napi_add_finalizerwhose finalizers delete their own ref, driven bytest/napi/napi-finalizer-delete-ref.test.ts:panic(main thread): napi_reference_unref— the exact production signaturenapi_delete_referencesucceeds; fulltest/napi/napi.test.tssuite still passes