From 2fac54e35508871e335a95c5b3969be1c3bf62b4 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:01:54 -0700 Subject: [PATCH 1/7] 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 67066d92..700a011a 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 6d1ade9c..4a8dfe65 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 c9e2823d..2f090921 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 cdc9416b..353ada71 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -7,6 +7,7 @@ Mocha.reporter('spec'); declare const hostPlatform: string; declare const setExitCode: (code: number) => void; +declare const napiGetPropertyNames: (object: any) => string[]; describe("AbortController", function () { @@ -1621,6 +1622,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 a920fa1f..4c3b0c41 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -100,6 +100,16 @@ TEST(JavaScript, All) env.Global().Set("setExitCode", setExitCodeCallback); env.Global().Set("hostPlatform", Napi::Value::From(env, JSRUNTIMEHOST_PLATFORM)); + + // 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 9e3c304a1c397e1e6aadb8629aa4a521d701419e Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:25:59 -0700 Subject: [PATCH 2/7] 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 4a8dfe65..922952eb 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 353ada71..bf4afbdd 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1637,6 +1637,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"]); }); @@ -1701,8 +1709,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 3b857e7c4f5e539fe623a02d129787c806820ce8 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:49:44 -0700 Subject: [PATCH 3/7] 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 c3def88da9fa66f694366fa2269997dfa7feb5b4 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 08:06:38 -0700 Subject: [PATCH 4/7] 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 bf4afbdd..323b1ed1 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 setExitCode: (code: number) => void; declare const napiGetPropertyNames: (object: any) => string[]; +declare const napiGetPropertyNamesRaw: (object: any) => string[]; describe("AbortController", function () { @@ -1714,6 +1715,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 4c3b0c41..35bb3a83 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include namespace @@ -110,6 +111,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 ad868a2ed61c3261abded2b04bd270adde494404 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 08:44:40 -0700 Subject: [PATCH 5/7] 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/CMakeLists.txt | 1 + Tests/UnitTests/Scripts/tests.ts | 26 ++++++++++++++++++-------- Tests/UnitTests/Shared/Shared.cpp | 1 + 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index 2dbc7619..c12b12f8 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -45,6 +45,7 @@ endif() add_executable(UnitTests ${SOURCES} ${SCRIPTS} ${TYPE_SCRIPTS} ${ASSETS}) target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") +target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") # The V8JSI Node-API shim does not implement napi_create_dataview, so the # CreateDataViewRejectsOverflowingRange test is compiled out on that backend. diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 323b1ed1..5498bffa 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -9,6 +9,7 @@ declare const hostPlatform: string; declare const setExitCode: (code: number) => void; declare const napiGetPropertyNames: (object: any) => string[]; declare const napiGetPropertyNamesRaw: (object: any) => string[]; +declare const napiEngine: string; describe("AbortController", function () { @@ -1724,16 +1725,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 = napiEngine === "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 () { diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 35bb3a83..5f6c535e 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -101,6 +101,7 @@ TEST(JavaScript, All) env.Global().Set("setExitCode", setExitCodeCallback); env.Global().Set("hostPlatform", Napi::Value::From(env, JSRUNTIMEHOST_PLATFORM)); + env.Global().Set("napiEngine", Napi::Value::From(env, JSRUNTIMEHOST_NAPI_ENGINE)); // Exposes napi_get_property_names, via its C++ wrapper, so that the // script tests can compare it against `for...in`. See From 1767446abc01d93a07530535f6cb1e0aa547c171 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 08:59:37 -0700 Subject: [PATCH 6/7] Define JSRUNTIMEHOST_NAPI_ENGINE for the Android build too Shared.cpp is compiled by two targets -- UnitTests and Android's UnitTestsJNI -- and only the former got the new define, so all four Android legs failed with "use of undeclared identifier 'JSRUNTIMEHOST_NAPI_ENGINE'". Mirror it next to JSRUNTIMEHOST_PLATFORM, exactly as that one is already handled in both places. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt index 0af5caa8..c870151c 100644 --- a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt +++ b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt @@ -22,6 +22,7 @@ add_library(UnitTestsJNI SHARED ${UNIT_TESTS_DIR}/Shared/Shared.cpp) target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") +target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TEST_HOOKS) target_include_directories(UnitTestsJNI From cbe9c16b96d155546c4f344b31047f6496ba0536 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 7/7] 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 5498bffa..54f13120 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1628,8 +1628,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[] = []; @@ -1643,6 +1643,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; @@ -1729,9 +1730,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 = napiEngine === "Hermes" ? describe.skip : describe; + if (napiEngine === "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"]);