Repository navigation
napi: stop napi_get_property_names poisoning JSC's per-Structure own-keys cache - #34130
Conversation
…keys cache
napi_get_all_property_names(include_prototypes) and napi_get_property_names
used JSC::allPropertyKeys(), which walks the prototype chain and then
writes the chain-walked list into the Structure's own-keys cache via
setCachedPropertyNames(). After enough calls on same-shaped objects,
Reflect.ownKeys({a,b}) would permanently return 14 keys (all of
Object.prototype leaked in) and Object.keys would return inherited
enumerable keys, for every object sharing that Structure, process-wide.
Replace both call sites with a local helper that walks the chain via
JSObject::getPropertyNames into a PropertyNameArrayBuilder and builds
the result JSArray directly, bypassing the cache entirely. The own-only
path keeps using ownPropertyKeys() since that cache write is correct.
|
Warning Review limit reached
Next review available in: 20 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.
No issues found, but this touches JSC-facing C++ (GC-visible array construction, identifier lifetime via jsOwnedString, Structure-cache bypass), so it's worth a human look.
Checked: collectInheritedPropertyKeys mirrors the PropertyNameArrayBuilder + getPropertyNames pattern already used in bindings.cpp and JSPropertyIterator.cpp; the Symbol::create/jsOwnedString + putDirectIndex loop matches JSC's own ownPropertyKeys shape and identifiers are ref-counted AtomStringImpls so the borrowed-string path is safe. RETURN_IF_EXCEPTION is present after both throwing calls. Confirmed allPropertyKeys had no other callers in the tree, and the napi_key_own_only branch is untouched. Test uses checkSameOutput against Node and covers both entry points, skip_symbols, own-only, and that napi results still include inherited keys.
Extended reasoning...
Overview
This PR replaces two JSC::allPropertyKeys() calls in src/jsc/bindings/napi.cpp (napi_get_all_property_names with include_prototypes, and napi_get_property_names) with a new local helper collectInheritedPropertyKeys() that walks the prototype chain via JSObject::getPropertyNames into a PropertyNameArrayBuilder and materializes a JSArray directly. The rest of the diff is test scaffolding: two thin native wrappers in js_test_helpers.cpp, a JS driver in module.js, and a checkSameOutput-based test in napi.test.ts.
Security risks
None identified. This is a read-only property-name enumeration path; no user-controlled sizes drive allocation, no auth/crypto/permission logic is involved, and the change narrows behavior (stops writing to a shared cache) rather than widening it.
Level of scrutiny
High. napi.cpp is a production-critical C++ binding on a GC-sensitive path. The new helper allocates a JSArray, iterates identifiers, and calls putDirectIndex (which can allocate and thus GC) inside a loop — exactly the class of code CLAUDE.md flags for careful review. It looks correct: keys is stack-rooted, identifiers are ref-counted AtomStringImpls so jsOwnedString is the right constructor, exception scopes are declared and checked after both fallible calls, and the pattern is byte-for-byte the same shape JSC and bindings.cpp already use for property-name-array → JSArray conversion. But a maintainer with JSC context should confirm the choice to bypass the Structure cache here (vs. fixing allPropertyKeys upstream in the WebKit fork) is the intended layering.
Other factors
- Verified
allPropertyKeyshas no other call sites in the repo, matching the PR's claim. - The
napi_key_own_onlybranch still usesownPropertyKeys(), which is correct (own keys → own-keys cache). - Test compares against Node via
checkSameOutputand asserts specific expected lines, so it can't vacuously pass; it exercisesReflect.ownKeys,Object.keys,Object.getOwnPropertyNames, own-only mode, and that the napi results themselves still include inherited keys. - The PR description notes CI (not local) verification since the napi test is platform-gated.
|
On the layering question (fix in napi.cpp vs the WebKit fork): |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Isn't the bug that teh caching-related flags don't match the flags used by napi? This sounds like a worthwhile optimization to keep as long as we call it the right way.
|
Good question. I looked at this more carefully and there's no flag combination that makes
Two ways to go:
Either way there's no caching win available for the inherit path without building prototype-chain invalidation, which is a bigger project. Happy to do whichever you prefer; (1) seems cleaner to me. |
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
Problem
napi_get_all_property_names(include_prototypes)andnapi_get_property_namespermanently corrupt plain-JSReflect.ownKeys/Object.keysprocess-wide by poisoning JSC's per-Structure own-keys cache.Repro
Any native module that enumerates properties via
napi_get_property_names(node-addon-api'sObject::GetPropertyNames()maps directly to this) silently rewrites core JS semantics for unrelated pure-JS code: dedupe, serialization, spread and shape checks on same-shaped objects start seeing prototype methods as own keys.Cause
Both call sites use
JSC::allPropertyKeys(), which routes throughgetPropertyKeys<Inherit=true>inObjectConstructor.cpp. That template collects names from the full prototype chain viagetPropertyNames, then on the second call on a cacheable Structure stores that chain-walked list viastructure->setCachedPropertyNames(vm, kind, ...)into the same per-Structure cache slot thatReflect.ownKeys(StringsAndSymbols) andObject.keys(EnumerableStrings) serve from. Bun's napi bridge is the only caller ofallPropertyKeys()in the codebase.Fix
Replace both
allPropertyKeys()call sites with a localcollectInheritedPropertyKeys()helper that walks the chain viaJSObject::getPropertyNamesinto aPropertyNameArrayBuilderand builds the resultJSArraydirectly, bypassing the Structure cache. Thenapi_key_own_onlypath keeps usingownPropertyKeys()since that cache write is correct (own keys into an own-keys cache).Verification
The test compares Bun's output against Node's for:
Reflect.ownKeysafter 20xnapi_get_all_property_names(include_prototypes, all, keep)Object.keysafter 20xnapi_get_property_nameson an object with a custom prototypeObject.getOwnPropertyNamesafter 20x chain walk withnapi_key_skip_symbolsNode's upstream
test_objectnapi suite passes unchanged.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