Commit Graph

25 Commits

Author SHA1 Message Date
Dylan Conway
2f510724a9 fix(napi): return napi_function for AsyncContextFrame in napi_typeof (#26511)
## Summary
- `napi_typeof` was returning `napi_object` for `AsyncContextFrame`
values, which are internally callable JSObjects
- Native addons that check callback types (e.g. encore.dev's runtime)
would fail with `expect Function, got: Object` and panic
- Added a `jsDynamicCast<AsyncContextFrame*>` check before the final
`napi_object` fallback to correctly report these values as
`napi_function`

Closes #25933

## Test plan
- [x] Verify encore.dev + supertokens reproduction from the issue no
longer panics
- [ ] Existing napi tests continue to pass

Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-01-27 13:35:15 -08:00
robobun
6ce419d3f8 fix(napi): napi_typeof returns napi_object for String objects (#25365)
## Summary

- Fix `napi_typeof` to return `napi_object` for boxed String objects
(`new String("hello")`) instead of incorrectly returning `napi_string`
- Add regression test for boxed primitive objects (String, Number,
Boolean)

The issue was that `StringObjectType` and `DerivedStringObjectType` JSC
cell types were falling through to return `napi_string`, but these
represent object wrappers around strings, not primitive strings.

## Test plan

- [x] `bun bd test test/napi/napi.test.ts -t "napi_typeof"` passes
- [x] Test fails with `USE_SYSTEM_BUN=1` (confirming the bug exists in
released version)

Fixes #25351

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Bot <claude-bot@bun.sh>
Co-authored-by: Claude <noreply@anthropic.com>
2025-12-05 18:27:06 -08:00
robobun
52b82cbe40 Fix: Allow napi_reference_unref to be called during GC (#22597)
## 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>
2025-09-12 23:13:44 -07:00
robobun
99c3824b31 fix(napi): Make cleanup hooks behavior match Node.js exactly (#21883)
# Fix NAPI cleanup hook behavior to match Node.js

This PR addresses critical differences in NAPI cleanup hook
implementation that cause crashes when native modules attempt to remove
cleanup hooks. The fixes ensure Bun's behavior matches Node.js exactly.

## Issues Fixed

Fixes #20835
Fixes #18827
Fixes #21392
Fixes #21682
Fixes #13253

All these issues show crashes related to NAPI cleanup hook management:
- #20835, #18827, #21392, #21682: Show "Attempted to remove a NAPI
environment cleanup hook that had never been added" crashes with
`napi_remove_env_cleanup_hook`
- #13253: Shows `napi_remove_async_cleanup_hook` crashes in the stack
trace during Vite dev server cleanup

## Key Behavioral Differences Addressed

### 1. Error Handling for Non-existent Hook Removal
- **Node.js**: Silently ignores removal of non-existent hooks (see
`node/src/cleanup_queue-inl.h:27-30`)
- **Bun Before**: Crashes with `NAPI_PERISH` error
- **Bun After**: Silently ignores, matching Node.js behavior

### 2. Duplicate Hook Prevention 
- **Node.js**: Uses `CHECK_EQ` which crashes in ALL builds when adding
duplicate hooks (see `node/src/cleanup_queue-inl.h:24`)
- **Bun Before**: Used debug-only assertions
- **Bun After**: Uses `NAPI_RELEASE_ASSERT` to crash in all builds,
matching Node.js

### 3. VM Termination Checks
- **Node.js**: No VM termination checks in cleanup hook APIs
- **Bun Before**: Had VM termination checks that could cause spurious
failures
- **Bun After**: Removed VM termination checks to match Node.js

### 4. Async Cleanup Hook Handle Validation
- **Node.js**: Validates handle is not NULL before processing
- **Bun Before**: Missing NULL handle validation 
- **Bun After**: Added proper NULL handle validation with
`napi_invalid_arg` return

## Execution Order Verified

Both Bun and Node.js execute cleanup hooks in LIFO order (Last In, First
Out) as expected.

## Additional Architectural Differences Identified

Two major architectural differences remain that affect compatibility but
don't cause crashes:

1. **Queue Architecture**: Node.js uses a single unified queue for all
cleanup hooks, while Bun uses separate queues for regular vs async
cleanup hooks
2. **Iteration Safety**: Different behavior when hooks are added/removed
during cleanup iteration

These will be addressed in future work as they require more extensive
architectural changes.

## Testing

- Added comprehensive test suite covering all cleanup hook scenarios
- Tests verify identical behavior between Bun and Node.js
- Includes edge cases like duplicate hooks, non-existent removal, and
execution order
- All tests pass with the current fixes

The changes ensure NAPI modules using cleanup hooks (like LMDB, native
Rust modules, etc.) work reliably without crashes.

---------

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: Kai Tamkun <kai@tamkun.io>
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2025-08-15 21:08:53 -07:00
Dylan Conway
c3c2dccc55 Fix N-API BigInt word count issue (#21652)
## Summary
Fixes a bug in napi_get_value_bigint_words where the function would
return the number of words copied instead of the actual word count
needed when the provided buffer is smaller than required.

## The Problem
When napi_get_value_bigint_words was called with a buffer smaller than
the actual BigInt size, it would incorrectly return the buffer size
instead of the actual word count needed. This doesn't match Node.js
behavior.

### Example
BigInt that requires 2 words: 0x123456789ABCDEF0123456789ABCDEFn
Call with buffer for only 1 word
- Before fix: word_count = 1 (buffer size)
- After fix: word_count = 2 (actual words needed)

## The Fix
Changed napi_get_value_bigint_words to always set word_count to the
actual number of words in the BigInt, regardless of buffer size.

## Test Plan
- Added test test_bigint_word_count that verifies the word count is
correctly returned
- Added test test_ref_unref_underflow for the existing
napi_reference_unref underflow protection
- Both tests pass with the fix and match Node.js behavior

🤖 Generated with [Claude Code](https://claude.ai/code)

---------

Co-authored-by: Claude <noreply@anthropic.com>
2025-08-07 18:15:12 -07:00
190n
03dfd7d96b Run callback passed to napi_module_register after dlopen returns instead of during call (#20478) 2025-07-24 11:46:56 -07:00
Jarred Sumner
bc79a48ce4 Fix crash in NapiClass_ConstructorFunction due to incorrect handling of newTarget (#20552)
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
2025-06-23 19:50:09 -07:00
Ben Grant
1ebec90d6e Revert "Add test from #18287 (#19775)"
This reverts commit f1504c4265.
2025-05-20 12:22:01 -07:00
190n
f1504c4265 Add test from #18287 (#19775) 2025-05-20 11:56:30 -07:00
Dylan Conway
ba0bd426ed deflake napi_async_work test (#18836) 2025-04-07 18:52:05 -07:00
Dylan Conway
340ae94d0f napi_async_work fixes (#18825) 2025-04-07 05:20:24 -07:00
190n
cde668b54c Better edge case handling in napi_value<->String conversion (#18107) 2025-03-12 18:15:00 -07:00
190n
efabdcbe1f Start fixing bugs discovered by Node.js's Node-API tests (#14501)
Co-authored-by: Kai Tamkun <kai@tamkun.io>
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Co-authored-by: Ashcon Partovi <ashcon@partovi.net>
Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
Co-authored-by: Dylan Conway <35280289+dylan-conway@users.noreply.github.com>
Co-authored-by: 190n <190n@users.noreply.github.com>
2025-02-26 22:11:42 -08:00
Don Isaac
c3c27b8e0d fix(napi): napi_get_value_uint32 now handles int32s correctly 2025-01-18 12:08:26 -08:00
190n
59e06b0df5 fix(napi): set lossless parameter in napi_get_value_bigint_{int64,uint64}, and trim leading zeroes in napi_create_bigint_words (#15804) 2024-12-17 17:38:12 -08:00
190n
4eae3a90e8 fix(napi): Make napi_wrap work on regular objects (#15622)
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2024-12-16 15:54:39 -08:00
190n
71fdb59918 Fix napi property methods on non-objects (#14935) 2024-10-31 21:02:26 -07:00
190n
9647291d73 Implement NAPI type tagging (#14915) 2024-10-30 19:57:48 -07:00
190n
50e9be0dc7 Fix napi_value<=>integer conversions and napi_create_empty_array (#14479) 2024-10-10 23:50:39 -07:00
190n
50bb5fa1f6 Fix napi_throw_*/napi_create_*_error (#14446) 2024-10-10 02:35:38 -07:00
190n
b0b38b42ba Return undefined from napi_get_property when property does not exist (#14366) 2024-10-07 18:05:31 -07:00
190n
0a54c24bd3 Allow throwing exceptions from napi_async_complete_callback (#14302)
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Co-authored-by: Jarred-Sumner <Jarred-Sumner@users.noreply.github.com>
2024-10-02 22:35:45 -07:00
190n
08a77267da Keep event loop alive when refConcurrently has been called (#14068) 2024-09-20 14:57:55 -07:00
Jarred Sumner
c48997050d Avoid creating a Napi handle scope within a finalizer (#13870)
Co-authored-by: Ben Grant <ben@bun.sh>
2024-09-11 20:05:44 -07:00
190n
084734db64 Implement napi_handle_scope and napi_escapable_handle_scope (#13756) 2024-09-07 00:55:19 -07:00