From 2c32a8f43ae9d3330e4f543da04e911438b56d7f Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:01:54 -0700 Subject: [PATCH 01/21] Fix napi_get_property_names conformance on JSC, ChakraCore and QuickJS napi_get_property_names is specified to return the enumerable string-keyed properties of an object *and of its prototype chain* -- the same set a `for...in` loop visits. Only the V8 backend did that, via GetPropertyNames configured with kIncludePrototypes | ONLY_ENUMERABLE | SKIP_SYMBOLS. The other three each diverged: | backend | enumerable-only | includes prototypes | throws | |----------------|-----------------|---------------------|--------| | V8 | yes | yes | no | | JavaScriptCore | n/a | n/a | yes | | ChakraCore | no | no | no | | QuickJS | yes | no | no | JavaScriptCore was outright broken: it called Object.getOwnPropertyNames with argc 0, so the `object` argument was never used and the call always threw "TypeError: undefined is not an object". ChakraCore used JsGetOwnPropertyNames, which is own-only and also reports non-enumerable properties. QuickJS used JS_GetOwnPropertyNames with JS_GPN_ENUM_ONLY, which is enumerable-only but still own-only. None of the three engines exposes a native equivalent of V8's key collection, so add a single shared implementation that walks the prototype chain explicitly, written purely against the public napi_* surface, and have all three backends delegate to it. Per level it takes Object.keys for the properties `for...in` reports, and Object.getOwnPropertyNames for the shadowing set: a non-enumerable own property is not reported itself, but it does hide a same-named enumerable property further up the chain. JavaScriptCore's JSObjectCopyPropertyNames was considered for that backend since it walks the chain natively, but it drops the shadowing rule, so the shared walk is used there too and all backends stay consistent. Adds nine script tests, exercised through a new napiGetPropertyNames global that calls Napi::Object::GetPropertyNames. Verified locally on ChakraCore and QuickJS (225 passing, 0 failing on both). As a negative control, five of the nine fail when the old ChakraCore implementation is restored. Fixes #216 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Core/Node-API/CMakeLists.txt | 12 +- Core/Node-API/Source/js_native_api_chakra.cc | 11 +- .../Source/js_native_api_javascriptcore.cc | 11 +- Core/Node-API/Source/js_native_api_quickjs.cc | 27 +---- Core/Node-API/Source/js_native_api_shared.cc | 114 ++++++++++++++++++ Core/Node-API/Source/js_native_api_shared.h | 22 ++++ Tests/UnitTests/Scripts/tests.ts | 86 +++++++++++++ Tests/UnitTests/Shared/Shared.cpp | 10 ++ 8 files changed, 260 insertions(+), 33 deletions(-) create mode 100644 Core/Node-API/Source/js_native_api_shared.cc create mode 100644 Core/Node-API/Source/js_native_api_shared.h diff --git a/Core/Node-API/CMakeLists.txt b/Core/Node-API/CMakeLists.txt index 5f495695..04977aa5 100644 --- a/Core/Node-API/CMakeLists.txt +++ b/Core/Node-API/CMakeLists.txt @@ -51,13 +51,17 @@ if(NAPI_BUILD_ABI) set(SOURCES ${SOURCES} "Source/env_quickjs.cc" "Source/js_native_api_quickjs.cc" - "Source/js_native_api_quickjs.h") + "Source/js_native_api_quickjs.h" + "Source/js_native_api_shared.cc" + "Source/js_native_api_shared.h") set(LINK_LIBRARIES ${LINK_LIBRARIES} PUBLIC qjs) elseif(NAPI_JAVASCRIPT_ENGINE STREQUAL "Chakra") set(SOURCES ${SOURCES} "Source/env_chakra.cc" "Source/js_native_api_chakra.cc" - "Source/js_native_api_chakra.h") + "Source/js_native_api_chakra.h" + "Source/js_native_api_shared.cc" + "Source/js_native_api_shared.h") set(LINK_LIBRARIES ${LINK_LIBRARIES} INTERFACE "chakrart.lib") @@ -65,7 +69,9 @@ if(NAPI_BUILD_ABI) set(SOURCES ${SOURCES} "Source/env_javascriptcore.cc" "Source/js_native_api_javascriptcore.cc" - "Source/js_native_api_javascriptcore.h") + "Source/js_native_api_javascriptcore.h" + "Source/js_native_api_shared.cc" + "Source/js_native_api_shared.h") if(ANDROID) set(V8_PACKAGE_NAME "jsc-android") diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 6e5d3e72..3c4bf5f3 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -1,4 +1,5 @@ #include "js_native_api_chakra.h" +#include "js_native_api_shared.h" #include #include #include @@ -678,11 +679,13 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* result) { CHECK_ENV(env); + CHECK_ARG(env, object); CHECK_ARG(env, result); - JsValueRef obj = reinterpret_cast(object); - JsValueRef propertyNames; - CHECK_JSRT(env, JsGetOwnPropertyNames(obj, &propertyNames)); - *result = reinterpret_cast(propertyNames); + + // `JsGetOwnPropertyNames` is own-only and includes non-enumerable properties, + // so use the shared prototype-chain walk instead. + CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); + return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 5c8583bc..031e1875 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -1,4 +1,5 @@ #include "js_native_api_javascriptcore.h" +#include "js_native_api_shared.h" #include #include #include @@ -964,13 +965,13 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* result) { CHECK_ENV(env); + CHECK_ARG(env, object); CHECK_ARG(env, result); - napi_value global{}, object_ctor{}, function{}; - CHECK_NAPI(napi_get_global(env, &global)); - CHECK_NAPI(napi_get_named_property(env, global, "Object", &object_ctor)); - CHECK_NAPI(napi_get_named_property(env, object_ctor, "getOwnPropertyNames", &function)); - CHECK_NAPI(napi_call_function(env, object_ctor, function, 0, nullptr, result)); + // JavaScriptCore's `JSObjectCopyPropertyNames` walks the prototype chain but + // silently drops properties shadowed by a non-enumerable own property, so use + // the shared prototype-chain walk instead. + CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index 6db7f662..b063f149 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1,4 +1,5 @@ #include "js_native_api_quickjs.h" +#include "js_native_api_shared.h" #include #if defined(__clang__) #pragma clang diagnostic push @@ -1394,27 +1395,11 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* CHECK_ENV(env); CHECK_ARG(env, object); CHECK_ARG(env, result); - - JSValue jsObject = ToJSValue(object); - - JSPropertyEnum* ptab; - uint32_t plen; - - if (JS_GetOwnPropertyNames(env->context, &ptab, &plen, jsObject, - JS_GPN_STRING_MASK | JS_GPN_ENUM_ONLY) < 0) { - return napi_set_last_error(env, napi_generic_failure); - } - - JSValue arr = JS_NewArray(env->context); - - for (uint32_t i = 0; i < plen; i++) { - JSValue name = JS_AtomToString(env->context, ptab[i].atom); - JS_SetPropertyUint32(env->context, arr, i, name); - } - - JS_FreePropertyEnum(env->context, ptab, plen); - - *result = FromJSValue(env, arr); + + // `JS_GetOwnPropertyNames` is own-only, so use the shared prototype-chain + // walk instead. + CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); + napi_clear_last_error(env); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc new file mode 100644 index 00000000..8bd2b00b --- /dev/null +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -0,0 +1,114 @@ +#include "js_native_api_shared.h" + +#include + +#include +#include +#include + +namespace napi_shared { + namespace { + #define RETURN_IF_NOT_OK(expression) \ + do { \ + const napi_status status__{(expression)}; \ + if (status__ != napi_ok) { \ + return status__; \ + } \ + } while (0) + + napi_status GetUtf8Value(napi_env env, napi_value value, std::string& result) { + size_t length{}; + RETURN_IF_NOT_OK(napi_get_value_string_utf8(env, value, nullptr, 0, &length)); + + std::vector buffer(length + 1); + size_t copied{}; + RETURN_IF_NOT_OK(napi_get_value_string_utf8(env, value, buffer.data(), buffer.size(), &copied)); + + result.assign(buffer.data(), copied); + return napi_ok; + } + + napi_status IsObjectLike(napi_env env, napi_value value, bool& result) { + napi_valuetype type{}; + RETURN_IF_NOT_OK(napi_typeof(env, value, &type)); + result = (type == napi_object || type == napi_function || type == napi_external); + return napi_ok; + } + + // Appends every element of the string array `names` to `shadowed`. + napi_status AddAll(napi_env env, napi_value names, std::unordered_set& shadowed) { + uint32_t count{}; + RETURN_IF_NOT_OK(napi_get_array_length(env, names, &count)); + + std::string key{}; + for (uint32_t index = 0; index < count; ++index) { + napi_value name{}; + RETURN_IF_NOT_OK(napi_get_element(env, names, index, &name)); + RETURN_IF_NOT_OK(GetUtf8Value(env, name, key)); + shadowed.insert(std::move(key)); + } + + return napi_ok; + } + } + + napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result) { + // `Object.keys` reports one level's own enumerable string-keyed properties + // in specification order, which is exactly what `for...in` visits at that + // level. `Object.getOwnPropertyNames` additionally reports the + // non-enumerable ones: `for...in` does not visit those, but they still + // shadow same-named properties further up the prototype chain, so they have + // to be tracked as well. + napi_value global{}; + napi_value objectConstructor{}; + napi_value keys{}; + napi_value getOwnPropertyNames{}; + RETURN_IF_NOT_OK(napi_get_global(env, &global)); + RETURN_IF_NOT_OK(napi_get_named_property(env, global, "Object", &objectConstructor)); + RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "keys", &keys)); + RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "getOwnPropertyNames", &getOwnPropertyNames)); + + napi_value names{}; + RETURN_IF_NOT_OK(napi_create_array(env, &names)); + uint32_t nameCount{}; + + std::unordered_set shadowed{}; + std::string key{}; + + napi_value current{}; + RETURN_IF_NOT_OK(napi_coerce_to_object(env, object, ¤t)); + + while (true) { + bool isObjectLike{}; + RETURN_IF_NOT_OK(IsObjectLike(env, current, isObjectLike)); + if (!isObjectLike) { + break; + } + + napi_value ownEnumerableNames{}; + RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, keys, 1, ¤t, &ownEnumerableNames)); + + uint32_t ownEnumerableCount{}; + RETURN_IF_NOT_OK(napi_get_array_length(env, ownEnumerableNames, &ownEnumerableCount)); + for (uint32_t index = 0; index < ownEnumerableCount; ++index) { + napi_value name{}; + RETURN_IF_NOT_OK(napi_get_element(env, ownEnumerableNames, index, &name)); + RETURN_IF_NOT_OK(GetUtf8Value(env, name, key)); + if (shadowed.find(key) == shadowed.end()) { + RETURN_IF_NOT_OK(napi_set_element(env, names, nameCount++, name)); + } + } + + napi_value ownNames{}; + RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); + RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed)); + + RETURN_IF_NOT_OK(napi_get_prototype(env, current, ¤t)); + } + + *result = names; + return napi_ok; + } + + #undef RETURN_IF_NOT_OK +} diff --git a/Core/Node-API/Source/js_native_api_shared.h b/Core/Node-API/Source/js_native_api_shared.h new file mode 100644 index 00000000..ae3907d1 --- /dev/null +++ b/Core/Node-API/Source/js_native_api_shared.h @@ -0,0 +1,22 @@ +#pragma once + +#include + +// Engine-agnostic pieces of the Node-API surface, implemented purely in terms +// of the public `napi_*` entry points so that every backend behaves the same. +// Backends whose engine offers a faithful native equivalent should keep using +// it; these helpers exist for the ones that do not. +namespace napi_shared { + // Implements `napi_get_property_names` semantics: the names of all + // enumerable string-keyed properties of `object` and of its prototype chain, + // as an array of strings, matching a `for...in` enumeration. + // + // V8 gets this from a single `GetPropertyNames` call configured with + // `kIncludePrototypes | ONLY_ENUMERABLE | SKIP_SYMBOLS`. JavaScriptCore, + // ChakraCore and QuickJS have no equivalent, so this walks the prototype + // chain explicitly. See https://github.com/BabylonJS/JsRuntimeHost/issues/216. + // + // `object` is coerced with `napi_coerce_to_object`, as V8's `CHECK_TO_OBJECT` + // does. Callers are expected to have already validated `env` and `result`. + napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result); +} diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 23b8e4e5..27ae42bb 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -8,6 +8,7 @@ Mocha.reporter('spec'); declare const hostPlatform: string; declare const hostEngine: string; declare const setExitCode: (code: number) => void; +declare const napiGetPropertyNames: (object: any) => string[]; describe("AbortController", function () { @@ -1696,6 +1697,91 @@ describe("napi class prototype isolation (#172)", function () { }); +describe("napi_get_property_names (#216)", function () { + // Regression coverage for #216: napi_get_property_names must report the + // enumerable string-keyed properties of an object *and its prototype + // chain*, i.e. exactly what `for...in` visits. JavaScriptCore used to throw + // outright, while ChakraCore and QuickJS only reported own properties + // (ChakraCore additionally reported non-enumerable ones). + + function forIn(object: any): string[] { + const keys: string[] = []; + for (const key in object) { + keys.push(key); + } + return keys; + } + + it("returns own enumerable string keys", function () { + expect(napiGetPropertyNames({ a: 1, b: 2 })).to.deep.equal(["a", "b"]); + }); + + it("includes enumerable properties inherited from the prototype chain", function () { + const object = Object.create({ inherited: 1 }); + object.own = 2; + expect(napiGetPropertyNames(object)).to.deep.equal(["own", "inherited"]); + }); + + it("excludes non-enumerable own properties", function () { + const object = { visible: 1 }; + Object.defineProperty(object, "hidden", { value: 2, enumerable: false }); + expect(napiGetPropertyNames(object)).to.deep.equal(["visible"]); + }); + + it("excludes symbol keys", function () { + const object: any = { a: 1 }; + object[Symbol("s")] = 2; + expect(napiGetPropertyNames(object)).to.deep.equal(["a"]); + }); + + it("reports a shadowed inherited property only once", function () { + const object = Object.create({ shared: 1 }); + object.shared = 2; + expect(napiGetPropertyNames(object)).to.deep.equal(["shared"]); + }); + + it("omits an inherited property shadowed by a non-enumerable own property", function () { + const object = Object.create({ shared: 1 }); + Object.defineProperty(object, "shared", { value: 2, enumerable: false }); + expect(napiGetPropertyNames(object)).to.deep.equal([]); + }); + + it("excludes class methods, which are non-enumerable", function () { + class Point { + x: number; + y: number; + constructor() { + this.x = 1; + this.y = 2; + } + length(): number { + return 0; + } + } + expect(napiGetPropertyNames(new Point())).to.deep.equal(["x", "y"]); + }); + + it("reports array indices as strings and omits the non-enumerable length", function () { + expect(napiGetPropertyNames(["a", "b"])).to.deep.equal(["0", "1"]); + }); + + it("matches for...in over a multi-level prototype chain", function () { + const grandparent = { deep: 0 }; + const parent: any = Object.create(grandparent); + parent.middle = 1; + Object.defineProperty(parent, "hiddenMiddle", { value: 2, enumerable: false }); + + const object: any = Object.create(parent); + object.own = 3; + object[Symbol("s")] = 4; + Object.defineProperty(object, "deep", { value: 5, enumerable: false }); + + expect(napiGetPropertyNames(object)).to.deep.equal(forIn(object)); + expect(napiGetPropertyNames(object)).to.deep.equal(["own", "middle"]); + }); +}); + + describe("Performance", function () { this.timeout(1000); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 1c7e9ff7..df232ba0 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -101,6 +101,16 @@ TEST(JavaScript, All) env.Global().Set("hostPlatform", Napi::Value::From(env, JSRUNTIMEHOST_PLATFORM)); env.Global().Set("hostEngine", Napi::Value::From(env, NAPI_JAVASCRIPT_ENGINE)); + + // Exposes napi_get_property_names, via its C++ wrapper, so that the + // script tests can compare it against `for...in`. See + // https://github.com/BabylonJS/JsRuntimeHost/issues/216. + auto getPropertyNamesCallback = Napi::Function::New( + env, [](const Napi::CallbackInfo& info) -> Napi::Value { + return info[0].As().GetPropertyNames(); + }, + "napiGetPropertyNames"); + env.Global().Set("napiGetPropertyNames", getPropertyNamesCallback); }); Babylon::ScriptLoader loader{runtime}; From 8c5077f7483a66432e7822d6c456598f52391cff Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:25:59 -0700 Subject: [PATCH 02/21] Fix napi_get_prototype on JSC and skip the for...in oracle where it is broken Two follow-ups from CI on the previous commit. napi_get_prototype on JavaScriptCore ran the result of JSObjectGetPrototype through JSValueToObject. At the top of a prototype chain that value is `null`, so the conversion threw "TypeError: null is not an object" instead of reporting the end of the chain, which made the chain impossible to walk and failed all nine new tests. Return the raw prototype value, as V8 does. It has no other callers in this repository: Blob.cpp deliberately uses Object.getPrototypeOf instead. The "matches for...in" assertion also failed on Hermes, but in the opposite direction: napi_get_property_names returned the correct ['own', 'middle'] while Hermes' own `for...in` returned ['own', 'middle', 'deep'], i.e. Hermes does not implement the rule that a non-enumerable own property shadows an inherited enumerable one. Probe for that behaviour at runtime rather than naming engines, and only use `for...in` as an oracle where it holds. The explicit expected-value assertion still runs everywhere. Re-verified on ChakraCore and QuickJS: 225 passing, 0 failing on both. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 11 +++++++---- Tests/UnitTests/Scripts/tests.ts | 12 +++++++++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 031e1875..1f3476ab 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -1329,13 +1329,16 @@ napi_status napi_get_prototype(napi_env env, napi_value object, napi_value* result) { CHECK_ENV(env); + CHECK_ARG(env, object); CHECK_ARG(env, result); - JSValueRef exception{}; - JSObjectRef prototype{JSValueToObject(env->context, JSObjectGetPrototype(env->context, ToJSObject(env, object)), &exception)}; - CHECK_JSC(env, exception); + // `JSObjectGetPrototype` already yields a JSValueRef, and that value is + // `null` at the top of a prototype chain. Running it through + // `JSValueToObject` threw "TypeError: null is not an object" there instead of + // reporting the end of the chain, which made the chain impossible to walk. + // V8 likewise returns the raw prototype value. + *result = ToNapi(JSObjectGetPrototype(env->context, ToJSObject(env, object))); - *result = ToNapi(prototype); return napi_ok; } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 27ae42bb..4addccc5 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1712,6 +1712,14 @@ describe("napi_get_property_names (#216)", function () { return keys; } + // Some engines' own `for...in` does not implement the shadowing rule that + // the tests below rely on -- Hermes reports an inherited property that a + // non-enumerable own property is supposed to hide. Probe for that rather + // than name engines, and only use `for...in` as an oracle where it holds. + const shadowingProbe = Object.create({ probe: 1 }); + Object.defineProperty(shadowingProbe, "probe", { value: 2, enumerable: false }); + const forInHonoursShadowing = forIn(shadowingProbe).length === 0; + it("returns own enumerable string keys", function () { expect(napiGetPropertyNames({ a: 1, b: 2 })).to.deep.equal(["a", "b"]); }); @@ -1776,8 +1784,10 @@ describe("napi_get_property_names (#216)", function () { object[Symbol("s")] = 4; Object.defineProperty(object, "deep", { value: 5, enumerable: false }); - expect(napiGetPropertyNames(object)).to.deep.equal(forIn(object)); expect(napiGetPropertyNames(object)).to.deep.equal(["own", "middle"]); + if (forInHonoursShadowing) { + expect(napiGetPropertyNames(object)).to.deep.equal(forIn(object)); + } }); }); From c8d141855798b7e28ebfed166ab56b8248dee231 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:49:44 -0700 Subject: [PATCH 03/21] Implement Object::GetPropertyNames in the JSI Node-API adapter The JSI adapter provides the Napi C++ surface directly on top of JSI rather than over the C Node-API, and Object::GetPropertyNames was still a stub that threw std::runtime_error{"TODO"}, surfacing in script as "Error: Exception in HostFunction: TODO". jsi::Object::getPropertyNames returns the enumerable string-keyed properties of an object and of its prototype chain, which is exactly the specified behaviour, so forward to it. Verified locally against the ReactNative.V8Jsi runtime: all nine napi_get_property_names tests pass, 225 passing, 0 failing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Core/Node-API-JSI/Include/napi/napi-inl.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Core/Node-API-JSI/Include/napi/napi-inl.h b/Core/Node-API-JSI/Include/napi/napi-inl.h index 14fb745c..47120224 100644 --- a/Core/Node-API-JSI/Include/napi/napi-inl.h +++ b/Core/Node-API-JSI/Include/napi/napi-inl.h @@ -772,7 +772,10 @@ inline bool Object::Delete(uint32_t index) { } inline Array Object::GetPropertyNames() const { - throw std::runtime_error{"TODO"}; + // `jsi::Object::getPropertyNames` returns the enumerable string-keyed + // properties of this object and of its prototype chain, which is exactly what + // `napi_get_property_names` is specified to produce. + return {_env, _object->getPropertyNames(_env->rt)}; } // TODO: not implemented From d02a1f1259c27ab96c7163ec52f921a4fa11f2e8 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 08:06:38 -0700 Subject: [PATCH 04/21] Test the ToObject coercion, and make null/undefined consistent Review feedback: the implementation coerces its argument with ToObject, but nothing covered that. `Napi::Object::GetPropertyNames` can only be called on an already-constructed `Napi::Object`, so the C++ harness structurally could not reach the coercion path. Expose the C entry point directly as `napiGetPropertyNamesRaw` and pass the raw value through. The Node-API-JSI backend implements the `Napi::` C++ surface straight on top of JSI and has no C Node-API at all, so the global is left undefined there (it already has a `JSRUNTIMEHOST_NAPI_ENGINE_JSI` define) and the six new tests skip themselves. Testing that also exposed a divergence for `null` and `undefined`, which have no object wrapper: V8 reports `napi_object_expected`, QuickJS's `napi_coerce_to_object` uses `Object(value)` and happily returns an empty object, and JavaScriptCore's throws. Check `napi_typeof` explicitly in the shared implementation so all three match V8 instead. Verified locally: ChakraCore 231 passing / 0 failing, QuickJS 231 / 0, JSI 225 / 0 with 6 pending. Negative control: with the null/undefined check removed, QuickJS fails exactly the two tests that cover it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Core/Node-API/Source/js_native_api_shared.cc | 12 +++++++ Tests/UnitTests/Scripts/tests.ts | 36 ++++++++++++++++++++ Tests/UnitTests/Shared/Shared.cpp | 35 +++++++++++++++++++ 3 files changed, 83 insertions(+) diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index 8bd2b00b..e394ea20 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -75,6 +75,18 @@ namespace napi_shared { std::unordered_set shadowed{}; std::string key{}; + // `ToObject` is what the specification (and the V8 implementation) applies + // to the argument, so a primitive is wrapped and its properties reported. + // `null` and `undefined` have no wrapper, and V8 reports that as + // `napi_object_expected`; check explicitly rather than relying on + // `napi_coerce_to_object`, whose behaviour for those two values differs + // between engines (QuickJS yields an empty object, JavaScriptCore throws). + napi_valuetype type{}; + RETURN_IF_NOT_OK(napi_typeof(env, object, &type)); + if (type == napi_null || type == napi_undefined) { + return napi_object_expected; + } + napi_value current{}; RETURN_IF_NOT_OK(napi_coerce_to_object(env, object, ¤t)); diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 4addccc5..ad8c3e7a 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -9,6 +9,7 @@ declare const hostPlatform: string; declare const hostEngine: string; declare const setExitCode: (code: number) => void; declare const napiGetPropertyNames: (object: any) => string[]; +declare const napiGetPropertyNamesRaw: (object: any) => string[]; describe("AbortController", function () { @@ -1789,6 +1790,41 @@ describe("napi_get_property_names (#216)", function () { expect(napiGetPropertyNames(object)).to.deep.equal(forIn(object)); } }); + + // `Napi::Object::GetPropertyNames` requires an object, so the coercion the + // C entry point performs on its argument is only reachable through + // `napiGetPropertyNamesRaw`. That global is undefined on the JSI backend, + // which implements the `Napi::` C++ surface directly on JSI and has no C + // Node-API to call. + const describeCoercion = typeof napiGetPropertyNamesRaw === "function" ? describe : describe.skip; + + describeCoercion("argument coercion", function () { + it("wraps a string primitive and reports its indices", function () { + expect(napiGetPropertyNamesRaw("ab")).to.deep.equal(["0", "1"]); + }); + + it("wraps a number primitive, which has no enumerable properties", function () { + expect(napiGetPropertyNamesRaw(42)).to.deep.equal([]); + }); + + it("wraps a boolean primitive, which has no enumerable properties", function () { + expect(napiGetPropertyNamesRaw(true)).to.deep.equal([]); + }); + + it("still reports the prototype chain of a real object", function () { + const object = Object.create({ inherited: 1 }); + object.own = 2; + expect(napiGetPropertyNamesRaw(object)).to.deep.equal(["own", "inherited"]); + }); + + it("fails for null", function () { + expect(() => napiGetPropertyNamesRaw(null)).to.throw(); + }); + + it("fails for undefined", function () { + expect(() => napiGetPropertyNamesRaw(undefined)).to.throw(); + }); + }); }); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index df232ba0..9366b86c 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include namespace @@ -111,6 +112,40 @@ TEST(JavaScript, All) }, "napiGetPropertyNames"); env.Global().Set("napiGetPropertyNames", getPropertyNamesCallback); + +#ifndef JSRUNTIMEHOST_NAPI_ENGINE_JSI + // `Napi::Object::GetPropertyNames` can only be reached through an + // already-constructed `Napi::Object`, so it cannot exercise the + // `ToObject` coercion that `napi_get_property_names` performs on its + // argument. Expose the C entry point directly for those cases. The JSI + // backend implements the `Napi::` C++ surface straight on top of JSI and + // has no C Node-API at all, so this global is left undefined there and + // the coercion tests skip themselves. + auto getPropertyNamesRawCallback = Napi::Function::New( + env, [](const Napi::CallbackInfo& info) -> Napi::Value { + napi_env rawEnv{info.Env()}; + napi_value result{}; + const napi_status status{napi_get_property_names(rawEnv, info[0], &result)}; + if (status != napi_ok) + { + // A failed call may or may not have left a JavaScript + // exception pending; surface either as a thrown error so + // that the script tests can assert on it uniformly. + bool isExceptionPending{}; + if (napi_is_exception_pending(rawEnv, &isExceptionPending) == napi_ok && isExceptionPending) + { + napi_value error{}; + napi_get_and_clear_last_exception(rawEnv, &error); + } + + throw Napi::Error::New(info.Env(), "napi_get_property_names failed with status " + std::to_string(status)); + } + + return Napi::Value{rawEnv, result}; + }, + "napiGetPropertyNamesRaw"); + env.Global().Set("napiGetPropertyNamesRaw", getPropertyNamesRawCallback); +#endif }); Babylon::ScriptLoader loader{runtime}; From 1d3855bc2a46cbb7d93169f46f9dd60fc8b10514 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 08:44:40 -0700 Subject: [PATCH 05/21] Skip the primitive-wrapping cases on Hermes Hermes' Node-API is not implemented in this repository -- it comes from the Hermes dependency itself -- and it rejects primitives outright instead of applying ToObject, so the three wrapping tests failed there. It does reject null and undefined like everyone else, so those two still run. Hermes is documented as an experimental engine here and its napi is not ours to fix, so skip those cases explicitly rather than weakening the assertions for every backend. That needs an engine identifier in script, so plumb NAPI_JAVASCRIPT_ENGINE through as a `napiEngine` global alongside the existing `hostPlatform` one. Verified locally: ChakraCore 231 passing / 0 failing, QuickJS 231 / 0, JSI 225 / 0 with 6 pending. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Tests/UnitTests/Scripts/tests.ts | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index ad8c3e7a..6819fc3d 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1799,16 +1799,25 @@ describe("napi_get_property_names (#216)", function () { const describeCoercion = typeof napiGetPropertyNamesRaw === "function" ? describe : describe.skip; describeCoercion("argument coercion", function () { - it("wraps a string primitive and reports its indices", function () { - expect(napiGetPropertyNamesRaw("ab")).to.deep.equal(["0", "1"]); - }); + // Hermes' Node-API is not implemented in this repository -- it comes from + // the Hermes dependency itself -- and it rejects primitives outright + // rather than applying ToObject. Hermes is an experimental engine here, + // so assert the specified behaviour everywhere it is ours to control and + // skip the wrapping cases on Hermes rather than weakening them. + const describePrimitives = hostEngine === "Hermes" ? describe.skip : describe; + + describePrimitives("of a primitive", function () { + it("wraps a string primitive and reports its indices", function () { + expect(napiGetPropertyNamesRaw("ab")).to.deep.equal(["0", "1"]); + }); - it("wraps a number primitive, which has no enumerable properties", function () { - expect(napiGetPropertyNamesRaw(42)).to.deep.equal([]); - }); + it("wraps a number primitive, which has no enumerable properties", function () { + expect(napiGetPropertyNamesRaw(42)).to.deep.equal([]); + }); - it("wraps a boolean primitive, which has no enumerable properties", function () { - expect(napiGetPropertyNamesRaw(true)).to.deep.equal([]); + it("wraps a boolean primitive, which has no enumerable properties", function () { + expect(napiGetPropertyNamesRaw(true)).to.deep.equal([]); + }); }); it("still reports the prototype chain of a real object", function () { From fab5e24c70e5d31f26226e9ca5c8857372321665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 18:55:19 -0700 Subject: [PATCH 06/21] Address property-name review feedback Use Chakra terminology, keep a Hermes coercion tripwire linked to #219, and avoid building the final unused shadowing set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79ad319b-72f9-4b93-9c81-57858177c0a7 --- Core/Node-API/Source/js_native_api_shared.cc | 15 +++++++++++---- Core/Node-API/Source/js_native_api_shared.h | 2 +- Tests/UnitTests/Scripts/tests.ts | 15 ++++++++++++--- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index e394ea20..7b3f991f 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -111,11 +111,18 @@ namespace napi_shared { } } - napi_value ownNames{}; - RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); - RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed)); + napi_value next{}; + RETURN_IF_NOT_OK(napi_get_prototype(env, current, &next)); + + bool hasNextLevel{}; + RETURN_IF_NOT_OK(IsObjectLike(env, next, hasNextLevel)); + if (hasNextLevel) { + napi_value ownNames{}; + RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); + RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed)); + } - RETURN_IF_NOT_OK(napi_get_prototype(env, current, ¤t)); + current = next; } *result = names; diff --git a/Core/Node-API/Source/js_native_api_shared.h b/Core/Node-API/Source/js_native_api_shared.h index ae3907d1..14d13840 100644 --- a/Core/Node-API/Source/js_native_api_shared.h +++ b/Core/Node-API/Source/js_native_api_shared.h @@ -13,7 +13,7 @@ namespace napi_shared { // // V8 gets this from a single `GetPropertyNames` call configured with // `kIncludePrototypes | ONLY_ENUMERABLE | SKIP_SYMBOLS`. JavaScriptCore, - // ChakraCore and QuickJS have no equivalent, so this walks the prototype + // Chakra and QuickJS have no equivalent, so this walks the prototype // chain explicitly. See https://github.com/BabylonJS/JsRuntimeHost/issues/216. // // `object` is coerced with `napi_coerce_to_object`, as V8's `CHECK_TO_OBJECT` diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 6819fc3d..05682c38 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1702,8 +1702,8 @@ describe("napi_get_property_names (#216)", function () { // Regression coverage for #216: napi_get_property_names must report the // enumerable string-keyed properties of an object *and its prototype // chain*, i.e. exactly what `for...in` visits. JavaScriptCore used to throw - // outright, while ChakraCore and QuickJS only reported own properties - // (ChakraCore additionally reported non-enumerable ones). + // outright, while Chakra and QuickJS only reported own properties + // (Chakra additionally reported non-enumerable ones). function forIn(object: any): string[] { const keys: string[] = []; @@ -1717,6 +1717,7 @@ describe("napi_get_property_names (#216)", function () { // the tests below rely on -- Hermes reports an inherited property that a // non-enumerable own property is supposed to hide. Probe for that rather // than name engines, and only use `for...in` as an oracle where it holds. + // https://github.com/BabylonJS/JsRuntimeHost/issues/219 tracks the gap. const shadowingProbe = Object.create({ probe: 1 }); Object.defineProperty(shadowingProbe, "probe", { value: 2, enumerable: false }); const forInHonoursShadowing = forIn(shadowingProbe).length === 0; @@ -1803,9 +1804,17 @@ describe("napi_get_property_names (#216)", function () { // the Hermes dependency itself -- and it rejects primitives outright // rather than applying ToObject. Hermes is an experimental engine here, // so assert the specified behaviour everywhere it is ours to control and - // skip the wrapping cases on Hermes rather than weakening them. + // skip the wrapping cases on Hermes rather than weakening them. The + // upstream gap is tracked by + // https://github.com/BabylonJS/JsRuntimeHost/issues/219. const describePrimitives = hostEngine === "Hermes" ? describe.skip : describe; + if (hostEngine === "Hermes") { + it("rejects primitives instead of applying ToObject (upstream gap)", function () { + expect(() => napiGetPropertyNamesRaw("ab")).to.throw(); + }); + } + describePrimitives("of a primitive", function () { it("wraps a string primitive and reports its indices", function () { expect(napiGetPropertyNamesRaw("ab")).to.deep.equal(["0", "1"]); From 2c45610232f61fd7e7a228012097f59650d8d8f0 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Fri, 31 Jul 2026 09:53:37 -0700 Subject: [PATCH 07/21] Terminate the prototype walk on a cycle and report last_error consistently A `getPrototypeOf` Proxy trap may return an object that is already on the chain. Nothing in the specification forbids it -- the invariants on that trap constrain only the non-extensible case -- so `Object.getPrototypeOf(p) === p` is reachable from script. V8 walks the chain recursively and so terminates with a `RangeError`; the shared walk introduced here is iterative and spun forever instead. Confirmed against a real build: the test process burned 56 seconds of CPU and never returned, and no JavaScript-level timeout can preempt it, because control never re-enters the engine. Stopping at the first repeated level is exact rather than a bail-out. Every level adds its full own-property-name set to `shadowed` before the walk advances, so a level reached a second time can only re-encounter names that are already shadowed; breaking there yields precisely the fixed point the non-terminating walk converges on. Separately, `napi_get_property_names` disagreed with `napi_get_last_error_info`. The shared walk is written against the public `napi_*` surface and so cannot reach `napi_set_last_error`, `CHECK_NAPI` only propagates the status, and the `napi_typeof` performed just before the rejection clears the last error on success. A caller therefore saw `napi_object_expected` returned while the recorded error code was still `napi_ok`. The success path had the mirror-image problem: it left whatever error a previous call had recorded in place. Set and clear the error explicitly at all three call sites. Covered by four script tests for the cyclic cases and a native regression test for the error reporting; reverting either fix fails them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- Core/Node-API/Source/js_native_api_chakra.cc | 15 ++++- .../Source/js_native_api_javascriptcore.cc | 14 ++++- Core/Node-API/Source/js_native_api_quickjs.cc | 12 +++- Core/Node-API/Source/js_native_api_shared.cc | 34 +++++++++++ Tests/UnitTests/Scripts/tests.ts | 48 +++++++++++++++ Tests/UnitTests/Shared/Shared.cpp | 59 +++++++++++++++++++ 6 files changed, 174 insertions(+), 8 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 3c4bf5f3..a04cdae1 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -683,9 +683,18 @@ napi_status napi_get_property_names(napi_env env, CHECK_ARG(env, result); // `JsGetOwnPropertyNames` is own-only and includes non-enumerable properties, - // so use the shared prototype-chain walk instead. - CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); - + // so use the shared prototype-chain walk instead. It is written against the + // public `napi_*` surface and so cannot reach `napi_set_last_error`; do it + // here, since `CHECK_NAPI` only propagates the status and the preceding call + // inside the walk will have cleared the last error. The success path likewise + // has to clear it, so that a rejection recorded by an earlier call does not + // survive as the last error of a call that succeeded. + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result)}; + if (status != napi_ok) { + return napi_set_last_error(env, status); + } + + napi_clear_last_error(env); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 1f3476ab..17687005 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -970,10 +970,18 @@ napi_status napi_get_property_names(napi_env env, // JavaScriptCore's `JSObjectCopyPropertyNames` walks the prototype chain but // silently drops properties shadowed by a non-enumerable own property, so use - // the shared prototype-chain walk instead. - CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); + // the shared prototype-chain walk instead. It is written against the public + // `napi_*` surface and so cannot reach `napi_set_last_error`; do it here, + // since `CHECK_NAPI` only propagates the status and the preceding call inside + // the walk will have cleared the last error. The success path likewise has to + // clear it, so that a rejection recorded by an earlier call does not survive + // as the last error of a call that succeeded. + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result)}; + if (status != napi_ok) { + return napi_set_last_error(env, status); + } - return napi_ok; + return napi_clear_last_error(env); } napi_status napi_set_property(napi_env env, diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index b063f149..3c99aa79 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1397,8 +1397,16 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* CHECK_ARG(env, result); // `JS_GetOwnPropertyNames` is own-only, so use the shared prototype-chain - // walk instead. - CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); + // walk instead. It is written against the public `napi_*` surface and so + // cannot reach `napi_set_last_error`; do it here, since `CHECK_NAPI` only + // propagates the status and the preceding call inside the walk will have + // cleared the last error. The success path likewise has to clear it, so that + // a rejection recorded by an earlier call does not survive as the last error + // of a call that succeeded. + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result)}; + if (status != napi_ok) { + return napi_set_last_error(env, status); + } napi_clear_last_error(env); return napi_ok; diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index 7b3f991f..23dcdc5c 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -50,6 +50,21 @@ namespace napi_shared { return napi_ok; } + + // Whether `value` is strictly equal to something already in `seen`. + napi_status Contains(napi_env env, const std::vector& seen, napi_value value, bool& result) { + for (const napi_value candidate : seen) { + bool equal{}; + RETURN_IF_NOT_OK(napi_strict_equals(env, candidate, value, &equal)); + if (equal) { + result = true; + return napi_ok; + } + } + + result = false; + return napi_ok; + } } napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result) { @@ -73,6 +88,7 @@ namespace napi_shared { uint32_t nameCount{}; std::unordered_set shadowed{}; + std::vector visited{}; std::string key{}; // `ToObject` is what the specification (and the V8 implementation) applies @@ -97,6 +113,24 @@ namespace napi_shared { break; } + // A `getPrototypeOf` Proxy trap can return an object that is already on + // the chain -- nothing in the specification forbids it, so + // `Object.getPrototypeOf(p) === p` is reachable from script -- which + // makes this walk cyclic. V8 recurses and so terminates with a + // `RangeError`; this loop is iterative and would spin forever. + // + // Stopping at the repeat is exact rather than a bail-out: every level + // adds its own property names to `shadowed` before the walk continues, + // so a level visited a second time can only re-encounter names that are + // already shadowed. Breaking here therefore yields the same result the + // non-terminating walk converges on. + bool alreadyVisited{}; + RETURN_IF_NOT_OK(Contains(env, visited, current, alreadyVisited)); + if (alreadyVisited) { + break; + } + visited.push_back(current); + napi_value ownEnumerableNames{}; RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, keys, 1, ¤t, &ownEnumerableNames)); diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 05682c38..c0b694f3 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1843,6 +1843,54 @@ describe("napi_get_property_names (#216)", function () { expect(() => napiGetPropertyNamesRaw(undefined)).to.throw(); }); }); + + // A `getPrototypeOf` Proxy trap may put an object back onto its own + // prototype chain -- no specification invariant forbids it -- which makes + // the chain cyclic. V8 collects keys recursively and so terminates with a + // `RangeError`; the shared walk is iterative and used to spin forever, + // burning CPU in native code with no way for script or the test timeout to + // interrupt it. + // + // These only apply to the backends that use the shared walk. V8 and Hermes + // bring their own key collection, and the JSI adapter forwards to + // `jsi::Object::getPropertyNames`, so their behaviour here is not ours to + // specify. + const usesSharedWalk = napiEngine === "Chakra" || napiEngine === "QuickJS" || napiEngine === "JavaScriptCore"; + const describeCycles = usesSharedWalk ? describe : describe.skip; + + describeCycles("cyclic prototype chains", function () { + this.timeout(5000); + + it("terminates when a proxy is its own prototype", function () { + let object: any; + object = new Proxy({ own: 1 }, { getPrototypeOf() { return object; } }); + expect(napiGetPropertyNames(object)).to.deep.equal(["own"]); + }); + + it("terminates on a two-object cycle and reports each level once", function () { + let first: any; + let second: any; + first = new Proxy({ a: 1 }, { getPrototypeOf() { return second; } }); + second = new Proxy({ b: 2 }, { getPrototypeOf() { return first; } }); + expect(napiGetPropertyNames(first)).to.deep.equal(["a", "b"]); + }); + + it("still reports a long acyclic chain in full", function () { + const base = { deep: 1 }; + const middle = Object.create(base); + middle.middle = 2; + const leaf = Object.create(middle); + leaf.own = 3; + expect(napiGetPropertyNames(leaf)).to.deep.equal(["own", "middle", "deep"]); + }); + + it("propagates a throwing getPrototypeOf trap instead of hanging", function () { + const object = new Proxy({ own: 1 }, { + getPrototypeOf() { throw new Error("trap"); }, + }); + expect(() => napiGetPropertyNames(object)).to.throw(); + }); + }); }); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 9366b86c..2aed5321 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include namespace @@ -874,6 +875,64 @@ TEST(NodeApi, AdjacentEscapableScopesEscapeIndependently) #endif +// The V8JSI shim has no C Node-API at all, so this only builds elsewhere. +#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) +TEST(NodeApi, GetPropertyNamesReportsLastErrorConsistently) +{ + // Hermes supplies its own Node-API rather than the implementations in this + // repository, so this contract is not its to satisfy. Every backend that is + // ours is covered, including V8, which reaches napi_set_last_error through + // RETURN_STATUS_IF_FALSE. + if (std::string_view{JSRUNTIMEHOST_NAPI_ENGINE} == "Hermes") + { + GTEST_SKIP() << "Hermes supplies its own napi_get_property_names."; + } + + // Regression: napi_get_property_names rejects null/undefined with + // napi_object_expected, but the shared walk is written against the public + // napi_* surface and cannot reach napi_set_last_error. CHECK_NAPI only + // propagates the status, and the napi_typeof performed just before the + // rejection clears the last error on success, so the returned status and + // napi_get_last_error_info() disagreed: the caller saw + // napi_object_expected while the recorded error code was still napi_ok. + // Node-API's contract is that the two agree. + Babylon::AppRuntime runtime{}; + + std::promise nullConsistent; + std::promise undefinedConsistent; + std::promise successClears; + + runtime.Dispatch([&nullConsistent, &undefinedConsistent, &successClears](Napi::Env env) { + napi_env nenv{env}; + + const auto check = [nenv](napi_value value) { + napi_value names{nullptr}; + const napi_status status{napi_get_property_names(nenv, value, &names)}; + + const napi_extended_error_info* info{nullptr}; + napi_get_last_error_info(nenv, &info); + + return status == napi_object_expected && info != nullptr && info->error_code == status; + }; + + nullConsistent.set_value(check(napi_value{env.Null()})); + undefinedConsistent.set_value(check(napi_value{env.Undefined()})); + + // The success path must leave no stale error behind. + napi_value names{nullptr}; + const napi_status status{napi_get_property_names(nenv, napi_value{Napi::Object::New(env)}, &names)}; + + const napi_extended_error_info* info{nullptr}; + napi_get_last_error_info(nenv, &info); + successClears.set_value(status == napi_ok && info != nullptr && info->error_code == napi_ok); + }); + + EXPECT_TRUE(nullConsistent.get_future().get()); + EXPECT_TRUE(undefinedConsistent.get_future().get()); + EXPECT_TRUE(successClears.get_future().get()); +} +#endif + int RunTests() { testing::InitGoogleTest(); From 747bd1dc6e347548fdae1078c332751cf4889fa1 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Fri, 31 Jul 2026 12:06:25 -0700 Subject: [PATCH 08/21] Scope the new property-name tests to the backends they describe CI turned up two places where the tests asserted behaviour that belongs to an engine rather than to this change. JavaScriptCore's `napi_get_prototype` calls `JSObjectGetPrototype`, which reads the internal [[Prototype]] slot and never runs a proxy `getPrototypeOf` trap. Since a cycle can only be built with that trap, a trapped chain there reports the target's real prototype instead: the cycle is invisible, the throwing trap never fires, and the walk was never at risk on that backend to begin with. That is a pre-existing limitation of `napi_get_prototype`, not of the walk, and fixing it means giving JavaScriptCore `Reflect.getPrototypeOf` semantics, which is a separate change. Skip the three proxy-dependent cases there and say why. The acyclic-chain case needs no trap and still runs everywhere. The native error-reporting test asserted a contract V8 does not keep, and I was wrong to claim otherwise: `napi_get_property_names` there is vendored upstream Node code whose rejection leaves a pending exception, so the next call reports `napi_pending_exception` rather than `napi_object_expected`, and whose success path returns bare `napi_ok` through `GET_RETURN_STATUS` without clearing. Both are upstream's to define. Scope the test to the three backends that share the walk -- the ones this change actually touches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- Tests/UnitTests/Scripts/tests.ts | 19 ++++++++++++++++--- Tests/UnitTests/Shared/Shared.cpp | 17 +++++++++++------ 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index c0b694f3..b03479f2 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1858,16 +1858,29 @@ describe("napi_get_property_names (#216)", function () { const usesSharedWalk = napiEngine === "Chakra" || napiEngine === "QuickJS" || napiEngine === "JavaScriptCore"; const describeCycles = usesSharedWalk ? describe : describe.skip; + // A cycle can only be built with a `getPrototypeOf` trap, so observing one + // additionally requires `napi_get_prototype` to consult that trap. + // JavaScriptCore's does not: it calls `JSObjectGetPrototype`, which reads + // the internal [[Prototype]] slot directly and never runs proxy traps + // (`js_native_api_javascriptcore.cc:1348`). A trapped chain there simply + // reports the target's real prototype, so the cycle -- and the throwing + // trap -- are both invisible, and the walk was never at risk on that + // backend. That is a pre-existing limitation of `napi_get_prototype`, not + // of the walk, so it is left alone here; the termination check below is + // still correct and harmless on JavaScriptCore. + const proxyTrapsReachPrototypeWalk = usesSharedWalk && napiEngine !== "JavaScriptCore"; + const itTrapped = proxyTrapsReachPrototypeWalk ? it : it.skip; + describeCycles("cyclic prototype chains", function () { this.timeout(5000); - it("terminates when a proxy is its own prototype", function () { + itTrapped("terminates when a proxy is its own prototype", function () { let object: any; object = new Proxy({ own: 1 }, { getPrototypeOf() { return object; } }); expect(napiGetPropertyNames(object)).to.deep.equal(["own"]); }); - it("terminates on a two-object cycle and reports each level once", function () { + itTrapped("terminates on a two-object cycle and reports each level once", function () { let first: any; let second: any; first = new Proxy({ a: 1 }, { getPrototypeOf() { return second; } }); @@ -1884,7 +1897,7 @@ describe("napi_get_property_names (#216)", function () { expect(napiGetPropertyNames(leaf)).to.deep.equal(["own", "middle", "deep"]); }); - it("propagates a throwing getPrototypeOf trap instead of hanging", function () { + itTrapped("propagates a throwing getPrototypeOf trap instead of hanging", function () { const object = new Proxy({ own: 1 }, { getPrototypeOf() { throw new Error("trap"); }, }); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 2aed5321..5f976d84 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -879,13 +879,18 @@ TEST(NodeApi, AdjacentEscapableScopesEscapeIndependently) #if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) TEST(NodeApi, GetPropertyNamesReportsLastErrorConsistently) { - // Hermes supplies its own Node-API rather than the implementations in this - // repository, so this contract is not its to satisfy. Every backend that is - // ours is covered, including V8, which reaches napi_set_last_error through - // RETURN_STATUS_IF_FALSE. - if (std::string_view{JSRUNTIMEHOST_NAPI_ENGINE} == "Hermes") + // This asserts the contract for the three backends this change touches -- + // the ones that share the prototype walk. V8's napi_get_property_names is + // vendored upstream Node code with different behaviour on both counts: a + // rejected call leaves a pending exception, so a following call reports + // napi_pending_exception rather than napi_object_expected, and its success + // path returns bare napi_ok through GET_RETURN_STATUS without clearing. + // Both are upstream's to define, not ours to redefine here. Hermes and the + // JSI adapter likewise supply their own. + const std::string_view engine{JSRUNTIMEHOST_NAPI_ENGINE}; + if (engine != "Chakra" && engine != "QuickJS" && engine != "JavaScriptCore") { - GTEST_SKIP() << "Hermes supplies its own napi_get_property_names."; + GTEST_SKIP() << engine << " supplies its own napi_get_property_names."; } // Regression: napi_get_property_names rejects null/undefined with From 9ac64312f795008ae593989c81e7cd02cdc3ae3c Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Fri, 31 Jul 2026 13:00:35 -0700 Subject: [PATCH 09/21] Coerce the argument in napi_get_prototype on JavaScriptCore The conversion this function already performed was on the wrong operand. It belongs on the argument, which is where V8 puts it, and moving it there fixes a latent defect on the same line. The argument was handed straight to `ToJSObject`, which only asserts that its input is an object. A primitive therefore tripped the assert in debug builds and, in release, reinterpreted a non-object `JSValueRef` as a `JSObjectRef` before handing it to `JSObjectGetPrototype` -- undefined behaviour rather than a status. Coercing instead matches V8's `CHECK_TO_OBJECT`: a primitive yields its wrapper's prototype, and only `null` and `undefined` are rejected, with `napi_object_expected`. Nothing in the repository could reach this. The shared prototype walk is the only caller, and it recurses only into values it has already established are object-like, so the behaviour it depends on is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- .../Source/js_native_api_javascriptcore.cc | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 17687005..d95854a6 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -1345,9 +1345,25 @@ napi_status napi_get_prototype(napi_env env, // `JSValueToObject` threw "TypeError: null is not an object" there instead of // reporting the end of the chain, which made the chain impossible to walk. // V8 likewise returns the raw prototype value. - *result = ToNapi(JSObjectGetPrototype(env->context, ToJSObject(env, object))); + // + // The conversion belongs on the argument rather than the result. V8 coerces + // there (`CHECK_TO_OBJECT`), so a primitive yields its wrapper's prototype + // and only `null`/`undefined` are rejected. Passing the argument straight to + // `ToJSObject` instead would assert in debug and, in release, reinterpret a + // non-object `JSValueRef` as a `JSObjectRef` -- so a primitive was undefined + // behaviour rather than a status. + const JSValueRef value{ToJSValue(object)}; + if (JSValueIsNull(env->context, value) || JSValueIsUndefined(env->context, value)) { + return napi_set_last_error(env, napi_object_expected); + } - return napi_ok; + JSValueRef exception{}; + const JSObjectRef self{JSValueToObject(env->context, value, &exception)}; + CHECK_JSC(env, exception); + + *result = ToNapi(JSObjectGetPrototype(env->context, self)); + + return napi_clear_last_error(env); } napi_status napi_create_object(napi_env env, napi_value* result) { From af1720849b856eef623209aa7faee0996ba086ed Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Fri, 31 Jul 2026 15:22:38 -0700 Subject: [PATCH 10/21] Correct the JSObjectCopyPropertyNames comment The comment had the failure mode backwards. JavaScriptCore does not drop the shadowed inherited property; it reports it. `JSObject::getPropertyNames` walks the chain calling `getOwnPropertyNames` per level with `DontEnumPropertiesMode::Exclude`, so a non-enumerable own property is never added to the array and therefore cannot suppress a same-named enumerable property further up -- the inherited name survives where `for...in` correctly omits it. The conclusion is unchanged, and so is the code: it is still not a conforming replacement for the shared walk. Thanks to @matthargett for catching it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index d95854a6..e2c15e1b 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -968,9 +968,13 @@ napi_status napi_get_property_names(napi_env env, CHECK_ARG(env, object); CHECK_ARG(env, result); - // JavaScriptCore's `JSObjectCopyPropertyNames` walks the prototype chain but - // silently drops properties shadowed by a non-enumerable own property, so use - // the shared prototype-chain walk instead. It is written against the public + // JavaScriptCore's `JSObjectCopyPropertyNames` walks the prototype chain, but + // it does not apply the shadowing rule: `JSObject::getPropertyNames` calls + // `getOwnPropertyNames` per level with `DontEnumPropertiesMode::Exclude`, so + // a non-enumerable own property is never added to the array and so cannot + // suppress a same-named enumerable property further up the chain. The + // inherited name is reported where `for...in` correctly omits it. Use the + // shared prototype-chain walk instead. It is written against the public // `napi_*` surface and so cannot reach `napi_set_last_error`; do it here, // since `CHECK_NAPI` only propagates the status and the preceding call inside // the walk will have cleared the last error. The success path likewise has to From 6ee4f424f7c79e5a0aecab1eeb1fd599cb9e9659 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Mon, 14 Sep 2026 12:47:36 -0700 Subject: [PATCH 11/21] Reuse upstream engine test plumbing The refreshed base already exposes NAPI_JAVASCRIPT_ENGINE as hostEngine. Use it throughout the property-name tests instead of retaining a parallel napiEngine global and Android compile definition. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42 --- Tests/UnitTests/Scripts/tests.ts | 4 ++-- Tests/UnitTests/Shared/Shared.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index b03479f2..ae2d7000 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1855,7 +1855,7 @@ describe("napi_get_property_names (#216)", function () { // bring their own key collection, and the JSI adapter forwards to // `jsi::Object::getPropertyNames`, so their behaviour here is not ours to // specify. - const usesSharedWalk = napiEngine === "Chakra" || napiEngine === "QuickJS" || napiEngine === "JavaScriptCore"; + const usesSharedWalk = hostEngine === "Chakra" || hostEngine === "QuickJS" || hostEngine === "JavaScriptCore"; const describeCycles = usesSharedWalk ? describe : describe.skip; // A cycle can only be built with a `getPrototypeOf` trap, so observing one @@ -1868,7 +1868,7 @@ describe("napi_get_property_names (#216)", function () { // backend. That is a pre-existing limitation of `napi_get_prototype`, not // of the walk, so it is left alone here; the termination check below is // still correct and harmless on JavaScriptCore. - const proxyTrapsReachPrototypeWalk = usesSharedWalk && napiEngine !== "JavaScriptCore"; + const proxyTrapsReachPrototypeWalk = usesSharedWalk && hostEngine !== "JavaScriptCore"; const itTrapped = proxyTrapsReachPrototypeWalk ? it : it.skip; describeCycles("cyclic prototype chains", function () { diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 5f976d84..08381d78 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -887,7 +887,7 @@ TEST(NodeApi, GetPropertyNamesReportsLastErrorConsistently) // path returns bare napi_ok through GET_RETURN_STATUS without clearing. // Both are upstream's to define, not ours to redefine here. Hermes and the // JSI adapter likewise supply their own. - const std::string_view engine{JSRUNTIMEHOST_NAPI_ENGINE}; + const std::string_view engine{NAPI_JAVASCRIPT_ENGINE}; if (engine != "Chakra" && engine != "QuickJS" && engine != "JavaScriptCore") { GTEST_SKIP() << engine << " supplies its own napi_get_property_names."; From 2bb1fdd5211976fd17c4ae7c4c231e2c9ae66a4a Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Tue, 22 Sep 2026 14:51:15 -0700 Subject: [PATCH 12/21] Match for-in proxy enumeration semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Core/Node-API/Source/js_native_api_shared.cc | 111 +++++++------------ Tests/UnitTests/Scripts/tests.ts | 43 +++---- Tests/UnitTests/Shared/Shared.cpp | 5 +- 3 files changed, 70 insertions(+), 89 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index 23dcdc5c..186b78a7 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -2,8 +2,6 @@ #include -#include -#include #include namespace napi_shared { @@ -16,18 +14,6 @@ namespace napi_shared { } \ } while (0) - napi_status GetUtf8Value(napi_env env, napi_value value, std::string& result) { - size_t length{}; - RETURN_IF_NOT_OK(napi_get_value_string_utf8(env, value, nullptr, 0, &length)); - - std::vector buffer(length + 1); - size_t copied{}; - RETURN_IF_NOT_OK(napi_get_value_string_utf8(env, value, buffer.data(), buffer.size(), &copied)); - - result.assign(buffer.data(), copied); - return napi_ok; - } - napi_status IsObjectLike(napi_env env, napi_value value, bool& result) { napi_valuetype type{}; RETURN_IF_NOT_OK(napi_typeof(env, value, &type)); @@ -35,22 +21,6 @@ namespace napi_shared { return napi_ok; } - // Appends every element of the string array `names` to `shadowed`. - napi_status AddAll(napi_env env, napi_value names, std::unordered_set& shadowed) { - uint32_t count{}; - RETURN_IF_NOT_OK(napi_get_array_length(env, names, &count)); - - std::string key{}; - for (uint32_t index = 0; index < count; ++index) { - napi_value name{}; - RETURN_IF_NOT_OK(napi_get_element(env, names, index, &name)); - RETURN_IF_NOT_OK(GetUtf8Value(env, name, key)); - shadowed.insert(std::move(key)); - } - - return napi_ok; - } - // Whether `value` is strictly equal to something already in `seen`. napi_status Contains(napi_env env, const std::vector& seen, napi_value value, bool& result) { for (const napi_value candidate : seen) { @@ -68,28 +38,26 @@ namespace napi_shared { } napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result) { - // `Object.keys` reports one level's own enumerable string-keyed properties - // in specification order, which is exactly what `for...in` visits at that - // level. `Object.getOwnPropertyNames` additionally reports the - // non-enumerable ones: `for...in` does not visit those, but they still - // shadow same-named properties further up the prototype chain, so they have - // to be tracked as well. + // Take one own-key snapshot per prototype level, then inspect each + // descriptor to determine enumerability. This matches `for...in` for + // proxies, whose `ownKeys` trap must not be invoked twice at one level. napi_value global{}; napi_value objectConstructor{}; - napi_value keys{}; napi_value getOwnPropertyNames{}; + napi_value getOwnPropertyDescriptor{}; + napi_value getPrototypeOf{}; RETURN_IF_NOT_OK(napi_get_global(env, &global)); RETURN_IF_NOT_OK(napi_get_named_property(env, global, "Object", &objectConstructor)); - RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "keys", &keys)); RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "getOwnPropertyNames", &getOwnPropertyNames)); + RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "getOwnPropertyDescriptor", &getOwnPropertyDescriptor)); + RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "getPrototypeOf", &getPrototypeOf)); napi_value names{}; RETURN_IF_NOT_OK(napi_create_array(env, &names)); uint32_t nameCount{}; - std::unordered_set shadowed{}; + std::vector shadowed{}; std::vector visited{}; - std::string key{}; // `ToObject` is what the specification (and the V8 implementation) applies // to the argument, so a primitive is wrapped and its properties reported. @@ -113,48 +81,53 @@ namespace napi_shared { break; } - // A `getPrototypeOf` Proxy trap can return an object that is already on - // the chain -- nothing in the specification forbids it, so - // `Object.getPrototypeOf(p) === p` is reachable from script -- which - // makes this walk cyclic. V8 recurses and so terminates with a - // `RangeError`; this loop is iterative and would spin forever. - // - // Stopping at the repeat is exact rather than a bail-out: every level - // adds its own property names to `shadowed` before the walk continues, - // so a level visited a second time can only re-encounter names that are - // already shadowed. Breaking here therefore yields the same result the - // non-terminating walk converges on. bool alreadyVisited{}; RETURN_IF_NOT_OK(Contains(env, visited, current, alreadyVisited)); if (alreadyVisited) { - break; + RETURN_IF_NOT_OK(napi_throw_range_error(env, nullptr, "Cyclic prototype chain")); + return napi_pending_exception; } visited.push_back(current); - napi_value ownEnumerableNames{}; - RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, keys, 1, ¤t, &ownEnumerableNames)); + napi_value ownNames{}; + RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); - uint32_t ownEnumerableCount{}; - RETURN_IF_NOT_OK(napi_get_array_length(env, ownEnumerableNames, &ownEnumerableCount)); - for (uint32_t index = 0; index < ownEnumerableCount; ++index) { + uint32_t ownNameCount{}; + RETURN_IF_NOT_OK(napi_get_array_length(env, ownNames, &ownNameCount)); + for (uint32_t index = 0; index < ownNameCount; ++index) { napi_value name{}; - RETURN_IF_NOT_OK(napi_get_element(env, ownEnumerableNames, index, &name)); - RETURN_IF_NOT_OK(GetUtf8Value(env, name, key)); - if (shadowed.find(key) == shadowed.end()) { + RETURN_IF_NOT_OK(napi_get_element(env, ownNames, index, &name)); + + bool alreadyShadowed{}; + RETURN_IF_NOT_OK(Contains(env, shadowed, name, alreadyShadowed)); + if (alreadyShadowed) { + continue; + } + + napi_value descriptorArgs[]{current, name}; + napi_value descriptor{}; + RETURN_IF_NOT_OK(napi_call_function( + env, objectConstructor, getOwnPropertyDescriptor, 2, descriptorArgs, &descriptor)); + + napi_valuetype descriptorType{}; + RETURN_IF_NOT_OK(napi_typeof(env, descriptor, &descriptorType)); + if (descriptorType == napi_undefined) { + continue; + } + + shadowed.push_back(name); + + napi_value enumerableValue{}; + RETURN_IF_NOT_OK(napi_get_named_property(env, descriptor, "enumerable", &enumerableValue)); + bool enumerable{}; + RETURN_IF_NOT_OK(napi_get_value_bool(env, enumerableValue, &enumerable)); + if (enumerable) { RETURN_IF_NOT_OK(napi_set_element(env, names, nameCount++, name)); } } napi_value next{}; - RETURN_IF_NOT_OK(napi_get_prototype(env, current, &next)); - - bool hasNextLevel{}; - RETURN_IF_NOT_OK(IsObjectLike(env, next, hasNextLevel)); - if (hasNextLevel) { - napi_value ownNames{}; - RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); - RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed)); - } + RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getPrototypeOf, 1, ¤t, &next)); current = next; } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index ae2d7000..83138301 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1750,6 +1750,24 @@ describe("napi_get_property_names (#216)", function () { expect(napiGetPropertyNames(object)).to.deep.equal(["shared"]); }); + it("distinguishes property names containing different lone surrogates", function () { + const object = Object.create({ ["\udc00"]: 1 }); + object["\ud800"] = 2; + expect(napiGetPropertyNames(object)).to.deep.equal(["\ud800", "\udc00"]); + }); + + it("takes one ownKeys snapshot per prototype level", function () { + let ownKeysCalls = 0; + const object = new Proxy({ own: 1 }, { + ownKeys(target) { + ++ownKeysCalls; + return Reflect.ownKeys(target); + }, + }); + expect(napiGetPropertyNames(object)).to.deep.equal(["own"]); + expect(ownKeysCalls).to.equal(1); + }); + it("omits an inherited property shadowed by a non-enumerable own property", function () { const object = Object.create({ shared: 1 }); Object.defineProperty(object, "shared", { value: 2, enumerable: false }); @@ -1858,34 +1876,21 @@ describe("napi_get_property_names (#216)", function () { const usesSharedWalk = hostEngine === "Chakra" || hostEngine === "QuickJS" || hostEngine === "JavaScriptCore"; const describeCycles = usesSharedWalk ? describe : describe.skip; - // A cycle can only be built with a `getPrototypeOf` trap, so observing one - // additionally requires `napi_get_prototype` to consult that trap. - // JavaScriptCore's does not: it calls `JSObjectGetPrototype`, which reads - // the internal [[Prototype]] slot directly and never runs proxy traps - // (`js_native_api_javascriptcore.cc:1348`). A trapped chain there simply - // reports the target's real prototype, so the cycle -- and the throwing - // trap -- are both invisible, and the walk was never at risk on that - // backend. That is a pre-existing limitation of `napi_get_prototype`, not - // of the walk, so it is left alone here; the termination check below is - // still correct and harmless on JavaScriptCore. - const proxyTrapsReachPrototypeWalk = usesSharedWalk && hostEngine !== "JavaScriptCore"; - const itTrapped = proxyTrapsReachPrototypeWalk ? it : it.skip; - describeCycles("cyclic prototype chains", function () { this.timeout(5000); - itTrapped("terminates when a proxy is its own prototype", function () { + it("throws for a proxy that is its own prototype", function () { let object: any; object = new Proxy({ own: 1 }, { getPrototypeOf() { return object; } }); - expect(napiGetPropertyNames(object)).to.deep.equal(["own"]); + expect(() => napiGetPropertyNames(object)).to.throw(RangeError); }); - itTrapped("terminates on a two-object cycle and reports each level once", function () { + it("throws for a two-object prototype cycle", function () { let first: any; let second: any; first = new Proxy({ a: 1 }, { getPrototypeOf() { return second; } }); second = new Proxy({ b: 2 }, { getPrototypeOf() { return first; } }); - expect(napiGetPropertyNames(first)).to.deep.equal(["a", "b"]); + expect(() => napiGetPropertyNames(first)).to.throw(RangeError); }); it("still reports a long acyclic chain in full", function () { @@ -1897,11 +1902,11 @@ describe("napi_get_property_names (#216)", function () { expect(napiGetPropertyNames(leaf)).to.deep.equal(["own", "middle", "deep"]); }); - itTrapped("propagates a throwing getPrototypeOf trap instead of hanging", function () { + it("preserves an exception from a throwing getPrototypeOf trap", function () { const object = new Proxy({ own: 1 }, { getPrototypeOf() { throw new Error("trap"); }, }); - expect(() => napiGetPropertyNames(object)).to.throw(); + expect(() => napiGetPropertyNames(object)).to.throw("trap"); }); }); }); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 08381d78..55738ddc 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -136,7 +136,10 @@ TEST(JavaScript, All) if (napi_is_exception_pending(rawEnv, &isExceptionPending) == napi_ok && isExceptionPending) { napi_value error{}; - napi_get_and_clear_last_exception(rawEnv, &error); + if (napi_get_and_clear_last_exception(rawEnv, &error) == napi_ok) + { + throw Napi::Error{info.Env(), error}; + } } throw Napi::Error::New(info.Env(), "napi_get_property_names failed with status " + std::to_string(status)); From eaf8cfe89c23af8f24bff8f98cff672b909ccbc9 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 08:45:22 -0700 Subject: [PATCH 13/21] Capture property enumeration intrinsics at runtime attachment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Core/Node-API/Source/env_chakra.cc | 10 +++ Core/Node-API/Source/env_javascriptcore.cc | 10 +++ Core/Node-API/Source/env_quickjs.cc | 10 +++ Core/Node-API/Source/js_native_api_chakra.cc | 2 +- Core/Node-API/Source/js_native_api_chakra.h | 2 + .../Source/js_native_api_javascriptcore.cc | 4 +- .../Source/js_native_api_javascriptcore.h | 2 + Core/Node-API/Source/js_native_api_quickjs.cc | 2 +- Core/Node-API/Source/js_native_api_quickjs.h | 2 + Core/Node-API/Source/js_native_api_shared.cc | 84 +++++++++++++++---- Core/Node-API/Source/js_native_api_shared.h | 19 ++++- Tests/UnitTests/Scripts/tests.ts | 52 ++++++++++++ 12 files changed, 177 insertions(+), 22 deletions(-) diff --git a/Core/Node-API/Source/env_chakra.cc b/Core/Node-API/Source/env_chakra.cc index 0c0be91c..a6e3a195 100644 --- a/Core/Node-API/Source/env_chakra.cc +++ b/Core/Node-API/Source/env_chakra.cc @@ -1,6 +1,7 @@ #include #include "js_native_api_chakra.h" #include +#include #include namespace @@ -30,6 +31,11 @@ namespace Napi ThrowIfFailed(JsGetPrototype(object, &prototype)); ThrowIfFailed(JsGetPropertyIdFromName(L"hasOwnProperty", &propertyId)); ThrowIfFailed(JsGetProperty(prototype, propertyId, &env_ptr->has_own_property_function)); + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + delete env_ptr; + throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; + } JsValueRef wrapSymbolDescription; ThrowIfFailed(JsPointerToString(L"BabylonNative_External", 22, &wrapSymbolDescription)); @@ -44,6 +50,10 @@ namespace Napi void Detach(Env env) { napi_env env_ptr{env}; + if (napi_shared::ReleasePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + throw std::runtime_error{"Napi::Detach: failed to release property-name intrinsics"}; + } delete env_ptr; } } diff --git a/Core/Node-API/Source/env_javascriptcore.cc b/Core/Node-API/Source/env_javascriptcore.cc index d97db3a6..75e91015 100644 --- a/Core/Node-API/Source/env_javascriptcore.cc +++ b/Core/Node-API/Source/env_javascriptcore.cc @@ -1,18 +1,28 @@ #include #include #include "js_native_api_javascriptcore.h" +#include namespace Napi { Napi::Env Attach(JSGlobalContextRef context) { napi_env env_ptr{new napi_env__{context}}; + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + delete env_ptr; + throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; + } return {env_ptr}; } void Detach(Napi::Env env) { napi_env env_ptr{env}; + if (napi_shared::ReleasePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + throw std::runtime_error{"Napi::Detach: failed to release property-name intrinsics"}; + } delete env_ptr; } diff --git a/Core/Node-API/Source/env_quickjs.cc b/Core/Node-API/Source/env_quickjs.cc index 6cf36e18..d24eb004 100644 --- a/Core/Node-API/Source/env_quickjs.cc +++ b/Core/Node-API/Source/env_quickjs.cc @@ -56,6 +56,12 @@ namespace Napi } env_ptr->has_own_property_function = hasOwnProperty; + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + JS_FreeValue(context, hasOwnProperty); + delete env_ptr; + throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; + } return {env_ptr}; } @@ -65,6 +71,10 @@ namespace Napi napi_env env_ptr{env}; if (env_ptr) { + if (napi_shared::ReleasePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + throw std::runtime_error{"Napi::Detach: failed to release property-name intrinsics"}; + } // Release every strong napi_ref still outstanding. This mirrors // the V8 impl (napi_env__::DeleteMe) and is essential on QuickJS: // any surviving strong ref pins a JS value from outside the GC diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index a04cdae1..95bdeaac 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -689,7 +689,7 @@ napi_status napi_get_property_names(napi_env env, // inside the walk will have cleared the last error. The success path likewise // has to clear it, so that a rejection recorded by an earlier call does not // survive as the last error of a call that succeeded. - const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result)}; + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result, env->property_name_intrinsics)}; if (status != napi_ok) { return napi_set_last_error(env, status); } diff --git a/Core/Node-API/Source/js_native_api_chakra.h b/Core/Node-API/Source/js_native_api_chakra.h index 2dfa59de..84a7f2a7 100644 --- a/Core/Node-API/Source/js_native_api_chakra.h +++ b/Core/Node-API/Source/js_native_api_chakra.h @@ -5,6 +5,7 @@ #include #include +#include "js_native_api_shared.h" #include #include #include @@ -13,6 +14,7 @@ struct napi_env__ { JsSourceContext source_context = JS_SOURCE_CONTEXT_NONE; napi_extended_error_info last_error{ nullptr, nullptr, 0, napi_ok }; JsValueRef has_own_property_function = JS_INVALID_REFERENCE; + napi_shared::PropertyNameIntrinsics property_name_intrinsics{}; JsPropertyIdRef wrap_property_id = JS_INVALID_REFERENCE; diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index e2c15e1b..62657877 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -74,7 +74,7 @@ namespace { size_t length{JSStringGetLength(_string)}; const JSChar* chars{JSStringGetCharactersPtr(_string)}; size_t size{std::min(length, bufsize - 1)}; - std::memcpy(buf, chars, size); + std::memcpy(buf, chars, size * sizeof(JSChar)); buf[size] = 0; if (result != nullptr) { *result = size; @@ -980,7 +980,7 @@ napi_status napi_get_property_names(napi_env env, // the walk will have cleared the last error. The success path likewise has to // clear it, so that a rejection recorded by an earlier call does not survive // as the last error of a call that succeeded. - const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result)}; + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result, env->property_name_intrinsics)}; if (status != napi_ok) { return napi_set_last_error(env, status); } diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 8d8dbd02..7cc4f38a 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -2,6 +2,7 @@ #include #include +#include "js_native_api_shared.h" #include #include #include @@ -13,6 +14,7 @@ struct napi_env__ { JSGlobalContextRef context{}; JSValueRef last_exception{}; napi_extended_error_info last_error{nullptr, nullptr, 0, napi_ok}; + napi_shared::PropertyNameIntrinsics property_name_intrinsics{}; std::unordered_map active_ref_values{}; std::list strong_refs{}; diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index 3c99aa79..723295ce 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1403,7 +1403,7 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* // cleared the last error. The success path likewise has to clear it, so that // a rejection recorded by an earlier call does not survive as the last error // of a call that succeeded. - const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result)}; + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result, env->property_name_intrinsics)}; if (status != napi_ok) { return napi_set_last_error(env, status); } diff --git a/Core/Node-API/Source/js_native_api_quickjs.h b/Core/Node-API/Source/js_native_api_quickjs.h index 38f9118f..e1bc84b6 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.h +++ b/Core/Node-API/Source/js_native_api_quickjs.h @@ -9,6 +9,7 @@ #pragma clang diagnostic pop #endif #include +#include "js_native_api_shared.h" #include #include #include @@ -27,6 +28,7 @@ struct napi_env__ { JSContext* current_context = nullptr; napi_extended_error_info last_error{ nullptr, nullptr, 0, napi_ok }; JSValue has_own_property_function = JS_UNDEFINED; + napi_shared::PropertyNameIntrinsics property_name_intrinsics{}; const std::thread::id thread_id{std::this_thread::get_id()}; diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index 186b78a7..9d74b9b6 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -2,6 +2,9 @@ #include +#include +#include +#include #include namespace napi_shared { @@ -37,7 +40,52 @@ namespace napi_shared { } } - napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result) { + napi_status ReleasePropertyNameIntrinsics(napi_env env, PropertyNameIntrinsics& intrinsics) { + napi_status firstError{napi_ok}; + for (napi_ref* ref : {&intrinsics.object_constructor, &intrinsics.own_names, + &intrinsics.own_descriptor, &intrinsics.prototype}) { + if (*ref != nullptr) { + const napi_status status{napi_delete_reference(env, *ref)}; + if (status == napi_ok) { + *ref = nullptr; + } else if (firstError == napi_ok) { + firstError = status; + } + } + } + return firstError; + } + + napi_status CapturePropertyNameIntrinsics(napi_env env, PropertyNameIntrinsics& intrinsics) { + napi_value global{}; + napi_value functions[4]{}; + RETURN_IF_NOT_OK(napi_get_global(env, &global)); + RETURN_IF_NOT_OK(napi_get_named_property(env, global, "Object", &functions[0])); + RETURN_IF_NOT_OK(napi_get_named_property(env, functions[0], "getOwnPropertyNames", &functions[1])); + RETURN_IF_NOT_OK(napi_get_named_property(env, functions[0], "getOwnPropertyDescriptor", &functions[2])); + RETURN_IF_NOT_OK(napi_get_named_property(env, functions[0], "getPrototypeOf", &functions[3])); + for (const napi_value function : functions) { + napi_valuetype type{}; + RETURN_IF_NOT_OK(napi_typeof(env, function, &type)); + if (type != napi_function) { + return napi_function_expected; + } + } + + napi_ref* refs[]{&intrinsics.object_constructor, &intrinsics.own_names, + &intrinsics.own_descriptor, &intrinsics.prototype}; + for (size_t index = 0; index < 4; ++index) { + const napi_status status{napi_create_reference(env, functions[index], 1, refs[index])}; + if (status != napi_ok) { + const napi_status cleanupStatus{ReleasePropertyNameIntrinsics(env, intrinsics)}; + return cleanupStatus == napi_ok ? status : cleanupStatus; + } + } + return napi_ok; + } + + napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result, + const PropertyNameIntrinsics& intrinsics) { // Take one own-key snapshot per prototype level, then inspect each // descriptor to determine enumerability. This matches `for...in` for // proxies, whose `ownKeys` trap must not be invoked twice at one level. @@ -47,16 +95,16 @@ namespace napi_shared { napi_value getOwnPropertyDescriptor{}; napi_value getPrototypeOf{}; RETURN_IF_NOT_OK(napi_get_global(env, &global)); - RETURN_IF_NOT_OK(napi_get_named_property(env, global, "Object", &objectConstructor)); - RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "getOwnPropertyNames", &getOwnPropertyNames)); - RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "getOwnPropertyDescriptor", &getOwnPropertyDescriptor)); - RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "getPrototypeOf", &getPrototypeOf)); + RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.object_constructor, &objectConstructor)); + RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.own_names, &getOwnPropertyNames)); + RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.own_descriptor, &getOwnPropertyDescriptor)); + RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.prototype, &getPrototypeOf)); napi_value names{}; RETURN_IF_NOT_OK(napi_create_array(env, &names)); uint32_t nameCount{}; - std::vector shadowed{}; + std::unordered_set shadowed{}; std::vector visited{}; // `ToObject` is what the specification (and the V8 implementation) applies @@ -71,8 +119,10 @@ namespace napi_shared { return napi_object_expected; } - napi_value current{}; - RETURN_IF_NOT_OK(napi_coerce_to_object(env, object, ¤t)); + napi_value current{object}; + if (type != napi_object && type != napi_function && type != napi_external) { + RETURN_IF_NOT_OK(napi_call_function(env, global, objectConstructor, 1, &object, ¤t)); + } while (true) { bool isObjectLike{}; @@ -90,7 +140,7 @@ namespace napi_shared { visited.push_back(current); napi_value ownNames{}; - RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); + RETURN_IF_NOT_OK(napi_call_function(env, global, getOwnPropertyNames, 1, ¤t, &ownNames)); uint32_t ownNameCount{}; RETURN_IF_NOT_OK(napi_get_array_length(env, ownNames, &ownNameCount)); @@ -98,16 +148,20 @@ namespace napi_shared { napi_value name{}; RETURN_IF_NOT_OK(napi_get_element(env, ownNames, index, &name)); - bool alreadyShadowed{}; - RETURN_IF_NOT_OK(Contains(env, shadowed, name, alreadyShadowed)); - if (alreadyShadowed) { + size_t length{}; + RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, nullptr, 0, &length)); + std::u16string key(length + 1, u'\0'); + size_t copied{}; + RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, key.data(), key.size(), &copied)); + key.resize(copied); + if (shadowed.find(key) != shadowed.end()) { continue; } napi_value descriptorArgs[]{current, name}; napi_value descriptor{}; RETURN_IF_NOT_OK(napi_call_function( - env, objectConstructor, getOwnPropertyDescriptor, 2, descriptorArgs, &descriptor)); + env, global, getOwnPropertyDescriptor, 2, descriptorArgs, &descriptor)); napi_valuetype descriptorType{}; RETURN_IF_NOT_OK(napi_typeof(env, descriptor, &descriptorType)); @@ -115,7 +169,7 @@ namespace napi_shared { continue; } - shadowed.push_back(name); + shadowed.insert(std::move(key)); napi_value enumerableValue{}; RETURN_IF_NOT_OK(napi_get_named_property(env, descriptor, "enumerable", &enumerableValue)); @@ -127,7 +181,7 @@ namespace napi_shared { } napi_value next{}; - RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getPrototypeOf, 1, ¤t, &next)); + RETURN_IF_NOT_OK(napi_call_function(env, global, getPrototypeOf, 1, ¤t, &next)); current = next; } diff --git a/Core/Node-API/Source/js_native_api_shared.h b/Core/Node-API/Source/js_native_api_shared.h index 14d13840..eb84be21 100644 --- a/Core/Node-API/Source/js_native_api_shared.h +++ b/Core/Node-API/Source/js_native_api_shared.h @@ -7,6 +7,18 @@ // Backends whose engine offers a faithful native equivalent should keep using // it; these helpers exist for the ones that do not. namespace napi_shared { + // Strong references keep these built-ins independent of later changes to + // globalThis.Object and its static methods. + struct PropertyNameIntrinsics { + napi_ref object_constructor{}; + napi_ref own_names{}; + napi_ref own_descriptor{}; + napi_ref prototype{}; + }; + + napi_status CapturePropertyNameIntrinsics(napi_env env, PropertyNameIntrinsics& intrinsics); + napi_status ReleasePropertyNameIntrinsics(napi_env env, PropertyNameIntrinsics& intrinsics); + // Implements `napi_get_property_names` semantics: the names of all // enumerable string-keyed properties of `object` and of its prototype chain, // as an array of strings, matching a `for...in` enumeration. @@ -16,7 +28,8 @@ namespace napi_shared { // Chakra and QuickJS have no equivalent, so this walks the prototype // chain explicitly. See https://github.com/BabylonJS/JsRuntimeHost/issues/216. // - // `object` is coerced with `napi_coerce_to_object`, as V8's `CHECK_TO_OBJECT` - // does. Callers are expected to have already validated `env` and `result`. - napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result); + // Primitives are wrapped with the captured Object constructor, matching + // V8's `CHECK_TO_OBJECT`. Callers validate `env` and `result`. + napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result, + const PropertyNameIntrinsics& intrinsics); } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 83138301..27ca6387 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1768,6 +1768,46 @@ describe("napi_get_property_names (#216)", function () { expect(ownKeysCalls).to.equal(1); }); + it("does not depend on mutable Object globals", function () { + const objectConstructor = Object; + const ownNames = Object.getOwnPropertyNames; + const ownDescriptor = Object.getOwnPropertyDescriptor; + const prototype = Object.getPrototypeOf; + const object = Object.create({ inherited: 1 }); + object.own = 2; + let names: string[] | undefined; + + try { + Object.getOwnPropertyNames = () => ["forged"]; + Object.getOwnPropertyDescriptor = () => ({ enumerable: false }); + Object.getPrototypeOf = () => null; + Reflect.set(globalThis, "Object", {}); + names = napiGetPropertyNames(object); + } finally { + Reflect.set(globalThis, "Object", objectConstructor); + Object.getOwnPropertyNames = ownNames; + Object.getOwnPropertyDescriptor = ownDescriptor; + Object.getPrototypeOf = prototype; + } + + expect(names).to.deep.equal(["own", "inherited"]); + }); + + it("deduplicates large sets of enumerable and non-enumerable names", function () { + const object = Object.create({ inherited: 1 }); + const expected: string[] = []; + for (let index = 0; index < 1024; ++index) { + const name = `key${index}`; + const enumerable = index % 2 === 0; + Object.defineProperty(object, name, { value: index, enumerable }); + if (enumerable) { + expected.push(name); + } + } + expected.push("inherited"); + expect(napiGetPropertyNames(object)).to.deep.equal(expected); + }); + it("omits an inherited property shadowed by a non-enumerable own property", function () { const object = Object.create({ shared: 1 }); Object.defineProperty(object, "shared", { value: 2, enumerable: false }); @@ -1838,6 +1878,18 @@ describe("napi_get_property_names (#216)", function () { expect(napiGetPropertyNamesRaw("ab")).to.deep.equal(["0", "1"]); }); + it("wraps a primitive after the global Object binding is replaced", function () { + const objectConstructor = Object; + let names: string[] | undefined; + try { + Reflect.set(globalThis, "Object", {}); + names = napiGetPropertyNamesRaw("ab"); + } finally { + Reflect.set(globalThis, "Object", objectConstructor); + } + expect(names).to.deep.equal(["0", "1"]); + }); + it("wraps a number primitive, which has no enumerable properties", function () { expect(napiGetPropertyNamesRaw(42)).to.deep.equal([]); }); From 3daeba92f77698c20cb79a995974859da61384de Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 09:11:07 -0700 Subject: [PATCH 14/21] Release Chakra intrinsic references before runtime disposal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Core/AppRuntime/Source/AppRuntime_Chakra.cpp | 2 ++ Core/Node-API/Include/Engine/Chakra/napi/env.h | 2 ++ Core/Node-API/Source/env_chakra.cc | 10 ++++++++-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp index 8fe33493..e651d202 100644 --- a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp @@ -65,6 +65,8 @@ namespace Babylon Run(env); + // Strong intrinsic references must be released while the Chakra runtime is alive. + Napi::PrepareForRuntimeDisposal(env); ThrowIfFailed(JsSetCurrentContext(JS_INVALID_REFERENCE)); ThrowIfFailed(JsDisposeRuntime(jsRuntime)); diff --git a/Core/Node-API/Include/Engine/Chakra/napi/env.h b/Core/Node-API/Include/Engine/Chakra/napi/env.h index 4245fba1..3f0acf02 100644 --- a/Core/Node-API/Include/Engine/Chakra/napi/env.h +++ b/Core/Node-API/Include/Engine/Chakra/napi/env.h @@ -6,6 +6,8 @@ namespace Napi { Napi::Env Attach(); + void PrepareForRuntimeDisposal(Napi::Env); + void Detach(Napi::Env); Napi::Value Eval(Napi::Env env, const char* source, const char* sourceUrl); diff --git a/Core/Node-API/Source/env_chakra.cc b/Core/Node-API/Source/env_chakra.cc index a6e3a195..95e52710 100644 --- a/Core/Node-API/Source/env_chakra.cc +++ b/Core/Node-API/Source/env_chakra.cc @@ -47,13 +47,19 @@ namespace Napi return {env_ptr}; } - void Detach(Env env) + void PrepareForRuntimeDisposal(Env env) { napi_env env_ptr{env}; if (napi_shared::ReleasePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) { - throw std::runtime_error{"Napi::Detach: failed to release property-name intrinsics"}; + throw std::runtime_error{"Napi::PrepareForRuntimeDisposal: failed to release property-name intrinsics"}; } + } + + void Detach(Env env) + { + napi_env env_ptr{env}; + PrepareForRuntimeDisposal(env); delete env_ptr; } } From 0445f7d67bbdedc3f58148e7221952b88a1513de Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 09:46:02 -0700 Subject: [PATCH 15/21] Trace first property enumeration call for Chakra CI diagnosis Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Core/Node-API/Source/js_native_api_shared.cc | 25 ++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index 9d74b9b6..a4a0555a 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -2,6 +2,8 @@ #include +#include +#include #include #include #include @@ -86,6 +88,19 @@ namespace napi_shared { napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result, const PropertyNameIntrinsics& intrinsics) { +#ifdef _WIN32 + static std::atomic tracedCalls{0}; + const bool tracing = tracedCalls.fetch_add(1) == 0; + const auto trace = [tracing](const char* stage) { + if (tracing) { + std::fprintf(stderr, "PROPERTY_NAMES_TRACE: %s\n", stage); + std::fflush(stderr); + } + }; + trace("start"); +#else + const auto trace = [](const char*) {}; +#endif // Take one own-key snapshot per prototype level, then inspect each // descriptor to determine enumerability. This matches `for...in` for // proxies, whose `ownKeys` trap must not be invoked twice at one level. @@ -99,9 +114,11 @@ namespace napi_shared { RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.own_names, &getOwnPropertyNames)); RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.own_descriptor, &getOwnPropertyDescriptor)); RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.prototype, &getPrototypeOf)); + trace("references"); napi_value names{}; RETURN_IF_NOT_OK(napi_create_array(env, &names)); + trace("array"); uint32_t nameCount{}; std::unordered_set shadowed{}; @@ -115,6 +132,7 @@ namespace napi_shared { // between engines (QuickJS yields an empty object, JavaScriptCore throws). napi_valuetype type{}; RETURN_IF_NOT_OK(napi_typeof(env, object, &type)); + trace("type"); if (type == napi_null || type == napi_undefined) { return napi_object_expected; } @@ -125,6 +143,7 @@ namespace napi_shared { } while (true) { + trace("level"); bool isObjectLike{}; RETURN_IF_NOT_OK(IsObjectLike(env, current, isObjectLike)); if (!isObjectLike) { @@ -141,18 +160,22 @@ namespace napi_shared { napi_value ownNames{}; RETURN_IF_NOT_OK(napi_call_function(env, global, getOwnPropertyNames, 1, ¤t, &ownNames)); + trace("own names"); uint32_t ownNameCount{}; RETURN_IF_NOT_OK(napi_get_array_length(env, ownNames, &ownNameCount)); + trace("name count"); for (uint32_t index = 0; index < ownNameCount; ++index) { napi_value name{}; RETURN_IF_NOT_OK(napi_get_element(env, ownNames, index, &name)); size_t length{}; RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, nullptr, 0, &length)); + trace("utf16 length"); std::u16string key(length + 1, u'\0'); size_t copied{}; RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, key.data(), key.size(), &copied)); + trace("utf16 copied"); key.resize(copied); if (shadowed.find(key) != shadowed.end()) { continue; @@ -162,6 +185,7 @@ namespace napi_shared { napi_value descriptor{}; RETURN_IF_NOT_OK(napi_call_function( env, global, getOwnPropertyDescriptor, 2, descriptorArgs, &descriptor)); + trace("descriptor"); napi_valuetype descriptorType{}; RETURN_IF_NOT_OK(napi_typeof(env, descriptor, &descriptorType)); @@ -182,6 +206,7 @@ namespace napi_shared { napi_value next{}; RETURN_IF_NOT_OK(napi_call_function(env, global, getPrototypeOf, 1, ¤t, &next)); + trace("prototype"); current = next; } From b83e3c8323317b06bd2e07e4e5c8d768fa9367bf Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 10:02:52 -0700 Subject: [PATCH 16/21] Record Chakra property-name length before copying Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Core/Node-API/Source/js_native_api_shared.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index a4a0555a..5b4a3b5b 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -172,7 +172,14 @@ namespace napi_shared { size_t length{}; RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, nullptr, 0, &length)); trace("utf16 length"); +#ifdef _WIN32 + if (tracing) { + std::fprintf(stderr, "PROPERTY_NAMES_TRACE: length=%zu\n", length); + std::fflush(stderr); + } +#endif std::u16string key(length + 1, u'\0'); + trace("key allocated"); size_t copied{}; RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, key.data(), key.size(), &copied)); trace("utf16 copied"); From b8d98d037da838d8a616739f8361e251686e7228 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 10:08:22 -0700 Subject: [PATCH 17/21] Bound Chakra UTF-16 copies in bytes and report copied length Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Core/Node-API/Source/js_native_api_chakra.cc | 9 ++++-- Core/Node-API/Source/js_native_api_shared.cc | 32 ------------------- Tests/UnitTests/Shared/Shared.cpp | 33 ++++++++++++++++++++ 3 files changed, 39 insertions(+), 35 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 95bdeaac..064ac7e6 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -1,9 +1,11 @@ #include "js_native_api_chakra.h" #include "js_native_api_shared.h" #include +#include #include #include #include +#include #include #include #include @@ -49,13 +51,14 @@ JsErrorCode JsCopyStringUtf16(_In_ JsValueRef value, _Out_opt_ char16_t* buffer, size_t stringLength; CHECK_JSRT_ERROR_CODE(JsStringToPointer(value, &stringValue, &stringLength)); + const size_t copied = buffer == nullptr ? stringLength : std::min(bufferSize, stringLength); if (length != nullptr) { - *length = stringLength; + *length = copied; } - if (buffer != nullptr) { + if (buffer != nullptr && copied != 0) { static_assert(sizeof(char16_t) == sizeof(wchar_t)); - memcpy_s(buffer, bufferSize, stringValue, stringLength * sizeof(wchar_t)); + std::memcpy(buffer, stringValue, copied * sizeof(char16_t)); } return JsErrorCode::JsNoError; diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index 5b4a3b5b..9d74b9b6 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -2,8 +2,6 @@ #include -#include -#include #include #include #include @@ -88,19 +86,6 @@ namespace napi_shared { napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result, const PropertyNameIntrinsics& intrinsics) { -#ifdef _WIN32 - static std::atomic tracedCalls{0}; - const bool tracing = tracedCalls.fetch_add(1) == 0; - const auto trace = [tracing](const char* stage) { - if (tracing) { - std::fprintf(stderr, "PROPERTY_NAMES_TRACE: %s\n", stage); - std::fflush(stderr); - } - }; - trace("start"); -#else - const auto trace = [](const char*) {}; -#endif // Take one own-key snapshot per prototype level, then inspect each // descriptor to determine enumerability. This matches `for...in` for // proxies, whose `ownKeys` trap must not be invoked twice at one level. @@ -114,11 +99,9 @@ namespace napi_shared { RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.own_names, &getOwnPropertyNames)); RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.own_descriptor, &getOwnPropertyDescriptor)); RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.prototype, &getPrototypeOf)); - trace("references"); napi_value names{}; RETURN_IF_NOT_OK(napi_create_array(env, &names)); - trace("array"); uint32_t nameCount{}; std::unordered_set shadowed{}; @@ -132,7 +115,6 @@ namespace napi_shared { // between engines (QuickJS yields an empty object, JavaScriptCore throws). napi_valuetype type{}; RETURN_IF_NOT_OK(napi_typeof(env, object, &type)); - trace("type"); if (type == napi_null || type == napi_undefined) { return napi_object_expected; } @@ -143,7 +125,6 @@ namespace napi_shared { } while (true) { - trace("level"); bool isObjectLike{}; RETURN_IF_NOT_OK(IsObjectLike(env, current, isObjectLike)); if (!isObjectLike) { @@ -160,29 +141,18 @@ namespace napi_shared { napi_value ownNames{}; RETURN_IF_NOT_OK(napi_call_function(env, global, getOwnPropertyNames, 1, ¤t, &ownNames)); - trace("own names"); uint32_t ownNameCount{}; RETURN_IF_NOT_OK(napi_get_array_length(env, ownNames, &ownNameCount)); - trace("name count"); for (uint32_t index = 0; index < ownNameCount; ++index) { napi_value name{}; RETURN_IF_NOT_OK(napi_get_element(env, ownNames, index, &name)); size_t length{}; RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, nullptr, 0, &length)); - trace("utf16 length"); -#ifdef _WIN32 - if (tracing) { - std::fprintf(stderr, "PROPERTY_NAMES_TRACE: length=%zu\n", length); - std::fflush(stderr); - } -#endif std::u16string key(length + 1, u'\0'); - trace("key allocated"); size_t copied{}; RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, key.data(), key.size(), &copied)); - trace("utf16 copied"); key.resize(copied); if (shadowed.find(key) != shadowed.end()) { continue; @@ -192,7 +162,6 @@ namespace napi_shared { napi_value descriptor{}; RETURN_IF_NOT_OK(napi_call_function( env, global, getOwnPropertyDescriptor, 2, descriptorArgs, &descriptor)); - trace("descriptor"); napi_valuetype descriptorType{}; RETURN_IF_NOT_OK(napi_typeof(env, descriptor, &descriptorType)); @@ -213,7 +182,6 @@ namespace napi_shared { napi_value next{}; RETURN_IF_NOT_OK(napi_call_function(env, global, getPrototypeOf, 1, ¤t, &next)); - trace("prototype"); current = next; } diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 55738ddc..df48ab4d 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -506,6 +506,39 @@ TEST(NodeApi, GetValueStringUtf16HandlesZeroBufsize) EXPECT_TRUE(normalWorks.get_future().get()); } +TEST(NodeApi, GetValueStringUtf16CopiesAndTruncates) +{ + Babylon::AppRuntime runtime{}; + std::promise copiedCorrectly; + + runtime.Dispatch([&copiedCorrectly](Napi::Env env) { + napi_env nenv{env}; + const char16_t input[]{u'a', static_cast(0xD800), u'b'}; + napi_value value{}; + if (napi_create_string_utf16(nenv, input, 3, &value) != napi_ok) + { + copiedCorrectly.set_value(false); + return; + } + + size_t length{}; + char16_t truncated[2]{u'?', u'?'}; + size_t truncatedLength{}; + char16_t complete[4]{}; + size_t completeLength{}; + const bool correct = + napi_get_value_string_utf16(nenv, value, nullptr, 0, &length) == napi_ok && length == 3 && + napi_get_value_string_utf16(nenv, value, truncated, 2, &truncatedLength) == napi_ok && + truncatedLength == 1 && truncated[0] == u'a' && truncated[1] == u'\0' && + napi_get_value_string_utf16(nenv, value, complete, 4, &completeLength) == napi_ok && + completeLength == 3 && complete[0] == u'a' && complete[1] == input[1] && + complete[2] == u'b' && complete[3] == u'\0'; + copiedCorrectly.set_value(correct); + }); + + EXPECT_TRUE(copiedCorrectly.get_future().get()); +} + // Closes an escapable handle scope however the test leaves it. Without this, a // failing assertion returns with the scope still open, the enclosing // Napi::HandleScope then fails to close, and Napi::Error::Fatal throws out of its From 4de2e56fd37104a7da2332420b480936f91f3a20 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 10:24:41 -0700 Subject: [PATCH 18/21] Use the global object in Chakra property-name regressions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Tests/UnitTests/Scripts/tests.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 27ca6387..9cae6e35 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1704,6 +1704,8 @@ describe("napi_get_property_names (#216)", function () { // chain*, i.e. exactly what `for...in` visits. JavaScriptCore used to throw // outright, while Chakra and QuickJS only reported own properties // (Chakra additionally reported non-enumerable ones). + // Chakra does not define globalThis; a non-strict function returns the global object. + const globalObject = Function("return this")(); function forIn(object: any): string[] { const keys: string[] = []; @@ -1781,10 +1783,10 @@ describe("napi_get_property_names (#216)", function () { Object.getOwnPropertyNames = () => ["forged"]; Object.getOwnPropertyDescriptor = () => ({ enumerable: false }); Object.getPrototypeOf = () => null; - Reflect.set(globalThis, "Object", {}); + Reflect.set(globalObject, "Object", {}); names = napiGetPropertyNames(object); } finally { - Reflect.set(globalThis, "Object", objectConstructor); + Reflect.set(globalObject, "Object", objectConstructor); Object.getOwnPropertyNames = ownNames; Object.getOwnPropertyDescriptor = ownDescriptor; Object.getPrototypeOf = prototype; @@ -1882,10 +1884,10 @@ describe("napi_get_property_names (#216)", function () { const objectConstructor = Object; let names: string[] | undefined; try { - Reflect.set(globalThis, "Object", {}); + Reflect.set(globalObject, "Object", {}); names = napiGetPropertyNamesRaw("ab"); } finally { - Reflect.set(globalThis, "Object", objectConstructor); + Reflect.set(globalObject, "Object", objectConstructor); } expect(names).to.deep.equal(["0", "1"]); }); From e32c6421959642f14053142228cd8afe446272b3 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Fri, 25 Sep 2026 16:08:42 -0700 Subject: [PATCH 19/21] Own cached references across runtime teardown Carry the ownership fix and engine-specific regressions from a5a86a94947d90a5edc63defd82d66c51dfdd9d4 on the reorganized test layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc6f260a-f58d-49ed-9bed-d69248708138 --- Core/Node-API/Source/env_chakra.cc | 89 +++++++++++++------ Core/Node-API/Source/js_native_api_chakra.cc | 9 +- Core/Node-API/Source/js_native_api_chakra.h | 2 + .../Source/js_native_api_javascriptcore.cc | 28 +++--- .../Source/js_native_api_javascriptcore.h | 7 +- Tests/UnitTests/CMakeLists.txt | 6 ++ .../UnitTests/Source/Tests.NodeApi.Chakra.cpp | 34 +++++++ .../Source/Tests.NodeApi.JavaScriptCore.cpp | 32 +++++++ 8 files changed, 161 insertions(+), 46 deletions(-) create mode 100644 Tests/UnitTests/Source/Tests.NodeApi.Chakra.cpp create mode 100644 Tests/UnitTests/Source/Tests.NodeApi.JavaScriptCore.cpp diff --git a/Core/Node-API/Source/env_chakra.cc b/Core/Node-API/Source/env_chakra.cc index 95e52710..ca92f3aa 100644 --- a/Core/Node-API/Source/env_chakra.cc +++ b/Core/Node-API/Source/env_chakra.cc @@ -1,6 +1,8 @@ #include #include "js_native_api_chakra.h" #include +#include +#include #include #include @@ -13,46 +15,83 @@ namespace throw std::exception(); } } + + napi_status ReleaseCachedReferences(napi_env env) + { + napi_status firstError{napi_shared::ReleasePropertyNameIntrinsics(env, env->property_name_intrinsics)}; + for (napi_ref* ref : {&env->has_own_property_reference, &env->wrap_symbol_reference}) + { + if (*ref != nullptr) + { + const napi_status status{napi_delete_reference(env, *ref)}; + if (status == napi_ok) + { + *ref = nullptr; + } + else if (firstError == napi_ok) + { + firstError = status; + } + } + } + return firstError; + } } namespace Napi { Env Attach() { - napi_env env_ptr{new napi_env__}; - - JsValueRef global; - ThrowIfFailed(JsGetGlobalObject(&global)); - JsPropertyIdRef propertyId; - ThrowIfFailed(JsGetPropertyIdFromName(L"Object", &propertyId)); - JsValueRef object; - ThrowIfFailed(JsGetProperty(global, propertyId, &object)); - JsValueRef prototype; - ThrowIfFailed(JsGetPrototype(object, &prototype)); - ThrowIfFailed(JsGetPropertyIdFromName(L"hasOwnProperty", &propertyId)); - ThrowIfFailed(JsGetProperty(prototype, propertyId, &env_ptr->has_own_property_function)); - if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + auto env_ptr{std::make_unique()}; + try { - delete env_ptr; - throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; - } + JsValueRef global; + ThrowIfFailed(JsGetGlobalObject(&global)); + JsPropertyIdRef propertyId; + ThrowIfFailed(JsGetPropertyIdFromName(L"Object", &propertyId)); + JsValueRef object; + ThrowIfFailed(JsGetProperty(global, propertyId, &object)); + JsValueRef prototype; + ThrowIfFailed(JsGetPrototype(object, &prototype)); + ThrowIfFailed(JsGetPropertyIdFromName(L"hasOwnProperty", &propertyId)); + ThrowIfFailed(JsGetProperty(prototype, propertyId, &env_ptr->has_own_property_function)); + if (napi_create_reference(env_ptr.get(), reinterpret_cast(env_ptr->has_own_property_function), 1, &env_ptr->has_own_property_reference) != napi_ok) + { + throw std::runtime_error{"Napi::Attach: failed to retain hasOwnProperty"}; + } + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr.get(), env_ptr->property_name_intrinsics) != napi_ok) + { + throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; + } - JsValueRef wrapSymbolDescription; - ThrowIfFailed(JsPointerToString(L"BabylonNative_External", 22, &wrapSymbolDescription)); - JsValueRef wrapSymbol; - ThrowIfFailed(JsCreateSymbol(wrapSymbolDescription, &wrapSymbol)); - ThrowIfFailed(JsAddRef(wrapSymbol, nullptr)); - ThrowIfFailed(JsGetPropertyIdFromSymbol(wrapSymbol, &env_ptr->wrap_property_id)); + JsValueRef wrapSymbolDescription; + ThrowIfFailed(JsPointerToString(L"BabylonNative_External", 22, &wrapSymbolDescription)); + JsValueRef wrapSymbol; + ThrowIfFailed(JsCreateSymbol(wrapSymbolDescription, &wrapSymbol)); + if (napi_create_reference(env_ptr.get(), reinterpret_cast(wrapSymbol), 1, &env_ptr->wrap_symbol_reference) != napi_ok) + { + throw std::runtime_error{"Napi::Attach: failed to retain wrap symbol"}; + } + ThrowIfFailed(JsGetPropertyIdFromSymbol(wrapSymbol, &env_ptr->wrap_property_id)); - return {env_ptr}; + return {env_ptr.release()}; + } + catch (...) + { + if (ReleaseCachedReferences(env_ptr.get()) != napi_ok) + { + std::throw_with_nested(std::runtime_error{"Napi::Attach: failed to release cached references"}); + } + throw; + } } void PrepareForRuntimeDisposal(Env env) { napi_env env_ptr{env}; - if (napi_shared::ReleasePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + if (ReleaseCachedReferences(env_ptr) != napi_ok) { - throw std::runtime_error{"Napi::PrepareForRuntimeDisposal: failed to release property-name intrinsics"}; + throw std::runtime_error{"Napi::PrepareForRuntimeDisposal: failed to release cached references"}; } } diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 064ac7e6..58815bae 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -1840,17 +1841,13 @@ napi_status napi_create_reference(napi_env env, CHECK_ARG(env, result); auto jsValue = reinterpret_cast(value); - auto info = new RefInfo{ reinterpret_cast(value), initial_refcount }; - if (info == nullptr) { - return napi_set_last_error(env, napi_generic_failure); - } - + std::unique_ptr info{new RefInfo{jsValue, initial_refcount}}; if (info->count != 0) { CHECK_JSRT(env, JsAddRef(jsValue, nullptr)); } - *result = reinterpret_cast(info); + *result = reinterpret_cast(info.release()); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_chakra.h b/Core/Node-API/Source/js_native_api_chakra.h index 84a7f2a7..a8476880 100644 --- a/Core/Node-API/Source/js_native_api_chakra.h +++ b/Core/Node-API/Source/js_native_api_chakra.h @@ -14,9 +14,11 @@ struct napi_env__ { JsSourceContext source_context = JS_SOURCE_CONTEXT_NONE; napi_extended_error_info last_error{ nullptr, nullptr, 0, napi_ok }; JsValueRef has_own_property_function = JS_INVALID_REFERENCE; + napi_ref has_own_property_reference{}; napi_shared::PropertyNameIntrinsics property_name_intrinsics{}; JsPropertyIdRef wrap_property_id = JS_INVALID_REFERENCE; + napi_ref wrap_symbol_reference{}; // Escapable scope bookkeeping: token -> whether that scope has escaped. Values // are rooted by the engine rather than by a scope here, so this exists only to diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 62657877..fe0c269f 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -740,23 +740,27 @@ struct napi_ref__ { // track the ref values to support weak refs CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); if (_objectId == 0) { - CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value](ReferenceInfo* info) { - auto entry{info->Env()->active_ref_values.find(value)}; + CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [state = std::weak_ptr{env->reference_tracking_state}, value = _value](ReferenceInfo* info) { + const auto trackingState{state.lock()}; + if (!trackingState) { + return; + } // NOTE: The finalizer callback is actually on a "sentinel" JS object that is linked to the // actual JS object we are trying to track. This means it is possible for the tracked object // to be garbage collected and a new object created at the same memory address before we get // the callback for the sentinel object finalizer. Guard against this by checking that the // tracked object still has the same unique object id. - if (entry != info->Env()->active_ref_values.end() && entry->second == info->GetObjectId()) { - info->Env()->active_ref_values.erase(entry); + auto entry{trackingState->active_ref_values.find(value)}; + if (entry != trackingState->active_ref_values.end() && entry->second == info->GetObjectId()) { + trackingState->active_ref_values.erase(entry); } })); CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); assert(_objectId); - env->active_ref_values[_value] = _objectId; + env->reference_tracking_state->active_ref_values[_value] = _objectId; } else { - assert(env->active_ref_values.find(_value) != env->active_ref_values.end()); + assert(env->reference_tracking_state->active_ref_values.find(_value) != env->reference_tracking_state->active_ref_values.end()); } if (_count != 0) { @@ -796,7 +800,7 @@ struct napi_ref__ { napi_status value(napi_env env, napi_value* result) const { assert(_value); - if (env->active_ref_values.find(_value) != env->active_ref_values.end()) { + if (env->reference_tracking_state->active_ref_values.find(_value) != env->reference_tracking_state->active_ref_values.end()) { std::uintptr_t objectId{}; // NOTE: This check is needed for the same reason we need a similar check in the init function. // See the comment in init for more details. @@ -2084,13 +2088,9 @@ napi_status napi_create_reference(napi_env env, CHECK_ARG(env, value); CHECK_ARG(env, result); - napi_ref__* ref{new napi_ref__{}}; - if (ref == nullptr) { - return napi_set_last_error(env, napi_generic_failure); - } - - ref->init(env, value, initial_refcount); - *result = ref; + std::unique_ptr ref{new napi_ref__{}}; + CHECK_NAPI(ref->init(env, value, initial_refcount)); + *result = ref.release(); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 7cc4f38a..2ebefa20 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -4,18 +4,23 @@ #include #include "js_native_api_shared.h" #include +#include #include #include #include #include #include +struct napi_reference_tracking_state { + std::unordered_map active_ref_values{}; +}; + struct napi_env__ { JSGlobalContextRef context{}; JSValueRef last_exception{}; napi_extended_error_info last_error{nullptr, nullptr, 0, napi_ok}; napi_shared::PropertyNameIntrinsics property_name_intrinsics{}; - std::unordered_map active_ref_values{}; + std::shared_ptr reference_tracking_state{std::make_shared()}; std::list strong_refs{}; JSValueRef constructor_info_symbol{}; diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index 69cc6cac..6f6f88a6 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -63,6 +63,12 @@ elseif(UNIX AND NOT ANDROID) Source/Linux/App.cpp) endif() +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "Chakra") + set(SOURCES ${SOURCES} "Source/Tests.NodeApi.Chakra.cpp") +elseif(NAPI_JAVASCRIPT_ENGINE STREQUAL "JavaScriptCore") + set(SOURCES ${SOURCES} "Source/Tests.NodeApi.JavaScriptCore.cpp") +endif() + add_executable(UnitTests ${SOURCES} ${TEST_JAVASCRIPT} ${SCRIPTS} ${ASSETS}) if(NAPI_JAVASCRIPT_ENGINE STREQUAL "V8") target_sources(UnitTests PRIVATE "Source/Tests.V8ForegroundTaskRunner.cpp") diff --git a/Tests/UnitTests/Source/Tests.NodeApi.Chakra.cpp b/Tests/UnitTests/Source/Tests.NodeApi.Chakra.cpp new file mode 100644 index 00000000..5330db13 --- /dev/null +++ b/Tests/UnitTests/Source/Tests.NodeApi.Chakra.cpp @@ -0,0 +1,34 @@ +#include +#include +#include +#include + +TEST(NodeApi, CachedHasOwnPropertySurvivesReplacementAndCollection) +{ + Babylon::AppRuntime runtime{}; + std::promise result; + runtime.Dispatch([&result](Napi::Env env) { + napi_env rawEnv{env}; + Napi::Object prototype{env.Global().Get("Object").As().Get("prototype").As()}; + prototype.Set("hasOwnProperty", Napi::Function::New(env, [](const Napi::CallbackInfo& info) { + return Napi::Boolean::New(info.Env(), false); + })); + + JsContextRef context{}; + JsRuntimeHandle jsRuntime{}; + if (JsGetCurrentContext(&context) != JsNoError || + JsGetRuntime(context, &jsRuntime) != JsNoError || + JsCollectGarbage(jsRuntime) != JsNoError) + { + result.set_value(false); + return; + } + + Napi::Object object{Napi::Object::New(env)}; + object.Set("owned", true); + Napi::String key{Napi::String::New(env, "owned")}; + bool hasOwn{}; + result.set_value(napi_has_own_property(rawEnv, object, key, &hasOwn) == napi_ok && hasOwn); + }); + EXPECT_TRUE(result.get_future().get()); +} diff --git a/Tests/UnitTests/Source/Tests.NodeApi.JavaScriptCore.cpp b/Tests/UnitTests/Source/Tests.NodeApi.JavaScriptCore.cpp new file mode 100644 index 00000000..63123413 --- /dev/null +++ b/Tests/UnitTests/Source/Tests.NodeApi.JavaScriptCore.cpp @@ -0,0 +1,32 @@ +#include +#include +#include + +TEST(NodeApi, ReferenceSentinelCanFinalizeAfterDetach) +{ + JSGlobalContextRef context{JSGlobalContextCreate(nullptr)}; + ASSERT_NE(context, nullptr); + Napi::Env env{Napi::Attach(context)}; + napi_env rawEnv{env}; + napi_value object{}; + napi_value global{}; + napi_ref ref{}; + const bool created{ + napi_create_object(rawEnv, &object) == napi_ok && + napi_get_global(rawEnv, &global) == napi_ok && + napi_set_named_property(rawEnv, global, "retained", object) == napi_ok && + napi_create_reference(rawEnv, object, 1, &ref) == napi_ok}; + EXPECT_TRUE(created); + if (ref != nullptr) + { + EXPECT_EQ(napi_delete_reference(rawEnv, ref), napi_ok); + } + Napi::Detach(env); + + JSStringRef name{JSStringCreateWithUTF8CString("retained")}; + EXPECT_TRUE(JSObjectDeleteProperty(context, JSContextGetGlobalObject(context), name, nullptr)); + JSStringRelease(name); + JSGarbageCollect(context); + JSGarbageCollect(context); + JSGlobalContextRelease(context); +} From 77fe3b88ad2b0093b2287fd1d69403c69a46ce9b Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Fri, 25 Sep 2026 16:31:59 -0700 Subject: [PATCH 20/21] Name property-name tests by API rather than issue Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc6f260a-f58d-49ed-9bed-d69248708138 --- Tests/UnitTests/Source/Scripts/tests.nodeApi.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Tests/UnitTests/Source/Scripts/tests.nodeApi.ts b/Tests/UnitTests/Source/Scripts/tests.nodeApi.ts index 654bd097..28fa8c16 100644 --- a/Tests/UnitTests/Source/Scripts/tests.nodeApi.ts +++ b/Tests/UnitTests/Source/Scripts/tests.nodeApi.ts @@ -40,12 +40,7 @@ describe("napi class prototype isolation (#172)", function () { }); }); -describe("napi_get_property_names (#216)", function () { - // Regression coverage for #216: napi_get_property_names must report the - // enumerable string-keyed properties of an object *and its prototype - // chain*, i.e. exactly what `for...in` visits. JavaScriptCore used to throw - // outright, while Chakra and QuickJS only reported own properties - // (Chakra additionally reported non-enumerable ones). +describe("napi_get_property_names", function () { // Chakra does not define globalThis; a non-strict function returns the global object. const globalObject = Function("return this")(); From f0ac6007f0b207ba89d91fae898a1a2dbccc7aa1 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Fri, 25 Sep 2026 16:42:34 -0700 Subject: [PATCH 21/21] Guard JavaScriptCore and QuickJS attachment with unique ownership Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc6f260a-f58d-49ed-9bed-d69248708138 --- Core/Node-API/Source/env_javascriptcore.cc | 8 ++++---- Core/Node-API/Source/env_quickjs.cc | 11 ++++------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/Core/Node-API/Source/env_javascriptcore.cc b/Core/Node-API/Source/env_javascriptcore.cc index 75e91015..76e074f6 100644 --- a/Core/Node-API/Source/env_javascriptcore.cc +++ b/Core/Node-API/Source/env_javascriptcore.cc @@ -1,19 +1,19 @@ #include #include #include "js_native_api_javascriptcore.h" +#include #include namespace Napi { Napi::Env Attach(JSGlobalContextRef context) { - napi_env env_ptr{new napi_env__{context}}; - if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + auto env_ptr{std::make_unique(context)}; + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr.get(), env_ptr->property_name_intrinsics) != napi_ok) { - delete env_ptr; throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; } - return {env_ptr}; + return {env_ptr.release()}; } void Detach(Napi::Env env) diff --git a/Core/Node-API/Source/env_quickjs.cc b/Core/Node-API/Source/env_quickjs.cc index d24eb004..67530a08 100644 --- a/Core/Node-API/Source/env_quickjs.cc +++ b/Core/Node-API/Source/env_quickjs.cc @@ -1,5 +1,6 @@ #include #include "js_native_api_quickjs.h" +#include #include #if defined(__clang__) #pragma clang diagnostic push @@ -14,7 +15,7 @@ namespace Napi { Env Attach(JSContext* context) { - napi_env env_ptr{new napi_env__}; + auto env_ptr{std::make_unique()}; env_ptr->context = context; env_ptr->current_context = env_ptr->context; @@ -29,7 +30,6 @@ namespace Napi if (JS_IsException(object) || !JS_IsObject(object)) { JS_FreeValue(context, object); - delete env_ptr; throw std::runtime_error{"Napi::Attach: failed to resolve the global 'Object' constructor"}; } @@ -42,7 +42,6 @@ namespace Napi if (JS_IsException(prototype) || !JS_IsObject(prototype)) { JS_FreeValue(context, prototype); - delete env_ptr; throw std::runtime_error{"Napi::Attach: failed to resolve Object.prototype"}; } @@ -51,19 +50,17 @@ namespace Napi if (JS_IsException(hasOwnProperty) || !JS_IsFunction(context, hasOwnProperty)) { JS_FreeValue(context, hasOwnProperty); - delete env_ptr; throw std::runtime_error{"Napi::Attach: failed to resolve Object.prototype.hasOwnProperty"}; } env_ptr->has_own_property_function = hasOwnProperty; - if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr.get(), env_ptr->property_name_intrinsics) != napi_ok) { JS_FreeValue(context, hasOwnProperty); - delete env_ptr; throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; } - return {env_ptr}; + return {env_ptr.release()}; } void Detach(Env env)