From d244cff24e87b27b3cf7144e498eaa921be47719 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Mon, 13 Jan 2025 15:41:44 -0800 Subject: [PATCH 01/31] Add changes plus a bunch of test code --- .../Source/js_native_api_javascriptcore.cc | 250 ++++++++++++++++-- .../Source/js_native_api_javascriptcore.h | 10 +- 2 files changed, 236 insertions(+), 24 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 3a630a28..b4437116 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -18,6 +18,12 @@ struct napi_callback_info__ { uint16_t argc; }; +namespace { + std::unordered_map>> ref_log{}; + uint32_t objectId = 0; + uint64_t referenceObjectId = 0; +} + namespace { class JSString { public: @@ -477,26 +483,87 @@ namespace { class ReferenceInfo : public BaseInfoT { public: - static napi_status Initialize(napi_env env, napi_value object, FinalizerT finalizer) { - ReferenceInfo* info = new ReferenceInfo(env); + static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) + { + *id = 0; + + if (JSObjectHasProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"))) + { + JSValueRef exception{}; + JSValueRef finalizerHook{JSObjectGetProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"), &exception)}; + CHECK_JSC(env, exception); + auto referenceInfo{ReferenceInfo::Get(JSValueToObject(env->context, finalizerHook, &exception))}; + *id = reinterpret_cast(referenceInfo); + } + +// napi_value finalizerHookName{}; +// napi_create_string_utf8(env, "__finalizerHook", 15, &finalizerHookName); +// bool hasFinalizerHook{}; +// CHECK_NAPI(napi_has_property(env, object, finalizerHookName, &hasFinalizerHook)); +// if (hasFinalizerHook) +// { +// napi_value finalizerHook{}; +// CHECK_NAPI(napi_get_property(env, object, finalizerHookName, &finalizerHook)); +// auto referenceInfo{ReferenceInfo::Get(ToJSObject(env, finalizerHook))}; +// *id = referenceInfo->GetObjectId(); +// } + + return napi_ok; + } + + //static napi_status Initialize(napi_env env, napi_value object, FinalizerT finalizer, std::uintptr_t* objectId) { + static napi_status Initialize(napi_env env, napi_value object, FinalizerT finalizer) { + + JSValueRef exception{}; +// if (JSObjectHasProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"))) +// { +// JSValueRef value{JSObjectGetProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"), &exception)}; +// auto existingReferenceInfo{ReferenceInfo::Get(JSValueToObject(env->context, value, &exception))}; +// *objectId = reinterpret_cast(existingReferenceInfo); +// return napi_ok; +// } +// + ReferenceInfo* info = new ReferenceInfo(env); if (info == nullptr) { return napi_set_last_error(env, napi_generic_failure); } JSObjectRef prototype{JSObjectMake(env->context, info->_class, info)}; - JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, ToJSObject(env, object))); - JSObjectSetPrototype(env->context, ToJSObject(env, object), prototype); +// JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, ToJSObject(env, object))); +// JSObjectSetPrototype(env->context, ToJSObject(env, object), prototype); + + JSObjectSetProperty( + env->context, + ToJSObject(env, object), + JSString("__finalizerHook"), + prototype, + kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete, + &exception); + CHECK_JSC(env, exception); info->AddFinalizer(finalizer); + +// *objectId = reinterpret_cast(info); + return napi_ok; } + std::uintptr_t GetObjectId() + { + return reinterpret_cast(this); + } + private: +// static uint64_t _lastObjectId; +// uint64_t _objectId; ReferenceInfo(napi_env env) +// : _objectId{_lastObjectId++}, BaseInfoT{env, "Native (Reference)"} { : BaseInfoT{env, "Native (Reference)"} { } }; +// uint64_t ReferenceInfo::_lastObjectId = 0; + class WrapperInfo : public BaseInfoT { public: static napi_status Wrap(napi_env env, napi_value object, WrapperInfo** result) { @@ -577,21 +644,120 @@ namespace { } struct napi_ref__ { + void log(std::string action) + { + auto entry = ref_log.find(_value); + if (entry == ref_log.end()) + { + entry = ref_log.insert({_value, {}}).first; + } + entry->second.push_back({reinterpret_cast(this), std::get<0>(*_data), std::get<1>(*_data), action}); + } + napi_ref__(napi_value value, uint32_t count) - : _value{value} - , _count{count} { + : _value{value} { + std::get<0>(*_data) = count; + log("constructed"); } + napi_ref__(const napi_ref__&) = delete; + napi_ref__& operator=(const napi_ref__&) = delete; + napi_status init(napi_env env) { // track the ref values to support weak refs - auto pair{env->active_ref_values.insert(_value)}; - if (pair.second) { - CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value](ReferenceInfo* info) { - info->Env()->active_ref_values.erase(value); - })); - } + + std::string funcName{}; + char stringBuff[1024]{}; + if (JSValueIsObject(env->context, reinterpret_cast(_value))) + { + napi_value name; +// napi_create_string_utf8(env, "name", 4, &name); +// napi_value functionName; +// napi_get_property(env, _value, name, &functionName); +// napi_get_value_string_utf8(env, functionName, stringBuff, sizeof(stringBuff), nullptr); + + napi_create_string_utf8(env, "toString", 8, &name); + napi_value function; + napi_get_property(env, _value, name, &function); + if (JSValueIsObject(env->context, reinterpret_cast(function)) && JSObjectIsFunction(env->context, ToJSObject(env, function))) + { + napi_value funcStr; + napi_call_function(env, _value, function, 0, nullptr, &funcStr); + napi_get_value_string_utf8(env, funcStr, stringBuff, sizeof(stringBuff), nullptr); + funcName = stringBuff; + std::get<2>(*_data) = funcName; + } + + napi_value objectIdName; + napi_create_string_utf8(env, "_ryanObjectId", 13, &objectIdName); + bool hasObjectId{}; + napi_has_property(env, _value, objectIdName, &hasObjectId); + if (hasObjectId) + { + napi_value objectId; + napi_get_property(env, _value, objectIdName, &objectId); + int32_t objectIdRaw{}; + napi_get_value_int32(env, objectId, &objectIdRaw); + log(std::to_string(objectIdRaw)); + } + + napi_value finalizerHookName; + napi_create_string_utf8(env, "__finalizerHook", 15, &finalizerHookName); + bool hasFinalizerHook{}; + napi_has_property(env, _value, finalizerHookName, &hasFinalizerHook); + if (hasFinalizerHook) { + log("finalizer previously attached"); + } + } + log("init"); + log(funcName); + env->active_ref_values_names.insert({_value, funcName}); + +// CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value, data = _data, thisPtr = reinterpret_cast(this)](ReferenceInfo* info) { +// //if (_count != 0) +// if (std::get<0>(*data) != 0) +// { +// throw std::runtime_error{"finalizing with non-zero ref count"}; +// } +//// log("finalized"); +// info->Env()->active_ref_values.erase(value); +// std::get<1>(*data) = true; +// ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "finalized"}); +// })); + +// auto existingEntry{env->active_ref_values.find(_value)}; +// if (existingEntry == env->active_ref_values.end() || existingEntry->second) + + CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); + if (_objectId == 0) { + log("added finalizer"); + CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value, data = _data, thisPtr = reinterpret_cast(this)](ReferenceInfo* info) { + //if (_count != 0) + if (std::get<0>(*data) != 0) + { + throw std::runtime_error{"finalizing with non-zero ref count"}; + } + auto entry{info->Env()->active_ref_values.find(value)}; + if (entry != info->Env()->active_ref_values.end() && entry->second == info->GetObjectId()) + { + info->Env()->active_ref_values.erase(entry); + } +// info->Env()->active_ref_values.erase(value); + std::get<1>(*data) = true; + ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "finalized"}); + })); + + CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); + assert(_objectId); + env->active_ref_values[_value] = _objectId; + } - if (_count != 0) { + //if (_count != 0) { + if (std::get<0>(*_data) != 0) { + if (std::get<0>(*_data) != 1) { + throw std::runtime_error{"ref count must be 0 or 1"}; + } + log("init protect"); protect(env); } @@ -599,36 +765,69 @@ struct napi_ref__ { } void deinit(napi_env env) { - if (_count != 0) { + //if (_count != 0) { + if (std::get<0>(*_data) != 0) { + log("deinit unprotect"); unprotect(env); } _value = nullptr; - _count = 0; +// _count = 0; + std::get<0>(*_data) = 0; } void ref(napi_env env) { - if (_count++ == 0) { +// if (_count++ == 0) { + if (std::get<1>(*_data)) + { + throw std::runtime_error{"object is already finalized"}; + } + if (std::get<0>(*_data)++ == 0) { + log("ref protect"); protect(env); } } void unref(napi_env env) { - if (--_count == 0) { +// if (--_count == 0) { + if (std::get<0>(*_data) == 0) { + throw std::runtime_error{"deref below zero"}; + } + if (--std::get<0>(*_data) == 0) { + log("unref unprotect"); unprotect(env); } } uint32_t count() const { - return _count; + //return _count; + return std::get<0>(*_data); } napi_value value(napi_env env) const { - if (env->active_ref_values.find(_value) == env->active_ref_values.end()) { + if (env->active_ref_values.find(_value) != env->active_ref_values.end()) + { + std::uintptr_t objectId{}; + if (ReferenceInfo::GetObjectId(env, _value, &objectId) == napi_ok && objectId == _objectId) { + return _value; + } + } + + auto nameValue = env->active_ref_values_names.find(_value); + auto funcName = nameValue->second; + auto funcName2 = funcName.data(); + auto log = ref_log.find(_value); return nullptr; - } - return _value; +// if (env->active_ref_values.find(_value) == env->active_ref_values.end()) { +// auto nameValue = env->active_ref_values_names.find(_value); +// auto funcName = nameValue->second; +// auto funcName2 = funcName.data(); +// auto log = ref_log.find(_value); +// return nullptr; +// } +// +// return _value; } private: @@ -643,7 +842,10 @@ struct napi_ref__ { } napi_value _value{}; - uint32_t _count{}; +// std::shared_ptr> _data{0, false, ""}; //ryan + std::shared_ptr> _data{std::make_shared>(0, false, "")}; +// uint32_t _count{}; + std::uintptr_t _objectId{}; std::list::iterator _iter{}; }; @@ -2332,6 +2534,10 @@ napi_status napi_create_promise(napi_env env, napi_value deferred_value{}; CHECK_NAPI(napi_create_object(env, &deferred_value)); + // ryan + napi_value object_id{}; + CHECK_NAPI(napi_create_int32(env, objectId++, &object_id)); + CHECK_NAPI(napi_set_named_property(env, deferred_value, "_ryanObjectId", object_id)); CHECK_NAPI(napi_set_named_property(env, deferred_value, "resolve", wrapper.resolve)); CHECK_NAPI(napi_set_named_property(env, deferred_value, "reject", wrapper.reject)); diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 8dc1048e..2353f1a7 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -12,7 +13,9 @@ struct napi_env__ { JSGlobalContextRef context{}; JSValueRef last_exception{}; napi_extended_error_info last_error{nullptr, nullptr, 0, napi_ok}; - std::unordered_set active_ref_values{}; +// std::unordered_set active_ref_values{}; + std::unordered_map active_ref_values{}; + std::unordered_map active_ref_values_names{}; std::list strong_refs{}; const std::thread::id thread_id{std::this_thread::get_id()}; @@ -46,7 +49,10 @@ struct napi_env__ { } while (0) #define CHECK_ARG(env, arg) \ - RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg) +do { \ + if ((arg) == nullptr) *arg; \ + RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg); \ +} while (0) #define CHECK_JSC(env, exception) \ do { \ From c21f4bbc7571b2d3e2992a12b76836ed82546b5e Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Mon, 13 Jan 2025 15:42:02 -0800 Subject: [PATCH 02/31] A little more testing --- .../Source/js_native_api_javascriptcore.cc | 23 +++++++++++++++---- 1 file 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 b4437116..6f51acc7 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -644,7 +644,7 @@ namespace { } struct napi_ref__ { - void log(std::string action) + void log(std::string action) const { auto entry = ref_log.find(_value); if (entry == ref_log.end()) @@ -731,21 +731,30 @@ struct napi_ref__ { CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); if (_objectId == 0) { log("added finalizer"); - CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value, data = _data, thisPtr = reinterpret_cast(this)](ReferenceInfo* info) { + auto napi_result = ReferenceInfo::Initialize(env, _value, [value = _value, data = _data, thisPtr = reinterpret_cast(this)](ReferenceInfo* info) { //if (_count != 0) if (std::get<0>(*data) != 0) { throw std::runtime_error{"finalizing with non-zero ref count"}; } auto entry{info->Env()->active_ref_values.find(value)}; - if (entry != info->Env()->active_ref_values.end() && entry->second == info->GetObjectId()) + if (entry != info->Env()->active_ref_values.end()) { - info->Env()->active_ref_values.erase(entry); + if (entry->second == info->GetObjectId()) + { + ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "removing from active values"}); + info->Env()->active_ref_values.erase(entry); + } + else + { + ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "not removing from active values because replaced"}); + } } // info->Env()->active_ref_values.erase(value); std::get<1>(*data) = true; ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "finalized"}); - })); + }); + CHECK_NAPI(napi_result); CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); assert(_objectId); @@ -811,6 +820,10 @@ struct napi_ref__ { if (ReferenceInfo::GetObjectId(env, _value, &objectId) == napi_ok && objectId == _objectId) { return _value; } + else + { + log("return null value because this ref is stale"); + } } auto nameValue = env->active_ref_values_names.find(_value); From b8e9a4b2f2a71c2a8fdced6d15dd0f5209e63787 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Mon, 13 Jan 2025 17:18:31 -0800 Subject: [PATCH 03/31] Cleanup --- .../Source/js_native_api_javascriptcore.cc | 279 ++++-------------- .../Source/js_native_api_javascriptcore.h | 8 +- 2 files changed, 53 insertions(+), 234 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 6f51acc7..376fe800 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -18,12 +18,6 @@ struct napi_callback_info__ { uint16_t argc; }; -namespace { - std::unordered_map>> ref_log{}; - uint32_t objectId = 0; - uint64_t referenceObjectId = 0; -} - namespace { class JSString { public: @@ -483,87 +477,53 @@ namespace { class ReferenceInfo : public BaseInfoT { public: - static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) - { - *id = 0; - - if (JSObjectHasProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"))) - { - JSValueRef exception{}; - JSValueRef finalizerHook{JSObjectGetProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"), &exception)}; - CHECK_JSC(env, exception); - auto referenceInfo{ReferenceInfo::Get(JSValueToObject(env->context, finalizerHook, &exception))}; - *id = reinterpret_cast(referenceInfo); - } + static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) { + *id = 0; -// napi_value finalizerHookName{}; -// napi_create_string_utf8(env, "__finalizerHook", 15, &finalizerHookName); -// bool hasFinalizerHook{}; -// CHECK_NAPI(napi_has_property(env, object, finalizerHookName, &hasFinalizerHook)); -// if (hasFinalizerHook) -// { -// napi_value finalizerHook{}; -// CHECK_NAPI(napi_get_property(env, object, finalizerHookName, &finalizerHook)); -// auto referenceInfo{ReferenceInfo::Get(ToJSObject(env, finalizerHook))}; -// *id = referenceInfo->GetObjectId(); -// } - - return napi_ok; + if (JSObjectHasProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"))) { + JSValueRef exception{}; + JSValueRef finalizerHook{JSObjectGetProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"), &exception)}; + CHECK_JSC(env, exception); + auto referenceInfo{ReferenceInfo::Get(JSValueToObject(env->context, finalizerHook, &exception))}; + *id = referenceInfo->GetObjectId(); } - //static napi_status Initialize(napi_env env, napi_value object, FinalizerT finalizer, std::uintptr_t* objectId) { - static napi_status Initialize(napi_env env, napi_value object, FinalizerT finalizer) { - - JSValueRef exception{}; -// if (JSObjectHasProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"))) -// { -// JSValueRef value{JSObjectGetProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"), &exception)}; -// auto existingReferenceInfo{ReferenceInfo::Get(JSValueToObject(env->context, value, &exception))}; -// *objectId = reinterpret_cast(existingReferenceInfo); -// return napi_ok; -// } -// - ReferenceInfo* info = new ReferenceInfo(env); + return napi_ok; + } + + static napi_status Initialize(napi_env env, napi_value object, FinalizerT finalizer) { + ReferenceInfo* info = new ReferenceInfo(env); if (info == nullptr) { return napi_set_last_error(env, napi_generic_failure); } JSObjectRef prototype{JSObjectMake(env->context, info->_class, info)}; -// JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, ToJSObject(env, object))); -// JSObjectSetPrototype(env->context, ToJSObject(env, object), prototype); - - JSObjectSetProperty( - env->context, - ToJSObject(env, object), - JSString("__finalizerHook"), - prototype, - kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete, - &exception); - CHECK_JSC(env, exception); + + JSValueRef exception{}; + JSObjectSetProperty( + env->context, + ToJSObject(env, object), + JSString("__finalizerHook"), + prototype, + kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete, + &exception); + CHECK_JSC(env, exception); info->AddFinalizer(finalizer); - -// *objectId = reinterpret_cast(info); - return napi_ok; } std::uintptr_t GetObjectId() { - return reinterpret_cast(this); + return reinterpret_cast(this); } private: -// static uint64_t _lastObjectId; -// uint64_t _objectId; ReferenceInfo(napi_env env) -// : _objectId{_lastObjectId++}, BaseInfoT{env, "Native (Reference)"} { : BaseInfoT{env, "Native (Reference)"} { } }; -// uint64_t ReferenceInfo::_lastObjectId = 0; - class WrapperInfo : public BaseInfoT { public: static napi_status Wrap(napi_env env, napi_value object, WrapperInfo** result) { @@ -644,20 +604,9 @@ namespace { } struct napi_ref__ { - void log(std::string action) const - { - auto entry = ref_log.find(_value); - if (entry == ref_log.end()) - { - entry = ref_log.insert({_value, {}}).first; - } - entry->second.push_back({reinterpret_cast(this), std::get<0>(*_data), std::get<1>(*_data), action}); - } - napi_ref__(napi_value value, uint32_t count) - : _value{value} { - std::get<0>(*_data) = count; - log("constructed"); + : _value{value} + , _count{count} { } napi_ref__(const napi_ref__&) = delete; @@ -665,108 +614,22 @@ struct napi_ref__ { napi_status init(napi_env env) { // track the ref values to support weak refs - - std::string funcName{}; - char stringBuff[1024]{}; - if (JSValueIsObject(env->context, reinterpret_cast(_value))) - { - napi_value name; -// napi_create_string_utf8(env, "name", 4, &name); -// napi_value functionName; -// napi_get_property(env, _value, name, &functionName); -// napi_get_value_string_utf8(env, functionName, stringBuff, sizeof(stringBuff), nullptr); - - napi_create_string_utf8(env, "toString", 8, &name); - napi_value function; - napi_get_property(env, _value, name, &function); - if (JSValueIsObject(env->context, reinterpret_cast(function)) && JSObjectIsFunction(env->context, ToJSObject(env, function))) - { - napi_value funcStr; - napi_call_function(env, _value, function, 0, nullptr, &funcStr); - napi_get_value_string_utf8(env, funcStr, stringBuff, sizeof(stringBuff), nullptr); - funcName = stringBuff; - std::get<2>(*_data) = funcName; - } - - napi_value objectIdName; - napi_create_string_utf8(env, "_ryanObjectId", 13, &objectIdName); - bool hasObjectId{}; - napi_has_property(env, _value, objectIdName, &hasObjectId); - if (hasObjectId) + 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)}; + if (entry != info->Env()->active_ref_values.end() && entry->second == info->GetObjectId()) { - napi_value objectId; - napi_get_property(env, _value, objectIdName, &objectId); - int32_t objectIdRaw{}; - napi_get_value_int32(env, objectId, &objectIdRaw); - log(std::to_string(objectIdRaw)); + info->Env()->active_ref_values.erase(entry); } - - napi_value finalizerHookName; - napi_create_string_utf8(env, "__finalizerHook", 15, &finalizerHookName); - bool hasFinalizerHook{}; - napi_has_property(env, _value, finalizerHookName, &hasFinalizerHook); - if (hasFinalizerHook) { - log("finalizer previously attached"); - } - } - log("init"); - log(funcName); - env->active_ref_values_names.insert({_value, funcName}); - -// CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value, data = _data, thisPtr = reinterpret_cast(this)](ReferenceInfo* info) { -// //if (_count != 0) -// if (std::get<0>(*data) != 0) -// { -// throw std::runtime_error{"finalizing with non-zero ref count"}; -// } -//// log("finalized"); -// info->Env()->active_ref_values.erase(value); -// std::get<1>(*data) = true; -// ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "finalized"}); -// })); - -// auto existingEntry{env->active_ref_values.find(_value)}; -// if (existingEntry == env->active_ref_values.end() || existingEntry->second) + })); CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); - if (_objectId == 0) { - log("added finalizer"); - auto napi_result = ReferenceInfo::Initialize(env, _value, [value = _value, data = _data, thisPtr = reinterpret_cast(this)](ReferenceInfo* info) { - //if (_count != 0) - if (std::get<0>(*data) != 0) - { - throw std::runtime_error{"finalizing with non-zero ref count"}; - } - auto entry{info->Env()->active_ref_values.find(value)}; - if (entry != info->Env()->active_ref_values.end()) - { - if (entry->second == info->GetObjectId()) - { - ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "removing from active values"}); - info->Env()->active_ref_values.erase(entry); - } - else - { - ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "not removing from active values because replaced"}); - } - } -// info->Env()->active_ref_values.erase(value); - std::get<1>(*data) = true; - ref_log.find(value)->second.push_back({thisPtr, std::get<0>(*data), true, "finalized"}); - }); - CHECK_NAPI(napi_result); - - CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); - assert(_objectId); - env->active_ref_values[_value] = _objectId; - } + assert(_objectId); + env->active_ref_values[_value] = _objectId; + } - //if (_count != 0) { - if (std::get<0>(*_data) != 0) { - if (std::get<0>(*_data) != 1) { - throw std::runtime_error{"ref count must be 0 or 1"}; - } - log("init protect"); + if (_count != 0) { protect(env); } @@ -774,73 +637,41 @@ struct napi_ref__ { } void deinit(napi_env env) { - //if (_count != 0) { - if (std::get<0>(*_data) != 0) { - log("deinit unprotect"); + if (_count != 0) { unprotect(env); } _value = nullptr; -// _count = 0; - std::get<0>(*_data) = 0; + _count = 0; } void ref(napi_env env) { -// if (_count++ == 0) { - if (std::get<1>(*_data)) - { - throw std::runtime_error{"object is already finalized"}; - } - if (std::get<0>(*_data)++ == 0) { - log("ref protect"); + if (_count++ == 0) { protect(env); } } void unref(napi_env env) { -// if (--_count == 0) { - if (std::get<0>(*_data) == 0) { - throw std::runtime_error{"deref below zero"}; - } - if (--std::get<0>(*_data) == 0) { - log("unref unprotect"); + assert(_count != 0); + if (--_count == 0) { unprotect(env); } } uint32_t count() const { - //return _count; - return std::get<0>(*_data); + return _count; } napi_value value(napi_env env) const { - if (env->active_ref_values.find(_value) != env->active_ref_values.end()) - { - std::uintptr_t objectId{}; - if (ReferenceInfo::GetObjectId(env, _value, &objectId) == napi_ok && objectId == _objectId) { - return _value; - } - else - { - log("return null value because this ref is stale"); - } + if (env->active_ref_values.find(_value) != env->active_ref_values.end()) + { + std::uintptr_t objectId{}; + if (ReferenceInfo::GetObjectId(env, _value, &objectId) == napi_ok && objectId == _objectId) { + return _value; } - - auto nameValue = env->active_ref_values_names.find(_value); - auto funcName = nameValue->second; - auto funcName2 = funcName.data(); - auto log = ref_log.find(_value); - return nullptr; + } -// if (env->active_ref_values.find(_value) == env->active_ref_values.end()) { -// auto nameValue = env->active_ref_values_names.find(_value); -// auto funcName = nameValue->second; -// auto funcName2 = funcName.data(); -// auto log = ref_log.find(_value); -// return nullptr; -// } -// -// return _value; + return nullptr; } private: @@ -855,10 +686,8 @@ struct napi_ref__ { } napi_value _value{}; -// std::shared_ptr> _data{0, false, ""}; //ryan - std::shared_ptr> _data{std::make_shared>(0, false, "")}; -// uint32_t _count{}; - std::uintptr_t _objectId{}; + uint32_t _count{}; + std::uintptr_t _objectId{}; std::list::iterator _iter{}; }; @@ -2547,10 +2376,6 @@ napi_status napi_create_promise(napi_env env, napi_value deferred_value{}; CHECK_NAPI(napi_create_object(env, &deferred_value)); - // ryan - napi_value object_id{}; - CHECK_NAPI(napi_create_int32(env, objectId++, &object_id)); - CHECK_NAPI(napi_set_named_property(env, deferred_value, "_ryanObjectId", object_id)); CHECK_NAPI(napi_set_named_property(env, deferred_value, "resolve", wrapper.resolve)); CHECK_NAPI(napi_set_named_property(env, deferred_value, "reject", wrapper.reject)); diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 2353f1a7..5bc025c8 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -13,9 +12,7 @@ struct napi_env__ { JSGlobalContextRef context{}; JSValueRef last_exception{}; napi_extended_error_info last_error{nullptr, nullptr, 0, napi_ok}; -// std::unordered_set active_ref_values{}; std::unordered_map active_ref_values{}; - std::unordered_map active_ref_values_names{}; std::list strong_refs{}; const std::thread::id thread_id{std::this_thread::get_id()}; @@ -49,10 +46,7 @@ struct napi_env__ { } while (0) #define CHECK_ARG(env, arg) \ -do { \ - if ((arg) == nullptr) *arg; \ - RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg); \ -} while (0) + RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg) #define CHECK_JSC(env, exception) \ do { \ From 173d9a9a67e5c6a3b8e13a102acf0d5d7391e13f Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Tue, 14 Jan 2025 15:44:12 -0800 Subject: [PATCH 04/31] Use symbol for property key --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 11 +++++++---- Core/Node-API/Source/js_native_api_javascriptcore.h | 8 ++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 376fe800..99d642de 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -480,9 +480,12 @@ namespace { static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) { *id = 0; - if (JSObjectHasProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"))) { + JSValueRef exception{}; + const auto hasFinalizerHook{JSObjectHasPropertyForKey(env->context, ToJSObject(env, object), env->reference_info_symbol, &exception)}; + CHECK_JSC(env, exception); + if (hasFinalizerHook) { JSValueRef exception{}; - JSValueRef finalizerHook{JSObjectGetProperty(env->context, ToJSObject(env, object), JSString("__finalizerHook"), &exception)}; + JSValueRef finalizerHook{JSObjectGetPropertyForKey(env->context, ToJSObject(env, object), env->reference_info_symbol, &exception)}; CHECK_JSC(env, exception); auto referenceInfo{ReferenceInfo::Get(JSValueToObject(env->context, finalizerHook, &exception))}; *id = referenceInfo->GetObjectId(); @@ -500,10 +503,10 @@ namespace { JSObjectRef prototype{JSObjectMake(env->context, info->_class, info)}; JSValueRef exception{}; - JSObjectSetProperty( + JSObjectSetPropertyForKey( env->context, ToJSObject(env, object), - JSString("__finalizerHook"), + env->reference_info_symbol, prototype, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete, &exception); diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 5bc025c8..1dae160f 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -15,14 +15,22 @@ struct napi_env__ { std::unordered_map active_ref_values{}; std::list strong_refs{}; + JSValueRef reference_info_symbol{}; + const std::thread::id thread_id{std::this_thread::get_id()}; napi_env__(JSGlobalContextRef context) : context{context} { JSGlobalContextRetain(context); + { + JSStringRef descriptionRef{JSStringCreateWithUTF8CString("ReferenceInfo")}; + reference_info_symbol = JSValueMakeSymbol(context, descriptionRef); + JSValueProtect(context, reference_info_symbol); + } } ~napi_env__() { deinit_refs(); + JSValueUnprotect(context, reference_info_symbol); JSGlobalContextRelease(context); } From 612cbb9ed7eb1a368b535246545a2c662996accc Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Wed, 15 Jan 2025 11:27:01 -0800 Subject: [PATCH 05/31] Add reusable Apply/Lookup functions --- .../Source/js_native_api_javascriptcore.cc | 73 ++++++++++++------- 1 file changed, 48 insertions(+), 25 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 99d642de..11617618 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -187,11 +187,51 @@ namespace { return _type; } + template + static void Apply(napi_env env, JSObjectRef target, JSObjectRef sentinel) { + JSValueRef exception{}; + JSObjectSetPropertyForKey( + env->context, + target, + T::GetKey(env), + sentinel, + kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete, + &exception); + + CHECK_JSC(env, exception); + } + template static T* Get(JSObjectRef obj) { return reinterpret_cast(JSObjectGetPrivate(obj)); } + template + static T* Lookup(napi_env env, JSObjectRef obj) { + JSValueRef exception{}; + const auto hasSentinel{JSObjectHasPropertyForKey(env->context, obj, T::GetKey(env), &exception)}; + if (exception != nullptr) { + return nullptr; + } + + if (hasSentinel) { + JSValueRef exception{}; + JSValueRef sentinelValue{JSObjectGetPropertyForKey(env->context, obj, T::GetKey(env), &exception)}; + if (exception != nullptr) { + return nullptr; + } + + JSObjectRef sentinel{JSValueToObject(env->context, sentinelValue, &exception)}; + if (exception != nullptr) { + return nullptr; + } + + return Get(sentinel); + } + + return nullptr; + } + template static T* FindInPrototypeChain(JSContextRef ctx, JSObjectRef obj) { while (true) { @@ -477,21 +517,14 @@ namespace { class ReferenceInfo : public BaseInfoT { public: - static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) { - *id = 0; - - JSValueRef exception{}; - const auto hasFinalizerHook{JSObjectHasPropertyForKey(env->context, ToJSObject(env, object), env->reference_info_symbol, &exception)}; - CHECK_JSC(env, exception); - if (hasFinalizerHook) { - JSValueRef exception{}; - JSValueRef finalizerHook{JSObjectGetPropertyForKey(env->context, ToJSObject(env, object), env->reference_info_symbol, &exception)}; - CHECK_JSC(env, exception); - auto referenceInfo{ReferenceInfo::Get(JSValueToObject(env->context, finalizerHook, &exception))}; - *id = referenceInfo->GetObjectId(); + static JSValueRef GetKey(napi_env env) { + return env->reference_info_symbol; } - return napi_ok; + static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) { + ReferenceInfo* referenceInfo{ReferenceInfo::Lookup(env, ToJSObject(env, object))}; + *id = referenceInfo == nullptr ? 0 : referenceInfo->GetObjectId(); + return napi_ok; } static napi_status Initialize(napi_env env, napi_value object, FinalizerT finalizer) { @@ -500,18 +533,8 @@ namespace { return napi_set_last_error(env, napi_generic_failure); } - JSObjectRef prototype{JSObjectMake(env->context, info->_class, info)}; - - JSValueRef exception{}; - JSObjectSetPropertyForKey( - env->context, - ToJSObject(env, object), - env->reference_info_symbol, - prototype, - kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete, - &exception); - CHECK_JSC(env, exception); - + JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; + ReferenceInfo::Apply(env, ToJSObject(env, object), sentinel); info->AddFinalizer(finalizer); return napi_ok; } From ce8ab72715ae1ab22e8028de887cf31f3207dce0 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Wed, 15 Jan 2025 11:57:21 -0800 Subject: [PATCH 06/31] Swithc over WrapperInfo --- .../Source/js_native_api_javascriptcore.cc | 15 +++++++----- .../Source/js_native_api_javascriptcore.h | 23 +++++++++++++------ 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 11617618..0e29b204 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -522,7 +522,7 @@ namespace { } static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) { - ReferenceInfo* referenceInfo{ReferenceInfo::Lookup(env, ToJSObject(env, object))}; + ReferenceInfo* referenceInfo{NativeInfo::Lookup(env, ToJSObject(env, object))}; *id = referenceInfo == nullptr ? 0 : referenceInfo->GetObjectId(); return napi_ok; } @@ -534,7 +534,7 @@ namespace { } JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - ReferenceInfo::Apply(env, ToJSObject(env, object), sentinel); + NativeInfo::Apply(env, ToJSObject(env, object), sentinel); info->AddFinalizer(finalizer); return napi_ok; } @@ -552,6 +552,10 @@ namespace { class WrapperInfo : public BaseInfoT { public: + static JSValueRef GetKey(napi_env env) { + return env->wrapper_info_symbol; + } + static napi_status Wrap(napi_env env, napi_value object, WrapperInfo** result) { WrapperInfo* info{}; CHECK_NAPI(Unwrap(env, object, &info)); @@ -561,9 +565,8 @@ namespace { return napi_set_last_error(env, napi_generic_failure); } - JSObjectRef prototype{JSObjectMake(env->context, info->_class, info)}; - JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, ToJSObject(env, object))); - JSObjectSetPrototype(env->context, ToJSObject(env, object), prototype); + JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; + NativeInfo::Apply(env, ToJSObject(env, object), sentinel); } *result = info; @@ -571,7 +574,7 @@ namespace { } static napi_status Unwrap(napi_env env, napi_value object, WrapperInfo** result) { - *result = NativeInfo::FindInPrototypeChain(env->context, ToJSObject(env, object)); + *result = NativeInfo::Lookup(env, ToJSObject(env, object)); 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 1dae160f..65891f45 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -16,26 +16,35 @@ struct napi_env__ { std::list strong_refs{}; JSValueRef reference_info_symbol{}; + JSValueRef wrapper_info_symbol{}; const std::thread::id thread_id{std::this_thread::get_id()}; napi_env__(JSGlobalContextRef context) : context{context} { JSGlobalContextRetain(context); - { - JSStringRef descriptionRef{JSStringCreateWithUTF8CString("ReferenceInfo")}; - reference_info_symbol = JSValueMakeSymbol(context, descriptionRef); - JSValueProtect(context, reference_info_symbol); - } + init_symbol(reference_info_symbol, "BabylonNative_ReferenceInfo"); + init_symbol(wrapper_info_symbol, "BabylonNative_WrapperInfo"); } - + ~napi_env__() { deinit_refs(); - JSValueUnprotect(context, reference_info_symbol); + deinit_symbol(wrapper_info_symbol); + deinit_symbol(reference_info_symbol); JSGlobalContextRelease(context); } private: void deinit_refs(); + + void init_symbol(JSValueRef& symbol, const char* description) { + JSStringRef descriptionRef{JSStringCreateWithUTF8CString(description)}; + symbol = JSValueMakeSymbol(context, descriptionRef); + JSValueProtect(context, symbol); + } + + void deinit_symbol(JSValueRef symbol) { + JSValueUnprotect(context, symbol); + } }; #define RETURN_STATUS_IF_FALSE(env, condition, status) \ From 34db18c0ae3dadcd8b9d1e4a74a836fe96fb0a51 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Wed, 15 Jan 2025 14:06:35 -0800 Subject: [PATCH 07/31] Swithc over FunctionInfo --- .../Source/js_native_api_javascriptcore.cc | 21 +++++++++++++++---- .../Source/js_native_api_javascriptcore.h | 19 +++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 0e29b204..ec8bad63 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -139,6 +139,10 @@ namespace { return reinterpret_cast(const_cast(values)); } + napi_env ToNapi(const JSContextRef context) { + return napi_env__::get(context); + } + napi_status napi_clear_last_error(napi_env env) { env->last_error.error_code = napi_ok; env->last_error.engine_error_code = 0; @@ -263,6 +267,10 @@ namespace { public: static const NativeType StaticType = NativeType::Constructor; + static JSValueRef GetKey(napi_env env) { + return env->constructor_info_symbol; + } + static napi_status Create(napi_env env, const char* utf8name, size_t length, @@ -355,6 +363,10 @@ namespace { public: static const NativeType StaticType = NativeType::Function; + static JSValueRef GetKey(napi_env env) { + return env->function_info_symbol; + } + static napi_status Create(napi_env env, const char* utf8name, size_t length, @@ -367,9 +379,8 @@ namespace { } JSObjectRef function{JSObjectMakeFunctionWithCallback(env->context, JSString(utf8name), CallAsFunction)}; - JSObjectRef prototype{JSObjectMake(env->context, info->_class, info)}; - JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, function)); - JSObjectSetPrototype(env->context, function, prototype); + JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; + NativeInfo::Apply(env, function, sentinel); *result = ToNapi(function); return napi_ok; @@ -398,7 +409,7 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - FunctionInfo* info = NativeInfo::FindInPrototypeChain(ctx, function); + FunctionInfo* info = NativeInfo::Lookup(ToNapi(ctx), function); // Make sure any errors encountered last time we were in N-API are gone. napi_clear_last_error(info->_env); @@ -727,6 +738,8 @@ void napi_env__::deinit_refs() { } } +std::unordered_map napi_env__::napi_envs{}; + // Warning: Keep in-sync with napi_status enum static const char* error_messages[] = { nullptr, diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 65891f45..128c690c 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -15,13 +15,18 @@ struct napi_env__ { std::unordered_map active_ref_values{}; std::list strong_refs{}; + JSValueRef constructor_info_symbol{}; + JSValueRef function_info_symbol{}; JSValueRef reference_info_symbol{}; JSValueRef wrapper_info_symbol{}; const std::thread::id thread_id{std::this_thread::get_id()}; napi_env__(JSGlobalContextRef context) : context{context} { + napi_envs[context] = this; JSGlobalContextRetain(context); + init_symbol(constructor_info_symbol, "BabylonNative_ConstructorInfo"); + init_symbol(function_info_symbol, "BabylonNative_FunctionInfo"); init_symbol(reference_info_symbol, "BabylonNative_ReferenceInfo"); init_symbol(wrapper_info_symbol, "BabylonNative_WrapperInfo"); } @@ -30,10 +35,24 @@ struct napi_env__ { deinit_refs(); deinit_symbol(wrapper_info_symbol); deinit_symbol(reference_info_symbol); + deinit_symbol(function_info_symbol); + deinit_symbol(constructor_info_symbol); JSGlobalContextRelease(context); + napi_envs.erase(context); + } + + static napi_env get(JSContextRef context) { + auto it = napi_envs.find(context); + if (it != napi_envs.end()) { + return it->second; + } else { + return nullptr; + } } private: + static std::unordered_map napi_envs; + void deinit_refs(); void init_symbol(JSValueRef& symbol, const char* description) { From e738b2a856f176715b4a50489f3bf908ff9d606c Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Wed, 15 Jan 2025 16:10:33 -0800 Subject: [PATCH 08/31] Swithc over ConstructorInfo --- .../Source/js_native_api_javascriptcore.cc | 37 +++++++++---------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index ec8bad63..34f3292a 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -236,24 +236,6 @@ namespace { return nullptr; } - template - static T* FindInPrototypeChain(JSContextRef ctx, JSObjectRef obj) { - while (true) { - JSValueRef exception{}; - JSObjectRef prototype = JSValueToObject(ctx, JSObjectGetPrototype(ctx, obj), &exception); - if (exception != nullptr) { - return nullptr; - } - - NativeInfo* info = Get(prototype); - if (info != nullptr && info->Type() == T::StaticType) { - return reinterpret_cast(info); - } - - obj = prototype; - } - } - protected: NativeInfo(NativeType type) : _type{type} { @@ -283,7 +265,7 @@ namespace { } JSObjectRef constructor{JSObjectMakeConstructor(env->context, nullptr, CallAsConstructor)}; - JSObjectRef prototype{JSObjectMake(env->context, info->_class, info)}; + JSObjectRef prototype{JSObjectMake(env->context, info->_class, nullptr)}; JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, constructor)); JSObjectSetPrototype(env->context, constructor, prototype); @@ -292,6 +274,9 @@ namespace { kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete, &exception); CHECK_JSC(env, exception); + JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; + NativeInfo::Apply(env, constructor, sentinel); + *result = ToNapi(constructor); return napi_ok; } @@ -319,7 +304,7 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - ConstructorInfo* info = NativeInfo::FindInPrototypeChain(ctx, constructor); + ConstructorInfo* info = NativeInfo::Lookup(ToNapi(ctx), constructor); // Make sure any errors encountered last time we were in N-API are gone. napi_clear_last_error(info->_env); @@ -2511,6 +2496,18 @@ napi_status napi_run_script(napi_env env, JSValueRef return_value{JSEvaluateScript( env->context, script_str, nullptr, JSString(source_url), 0, &exception)}; + if (exception) { + // Get the stack property of the exception object + JSValueRef stackValue = JSObjectGetProperty(env->context, JSValueToObject(env->context, exception, nullptr), JSString("stack"), nullptr); + if (stackValue) { + // Convert the stack value to a string + JSStringRef stackStringRef = JSValueToStringCopy(env->context, stackValue, nullptr); + size_t maxSize = JSStringGetMaximumUTF8CStringSize(stackStringRef); + std::string stack(maxSize, '\0'); + JSStringGetUTF8CString(stackStringRef, &stack[0], maxSize); + JSStringRelease(stackStringRef); + } + } CHECK_JSC(env, exception); if (result != nullptr) { From aef73ebf6f5509ca6f31cd9ec9a8c2ac7f8e0444 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Wed, 15 Jan 2025 16:19:16 -0800 Subject: [PATCH 09/31] Move init_symbol into impl and use JSString so it correctly gets released --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 12 ++++++++++++ Core/Node-API/Source/js_native_api_javascriptcore.h | 12 ++---------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 34f3292a..a67b8fa0 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -265,6 +265,8 @@ namespace { } JSObjectRef constructor{JSObjectMakeConstructor(env->context, nullptr, CallAsConstructor)}; + // BEGIN TODO: This extra prototype should no longer be needed, but for some reason removing it leads to errors + // when setting properties on some prototypes. This should be investigated and removed. JSObjectRef prototype{JSObjectMake(env->context, info->_class, nullptr)}; JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, constructor)); JSObjectSetPrototype(env->context, constructor, prototype); @@ -273,6 +275,7 @@ namespace { JSObjectSetProperty(env->context, prototype, JSString("constructor"), constructor, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete, &exception); CHECK_JSC(env, exception); + // END TODO JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; NativeInfo::Apply(env, constructor, sentinel); @@ -723,6 +726,15 @@ void napi_env__::deinit_refs() { } } +void napi_env__::init_symbol(JSValueRef &symbol, const char *description) { + symbol = JSValueMakeSymbol(context, JSString(description)); + JSValueProtect(context, symbol); +} + +void napi_env__::deinit_symbol(JSValueRef symbol) { + JSValueUnprotect(context, symbol); +} + std::unordered_map napi_env__::napi_envs{}; // Warning: Keep in-sync with napi_status enum diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 128c690c..a9270584 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -54,16 +54,8 @@ struct napi_env__ { static std::unordered_map napi_envs; void deinit_refs(); - - void init_symbol(JSValueRef& symbol, const char* description) { - JSStringRef descriptionRef{JSStringCreateWithUTF8CString(description)}; - symbol = JSValueMakeSymbol(context, descriptionRef); - JSValueProtect(context, symbol); - } - - void deinit_symbol(JSValueRef symbol) { - JSValueUnprotect(context, symbol); - } + void init_symbol(JSValueRef& symbol, const char* description); + void deinit_symbol(JSValueRef symbol); }; #define RETURN_STATUS_IF_FALSE(env, condition, status) \ From f63825ddc20ce41e9bbde042ef85709d9a7e7ccf Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Wed, 15 Jan 2025 17:09:17 -0800 Subject: [PATCH 10/31] Rename helper functions Add more asserts Move value,count to init instead of ctor --- .../Source/js_native_api_javascriptcore.cc | 42 +++++++++++-------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index a67b8fa0..c5b098d6 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -192,7 +192,7 @@ namespace { } template - static void Apply(napi_env env, JSObjectRef target, JSObjectRef sentinel) { + static void Link(napi_env env, JSObjectRef target, JSObjectRef sentinel) { JSValueRef exception{}; JSObjectSetPropertyForKey( env->context, @@ -211,7 +211,7 @@ namespace { } template - static T* Lookup(napi_env env, JSObjectRef obj) { + static T* Query(napi_env env, JSObjectRef obj) { JSValueRef exception{}; const auto hasSentinel{JSObjectHasPropertyForKey(env->context, obj, T::GetKey(env), &exception)}; if (exception != nullptr) { @@ -278,7 +278,7 @@ namespace { // END TODO JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - NativeInfo::Apply(env, constructor, sentinel); + NativeInfo::Link(env, constructor, sentinel); *result = ToNapi(constructor); return napi_ok; @@ -307,7 +307,7 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - ConstructorInfo* info = NativeInfo::Lookup(ToNapi(ctx), constructor); + ConstructorInfo* info = NativeInfo::Query(ToNapi(ctx), constructor); // Make sure any errors encountered last time we were in N-API are gone. napi_clear_last_error(info->_env); @@ -368,7 +368,7 @@ namespace { JSObjectRef function{JSObjectMakeFunctionWithCallback(env->context, JSString(utf8name), CallAsFunction)}; JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - NativeInfo::Apply(env, function, sentinel); + NativeInfo::Link(env, function, sentinel); *result = ToNapi(function); return napi_ok; @@ -397,7 +397,7 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - FunctionInfo* info = NativeInfo::Lookup(ToNapi(ctx), function); + FunctionInfo* info = NativeInfo::Query(ToNapi(ctx), function); // Make sure any errors encountered last time we were in N-API are gone. napi_clear_last_error(info->_env); @@ -521,7 +521,7 @@ namespace { } static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) { - ReferenceInfo* referenceInfo{NativeInfo::Lookup(env, ToJSObject(env, object))}; + ReferenceInfo* referenceInfo{NativeInfo::Query(env, ToJSObject(env, object))}; *id = referenceInfo == nullptr ? 0 : referenceInfo->GetObjectId(); return napi_ok; } @@ -533,7 +533,7 @@ namespace { } JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - NativeInfo::Apply(env, ToJSObject(env, object), sentinel); + NativeInfo::Link(env, ToJSObject(env, object), sentinel); info->AddFinalizer(finalizer); return napi_ok; } @@ -565,7 +565,7 @@ namespace { } JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - NativeInfo::Apply(env, ToJSObject(env, object), sentinel); + NativeInfo::Link(env, ToJSObject(env, object), sentinel); } *result = info; @@ -573,7 +573,7 @@ namespace { } static napi_status Unwrap(napi_env env, napi_value object, WrapperInfo** result) { - *result = NativeInfo::Lookup(env, ToJSObject(env, object)); + *result = NativeInfo::Query(env, ToJSObject(env, object)); return napi_ok; } @@ -632,15 +632,15 @@ namespace { } struct napi_ref__ { - napi_ref__(napi_value value, uint32_t count) - : _value{value} - , _count{count} { - } - + napi_ref__() = default; napi_ref__(const napi_ref__&) = delete; napi_ref__& operator=(const napi_ref__&) = delete; - napi_status init(napi_env env) { + napi_status init(napi_env env, napi_value value, uint32_t count) { + assert(!_value); + _value = value; + _count = count; + // track the ref values to support weak refs CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); if (_objectId == 0) { @@ -655,6 +655,8 @@ struct napi_ref__ { CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); assert(_objectId); env->active_ref_values[_value] = _objectId; + } else { + assert(env->active_ref_values.find(_value) != env->active_ref_values.end()); } if (_count != 0) { @@ -665,6 +667,7 @@ struct napi_ref__ { } void deinit(napi_env env) { + assert(_value); if (_count != 0) { unprotect(env); } @@ -674,12 +677,14 @@ struct napi_ref__ { } void ref(napi_env env) { + assert(_value); if (_count++ == 0) { protect(env); } } void unref(napi_env env) { + assert(_value); assert(_count != 0); if (--_count == 0) { unprotect(env); @@ -691,6 +696,7 @@ struct napi_ref__ { } napi_value value(napi_env env) const { + assert(_value); if (env->active_ref_values.find(_value) != env->active_ref_values.end()) { std::uintptr_t objectId{}; @@ -1905,12 +1911,12 @@ napi_status napi_create_reference(napi_env env, CHECK_ARG(env, value); CHECK_ARG(env, result); - napi_ref__* ref{new napi_ref__{value, initial_refcount}}; + napi_ref__* ref{new napi_ref__{}}; if (ref == nullptr) { return napi_set_last_error(env, napi_generic_failure); } - ref->init(env); + ref->init(env, value, initial_refcount); *result = ref; return napi_ok; From a339e58239e2b44b070b78b2474288a36fcbc6a2 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Wed, 15 Jan 2025 17:14:46 -0800 Subject: [PATCH 11/31] Add some comments --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index c5b098d6..21c92700 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -646,6 +646,11 @@ struct napi_ref__ { if (_objectId == 0) { CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value](ReferenceInfo* info) { auto entry{info->Env()->active_ref_values.find(value)}; + // 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); @@ -700,6 +705,8 @@ struct napi_ref__ { if (env->active_ref_values.find(_value) != env->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. if (ReferenceInfo::GetObjectId(env, _value, &objectId) == napi_ok && objectId == _objectId) { return _value; } From 3c2a8b34c085d90e26325bb7b6c3954dc8322669 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Thu, 16 Jan 2025 16:20:41 -0800 Subject: [PATCH 12/31] Review feedback --- .../Source/js_native_api_javascriptcore.cc | 133 +++++++++--------- .../Source/js_native_api_javascriptcore.h | 2 +- 2 files changed, 71 insertions(+), 64 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 21c92700..4441130d 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -192,7 +192,7 @@ namespace { } template - static void Link(napi_env env, JSObjectRef target, JSObjectRef sentinel) { + static napi_status Link(napi_env env, JSObjectRef target, JSObjectRef sentinel) { JSValueRef exception{}; JSObjectSetPropertyForKey( env->context, @@ -203,37 +203,40 @@ namespace { &exception); CHECK_JSC(env, exception); + return napi_ok; } template - static T* Get(JSObjectRef obj) { - return reinterpret_cast(JSObjectGetPrivate(obj)); + static T* Get(JSObjectRef sentinel) { + return reinterpret_cast(JSObjectGetPrivate(sentinel)); } template - static T* Query(napi_env env, JSObjectRef obj) { - JSValueRef exception{}; - const auto hasSentinel{JSObjectHasPropertyForKey(env->context, obj, T::GetKey(env), &exception)}; - if (exception != nullptr) { + static T* Query(napi_env env, JSObjectRef obj, JSValueRef* exception) { + const auto hasSentinel{JSObjectHasPropertyForKey(env->context, obj, T::GetKey(env), exception)}; + if (*exception || !hasSentinel) { return nullptr; } - if (hasSentinel) { - JSValueRef exception{}; - JSValueRef sentinelValue{JSObjectGetPropertyForKey(env->context, obj, T::GetKey(env), &exception)}; - if (exception != nullptr) { - return nullptr; - } - - JSObjectRef sentinel{JSValueToObject(env->context, sentinelValue, &exception)}; - if (exception != nullptr) { - return nullptr; - } + JSValueRef sentinelValue{JSObjectGetPropertyForKey(env->context, obj, T::GetKey(env), exception)}; + if (*exception) { + return nullptr; + } - return Get(sentinel); + JSObjectRef sentinel{JSValueToObject(env->context, sentinelValue, exception)}; + if (*exception) { + return nullptr; } - return nullptr; + return Get(sentinel); + } + + template + static napi_status Query(napi_env env, JSObjectRef obj, T** info) { + JSValueRef exception{}; + *info = Query(env, obj, &exception); + CHECK_JSC(env, exception); + return napi_ok; } protected: @@ -278,7 +281,7 @@ namespace { // END TODO JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - NativeInfo::Link(env, constructor, sentinel); + CHECK_NAPI(NativeInfo::Link(env, constructor, sentinel)); *result = ToNapi(constructor); return napi_ok; @@ -307,10 +310,14 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - ConstructorInfo* info = NativeInfo::Query(ToNapi(ctx), constructor); + napi_env env{ToNapi(ctx)}; + ConstructorInfo* info = NativeInfo::Query(env, constructor, exception); + if (*exception) { + return nullptr; + } // Make sure any errors encountered last time we were in N-API are gone. - napi_clear_last_error(info->_env); + napi_clear_last_error(env); JSObjectRef instance{JSObjectMake(ctx, nullptr, nullptr)}; JSObjectSetPrototype(ctx, instance, JSObjectGetPrototype(ctx, constructor)); @@ -322,14 +329,15 @@ namespace { cbinfo.argv = ToNapi(arguments); cbinfo.data = info->_data; - napi_value result = info->_cb(info->_env, &cbinfo); + napi_value result = info->_cb(env, &cbinfo); - if (info->_env->last_exception != nullptr) { - *exception = info->_env->last_exception; - info->_env->last_exception = nullptr; + if (env->last_exception != nullptr) { + *exception = env->last_exception; + env->last_exception = nullptr; + return nullptr; } - return ToJSObject(info->_env, result); + return ToJSObject(env, result); } // JSObjectFinalizeCallback @@ -368,7 +376,7 @@ namespace { JSObjectRef function{JSObjectMakeFunctionWithCallback(env->context, JSString(utf8name), CallAsFunction)}; JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - NativeInfo::Link(env, function, sentinel); + CHECK_NAPI(NativeInfo::Link(env, function, sentinel)); *result = ToNapi(function); return napi_ok; @@ -397,10 +405,14 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - FunctionInfo* info = NativeInfo::Query(ToNapi(ctx), function); + napi_env env{ToNapi(ctx)}; + FunctionInfo* info = NativeInfo::Query(env, function, exception); + if (*exception) { + return nullptr; + } // Make sure any errors encountered last time we were in N-API are gone. - napi_clear_last_error(info->_env); + napi_clear_last_error(env); napi_callback_info__ cbinfo{}; cbinfo.thisArg = ToNapi(thisObject); @@ -409,11 +421,12 @@ namespace { cbinfo.argv = ToNapi(arguments); cbinfo.data = info->_data; - napi_value result = info->_cb(info->_env, &cbinfo); + napi_value result = info->_cb(env, &cbinfo); - if (info->_env->last_exception != nullptr) { - *exception = info->_env->last_exception; - info->_env->last_exception = nullptr; + if (env->last_exception != nullptr) { + *exception = env->last_exception; + env->last_exception = nullptr; + return nullptr; } return ToJSValue(result); @@ -516,14 +529,15 @@ namespace { class ReferenceInfo : public BaseInfoT { public: - static JSValueRef GetKey(napi_env env) { - return env->reference_info_symbol; - } + static JSValueRef GetKey(napi_env env) { + return env->reference_info_symbol; + } static napi_status GetObjectId(napi_env env, napi_value object, std::uintptr_t* id) { - ReferenceInfo* referenceInfo{NativeInfo::Query(env, ToJSObject(env, object))}; - *id = referenceInfo == nullptr ? 0 : referenceInfo->GetObjectId(); - return napi_ok; + ReferenceInfo* referenceInfo{}; + CHECK_NAPI(NativeInfo::Query(env, ToJSObject(env, object), &referenceInfo)); + *id = referenceInfo == nullptr ? 0 : referenceInfo->GetObjectId(); + return napi_ok; } static napi_status Initialize(napi_env env, napi_value object, FinalizerT finalizer) { @@ -533,7 +547,7 @@ namespace { } JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - NativeInfo::Link(env, ToJSObject(env, object), sentinel); + CHECK_NAPI(NativeInfo::Link(env, ToJSObject(env, object), sentinel)); info->AddFinalizer(finalizer); return napi_ok; } @@ -565,7 +579,7 @@ namespace { } JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; - NativeInfo::Link(env, ToJSObject(env, object), sentinel); + CHECK_NAPI(NativeInfo::Link(env, ToJSObject(env, object), sentinel)); } *result = info; @@ -573,7 +587,7 @@ namespace { } static napi_status Unwrap(napi_env env, napi_value object, WrapperInfo** result) { - *result = NativeInfo::Query(env, ToJSObject(env, object)); + CHECK_NAPI(NativeInfo::Query(env, ToJSObject(env, object), result)); return napi_ok; } @@ -633,9 +647,15 @@ namespace { struct napi_ref__ { napi_ref__() = default; + + // Copy semantics napi_ref__(const napi_ref__&) = delete; napi_ref__& operator=(const napi_ref__&) = delete; + // Move semantics + napi_ref__(napi_ref__&&) = delete; + napi_ref__& operator=(napi_ref__&&) = delete; + napi_status init(napi_env env, napi_value value, uint32_t count) { assert(!_value); _value = value; @@ -700,19 +720,20 @@ struct napi_ref__ { return _count; } - napi_value value(napi_env env) const { + napi_status value(napi_env env, napi_value* result) const { assert(_value); if (env->active_ref_values.find(_value) != env->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. - if (ReferenceInfo::GetObjectId(env, _value, &objectId) == napi_ok && objectId == _objectId) { - return _value; + CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &objectId)); + if (objectId == _objectId) { + *result = _value; } } - return nullptr; + return napi_ok; } private: @@ -748,8 +769,6 @@ void napi_env__::deinit_symbol(JSValueRef symbol) { JSValueUnprotect(context, symbol); } -std::unordered_map napi_env__::napi_envs{}; - // Warning: Keep in-sync with napi_status enum static const char* error_messages[] = { nullptr, @@ -1983,7 +2002,7 @@ napi_status napi_get_reference_value(napi_env env, CHECK_ARG(env, ref); CHECK_ARG(env, result); - *result = ref->value(env); + CHECK_NAPI(ref->value(env, result)); return napi_ok; } @@ -2521,18 +2540,6 @@ napi_status napi_run_script(napi_env env, JSValueRef return_value{JSEvaluateScript( env->context, script_str, nullptr, JSString(source_url), 0, &exception)}; - if (exception) { - // Get the stack property of the exception object - JSValueRef stackValue = JSObjectGetProperty(env->context, JSValueToObject(env->context, exception, nullptr), JSString("stack"), nullptr); - if (stackValue) { - // Convert the stack value to a string - JSStringRef stackStringRef = JSValueToStringCopy(env->context, stackValue, nullptr); - size_t maxSize = JSStringGetMaximumUTF8CStringSize(stackStringRef); - std::string stack(maxSize, '\0'); - JSStringGetUTF8CString(stackStringRef, &stack[0], maxSize); - JSStringRelease(stackStringRef); - } - } CHECK_JSC(env, exception); if (result != nullptr) { diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index a9270584..50af70cc 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -51,7 +51,7 @@ struct napi_env__ { } private: - static std::unordered_map napi_envs; + static inline std::unordered_map napi_envs{}; void deinit_refs(); void init_symbol(JSValueRef& symbol, const char* description); From e4b3518e08a8dd3c635f8f0be187ad56ac1b4ae1 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 10:46:54 -0800 Subject: [PATCH 13/31] Update build tooling versions --- .github/azure-pipelines.yml | 10 +++++----- .github/jobs/macos.yml | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/azure-pipelines.yml b/.github/azure-pipelines.yml index 1ea4b23e..49301f7b 100644 --- a/.github/azure-pipelines.yml +++ b/.github/azure-pipelines.yml @@ -76,16 +76,16 @@ jobs: # iOS - template: jobs/ios.yml parameters: - name: 'iOS_Xcode142' + name: 'iOS_Xcode162' vmImage: 'macOS-latest' - xCodeVersion: 14.2 - simulator: 'iPhone 11' + xCodeVersion: 16.2 + simulator: 'iPhone 16' - template: jobs/ios.yml parameters: - name: 'iOS_Xcode150' + name: 'iOS_Xcode152' vmImage: 'macOS-13' - xCodeVersion: 15.0 + xCodeVersion: 15.2 simulator: 'iPhone 14' # Linux diff --git a/.github/jobs/macos.yml b/.github/jobs/macos.yml index 2a321a92..4640e638 100644 --- a/.github/jobs/macos.yml +++ b/.github/jobs/macos.yml @@ -7,8 +7,8 @@ jobs: steps: - script: | - sudo xcode-select --switch /Applications/Xcode_14.0.app/Contents/Developer - displayName: 'Select Xcode 14.0' + sudo xcode-select --switch /Applications/Xcode_15.1.app/Contents/Developer + displayName: 'Select Xcode 15.1' - script: | cmake -B Build/macOS -GXcode From 3bb04eb42abc45180402159f1936a41a6df7e79f Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 11:02:58 -0800 Subject: [PATCH 14/31] Switch MacOS 14 to large (not ARM64) Try updating Android tooling versions --- .github/azure-pipelines.yml | 2 +- .github/jobs/android.yml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/azure-pipelines.yml b/.github/azure-pipelines.yml index 49301f7b..e73341d9 100644 --- a/.github/azure-pipelines.yml +++ b/.github/azure-pipelines.yml @@ -77,7 +77,7 @@ jobs: - template: jobs/ios.yml parameters: name: 'iOS_Xcode162' - vmImage: 'macOS-latest' + vmImage: 'macOS-latest-large' xCodeVersion: 16.2 simulator: 'iPhone 16' diff --git a/.github/jobs/android.yml b/.github/jobs/android.yml index 1ca3382c..09924157 100644 --- a/.github/jobs/android.yml +++ b/.github/jobs/android.yml @@ -14,15 +14,15 @@ jobs: steps: - script: | echo Install Android image - echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --install 'system-images;android-27;default;x86_64' + echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --install 'system-images;android-35;default;x86_64' echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --licenses echo Create AVD - $ANDROID_HOME/tools/bin/avdmanager create avd -n Pixel_API_27 -d pixel -k 'system-images;android-27;default;x86_64' + $ANDROID_HOME/tools/bin/avdmanager create avd -n Pixel_API_35 -d pixel -k 'system-images;android-35;default;x86_64' displayName: 'Install Android Emulator' - script: | echo Start emulator - nohup $ANDROID_HOME/emulator/emulator -avd Pixel_API_27 -gpu host -no-window 2>&1 & + nohup $ANDROID_HOME/emulator/emulator -avd Pixel_API_35 -gpu host -no-window 2>&1 & echo Wait for emulator $ANDROID_HOME/platform-tools/adb wait-for-device shell 'while [[ -z $(getprop sys.boot_completed) ]]; do echo '.'; sleep 1; done' $ANDROID_HOME/platform-tools/adb devices From f70b3f9990bb523a33e730c9fe7befe3e7e3b4a0 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 11:07:14 -0800 Subject: [PATCH 15/31] Update casing (not sure why the image can't be selected) --- .github/azure-pipelines.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/azure-pipelines.yml b/.github/azure-pipelines.yml index e73341d9..0e81dfef 100644 --- a/.github/azure-pipelines.yml +++ b/.github/azure-pipelines.yml @@ -77,14 +77,14 @@ jobs: - template: jobs/ios.yml parameters: name: 'iOS_Xcode162' - vmImage: 'macOS-latest-large' + vmImage: 'macos-latest-large' xCodeVersion: 16.2 simulator: 'iPhone 16' - template: jobs/ios.yml parameters: name: 'iOS_Xcode152' - vmImage: 'macOS-13' + vmImage: 'macos-13' xCodeVersion: 15.2 simulator: 'iPhone 14' From 2c12063314c1c839fee7a6d4328cb79d48f3b8e7 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 11:12:23 -0800 Subject: [PATCH 16/31] Try another option for image name Remove step to install the Android emulator --- .github/azure-pipelines.yml | 2 +- .github/jobs/android.yml | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/.github/azure-pipelines.yml b/.github/azure-pipelines.yml index 0e81dfef..307c823e 100644 --- a/.github/azure-pipelines.yml +++ b/.github/azure-pipelines.yml @@ -77,7 +77,7 @@ jobs: - template: jobs/ios.yml parameters: name: 'iOS_Xcode162' - vmImage: 'macos-latest-large' + vmImage: 'macos-14-large' xCodeVersion: 16.2 simulator: 'iPhone 16' diff --git a/.github/jobs/android.yml b/.github/jobs/android.yml index 09924157..dc497a70 100644 --- a/.github/jobs/android.yml +++ b/.github/jobs/android.yml @@ -12,14 +12,6 @@ jobs: vmImage: macos-latest steps: - - script: | - echo Install Android image - echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --install 'system-images;android-35;default;x86_64' - echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --licenses - echo Create AVD - $ANDROID_HOME/tools/bin/avdmanager create avd -n Pixel_API_35 -d pixel -k 'system-images;android-35;default;x86_64' - displayName: 'Install Android Emulator' - - script: | echo Start emulator nohup $ANDROID_HOME/emulator/emulator -avd Pixel_API_35 -gpu host -no-window 2>&1 & From b71120df1474cb3c91ecf69b2abb671abc60dba3 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 11:21:44 -0800 Subject: [PATCH 17/31] Revert image names --- .github/azure-pipelines.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/azure-pipelines.yml b/.github/azure-pipelines.yml index 307c823e..49301f7b 100644 --- a/.github/azure-pipelines.yml +++ b/.github/azure-pipelines.yml @@ -77,14 +77,14 @@ jobs: - template: jobs/ios.yml parameters: name: 'iOS_Xcode162' - vmImage: 'macos-14-large' + vmImage: 'macOS-latest' xCodeVersion: 16.2 simulator: 'iPhone 16' - template: jobs/ios.yml parameters: name: 'iOS_Xcode152' - vmImage: 'macos-13' + vmImage: 'macOS-13' xCodeVersion: 15.2 simulator: 'iPhone 14' From a220bd39ddb0b4b3d628e2bf8bde82034a7af9ed Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 11:26:49 -0800 Subject: [PATCH 18/31] Revert Android emulator avd name --- .github/jobs/android.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/jobs/android.yml b/.github/jobs/android.yml index dc497a70..3b131878 100644 --- a/.github/jobs/android.yml +++ b/.github/jobs/android.yml @@ -14,7 +14,7 @@ jobs: steps: - script: | echo Start emulator - nohup $ANDROID_HOME/emulator/emulator -avd Pixel_API_35 -gpu host -no-window 2>&1 & + nohup $ANDROID_HOME/emulator/emulator -avd Pixel_API_27 -gpu host -no-window 2>&1 & echo Wait for emulator $ANDROID_HOME/platform-tools/adb wait-for-device shell 'while [[ -z $(getprop sys.boot_completed) ]]; do echo '.'; sleep 1; done' $ANDROID_HOME/platform-tools/adb devices From 465ab9f740752f9fc9643a2f2d0e389ca45e1de2 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 11:48:58 -0800 Subject: [PATCH 19/31] Bring back emulator installation step but try to list avds --- .github/jobs/android.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/jobs/android.yml b/.github/jobs/android.yml index 3b131878..340b3740 100644 --- a/.github/jobs/android.yml +++ b/.github/jobs/android.yml @@ -12,6 +12,15 @@ jobs: vmImage: macos-latest steps: + - script: | + echo Install Android image + echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --install 'system-images;android-27;default;x86_64' + echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --licenses + echo 'y' | $ANDROID_HOME/tools/bin/avdmanager list sdk + echo Create AVD + $ANDROID_HOME/tools/bin/avdmanager create avd -n Pixel_API_27 -d pixel -k 'system-images;android-27;default;x86_64' + displayName: 'Install Android Emulator' + - script: | echo Start emulator nohup $ANDROID_HOME/emulator/emulator -avd Pixel_API_27 -gpu host -no-window 2>&1 & From c812a74d9809346f77e92ae99bff3f63a51397e0 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 12:01:03 -0800 Subject: [PATCH 20/31] Try using older Java --- .github/jobs/android.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/jobs/android.yml b/.github/jobs/android.yml index 340b3740..1de60ec7 100644 --- a/.github/jobs/android.yml +++ b/.github/jobs/android.yml @@ -14,9 +14,11 @@ jobs: steps: - script: | echo Install Android image + export JAVA_HOME=$JAVA_HOME_8_X64 echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --install 'system-images;android-27;default;x86_64' echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --licenses - echo 'y' | $ANDROID_HOME/tools/bin/avdmanager list sdk + echo Available AVDs + echo 'y' | $ANDROID_HOME/tools/bin/avdmanager list avd echo Create AVD $ANDROID_HOME/tools/bin/avdmanager create avd -n Pixel_API_27 -d pixel -k 'system-images;android-27;default;x86_64' displayName: 'Install Android Emulator' From a694d8f549528abd2c1274652afb1c66586717f2 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 14:24:02 -0800 Subject: [PATCH 21/31] Take https://github.com/nodejs/node-addon-api/pull/1607/files --- Core/Node-API/Include/Shared/napi/napi-inl.h | 3 ++- Core/Node-API/Include/Shared/napi/napi.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Core/Node-API/Include/Shared/napi/napi-inl.h b/Core/Node-API/Include/Shared/napi/napi-inl.h index 438a23d0..b336b044 100644 --- a/Core/Node-API/Include/Shared/napi/napi-inl.h +++ b/Core/Node-API/Include/Shared/napi/napi-inl.h @@ -4513,7 +4513,7 @@ template inline ObjectWrap::~ObjectWrap() { // If the JS object still exists at this point, remove the finalizer added // through `napi_wrap()`. - if (!IsEmpty()) { + if (!IsEmpty() && !_finalized) { Object object = Value(); // It is not valid to call `napi_remove_wrap()` with an empty `object`. // This happens e.g. during garbage collection. @@ -4955,6 +4955,7 @@ inline void ObjectWrap::FinalizeCallback(napi_env env, void* /*hint*/) { HandleScope scope(env); T* instance = static_cast(data); + instance->_finalized = true; instance->Finalize(Napi::Env(env)); delete instance; } diff --git a/Core/Node-API/Include/Shared/napi/napi.h b/Core/Node-API/Include/Shared/napi/napi.h index ef1a1836..88e68d64 100644 --- a/Core/Node-API/Include/Shared/napi/napi.h +++ b/Core/Node-API/Include/Shared/napi/napi.h @@ -2497,6 +2497,7 @@ class ObjectWrap : public InstanceWrap, public Reference { } bool _construction_failed = true; + bool _finalized = false; }; class HandleScope { From ac392a47b56936abe9ef625eab2f35a4aa11616f Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 14:25:57 -0800 Subject: [PATCH 22/31] Don't use info in extra JSObjectMake for empty prototype in ConstructorInfo (otherwise the Finalize for that JS prototype object gets nullptr for private data and we get a crash) --- .../Node-API/Source/js_native_api_javascriptcore.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 4441130d..65249ea2 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -262,15 +262,10 @@ namespace { napi_callback cb, void* data, napi_value* result) { - ConstructorInfo* info{new ConstructorInfo(env, utf8name, length, cb, data)}; - if (info == nullptr) { - return napi_set_last_error(env, napi_generic_failure); - } - JSObjectRef constructor{JSObjectMakeConstructor(env->context, nullptr, CallAsConstructor)}; // BEGIN TODO: This extra prototype should no longer be needed, but for some reason removing it leads to errors // when setting properties on some prototypes. This should be investigated and removed. - JSObjectRef prototype{JSObjectMake(env->context, info->_class, nullptr)}; + JSObjectRef prototype{JSObjectMake(env->context, nullptr, nullptr)}; JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, constructor)); JSObjectSetPrototype(env->context, constructor, prototype); @@ -280,6 +275,11 @@ namespace { CHECK_JSC(env, exception); // END TODO + ConstructorInfo* info{new ConstructorInfo(env, utf8name, length, cb, data)}; + if (info == nullptr) { + return napi_set_last_error(env, napi_generic_failure); + } + JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; CHECK_NAPI(NativeInfo::Link(env, constructor, sentinel)); @@ -692,7 +692,6 @@ struct napi_ref__ { } void deinit(napi_env env) { - assert(_value); if (_count != 0) { unprotect(env); } From debfb94a85f5e5f86334620629b3190230befe7d Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 16:33:49 -0800 Subject: [PATCH 23/31] Try macos 13 for Android --- .github/jobs/android.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/jobs/android.yml b/.github/jobs/android.yml index 1de60ec7..04895cec 100644 --- a/.github/jobs/android.yml +++ b/.github/jobs/android.yml @@ -9,7 +9,7 @@ jobs: timeoutInMinutes: 30 pool: - vmImage: macos-latest + vmImage: macos-13 steps: - script: | @@ -17,8 +17,6 @@ jobs: export JAVA_HOME=$JAVA_HOME_8_X64 echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --install 'system-images;android-27;default;x86_64' echo 'y' | $ANDROID_HOME/tools/bin/sdkmanager --licenses - echo Available AVDs - echo 'y' | $ANDROID_HOME/tools/bin/avdmanager list avd echo Create AVD $ANDROID_HOME/tools/bin/avdmanager create avd -n Pixel_API_27 -d pixel -k 'system-images;android-27;default;x86_64' displayName: 'Install Android Emulator' From d98cf7d922098b85352f8acb10db72efe66afb55 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 16:43:19 -0800 Subject: [PATCH 24/31] Disable Android emulator audio --- .github/jobs/android.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/jobs/android.yml b/.github/jobs/android.yml index 04895cec..bc5cdc00 100644 --- a/.github/jobs/android.yml +++ b/.github/jobs/android.yml @@ -23,7 +23,7 @@ jobs: - script: | echo Start emulator - nohup $ANDROID_HOME/emulator/emulator -avd Pixel_API_27 -gpu host -no-window 2>&1 & + nohup $ANDROID_HOME/emulator/emulator -avd Pixel_API_27 -gpu host -no-window -no-audio -no-boot-anim 2>&1 & echo Wait for emulator $ANDROID_HOME/platform-tools/adb wait-for-device shell 'while [[ -z $(getprop sys.boot_completed) ]]; do echo '.'; sleep 1; done' $ANDROID_HOME/platform-tools/adb devices From c9ee1f1b962c318e119a5311b387c32f56e0d382 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 17:02:43 -0800 Subject: [PATCH 25/31] Add some diagnostic output for xcode 'destiniations' --- .github/jobs/ios.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/jobs/ios.yml b/.github/jobs/ios.yml index 2f7ce5d3..9580ce7c 100644 --- a/.github/jobs/ios.yml +++ b/.github/jobs/ios.yml @@ -20,6 +20,10 @@ jobs: cmake -B Build/iOS -G Xcode -D IOS=ON displayName: 'Configure CMake' + - script: | + /Applications/Xcode_16.2.app/Contents/Developer/usr/bin/xcodebuild -project Build/iOS/JsRuntimeHost.xcodeproj -scheme UnitTests -showdestination + displayName: 'Show Destinations' + - task: Xcode@5 inputs: xcWorkspacePath: 'Build/iOS/JsRuntimeHost.xcodeproj' From 8c3b1f32eb638533500e9b469e2a52cf7243e4b3 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 17:09:40 -0800 Subject: [PATCH 26/31] Fix typo --- .github/jobs/ios.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/jobs/ios.yml b/.github/jobs/ios.yml index 9580ce7c..28061aac 100644 --- a/.github/jobs/ios.yml +++ b/.github/jobs/ios.yml @@ -21,7 +21,7 @@ jobs: displayName: 'Configure CMake' - script: | - /Applications/Xcode_16.2.app/Contents/Developer/usr/bin/xcodebuild -project Build/iOS/JsRuntimeHost.xcodeproj -scheme UnitTests -showdestination + /Applications/Xcode_16.2.app/Contents/Developer/usr/bin/xcodebuild -project Build/iOS/JsRuntimeHost.xcodeproj -scheme UnitTests -showdestinations displayName: 'Show Destinations' - task: Xcode@5 From 9e0b97107c697d014ebbcd44b98b5d0668e4e08b Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 17:16:06 -0800 Subject: [PATCH 27/31] Try booting iOS simulator before building --- .github/jobs/ios.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/jobs/ios.yml b/.github/jobs/ios.yml index 28061aac..13e2156d 100644 --- a/.github/jobs/ios.yml +++ b/.github/jobs/ios.yml @@ -20,6 +20,11 @@ jobs: cmake -B Build/iOS -G Xcode -D IOS=ON displayName: 'Configure CMake' + - script: | + echo Boot "${{parameters.simulator}}" + xcrun simctl boot "${{parameters.simulator}}" + displayName: 'Boot Simulator' + - script: | /Applications/Xcode_16.2.app/Contents/Developer/usr/bin/xcodebuild -project Build/iOS/JsRuntimeHost.xcodeproj -scheme UnitTests -showdestinations displayName: 'Show Destinations' @@ -34,8 +39,6 @@ jobs: displayName: 'Build Xcode Project' - script: | - echo Boot "${{parameters.simulator}}" - xcrun simctl boot "${{parameters.simulator}}" echo Install UnitTests app xcrun simctl install booted "Build/iOS/Tests/UnitTests/RelWithDebInfo-iphonesimulator/UnitTests.app" echo Launch UnitTests app From 38f6ccefb25cdf98ae03c6fe1dc275d4f711794e Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Fri, 17 Jan 2025 17:44:23 -0800 Subject: [PATCH 28/31] Remove showdestinations --- .github/jobs/ios.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.github/jobs/ios.yml b/.github/jobs/ios.yml index 13e2156d..e3b7aba2 100644 --- a/.github/jobs/ios.yml +++ b/.github/jobs/ios.yml @@ -25,10 +25,6 @@ jobs: xcrun simctl boot "${{parameters.simulator}}" displayName: 'Boot Simulator' - - script: | - /Applications/Xcode_16.2.app/Contents/Developer/usr/bin/xcodebuild -project Build/iOS/JsRuntimeHost.xcodeproj -scheme UnitTests -showdestinations - displayName: 'Show Destinations' - - task: Xcode@5 inputs: xcWorkspacePath: 'Build/iOS/JsRuntimeHost.xcodeproj' From 97d7bc8670ad0ed9ffccc3390dafff5fc8d6f079 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Tue, 21 Jan 2025 10:40:38 -0800 Subject: [PATCH 29/31] Apply suggestions from code review Co-authored-by: Gary Hsu --- .../Source/js_native_api_javascriptcore.cc | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 65249ea2..f8cda4d1 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -552,8 +552,7 @@ namespace { return napi_ok; } - std::uintptr_t GetObjectId() - { + std::uintptr_t GetObjectId() { return reinterpret_cast(this); } @@ -653,8 +652,8 @@ struct napi_ref__ { napi_ref__& operator=(const napi_ref__&) = delete; // Move semantics - napi_ref__(napi_ref__&&) = delete; - napi_ref__& operator=(napi_ref__&&) = delete; + napi_ref__(napi_ref__&&) noexcept = delete; + napi_ref__& operator=(napi_ref__&&) noexcept = delete; napi_status init(napi_env env, napi_value value, uint32_t count) { assert(!_value); @@ -665,16 +664,16 @@ struct napi_ref__ { 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)}; - // 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{info->Env()->active_ref_values.find(value)}; + // 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); + } })); CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId)); @@ -721,8 +720,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->active_ref_values.find(_value) != env->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. From 8a2bcb55c3e22e439d74da91e3079a81b878c0e1 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Tue, 21 Jan 2025 10:41:28 -0800 Subject: [PATCH 30/31] Fix another brace change for consistency --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 3 +-- 1 file changed, 1 insertion(+), 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 f8cda4d1..791c3735 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -670,8 +670,7 @@ struct napi_ref__ { // 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()) - { + if (entry != info->Env()->active_ref_values.end() && entry->second == info->GetObjectId()) { info->Env()->active_ref_values.erase(entry); } })); From 97f15ea0d768332ff38b91b23937eaaccb4fcac1 Mon Sep 17 00:00:00 2001 From: Ryan Tremblay Date: Tue, 21 Jan 2025 12:07:46 -0800 Subject: [PATCH 31/31] Fix JSContextRef vs JSGlobalContextRef issue --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 6 +++--- Core/Node-API/Source/js_native_api_javascriptcore.h | 4 ++-- 2 files changed, 5 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 791c3735..4069e4b7 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -139,7 +139,7 @@ namespace { return reinterpret_cast(const_cast(values)); } - napi_env ToNapi(const JSContextRef context) { + napi_env ToNapi(const JSGlobalContextRef context) { return napi_env__::get(context); } @@ -310,7 +310,7 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - napi_env env{ToNapi(ctx)}; + napi_env env{ToNapi(JSContextGetGlobalContext(ctx))}; ConstructorInfo* info = NativeInfo::Query(env, constructor, exception); if (*exception) { return nullptr; @@ -405,7 +405,7 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - napi_env env{ToNapi(ctx)}; + napi_env env{ToNapi(JSContextGetGlobalContext(ctx))}; FunctionInfo* info = NativeInfo::Query(env, function, exception); if (*exception) { return nullptr; diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 50af70cc..da74596e 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -41,7 +41,7 @@ struct napi_env__ { napi_envs.erase(context); } - static napi_env get(JSContextRef context) { + static napi_env get(JSGlobalContextRef context) { auto it = napi_envs.find(context); if (it != napi_envs.end()) { return it->second; @@ -51,7 +51,7 @@ struct napi_env__ { } private: - static inline std::unordered_map napi_envs{}; + static inline std::unordered_map napi_envs{}; void deinit_refs(); void init_symbol(JSValueRef& symbol, const char* description);