mirror of
https://github.com/oven-sh/bun
synced 2026-02-10 10:58:56 +00:00
## 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>
163 lines
6.1 KiB
C
163 lines
6.1 KiB
C
// Test that napi_reference_unref can be called from a finalizer
|
|
// This is a regression test for https://github.com/oven-sh/bun/issues/22596
|
|
|
|
#include <assert.h>
|
|
#include <js_native_api.h>
|
|
#include <node_api.h>
|
|
#include <stdio.h>
|
|
#include <stdlib.h>
|
|
#include <string.h>
|
|
|
|
#define NODE_API_CALL(env, call) \
|
|
do { \
|
|
napi_status status = (call); \
|
|
if (status != napi_ok) { \
|
|
const napi_extended_error_info *error_info = NULL; \
|
|
napi_get_last_error_info((env), &error_info); \
|
|
const char *err_message = error_info->error_message; \
|
|
bool is_pending; \
|
|
napi_is_exception_pending((env), &is_pending); \
|
|
if (!is_pending) { \
|
|
const char *message = \
|
|
(err_message == NULL) ? "empty error message" : err_message; \
|
|
napi_throw_error((env), NULL, message); \
|
|
} \
|
|
return NULL; \
|
|
} \
|
|
} while (0)
|
|
|
|
#define MAX_REFS 100
|
|
typedef struct {
|
|
napi_ref ref;
|
|
int index;
|
|
} RefHolder;
|
|
|
|
static RefHolder* ref_holders[MAX_REFS];
|
|
static int finalizer_called_count = 0;
|
|
static int unref_succeeded_count = 0;
|
|
static bool test_ran = false;
|
|
|
|
// Called at exit to verify the test actually ran
|
|
static void check_test_ran(void* arg) {
|
|
(void)arg;
|
|
if (!test_ran) {
|
|
fprintf(stderr, "ERROR: Test did not run properly\n");
|
|
exit(1);
|
|
}
|
|
if (finalizer_called_count == 0) {
|
|
fprintf(stderr, "ERROR: No finalizers were called\n");
|
|
exit(1);
|
|
}
|
|
if (unref_succeeded_count == 0) {
|
|
fprintf(stderr, "ERROR: No napi_reference_unref calls succeeded\n");
|
|
exit(1);
|
|
}
|
|
printf("Test completed: %d finalizers called, %d unrefs succeeded\n",
|
|
finalizer_called_count, unref_succeeded_count);
|
|
}
|
|
|
|
static void finalizer_unref(napi_env env, void *data, void *hint) {
|
|
(void)hint;
|
|
RefHolder* holder = (RefHolder*)data;
|
|
finalizer_called_count++;
|
|
|
|
if (holder && holder->ref) {
|
|
uint32_t result;
|
|
// This is the critical test - calling napi_reference_unref during GC
|
|
// This would crash with NAPI_CHECK_ENV_NOT_IN_GC if not properly handled
|
|
napi_status status = napi_reference_unref(env, holder->ref, &result);
|
|
if (status == napi_ok) {
|
|
unref_succeeded_count++;
|
|
// Try to unref again to get to 0
|
|
if (result > 0) {
|
|
status = napi_reference_unref(env, holder->ref, &result);
|
|
}
|
|
// Clean up the reference if refcount is 0
|
|
if (result == 0) {
|
|
napi_delete_reference(env, holder->ref);
|
|
holder->ref = NULL;
|
|
}
|
|
}
|
|
free(holder);
|
|
}
|
|
}
|
|
|
|
static napi_value test_reference_unref_in_finalizer(napi_env env, napi_callback_info info) {
|
|
(void)info;
|
|
|
|
test_ran = true;
|
|
|
|
// Register atexit handler on first call
|
|
static bool atexit_registered = false;
|
|
if (!atexit_registered) {
|
|
napi_add_env_cleanup_hook(env, check_test_ran, NULL);
|
|
atexit_registered = true;
|
|
}
|
|
|
|
// Create many objects with finalizers that will call napi_reference_unref
|
|
napi_value objects_array;
|
|
NODE_API_CALL(env, napi_create_array_with_length(env, MAX_REFS, &objects_array));
|
|
|
|
for (int i = 0; i < MAX_REFS; i++) {
|
|
// Create an object to hold a reference to
|
|
napi_value target_obj;
|
|
NODE_API_CALL(env, napi_create_object(env, &target_obj));
|
|
|
|
// Create a holder for this reference
|
|
RefHolder* holder = (RefHolder*)malloc(sizeof(RefHolder));
|
|
holder->index = i;
|
|
|
|
// Create a reference with refcount 2 so we can unref it in the finalizer
|
|
NODE_API_CALL(env, napi_create_reference(env, target_obj, 2, &holder->ref));
|
|
ref_holders[i] = holder;
|
|
|
|
// Create a wrapper object that will trigger the finalizer when GC'd
|
|
napi_value wrapper_obj;
|
|
NODE_API_CALL(env, napi_create_object(env, &wrapper_obj));
|
|
|
|
// Add a finalizer that will call napi_reference_unref
|
|
NODE_API_CALL(env, napi_add_finalizer(env, wrapper_obj, holder, finalizer_unref, NULL, NULL));
|
|
|
|
// Store in array
|
|
NODE_API_CALL(env, napi_set_element(env, objects_array, i, wrapper_obj));
|
|
}
|
|
|
|
printf("Created %d objects with finalizers\n", MAX_REFS);
|
|
|
|
// Return the array so JS can control when to release it
|
|
return objects_array;
|
|
}
|
|
|
|
static napi_value get_stats(napi_env env, napi_callback_info info) {
|
|
(void)info;
|
|
napi_value result;
|
|
NODE_API_CALL(env, napi_create_object(env, &result));
|
|
|
|
napi_value finalizers_called;
|
|
NODE_API_CALL(env, napi_create_int32(env, finalizer_called_count, &finalizers_called));
|
|
NODE_API_CALL(env, napi_set_named_property(env, result, "finalizersCalled", finalizers_called));
|
|
|
|
napi_value unrefs_succeeded;
|
|
NODE_API_CALL(env, napi_create_int32(env, unref_succeeded_count, &unrefs_succeeded));
|
|
NODE_API_CALL(env, napi_set_named_property(env, result, "unrefsSucceeded", unrefs_succeeded));
|
|
|
|
return result;
|
|
}
|
|
|
|
static napi_value init(napi_env env, napi_value exports) {
|
|
napi_value test_fn;
|
|
NODE_API_CALL(env, napi_create_function(env, "test_reference_unref_in_finalizer",
|
|
NAPI_AUTO_LENGTH, test_reference_unref_in_finalizer,
|
|
NULL, &test_fn));
|
|
NODE_API_CALL(env, napi_set_named_property(env, exports, "test_reference_unref_in_finalizer", test_fn));
|
|
|
|
napi_value stats_fn;
|
|
NODE_API_CALL(env, napi_create_function(env, "get_stats",
|
|
NAPI_AUTO_LENGTH, get_stats,
|
|
NULL, &stats_fn));
|
|
NODE_API_CALL(env, napi_set_named_property(env, exports, "get_stats", stats_fn));
|
|
|
|
return exports;
|
|
}
|
|
|
|
NAPI_MODULE(test_reference_unref_in_finalizer, init) |