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 { diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 3a630a28..4069e4b7 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 JSGlobalContextRef 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; @@ -188,26 +192,51 @@ namespace { } template - static T* Get(JSObjectRef obj) { - return reinterpret_cast(JSObjectGetPrivate(obj)); + static napi_status Link(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); + return napi_ok; } 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; - } + static T* Get(JSObjectRef sentinel) { + return reinterpret_cast(JSObjectGetPrivate(sentinel)); + } - NativeInfo* info = Get(prototype); - if (info != nullptr && info->Type() == T::StaticType) { - return reinterpret_cast(info); - } + template + 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; + } - obj = prototype; + JSValueRef sentinelValue{JSObjectGetPropertyForKey(env->context, obj, T::GetKey(env), exception)}; + if (*exception) { + return nullptr; } + + JSObjectRef sentinel{JSValueToObject(env->context, sentinelValue, exception)}; + if (*exception) { + 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: @@ -223,19 +252,20 @@ 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, 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)}; - JSObjectRef prototype{JSObjectMake(env->context, info->_class, info)}; + // 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, nullptr, nullptr)}; JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, constructor)); JSObjectSetPrototype(env->context, constructor, prototype); @@ -243,6 +273,15 @@ namespace { JSObjectSetProperty(env->context, prototype, JSString("constructor"), constructor, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete, &exception); 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)); *result = ToNapi(constructor); return napi_ok; @@ -271,10 +310,14 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - ConstructorInfo* info = NativeInfo::FindInPrototypeChain(ctx, constructor); + napi_env env{ToNapi(JSContextGetGlobalContext(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)); @@ -286,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 @@ -315,6 +359,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, @@ -327,9 +375,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)}; + CHECK_NAPI(NativeInfo::Link(env, function, sentinel)); *result = ToNapi(function); return napi_ok; @@ -358,10 +405,14 @@ namespace { size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) { - FunctionInfo* info = NativeInfo::FindInPrototypeChain(ctx, function); + napi_env env{ToNapi(JSContextGetGlobalContext(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); @@ -370,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); @@ -477,20 +529,33 @@ namespace { class ReferenceInfo : public BaseInfoT { public: + 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{}; + 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) { 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); - + JSObjectRef sentinel{JSObjectMake(env->context, info->_class, info)}; + CHECK_NAPI(NativeInfo::Link(env, ToJSObject(env, object), sentinel)); info->AddFinalizer(finalizer); return napi_ok; } + std::uintptr_t GetObjectId() { + return reinterpret_cast(this); + } + private: ReferenceInfo(napi_env env) : BaseInfoT{env, "Native (Reference)"} { @@ -499,6 +564,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)); @@ -508,9 +577,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)}; + CHECK_NAPI(NativeInfo::Link(env, ToJSObject(env, object), sentinel)); } *result = info; @@ -518,7 +586,7 @@ namespace { } static napi_status Unwrap(napi_env env, napi_value object, WrapperInfo** result) { - *result = NativeInfo::FindInPrototypeChain(env->context, ToJSObject(env, object)); + CHECK_NAPI(NativeInfo::Query(env, ToJSObject(env, object), result)); return napi_ok; } @@ -577,18 +645,41 @@ namespace { } struct napi_ref__ { - napi_ref__(napi_value value, uint32_t count) - : _value{value} - , _count{count} { - } + napi_ref__() = default; + + // Copy semantics + napi_ref__(const napi_ref__&) = delete; + napi_ref__& operator=(const napi_ref__&) = delete; + + // Move semantics + 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); + _value = value; + _count = count; - 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::GetObjectId(env, _value, &_objectId)); + if (_objectId == 0) { CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value](ReferenceInfo* info) { - info->Env()->active_ref_values.erase(value); + 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)); + assert(_objectId); + env->active_ref_values[_value] = _objectId; + } else { + assert(env->active_ref_values.find(_value) != env->active_ref_values.end()); } if (_count != 0) { @@ -608,12 +699,15 @@ 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); } @@ -623,12 +717,19 @@ struct napi_ref__ { return _count; } - napi_value value(napi_env env) const { - if (env->active_ref_values.find(_value) == env->active_ref_values.end()) { - return nullptr; + 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. + CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &objectId)); + if (objectId == _objectId) { + *result = _value; + } } - return _value; + return napi_ok; } private: @@ -644,6 +745,7 @@ struct napi_ref__ { napi_value _value{}; uint32_t _count{}; + std::uintptr_t _objectId{}; std::list::iterator _iter{}; }; @@ -654,6 +756,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); +} + // Warning: Keep in-sync with napi_status enum static const char* error_messages[] = { nullptr, @@ -1822,12 +1933,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; @@ -1887,7 +1998,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; } diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 8dc1048e..da74596e 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -3,7 +3,7 @@ #include #include #include -#include +#include #include #include #include @@ -12,22 +12,50 @@ 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::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"); } - + ~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(JSGlobalContextRef context) { + auto it = napi_envs.find(context); + if (it != napi_envs.end()) { + return it->second; + } else { + return nullptr; + } } private: + static inline std::unordered_map napi_envs{}; + void deinit_refs(); + void init_symbol(JSValueRef& symbol, const char* description); + void deinit_symbol(JSValueRef symbol); }; #define RETURN_STATUS_IF_FALSE(env, condition, status) \