Fix napi_get_property_names conformance on JSC, Chakra and QuickJS

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

Core/Node-API/CMakeLists.txt

@@ -51,21 +51,27 @@ 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")
     elseif(NAPI_JAVASCRIPT_ENGINE STREQUAL "JavaScriptCore")
         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")

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 <napi/js_native_api.h>
 #include <array>
 #include <cassert>
@@ -678,11 +679,22 @@ 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<JsValueRef>(object);
-  JsValueRef propertyNames;
-  CHECK_JSRT(env, JsGetOwnPropertyNames(obj, &propertyNames));
-  *result = reinterpret_cast<napi_value>(propertyNames);
+
+  // `JsGetOwnPropertyNames` is own-only and includes non-enumerable properties,
+  // 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;
 }
 

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 <algorithm>
 #include <cassert>
 #include <cmath>
@@ -964,15 +965,27 @@ 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
+  // 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
+  // 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,
@@ -1328,14 +1341,33 @@ 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);
 
+  // `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.
+  //
+  // 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);
+  }
+
   JSValueRef exception{};
-  JSObjectRef prototype{JSValueToObject(env->context, JSObjectGetPrototype(env->context, ToJSObject(env, object)), &exception)};
+  const JSObjectRef self{JSValueToObject(env->context, value, &exception)};
   CHECK_JSC(env, exception);
 
-  *result = ToNapi(prototype);
-  return napi_ok;
+  *result = ToNapi(JSObjectGetPrototype(env->context, self));
+
+  return napi_clear_last_error(env);
 }
 
 napi_status napi_create_object(napi_env env, napi_value* result) {

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 <napi/js_native_api.h>
 #if defined(__clang__)
 #pragma clang diagnostic push
@@ -1394,27 +1395,19 @@ 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_GetOwnPropertyNames` is own-only, 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);
   }
-
-  JS_FreePropertyEnum(env->context, ptab, plen);
-
-  *result = FromJSValue(env, arr);
+
   napi_clear_last_error(env);
   return napi_ok;
 }

Core/Node-API/Source/js_native_api_shared.cc

@@ -0,0 +1,167 @@
+#include "js_native_api_shared.h"
+
+#include <napi/js_native_api.h>
+
+#include <string>
+#include <unordered_set>
+#include <vector>
+
+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<char> 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<std::string>& 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<napi_value>& 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) {
+    // `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<std::string> shadowed{};
+    std::vector<napi_value> 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.
+    // `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, &current));
+
+    while (true) {
+      bool isObjectLike{};
+      RETURN_IF_NOT_OK(IsObjectLike(env, current, isObjectLike));
+      if (!isObjectLike) {
+        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, &current, &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 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, &current, &ownNames));
+        RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed));
+      }
+
+      current = next;
+    }
+
+    *result = names;
+    return napi_ok;
+  }
+
+  #undef RETURN_IF_NOT_OK
+}

Core/Node-API/Source/js_native_api_shared.h

@@ -0,0 +1,22 @@
+#pragma once
+
+#include <napi/js_native_api_types.h>
+
+// 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,
+  // 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);
+}

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