mirror of
https://github.com/oven-sh/bun
synced 2026-02-09 10:28:47 +00:00
52b82cbe40
## Summary - Fixes #22596 where Nuxt crashes when building with rolldown-vite - Aligns Bun's NAPI GC safety checks with Node.js behavior by only enforcing them for experimental NAPI modules ## The Problem Bun was incorrectly enforcing GC safety checks (`NAPI_CHECK_ENV_NOT_IN_GC`) for ALL NAPI modules, regardless of version. This caused crashes when regular production NAPI modules called `napi_reference_unref` from finalizers, which is a common pattern in the ecosystem (e.g., rolldown-vite). The crash manifested as: ``` panic: Aborted - napi.h:306: napi_reference_unref ``` ## Root Cause: What We Did Wrong Our previous implementation always enforced the GC check for all NAPI modules: **Before (incorrect):** ```cpp // src/bun.js/bindings/napi.h:304-311 void checkGC() const { NAPI_RELEASE_ASSERT(!inGC(), "Attempted to call a non-GC-safe function inside a NAPI finalizer..."); // This was called for ALL modules, not just experimental ones } ``` This was overly restrictive and didn't match Node.js's behavior, causing legitimate use cases to crash. ## The Correct Solution: How Node.js Does It After investigating Node.js source code, we found that Node.js **only enforces GC safety checks for experimental NAPI modules**. Regular production modules are allowed to call functions like `napi_reference_unref` from finalizers for backward compatibility. ### Evidence from Node.js Source Code **1. The CheckGCAccess implementation** (`vendor/node/src/js_native_api_v8.h:132-143`): ```cpp void CheckGCAccess() { if (module_api_version == NAPI_VERSION_EXPERIMENTAL && in_gc_finalizer) { // Only fails if BOTH conditions are true: // 1. Module is experimental (version 2147483647) // 2. Currently in GC finalizer v8impl::OnFatalError(...); } } ``` **2. NAPI_VERSION_EXPERIMENTAL definition** (`vendor/node/src/js_native_api.h:9`): ```cpp #define NAPI_VERSION_EXPERIMENTAL 2147483647 // INT_MAX ``` **3. How it's used in napi_reference_unref** (`vendor/node/src/js_native_api_v8.cc:2814-2819`): ```cpp napi_status NAPI_CDECL napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) { CHECK_ENV_NOT_IN_GC(env); // This check only fails for experimental modules // ... rest of implementation } ``` ## Our Fix: Match Node.js Behavior Exactly **After (correct):** ```cpp // src/bun.js/bindings/napi.h:304-315 void checkGC() const { // Only enforce GC checks for experimental NAPI versions, matching Node.js behavior // See: https://github.com/nodejs/node/blob/main/src/js_native_api_v8.h#L132-L143 if (m_napiModule.nm_version == NAPI_VERSION_EXPERIMENTAL) { NAPI_RELEASE_ASSERT(!inGC(), ...); } // Regular modules (version <= 8) can call napi_reference_unref from finalizers } ``` This change means: - **Regular NAPI modules** (version 8 and below): ✅ Can call `napi_reference_unref` from finalizers - **Experimental NAPI modules** (version 2147483647): ❌ Cannot call `napi_reference_unref` from finalizers ## Why This Matters Many existing NAPI modules in the ecosystem were written before the stricter GC rules and rely on being able to call functions like `napi_reference_unref` from finalizers. Node.js maintains backward compatibility by only enforcing the stricter rules for modules that explicitly opt into experimental features. By not matching this behavior, Bun was breaking existing packages that work fine in Node.js. ## Test Plan Added comprehensive tests that verify both scenarios: ### 1. test_reference_unref_in_finalizer.c (Regular Module) - Uses default NAPI version (8) - Creates 100 objects with finalizers that call `napi_reference_unref` - **Expected:** Works without crashing - **Result:** ✅ Passes with both Node.js and Bun (with fix) ### 2. test_reference_unref_in_finalizer_experimental.c (Experimental Module) - Uses `NAPI_VERSION_EXPERIMENTAL` (2147483647) - Creates objects with finalizers that call `napi_reference_unref` - **Expected:** Crashes with GC safety assertion - **Result:** ✅ Correctly fails with both Node.js and Bun (with fix) ## Verification The tests prove our fix is correct: ```bash # Regular module - should work $ bun-debug --expose-gc main.js test_reference_unref_in_finalizer '[]' ✅ SUCCESS: napi_reference_unref worked in finalizers without crashing # Experimental module - should fail $ bun-debug --expose-gc main.js test_reference_unref_in_finalizer_experimental '[]' ✅ ASSERTION FAILED: Attempted to call a non-GC-safe function inside a NAPI finalizer ``` Both behaviors now match Node.js exactly. ## Impact This fix: 1. Resolves crashes with rolldown-vite and similar packages 2. Maintains backward compatibility with the Node.js ecosystem 3. Still enforces safety for experimental NAPI features 4. Aligns Bun's behavior with Node.js's intentional design decisions 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude Bot <claude-bot@bun.sh> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: Zack Radisic <zack@theradisic.com>
21 lines
628 B
JavaScript
21 lines
628 B
JavaScript
// Test script for experimental NAPI module that should crash
|
|
const m = require('./build/Debug/test_reference_unref_in_finalizer_experimental.node');
|
|
|
|
console.log('Loading experimental module...');
|
|
let arr = m.test_reference_unref_in_finalizer_experimental();
|
|
console.log('Test function returned');
|
|
|
|
// Clear reference and force GC
|
|
arr = null;
|
|
|
|
if (global.gc) {
|
|
global.gc();
|
|
console.log('GC triggered - should crash now');
|
|
} else if (process.isBun && Bun.gc) {
|
|
Bun.gc(true);
|
|
console.log('GC triggered - should crash now');
|
|
}
|
|
|
|
// This should never print
|
|
console.log('ERROR: Did not crash! Test failed!');
|
|
process.exit(1); |