Repository navigation
napi: fix napi_get_all_property_names(own_only, skip_strings|skip_symbols) - #34126
Conversation
napi_get_all_property_names mapped napi_key_collection_mode (own_only vs
include_prototypes) onto JSC's DontEnumPropertiesMode (whether to return
non-enumerable properties). These are orthogonal axes: the collection
mode only picks between ownPropertyKeys and allPropertyKeys.
With napi_key_own_only this resulted in DontEnumPropertiesMode::Exclude,
which:
- napi_key_skip_strings -> ownPropertyKeys(Symbols, Exclude), which
JSC asserts against in inferCachedPropertyNamesKind and aborts in
assert builds
- napi_key_skip_symbols -> ownPropertyKeys(Strings, Exclude), which
silently drops non-enumerable own string keys even though the caller
did not request enumerable-only filtering
Always pass DontEnumPropertiesMode::Include to JSC. The existing
second-pass filter already applies napi_key_enumerable when requested.
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the old code conflated napi_key_collection_mode with DontEnumPropertiesMode; always passing Include and letting the existing filter loop apply napi_key_enumerable is the right fix.
Checked that the include_prototypes branch was already Include before this change, so only own_only behavior changes.
Verified the second-pass filter loop at napi.cpp:1851-1889 already gates on napi_key_enumerable/writable/configurable, so no filtering is lost.
Confirmed the JS test's enum constants (own_only=1, enumerable=1<<1, skip_strings=1<<3, skip_symbols=1<<4) match js_native_api_types.h.
Test uses checkSameOutput (Bun vs Node) plus explicit expected values, so it can't pass by both being wrong the same way.
Extended reasoning...
Overview
One-line functional change in src/jsc/bindings/napi.cpp's napi_get_all_property_names: DontEnumPropertiesMode is now unconditionally Include instead of being derived from napi_key_collection_mode. The removed else branch (which forced Include for StringsAndSymbols to dodge a JSC assert) and a stale TODO comment are subsumed. The rest is test scaffolding: a native get_all_property_names helper in js_test_helpers.cpp, a JS driver in module.js, and a checkSameOutput case in napi.test.ts.
Correctness
The old mapping was semantically wrong: napi_key_collection_mode decides own-vs-prototype-chain (already handled by the ownPropertyKeys/allPropertyKeys branch at napi.cpp:1843-1847), while DontEnumPropertiesMode decides whether non-enumerable keys are returned. Deriving the latter from the former meant own_only silently dropped non-enumerable keys and, with skip_strings, hit JSC's inferCachedPropertyNamesKind assert (Symbols + Exclude is unsupported). I read the surrounding filter loop (napi.cpp:1851-1889) and confirmed it already applies napi_key_enumerable | napi_key_writable | napi_key_configurable when set, so the enumerable filter is not lost — it just moves to the correct axis. For napi_key_include_prototypes the mode was already Include, so that path is unchanged.
Security risks
None. This is property-name enumeration; no auth, crypto, untrusted parsing, or resource limits involved. Returning more own keys (the non-enumerable ones the caller asked for by not setting napi_key_enumerable) matches Node's documented contract.
Level of scrutiny
Low-to-moderate. The native change is a single well-reasoned constant, the root cause is clearly stated and matches the code I read, and the include_prototypes path is provably untouched. NAPI is production-critical but this function is narrow.
Other factors
Test quality is good: checkSameOutput compares Bun against Node on the same addon, and the test additionally asserts the exact expected key lists, so a shared-bug pass is impossible. Five filter combos cover both the assert-tripping case (own_only + skip_strings) and the silent-drop case (own_only + skip_symbols), plus the | enumerable variants to prove the filter loop still works. Enum constants in the JS driver match the N-API header. No CODEOWNERS on this path, no prior reviewer comments to address.
napi_get_version() returned 9 while the full v10 symbol set (node_api_create_external_string_*, node_api_create_property_key_*) is exported and functional, and process.versions.napi already reads "10". Addons that feature-detect via napi_get_version() >= 10 took the fallback path even though the v10 APIs work. Also fixes the two v10 gaps that actually existed: node_api_create_external_string_latin1/utf16 rejected length==0 with napi_invalid_arg because WTF::ExternalStringImpl does not allow empty strings. Now return jsEmptyString, set *copied=false, and invoke the finalizer immediately, matching Node.js/V8. The utf16 variant was also never writing *copied on success; it now does. Unblocks the upstream test_string empty-string cases that were guarded for Bun, and bumps the vendored test_general expected version. Also deduplicates get_all_property_names in the test addon (merged twice via #34126 and #34130), which was preventing the napi test addon from compiling on main.
…rray napi_is_typedarray already reported true for a Float16Array, but both napi_get_typedarray_info and napi_create_typedarray(napi_float16_array) returned napi_invalid_arg because the napi_typedarray_type <-> JSC type tables stopped at biguint64. Add napi_float16_array (= 11, matching Node's js_native_api_types.h) to the vendored header, to the Rust napi_typedarray_type enum and its from_js_type mapping, and to the three switches in napi.cpp that drive napi_create_typedarray. Also dedup a get_all_property_names helper in the napi test addon that was left doubly defined after #34126 and #34130 both added one; the addon did not compile on main.
## What `napi_detach_arraybuffer` on a `SharedArrayBuffer` returned `napi_ok` without detaching anything. Node returns `napi_arraybuffer_expected` (19). ## Repro ```c // sab = new SharedArrayBuffer(8); new Uint8Array(sab)[0] = 7; napi_status sd = napi_detach_arraybuffer(env, sab); // node: sd=19 (napi_arraybuffer_expected) // bun : sd=0 (napi_ok) <- claims success, nothing detached // aftermath in BOTH engines: sab.byteLength == 8, view[0] == 7 ``` ## Cause JSC backs `SharedArrayBuffer` with the same `JSC::JSArrayBuffer` cell type as a plain `ArrayBuffer`, so `dynamicDowncast<JSArrayBuffer>` succeeds. The `isDetachable()` check (which is false for shared buffers) then silently skipped the detach and fell through to `NAPI_RETURN_SUCCESS`. V8 treats `SharedArrayBuffer` as a distinct type, so Node's `value->IsArrayBuffer()` check rejects it up front. The `napi_ok` here is a memory-lifetime lie: an addon that trusts it may free or recycle a backing store that JS (and other threads holding the same SAB) still read and write. ## Fix In `napi_detach_arraybuffer`: reject shared buffers with `napi_arraybuffer_expected` (matching Node's `IsArrayBuffer()` gate), then require `isDetachable()` with `napi_detachable_arraybuffer_expected` instead of succeeding as a no-op. Detaching an already-detached buffer remains `napi_ok`. Also aligns `napi_is_detached_arraybuffer` with Node: it now returns `napi_ok` with `result=false` for any non-ArrayBuffer value (typed arrays, SAB, anything else) instead of `napi_arraybuffer_expected`. ## Verification New `test_detach_arraybuffer` in the napi-app addon prints the status for `[SharedArrayBuffer, ArrayBuffer, <same ArrayBuffer again>, Uint8Array]` and `checkSameOutput` asserts Bun matches Node byte-for-byte. Fails on system Bun (SAB row: `napi_detach_arraybuffer=0` vs Node's `19`; Uint8Array row: `napi_is_detached_arraybuffer=19` vs Node's `0`), passes with this change. ## Also in this PR Deduplicates `get_all_property_names` in `test/napi/napi-app/js_test_helpers.cpp`. #34126 and #34130 each landed a definition with a different return shape and the addon no longer compiled on main; kept the `{status, keys}` form and updated the one caller that expected a bare array. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts <!-- robobun:evidence:end -->
## Problem
`napi_adjust_external_memory` silently drops negative `change_in_bytes`,
so the external-memory counter Bun reports can only grow. Every
well-behaved addon that decrements in its finalizers (the documented
`Napi::MemoryManagement::AdjustExternalMemory` pattern) leaks phantom GC
pressure for the life of the process, and the documented return value
("the adjusted value") diverges from Node on the first decrease.
### Repro
```c
int64_t a, b, c;
napi_adjust_external_memory(env, 8192, &a); // allocate: report +8192
napi_adjust_external_memory(env, -8192, &b); // finalizer: report the free
napi_adjust_external_memory(env, 0, &c); // read back
// node: a-b=8192 b-c=0 (decrease applied; counter returns to baseline)
// bun : a-b=0 b-c=0 (decrease dropped; counter stuck +8192 forever)
```
## Cause
`src/jsc/bindings/napi.cpp`:
```cpp
if (change_in_bytes > 0) {
heap.deprecatedReportExtraMemory(change_in_bytes);
}
*adjusted_value = heap.extraMemorySize();
```
JSC's `deprecatedReportExtraMemory` has no decrement path (V8's
`AdjustAmountOfExternalAllocatedMemory` is signed), so negatives are
guarded out, and `heap.extraMemorySize()` is the VM-wide extra-memory
total rather than the napi-reported running total.
## Fix
Keep a signed `int64_t m_externalMemory` accumulator on `NapiEnv`. Apply
both directions to the accumulator, forward only positive growth to the
JSC heap (there is still no decrement API), and return the accumulator
as the adjusted value so the trajectory matches Node.
## Verification
```
$ bun bd test test/napi/napi.test.ts -t "napi_adjust_external_memory"
(pass) napi > napi_adjust_external_memory > applies negative deltas and reports the running total
```
The test compares Bun's output against Node's (`checkSameOutput`) for
`+8192 / -8192 / 0` and asserts the exact deltas (`+8192, -8192, 0, 0`).
---
Also deduplicates `get_all_property_names` in
`test/napi/napi-app/js_test_helpers.cpp`: #34126 and #34130 each added a
static function with that name, so the test addon failed to compile on
main. The `{status, keys}` variant is kept and the one call site that
expected a raw array now reads `.keys`.
<!-- robobun:evidence:begin -->
---
**no test proof** · iteration 1 · Platform-specific test(s) that do not
run on this machine. Deferring to CI, which covers all platforms:
test/napi/napi.test.ts
<!-- robobun:evidence:end -->
… in Node v26 Implements and exports node_api_set_prototype, node_api_create_object_with_properties, node_api_create_sharedarraybuffer, node_api_create_external_sharedarraybuffer, and node_api_is_sharedarraybuffer. These are the only five node_api_*/napi_* symbols from Node v26 that Bun does not already export. Because addon binding is lazy, an addon that references any of them loads cleanly on Bun and then dies with an uncatchable ld.so "symbol lookup error" (exit 127) the first time the code path is hit, with no JS exception to catch. The new tests in standalone_tests.cpp take link-time references to all five symbols and compare their behaviour against the ABI-matching Node (currently v26.3.0) via checkSameOutput. Also de-duplicates get_all_property_names in js_test_helpers.cpp, where two recent PRs (#34126, #34130) each added a helper with that name and the addon stopped building.
2 participants
Reproduction
With a native addon calling
napi_get_all_property_names:Cause
napi_get_all_property_namesmappednapi_key_collection_mode(whether to walk the prototype chain) onto JSC'sDontEnumPropertiesMode(whether to return non-enumerable properties):DontEnumPropertiesMode jsc_key_mode = key_mode == napi_key_include_prototypes ? DontEnumPropertiesMode::Include : DontEnumPropertiesMode::Exclude;These are orthogonal. The collection mode is already handled by the
ownPropertyKeys/allPropertyKeysbranch below. Withnapi_key_own_onlythis producedExclude, so:napi_key_skip_stringsreachedownPropertyKeys(Symbols, Exclude), a combination JSC asserts against ininferCachedPropertyNamesKind(onlyStringssupportsExclude), aborting assert builds.napi_key_skip_symbolsreachedownPropertyKeys(Strings, Exclude), silently dropping non-enumerable own keys even though the caller did not setnapi_key_enumerable.Fix
Always pass
DontEnumPropertiesMode::Includeto JSC. The existing second-pass filter loop already appliesnapi_key_enumerable/napi_key_writable/napi_key_configurablewhen the caller requests them, so no behavior is lost.Verification
New test in
test/napi/napi.test.ts(napi_get_all_property_names > own_only with skip_strings/skip_symbols) compares Bun vs Node output for five filter combinations over an object with enumerable and non-enumerable string and symbol own keys.bun bd, src/ stashed): aborts with thedontEnumPropertiesMode == Includeassertion.own_only + skip_symbolsreturns["x"]instead of["x","ne"];own_only + skip_stringsreturns[Symbol(s)]instead of[Symbol(s), Symbol(nes)].test/napi/node-napi-tests/test/js-native-api/test_objectalso passes.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts