Fix napi_get_property_names conformance on JSC, Chakra and QuickJS

Core/AppRuntime/Source/AppRuntime_Chakra.cpp

@@ -68,7 +68,7 @@ namespace Babylon
         ThrowIfFailed(JsSetCurrentContext(JS_INVALID_REFERENCE));
         ThrowIfFailed(JsDisposeRuntime(jsRuntime));
 
-        // Detach must come after JsDisposeRuntime since it triggers finalizers which require env.
+        // JsDisposeRuntime runs finalizers which require env, so detach afterward.
         Napi::Detach(env);
     }
 

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/env_chakra.cc

@@ -1,49 +1,111 @@
 #include <napi/env.h>
 #include "js_native_api_chakra.h"
 #include <jsrt.h>
+#include <array>
+#include <exception>
+#include <memory>
+#include <stdexcept>
 #include <strsafe.h>
 
 namespace
 {
+    std::array<napi_ref*, 6> CachedReferences(napi_env env)
+    {
+        auto& intrinsics{env->property_name_intrinsics};
+        return {&intrinsics.object_constructor, &intrinsics.own_names,
+                &intrinsics.own_descriptor, &intrinsics.prototype,
+                &env->has_own_property_reference, &env->wrap_symbol_reference};
+    }
+
     void ThrowIfFailed(JsErrorCode errorCode)
     {
         if (errorCode != JsErrorCode::JsNoError)
         {
             throw std::exception();
         }
     }
+
+    napi_status ReleaseCachedReferences(napi_env env)
+    {
+        napi_status firstError{napi_ok};
+        for (napi_ref* ref : CachedReferences(env))
+        {
+            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));
+        auto env_ptr{std::make_unique<napi_env__>()};
+        try
+        {
+            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<napi_value>(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<napi_value>(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 Detach(Env env)
     {
         napi_env env_ptr{env};
+        for (napi_ref* ref : CachedReferences(env_ptr))
+        {
+            if (*ref != nullptr)
+            {
+                napi_chakra_internal::DiscardReferenceAfterRuntimeDisposal(*ref);
+                *ref = nullptr;
+            }
+        }
         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 <memory>
+#include <stdexcept>
 
 namespace Napi
 {
     Napi::Env Attach(JSGlobalContextRef context)
     {
-        napi_env env_ptr{new napi_env__{context}};
-        return {env_ptr};
+        auto env_ptr{std::make_unique<napi_env__>(context)};
+        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"};
+        }
+        return {env_ptr.release()};
     }
 
     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

@@ -1,5 +1,6 @@
 #include <napi/env.h>
 #include "js_native_api_quickjs.h"
+#include <memory>
 #include <stdexcept>
 #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<napi_env__>()};
         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,20 +50,28 @@ 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.get(), env_ptr->property_name_intrinsics) != napi_ok)
+        {
+            JS_FreeValue(context, hasOwnProperty);
+            throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"};
+        }
 
-        return {env_ptr};
+        return {env_ptr.release()};
     }
 
     void Detach(Env env)
     {
         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,12 @@
 #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 <memory>
 #include <optional>
 #include <vector>
 #include <string>
@@ -48,13 +52,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;
@@ -509,6 +514,12 @@ napi_status DefineProperty(napi_env env,
 
 } // end anonymous namespace
 
+void napi_chakra_internal::DiscardReferenceAfterRuntimeDisposal(napi_ref ref)
+{
+  // The runtime has freed its JS values; JsRelease would access invalid handles.
+  delete reinterpret_cast<RefInfo*>(ref);
+}
+
 // Warning: Keep in-sync with napi_status enum
 static const char* error_messages[] = {
   nullptr,
@@ -678,11 +689,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;
 }
 
@@ -1825,17 +1847,13 @@ napi_status napi_create_reference(napi_env env,
   CHECK_ARG(env, result);
 
   auto jsValue = reinterpret_cast<JsValueRef>(value);
-  auto info = new RefInfo{ reinterpret_cast<JsValueRef>(value), initial_refcount };
-  if (info == nullptr) {
-    return napi_set_last_error(env, napi_generic_failure);
-  }
-
+  std::unique_ptr<RefInfo> info{new RefInfo{jsValue, initial_refcount}};
   if (info->count != 0)
   {
     CHECK_JSRT(env, JsAddRef(jsValue, nullptr));
   }
 
-  *result = reinterpret_cast<napi_ref>(info);
+  *result = reinterpret_cast<napi_ref>(info.release());
   return napi_ok;
 }
 

Core/Node-API/Source/js_native_api_chakra.h

@@ -5,16 +5,24 @@
 
 #include <jsrt.h>
 #include <napi/js_native_api_types.h>
+#include "js_native_api_shared.h"
 #include <thread>
 #include <cassert>
 #include <map>
 
+namespace napi_chakra_internal {
+  void DiscardReferenceAfterRuntimeDisposal(napi_ref ref);
+}
+
 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