Fix napi_get_property_names conformance on JSC, Chakra and QuickJS

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));
 

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/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);

Core/Node-API/Source/env_chakra.cc

@@ -1,6 +1,7 @@
 #include <napi/env.h>
 #include "js_native_api_chakra.h"
 #include <jsrt.h>
+#include <stdexcept>
 #include <strsafe.h>
 
 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));
@@ -41,9 +47,19 @@ namespace Napi
         return {env_ptr};
     }
 
+    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::PrepareForRuntimeDisposal: failed to release property-name intrinsics"};
+        }
+    }
+
     void Detach(Env env)
     {
         napi_env env_ptr{env};
+        PrepareForRuntimeDisposal(env);
         delete env_ptr;
     }
 }

Core/Node-API/Source/env_javascriptcore.cc

@@ -1,18 +1,28 @@
 #include <napi/env.h>
 #include <napi/js_native_api_types.h>
 #include "js_native_api_javascriptcore.h"
+#include <stdexcept>
 
 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;
     }
 

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

Core/Node-API/Source/js_native_api_chakra.cc

@@ -1,8 +1,11 @@
 #include "js_native_api_chakra.h"
+#include "js_native_api_shared.h"
 #include <napi/js_native_api.h>
+#include <algorithm>
 #include <array>
 #include <cassert>
 #include <cmath>
+#include <cstring>
 #include <optional>
 #include <vector>
 #include <string>
@@ -48,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;
@@ -678,11 +682,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, env->property_name_intrinsics)};
+  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_chakra.h

@@ -5,6 +5,7 @@
 
 #include <jsrt.h>
 #include <napi/js_native_api_types.h>
+#include "js_native_api_shared.h"
 #include <thread>
 #include <cassert>
 #include <map>
@@ -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;
 

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>
@@ -73,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;
@@ -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, env->property_name_intrinsics)};
+  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_javascriptcore.h

@@ -2,6 +2,7 @@
 
 #include <napi/js_native_api.h>
 #include <napi/js_native_api_types.h>
+#include "js_native_api_shared.h"
 #include <JavaScriptCore/JavaScript.h>
 #include <unordered_map>
 #include <list>
@@ -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<napi_value, std::uintptr_t> active_ref_values{};
   std::list<napi_ref> strong_refs{};
 

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, env->property_name_intrinsics)};
+  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_quickjs.h

@@ -9,6 +9,7 @@
 #pragma clang diagnostic pop
 #endif
 #include <napi/js_native_api_types.h>
+#include "js_native_api_shared.h"
 #include <thread>
 #include <cassert>
 #include <memory>
@@ -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()};