From 1494175000625f4e3c5b9aff2c10378b1cb104ef Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 20 Dec 2022 01:43:39 +0800 Subject: [PATCH] node-api: generalize finalizer second pass callback Generalize the finalizer's second pass callback to make it cancellable and simplify the code around the second pass callback. With this change, it is determined that Reference::Finalize or RefBase::Finalize are called once, either from the env's shutdown, or from the env's second pass callback. All existing node-api js tests should pass without a touch. The js_native_api cctest is no longer applicable with this change, just removing it. PR-URL: https://github.com/nodejs/node/pull/44141 Refs: https://github.com/nodejs/node/issues/44071 Reviewed-By: Michael Dawson --- node.gyp | 1 - src/js_native_api_v8.cc | 297 +++++++----------- src/js_native_api_v8.h | 153 ++++----- src/node_api.cc | 97 +++--- src/node_api_internals.h | 4 + test/cctest/test_js_native_api_v8.cc | 102 ------ .../6_object_wrap/6_object_wrap.cc | 63 +++- .../6_object_wrap/test-object-wrap-ref.js | 13 + 8 files changed, 317 insertions(+), 413 deletions(-) delete mode 100644 test/cctest/test_js_native_api_v8.cc create mode 100644 test/js-native-api/6_object_wrap/test-object-wrap-ref.js diff --git a/node.gyp b/node.gyp index 448cb8a8c7cd49..4ef15c6248df4f 100644 --- a/node.gyp +++ b/node.gyp @@ -1003,7 +1003,6 @@ 'test/cctest/test_base_object_ptr.cc', 'test/cctest/test_node_postmortem_metadata.cc', 'test/cctest/test_environment.cc', - 'test/cctest/test_js_native_api_v8.cc', 'test/cctest/test_linked_binding.cc', 'test/cctest/test_node_api.cc', 'test/cctest/test_per_process.cc', diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index abc9e56d6b0bbb..5365c9ceb23a83 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -218,7 +218,12 @@ inline napi_status Unwrap(napi_env env, if (action == RemoveWrap) { CHECK(obj->DeletePrivate(context, NAPI_PRIVATE_KEY(context, wrapper)) .FromJust()); - Reference::Delete(reference); + if (reference->ownership() == Ownership::kUserland) { + // When the wrap is been removed, the finalizer should be reset. + reference->ResetFinalizer(); + } else { + delete reference; + } } return GET_RETURN_STATUS(env); @@ -245,7 +250,8 @@ class CallbackBundle { bundle->env = env; v8::Local cbdata = v8::External::New(env->isolate, bundle); - Reference::New(env, cbdata, 0, true, Delete, bundle, nullptr); + Reference::New( + env, cbdata, 0, Ownership::kRuntime, Delete, bundle, nullptr); return cbdata; } napi_env env; // Necessary to invoke C++ NAPI callback @@ -429,8 +435,13 @@ inline napi_status Wrap(napi_env env, // before then, then the finalize callback will never be invoked.) // Therefore a finalize callback is required when returning a reference. CHECK_ARG(env, finalize_cb); - reference = v8impl::Reference::New( - env, obj, 0, false, finalize_cb, native_object, finalize_hint); + reference = v8impl::Reference::New(env, + obj, + 0, + v8impl::Ownership::kUserland, + finalize_cb, + native_object, + finalize_hint); *result = reinterpret_cast(reference); } else { // Create a self-deleting reference. @@ -438,7 +449,7 @@ inline napi_status Wrap(napi_env env, env, obj, 0, - true, + v8impl::Ownership::kRuntime, finalize_cb, native_object, finalize_cb == nullptr ? nullptr : finalize_hint); @@ -456,166 +467,144 @@ inline napi_status Wrap(napi_env env, } // end of anonymous namespace +void Finalizer::ResetFinalizer() { + finalize_callback_ = nullptr; + finalize_data_ = nullptr; + finalize_hint_ = nullptr; +} + // Wrapper around v8impl::Persistent that implements reference counting. RefBase::RefBase(napi_env env, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint) : Finalizer(env, finalize_callback, finalize_data, finalize_hint), - _refcount(initial_refcount), - _delete_self(delete_self) { + refcount_(initial_refcount), + ownership_(ownership) { Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist); } +// When a RefBase is being deleted, it may have been queued to call its +// finalizer. +RefBase::~RefBase() { + // Remove from the env's tracked list. + Unlink(); + // Try to remove the finalizer from the scheduled second pass callback. + env_->DequeueFinalizer(this); +} + RefBase* RefBase::New(napi_env env, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint) { return new RefBase(env, initial_refcount, - delete_self, + ownership, finalize_callback, finalize_data, finalize_hint); } -RefBase::~RefBase() { - Unlink(); -} - void* RefBase::Data() { - return _finalize_data; -} - -// Delete is called in 2 ways. Either from the finalizer or -// from one of Unwrap or napi_delete_reference. -// -// When it is called from Unwrap or napi_delete_reference we only -// want to do the delete if the finalizer has already run or -// cannot have been queued to run (ie the reference count is > 0), -// otherwise we may crash when the finalizer does run. -// If the finalizer may have been queued and has not already run -// delay the delete until the finalizer runs by not doing the delete -// and setting _delete_self to true so that the finalizer will -// delete it when it runs. -// -// The second way this is called is from -// the finalizer and _delete_self is set. In this case we -// know we need to do the deletion so just do it. -void RefBase::Delete(RefBase* reference) { - if ((reference->RefCount() != 0) || (reference->_delete_self) || - (reference->_finalize_ran)) { - delete reference; - } else { - // defer until finalizer runs as - // it may already be queued - reference->_delete_self = true; - } + return finalize_data_; } uint32_t RefBase::Ref() { - return ++_refcount; + return ++refcount_; } uint32_t RefBase::Unref() { - if (_refcount == 0) { + if (refcount_ == 0) { return 0; } - return --_refcount; + return --refcount_; } uint32_t RefBase::RefCount() { - return _refcount; -} - -void RefBase::Finalize(bool is_env_teardown) { - // In addition to being called during environment teardown, this method is - // also the entry point for the garbage collector. During environment - // teardown we have to remove the garbage collector's reference to this - // method so that, if, as part of the user's callback, JS gets executed, - // resulting in a garbage collection pass, this method is not re-entered as - // part of that pass, because that'll cause a double free (as seen in - // https://github.com/nodejs/node/issues/37236). - // - // Since this class does not have access to the V8 persistent reference, - // this method is overridden in the `Reference` class below. Therein the - // weak callback is removed, ensuring that the garbage collector does not - // re-enter this method, and the method chains up to continue the process of - // environment-teardown-induced finalization. - - // During environment teardown we have to convert a strong reference to - // a weak reference to force the deferring behavior if the user's finalizer - // happens to delete this reference so that the code in this function that - // follows the call to the user's finalizer may safely access variables from - // this instance. - if (is_env_teardown && RefCount() > 0) _refcount = 0; - - if (_finalize_callback != nullptr) { - // This ensures that we never call the finalizer twice. - napi_finalize fini = _finalize_callback; - _finalize_callback = nullptr; - _env->CallFinalizer(fini, _finalize_data, _finalize_hint); - } - - // this is safe because if a request to delete the reference - // is made in the finalize_callback it will defer deletion - // to this block and set _delete_self to true - if (_delete_self || is_env_teardown) { - Delete(this); - } else { - _finalize_ran = true; + return refcount_; +} + +void RefBase::Finalize() { + Ownership ownership = ownership_; + // Swap out the field finalize_callback so that it can not be accidentally + // called more than once. + napi_finalize finalize_callback = finalize_callback_; + void* finalize_data = finalize_data_; + void* finalize_hint = finalize_hint_; + ResetFinalizer(); + + // Either the RefBase is going to be deleted in the finalize_callback or not, + // it should be removed from the tracked list. + Unlink(); + // 1. If the finalize_callback is present, it should either delete the + // RefBase, or set ownership with Ownership::kRuntime. + // 2. If the finalizer is not present, the RefBase can be deleted after the + // call. + if (finalize_callback != nullptr) { + env_->CallFinalizer(finalize_callback, finalize_data, finalize_hint); + // No access to `this` after finalize_callback is called. + } + + // If the RefBase is not Ownership::kRuntime, userland code should delete it. + // Now delete it if it is Ownership::kRuntime. + if (ownership == Ownership::kRuntime) { + delete this; } } template Reference::Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), - _persistent(env->isolate, value), - _secondPassParameter(new SecondPassCallParameterRef(this)), - _secondPassScheduled(false) { + persistent_(env->isolate, value) { if (RefCount() == 0) { SetWeak(); } } +Reference::~Reference() { + // Reset the handle. And no weak callback will be invoked. + persistent_.Reset(); +} + Reference* Reference::New(napi_env env, v8::Local value, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint) { return new Reference(env, value, initial_refcount, - delete_self, + ownership, finalize_callback, finalize_data, finalize_hint); } -Reference::~Reference() { - // If the second pass callback is scheduled, it will delete the - // parameter passed to it, otherwise it will never be scheduled - // and we need to delete it here. - if (!_secondPassScheduled) { - delete _secondPassParameter; - } -} - uint32_t Reference::Ref() { + // When the persistent_ is cleared in the WeakCallback, and a second pass + // callback is pending, return 0 unconditionally. + if (persistent_.IsEmpty()) { + return 0; + } uint32_t refcount = RefBase::Ref(); if (refcount == 1) { - ClearWeak(); + persistent_.ClearWeak(); } return refcount; } uint32_t Reference::Unref() { + // When the persistent_ is cleared in the WeakCallback, and a second pass + // callback is pending, return 0 unconditionally. + if (persistent_.IsEmpty()) { + return 0; + } uint32_t old_refcount = RefCount(); uint32_t refcount = RefBase::Unref(); if (old_refcount == 1 && refcount == 0) { @@ -625,99 +614,36 @@ uint32_t Reference::Unref() { } v8::Local Reference::Get() { - if (_persistent.IsEmpty()) { + if (persistent_.IsEmpty()) { return v8::Local(); } else { - return v8::Local::New(_env->isolate, _persistent); + return v8::Local::New(env_->isolate, persistent_); } } -void Reference::Finalize(bool is_env_teardown) { - // During env teardown, `~napi_env()` alone is responsible for finalizing. - // Thus, we don't want any stray gc passes to trigger a second call to - // `RefBase::Finalize()`. ClearWeak will ensure that even if the - // gc is in progress no Finalization will be run for this Reference - // by the gc. - if (is_env_teardown) { - ClearWeak(); - } +void Reference::Finalize() { + // Unconditionally reset the persistent handle so that no weak callback will + // be invoked again. + persistent_.Reset(); // Chain up to perform the rest of the finalization. - RefBase::Finalize(is_env_teardown); -} - -// ClearWeak is marking the Reference so that the gc should not -// collect it, but it is possible that a second pass callback -// may have been scheduled already if we are in shutdown. We clear -// the secondPassParameter so that even if it has been -// scheduled no Finalization will be run. -void Reference::ClearWeak() { - if (!_persistent.IsEmpty()) { - _persistent.ClearWeak(); - } - if (_secondPassParameter != nullptr) { - *_secondPassParameter = nullptr; - } + RefBase::Finalize(); } // Mark the reference as weak and eligible for collection // by the gc. void Reference::SetWeak() { - if (_secondPassParameter == nullptr) { - // This means that the Reference has already been processed - // by the second pass callback, so its already been Finalized, do - // nothing - return; - } - _persistent.SetWeak( - _secondPassParameter, FinalizeCallback, v8::WeakCallbackType::kParameter); - *_secondPassParameter = this; + persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); } // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does -// not support calls back into it. However, it provides a mechanism for adding -// a finalizer which may make calls back into the engine by allowing us to -// attach such a second-pass finalizer from the first pass finalizer. Thus, -// we do that here to ensure that the N-API finalizer callback is free to call -// into the engine. -void Reference::FinalizeCallback( - const v8::WeakCallbackInfo& data) { - SecondPassCallParameterRef* parameter = data.GetParameter(); - Reference* reference = *parameter; - if (reference == nullptr) { - return; - } - - // The reference must be reset during the first pass. - reference->_persistent.Reset(); - // Mark the parameter not delete-able until the second pass callback is - // invoked. - reference->_secondPassScheduled = true; - - data.SetSecondPassCallback(SecondPassCallback); -} - -// Second pass callbacks are scheduled with platform tasks. At env teardown, -// the tasks may have already be scheduled and we are unable to cancel the -// second pass callback task. We have to make sure that parameter is kept -// alive until the second pass callback is been invoked. In order to do -// this and still allow our code to Finalize/delete the Reference during -// shutdown we have to use a separately allocated parameter instead -// of a parameter within the Reference object itself. This is what -// is stored in _secondPassParameter and it is allocated in the -// constructor for the Reference. -void Reference::SecondPassCallback( - const v8::WeakCallbackInfo& data) { - SecondPassCallParameterRef* parameter = data.GetParameter(); - Reference* reference = *parameter; - delete parameter; - if (reference == nullptr) { - // the reference itself has already been deleted so nothing to do - return; - } - reference->_secondPassParameter = nullptr; - reference->Finalize(); +// not support calls back into it. Enqueue the invocation of the finalizer. +void Reference::WeakCallback(const v8::WeakCallbackInfo& data) { + Reference* reference = data.GetParameter(); + // The reference must be reset during the weak callback as the API protocol. + reference->persistent_.Reset(); + reference->env_->EnqueueFinalizer(reference); } } // end of namespace v8impl @@ -2391,8 +2317,13 @@ napi_status NAPI_CDECL napi_create_external(napi_env env, if (finalize_cb) { // The Reference object will delete itself after invoking the finalizer // callback. - v8impl::Reference::New( - env, external_value, 0, true, finalize_cb, data, finalize_hint); + v8impl::Reference::New(env, + external_value, + 0, + v8impl::Ownership::kRuntime, + finalize_cb, + data, + finalize_hint); } *result = v8impl::JsValueFromV8LocalValue(external_value); @@ -2501,8 +2432,8 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env, return napi_set_last_error(env, napi_invalid_arg); } - v8impl::Reference* reference = - v8impl::Reference::New(env, v8_value, initial_refcount, false); + v8impl::Reference* reference = v8impl::Reference::New( + env, v8_value, initial_refcount, v8impl::Ownership::kUserland); *result = reinterpret_cast(reference); return napi_clear_last_error(env); @@ -2516,7 +2447,7 @@ napi_status NAPI_CDECL napi_delete_reference(napi_env env, napi_ref ref) { CHECK_ENV(env); CHECK_ARG(env, ref); - v8impl::Reference::Delete(reinterpret_cast(ref)); + delete reinterpret_cast(ref); return napi_clear_last_error(env); } @@ -3206,11 +3137,11 @@ napi_status NAPI_CDECL napi_set_instance_data(napi_env env, if (old_data != nullptr) { // Our contract so far has been to not finalize any old data there may be. // So we simply delete it. - v8impl::RefBase::Delete(old_data); + delete old_data; } - env->instance_data = - v8impl::RefBase::New(env, 0, true, finalize_cb, data, finalize_hint); + env->instance_data = v8impl::RefBase::New( + env, 0, v8impl::Ownership::kRuntime, finalize_cb, data, finalize_hint); return napi_clear_last_error(env); } diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 766398744c5dfb..b3d0446cb5239f 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -4,7 +4,7 @@ #include "js_native_api_types.h" #include "js_native_api_v8_internals.h" -static napi_status napi_clear_last_error(napi_env env); +inline napi_status napi_clear_last_error(napi_env env); namespace v8impl { @@ -12,7 +12,7 @@ class RefTracker { public: RefTracker() {} virtual ~RefTracker() {} - virtual void Finalize(bool isEnvTeardown) {} + virtual void Finalize() {} typedef RefTracker RefList; @@ -38,7 +38,7 @@ class RefTracker { static void FinalizeAll(RefList* list) { while (list->next_ != nullptr) { - list->next_->Finalize(true); + list->next_->Finalize(); } } @@ -47,12 +47,12 @@ class RefTracker { RefList* prev_ = nullptr; }; +class Finalizer; } // end of namespace v8impl struct napi_env__ { explicit napi_env__(v8::Local context) : isolate(context->GetIsolate()), context_persistent(isolate, context) { - CHECK_EQ(isolate, context->GetIsolate()); napi_clear_last_error(this); } @@ -89,13 +89,26 @@ struct napi_env__ { } } - // This should be overridden to schedule the finalization to a properiate - // timing, like next tick of the event loop. + // Call finalizer immediately. virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) { v8::HandleScope handle_scope(isolate); CallIntoModule([&](napi_env env) { cb(env, data, hint); }); } + // Enqueue the finalizer to the napi_env's own queue of the second pass + // weak callback. + // Implementation should drain the queue at the time it is safe to call + // into JavaScript. + virtual void EnqueueFinalizer(v8impl::RefTracker* finalizer) { + pending_finalizers.emplace(finalizer); + } + + // Remove the finalizer from the scheduled second pass weak callback queue. + // The finalizer can be deleted after this call. + virtual void DequeueFinalizer(v8impl::RefTracker* finalizer) { + pending_finalizers.erase(finalizer); + } + virtual void DeleteMe() { // First we must finalize those references that have `napi_finalizer` // callbacks. The reason is that addons might store other references which @@ -117,6 +130,8 @@ struct napi_env__ { // have such a callback. See `~napi_env__()` above for details. v8impl::RefTracker::RefList reflist; v8impl::RefTracker::RefList finalizing_reflist; + // The invocation order of the finalizers is not determined. + std::unordered_set pending_finalizers; napi_extended_error_info last_error; int open_handle_scopes = 0; int open_callback_scopes = 0; @@ -129,37 +144,8 @@ struct napi_env__ { virtual ~napi_env__() = default; }; -// This class is used to keep a napi_env live in a way that -// is exception safe versus calling Ref/Unref directly -class EnvRefHolder { - public: - explicit EnvRefHolder(napi_env env) : _env(env) { _env->Ref(); } - - explicit EnvRefHolder(const EnvRefHolder& other) : _env(other.env()) { - _env->Ref(); - } - - EnvRefHolder(EnvRefHolder&& other) { - _env = other._env; - other._env = nullptr; - } - - ~EnvRefHolder() { - if (_env != nullptr) { - _env->Unref(); - } - } - - napi_env env(void) const { return _env; } - - private: - napi_env _env; -}; - inline napi_status napi_clear_last_error(napi_env env) { env->last_error.error_code = napi_ok; - - // TODO(boingoing): Should this be a callback? env->last_error.engine_error_code = 0; env->last_error.engine_reserved = nullptr; env->last_error.error_message = nullptr; @@ -306,49 +292,37 @@ inline v8::Local V8LocalValueFromJsValue(napi_value v) { // Adapter for napi_finalize callbacks. class Finalizer { - public: - // Some Finalizers are run during shutdown when the napi_env is destroyed, - // and some need to keep an explicit reference to the napi_env because they - // are run independently. - enum EnvReferenceMode { kNoEnvReference, kKeepEnvReference }; - protected: Finalizer(napi_env env, napi_finalize finalize_callback, void* finalize_data, - void* finalize_hint, - EnvReferenceMode refmode = kNoEnvReference) - : _env(env), - _finalize_callback(finalize_callback), - _finalize_data(finalize_data), - _finalize_hint(finalize_hint), - _has_env_reference(refmode == kKeepEnvReference) { - if (_has_env_reference) _env->Ref(); - } + void* finalize_hint) + : env_(env), + finalize_callback_(finalize_callback), + finalize_data_(finalize_data), + finalize_hint_(finalize_hint) {} - ~Finalizer() { - if (_has_env_reference) _env->Unref(); - } + virtual ~Finalizer() = default; public: static Finalizer* New(napi_env env, napi_finalize finalize_callback = nullptr, void* finalize_data = nullptr, - void* finalize_hint = nullptr, - EnvReferenceMode refmode = kNoEnvReference) { - return new Finalizer( - env, finalize_callback, finalize_data, finalize_hint, refmode); + void* finalize_hint = nullptr) { + return new Finalizer(env, finalize_callback, finalize_data, finalize_hint); } - static void Delete(Finalizer* finalizer) { delete finalizer; } + napi_finalize callback() { return finalize_callback_; } + void* data() { return finalize_data_; } + void* hint() { return finalize_hint_; } + + void ResetFinalizer(); protected: - napi_env _env; - napi_finalize _finalize_callback; - void* _finalize_data; - void* _finalize_hint; - bool _finalize_ran = false; - bool _has_env_reference = false; + napi_env env_; + napi_finalize finalize_callback_; + void* finalize_data_; + void* finalize_hint_; }; class TryCatch : public v8::TryCatch { @@ -365,12 +339,22 @@ class TryCatch : public v8::TryCatch { napi_env _env; }; -// Wrapper around v8impl::Persistent that implements reference counting. -class RefBase : protected Finalizer, RefTracker { +// Ownership of a reference. +enum class Ownership { + // The reference is owned by the runtime. No userland call is needed to + // destruct the reference. + kRuntime, + // The reference is owned by the userland. User code is responsible to delete + // the reference with appropriate node-api calls. + kUserland, +}; + +// Wrapper around Finalizer that implements reference counting. +class RefBase : public Finalizer, public RefTracker { protected: RefBase(napi_env env, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint); @@ -378,30 +362,29 @@ class RefBase : protected Finalizer, RefTracker { public: static RefBase* New(napi_env env, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint); - - static inline void Delete(RefBase* reference); - virtual ~RefBase(); + void* Data(); uint32_t Ref(); uint32_t Unref(); uint32_t RefCount(); + Ownership ownership() { return ownership_; } + protected: - void Finalize(bool is_env_teardown = false) override; + void Finalize() override; private: - uint32_t _refcount; - bool _delete_self; + uint32_t refcount_; + Ownership ownership_; }; +// Wrapper around v8impl::Persistent. class Reference : public RefBase { - using SecondPassCallParameterRef = Reference*; - protected: template Reference(napi_env env, v8::Local value, Args&&... args); @@ -410,7 +393,7 @@ class Reference : public RefBase { static Reference* New(napi_env env, v8::Local value, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback = nullptr, void* finalize_data = nullptr, void* finalize_hint = nullptr); @@ -421,22 +404,14 @@ class Reference : public RefBase { v8::Local Get(); protected: - void Finalize(bool is_env_teardown = false) override; + void Finalize() override; private: - void ClearWeak(); - void SetWeak(); + static void WeakCallback(const v8::WeakCallbackInfo& data); - static void FinalizeCallback( - const v8::WeakCallbackInfo& data); - static void SecondPassCallback( - const v8::WeakCallbackInfo& data); - - v8impl::Persistent _persistent; - SecondPassCallParameterRef* _secondPassParameter; - bool _secondPassScheduled; + void SetWeak(); - FRIEND_TEST(JsNativeApiV8Test, Reference); + v8impl::Persistent persistent_; }; } // end of namespace v8impl diff --git a/src/node_api.cc b/src/node_api.cc index 8f1e4fb0e40d4a..c3f78c8b58bc27 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -27,6 +27,7 @@ node_napi_env__::node_napi_env__(v8::Local context, void node_napi_env__::DeleteMe() { destructing = true; + DrainFinalizerQueue(); napi_env__::DeleteMe(); } @@ -47,26 +48,38 @@ void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) { template void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) { - if (destructing) { - // we can not defer finalizers when the environment is being destructed. - v8::HandleScope handle_scope(isolate); - v8::Context::Scope context_scope(context()); - CallbackIntoModule( - [&](napi_env env) { cb(env, data, hint); }); - return; + v8::HandleScope handle_scope(isolate); + v8::Context::Scope context_scope(context()); + CallbackIntoModule( + [&](napi_env env) { cb(env, data, hint); }); +} + +void node_napi_env__::EnqueueFinalizer(v8impl::RefTracker* finalizer) { + napi_env__::EnqueueFinalizer(finalizer); + // Schedule a second pass only when it has not been scheduled, and not + // destructing the env. + // When the env is being destructed, queued finalizers are drained in the + // loop of `node_napi_env__::DrainFinalizerQueue`. + if (!finalization_scheduled && !destructing) { + finalization_scheduled = true; + Ref(); + node_env()->SetImmediate([this](node::Environment* node_env) { + finalization_scheduled = false; + Unref(); + DrainFinalizerQueue(); + }); + } +} + +void node_napi_env__::DrainFinalizerQueue() { + // As userland code can delete additional references in one finalizer, + // the list of pending finalizers may be mutated as we execute them, so + // we keep iterating it until it is empty. + while (!pending_finalizers.empty()) { + v8impl::RefTracker* ref_tracker = *pending_finalizers.begin(); + pending_finalizers.erase(ref_tracker); + ref_tracker->Finalize(); } - // we need to keep the env live until the finalizer has been run - // EnvRefHolder provides an exception safe wrapper to Ref and then - // Unref once the lambda is freed - EnvRefHolder liveEnv(static_cast(this)); - node_env()->SetImmediate( - [=, liveEnv = std::move(liveEnv)](node::Environment* node_env) { - node_napi_env__* env = static_cast(liveEnv.env()); - v8::HandleScope handle_scope(env->isolate); - v8::Context::Scope context_scope(env->context()); - env->CallbackIntoModule( - [&](napi_env env) { cb(env, data, hint); }); - }); } void node_napi_env__::trigger_fatal_exception(v8::Local local_err) { @@ -106,23 +119,40 @@ namespace { class BufferFinalizer : private Finalizer { public: + static BufferFinalizer* New(napi_env env, + napi_finalize finalize_callback = nullptr, + void* finalize_data = nullptr, + void* finalize_hint = nullptr) { + return new BufferFinalizer( + env, finalize_callback, finalize_data, finalize_hint); + } // node::Buffer::FreeCallback static void FinalizeBufferCallback(char* data, void* hint) { std::unique_ptr finalizer{ static_cast(hint)}; - finalizer->_finalize_data = data; + finalizer->finalize_data_ = data; - if (finalizer->_finalize_callback == nullptr) return; - finalizer->_env->CallFinalizer(finalizer->_finalize_callback, - finalizer->_finalize_data, - finalizer->_finalize_hint); + // It is safe to call into JavaScript at this point. + if (finalizer->finalize_callback_ == nullptr) return; + finalizer->env_->CallFinalizer(finalizer->finalize_callback_, + finalizer->finalize_data_, + finalizer->finalize_hint_); } struct Deleter { - void operator()(BufferFinalizer* finalizer) { - Finalizer::Delete(finalizer); - } + void operator()(BufferFinalizer* finalizer) { delete finalizer; } }; + + private: + BufferFinalizer(napi_env env, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) + : Finalizer(env, finalize_callback, finalize_data, finalize_hint) { + env_->Ref(); + } + + ~BufferFinalizer() { env_->Unref(); } }; inline napi_env NewEnv(v8::Local context, @@ -371,10 +401,7 @@ class ThreadSafeFunction : public node::AsyncResource { v8::HandleScope scope(env->isolate); if (finalize_cb) { CallbackScope cb_scope(this); - // Do not use CallFinalizer since it will defer the invocation, which - // would lead to accessing a deleted ThreadSafeFunction. - env->CallbackIntoModule( - [&](napi_env env) { finalize_cb(env, finalize_data, context); }); + env->CallFinalizer(finalize_cb, finalize_data, context); } EmptyQueueAndDelete(); } @@ -959,12 +986,8 @@ napi_status NAPI_CDECL napi_create_external_buffer(napi_env env, v8::Isolate* isolate = env->isolate; // The finalizer object will delete itself after invoking the callback. - v8impl::Finalizer* finalizer = - v8impl::Finalizer::New(env, - finalize_cb, - nullptr, - finalize_hint, - v8impl::Finalizer::kKeepEnvReference); + v8impl::BufferFinalizer* finalizer = + v8impl::BufferFinalizer::New(env, finalize_cb, nullptr, finalize_hint); v8::MaybeLocal maybe = node::Buffer::New(isolate, diff --git a/src/node_api_internals.h b/src/node_api_internals.h index de5d9dc0804367..8b4db661e65fde 100644 --- a/src/node_api_internals.h +++ b/src/node_api_internals.h @@ -19,6 +19,9 @@ struct node_napi_env__ : public napi_env__ { template void CallFinalizer(napi_finalize cb, void* data, void* hint); + void EnqueueFinalizer(v8impl::RefTracker* finalizer) override; + void DrainFinalizerQueue(); + void trigger_fatal_exception(v8::Local local_err); template void CallbackIntoModule(T&& call); @@ -32,6 +35,7 @@ struct node_napi_env__ : public napi_env__ { std::string filename; bool destructing = false; + bool finalization_scheduled = false; }; using node_napi_env = node_napi_env__*; diff --git a/test/cctest/test_js_native_api_v8.cc b/test/cctest/test_js_native_api_v8.cc deleted file mode 100644 index 06de770089d9e6..00000000000000 --- a/test/cctest/test_js_native_api_v8.cc +++ /dev/null @@ -1,102 +0,0 @@ -#include -#include -#include -#include "env-inl.h" -#include "gtest/gtest.h" -#include "js_native_api_v8.h" -#include "node_api_internals.h" -#include "node_binding.h" -#include "node_test_fixture.h" - -namespace v8impl { - -using v8::Local; -using v8::Object; - -static napi_env addon_env; -static uint32_t finalizer_call_count = 0; - -class JsNativeApiV8Test : public EnvironmentTestFixture { - private: - void SetUp() override { - EnvironmentTestFixture::SetUp(); - finalizer_call_count = 0; - } - - void TearDown() override { NodeTestFixture::TearDown(); } -}; - -TEST_F(JsNativeApiV8Test, Reference) { - const v8::HandleScope handle_scope(isolate_); - Argv argv; - - napi_ref ref; - void* embedder_fields[v8::kEmbedderFieldsInWeakCallback] = {nullptr, nullptr}; - v8::WeakCallbackInfo::Callback - callback; - Reference::SecondPassCallParameterRef* parameter = nullptr; - - { - Env test_env{handle_scope, argv}; - - node::Environment* env = *test_env; - node::LoadEnvironment(env, ""); - - napi_addon_register_func init = [](napi_env env, napi_value exports) { - addon_env = env; - return exports; - }; - Local module_obj = Object::New(isolate_); - Local exports_obj = Object::New(isolate_); - napi_module_register_by_symbol( - exports_obj, module_obj, env->context(), init); - ASSERT_NE(addon_env, nullptr); - node_napi_env internal_env = reinterpret_cast(addon_env); - EXPECT_EQ(internal_env->node_env(), env); - - // Create a new scope to manage the handles. - { - const v8::HandleScope handle_scope(isolate_); - napi_value value; - napi_create_object(addon_env, &value); - // Create a weak reference; - napi_add_finalizer( - addon_env, - value, - nullptr, - [](napi_env env, void* finalize_data, void* finalize_hint) { - finalizer_call_count++; - }, - nullptr, - &ref); - parameter = reinterpret_cast(ref)->_secondPassParameter; - } - - // We can hardly trigger a non-forced Garbage Collection in a stable way. - // Here we just invoke the weak callbacks directly. - // The persistent handles should be reset in the weak callback in respect - // to the API contract of v8 weak callbacks. - v8::WeakCallbackInfo data( - reinterpret_cast(isolate_), - parameter, - embedder_fields, - &callback); - Reference::FinalizeCallback(data); - EXPECT_EQ(callback, &Reference::SecondPassCallback); - } - // Env goes out of scope, the environment has been teardown. All node-api ref - // trackers should have been destroyed. - - // Now we call the second pass callback to verify the method do not abort with - // memory violations. - v8::WeakCallbackInfo data( - reinterpret_cast(isolate_), - parameter, - embedder_fields, - nullptr); - Reference::SecondPassCallback(data); - - // After Environment Teardown - EXPECT_EQ(finalizer_call_count, uint32_t(1)); -} -} // namespace v8impl diff --git a/test/js-native-api/6_object_wrap/6_object_wrap.cc b/test/js-native-api/6_object_wrap/6_object_wrap.cc index 7f8fdd341673e6..7ebe711a6dccf1 100644 --- a/test/js-native-api/6_object_wrap/6_object_wrap.cc +++ b/test/js-native-api/6_object_wrap/6_object_wrap.cc @@ -1,5 +1,6 @@ -#include "myobject.h" #include "../common.h" +#include "assert.h" +#include "myobject.h" napi_ref MyObject::constructor; @@ -151,9 +152,69 @@ napi_value MyObject::Multiply(napi_env env, napi_callback_info info) { return instance; } +// This finalizer should never be invoked. +void ObjectWrapDanglingReferenceFinalizer(napi_env env, + void* finalize_data, + void* finalize_hint) { + assert(0 && "unreachable"); +} + +napi_ref dangling_ref; +napi_value ObjectWrapDanglingReference(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value args[1]; + NODE_API_CALL(env, + napi_get_cb_info(env, info, &argc, args, nullptr, nullptr)); + + // Create a napi_wrap and remove it immediately, whilst leaving the out-param + // ref dangling (not deleted). + NODE_API_CALL(env, + napi_wrap(env, + args[0], + nullptr, + ObjectWrapDanglingReferenceFinalizer, + nullptr, + &dangling_ref)); + NODE_API_CALL(env, napi_remove_wrap(env, args[0], nullptr)); + + return args[0]; +} + +napi_value ObjectWrapDanglingReferenceTest(napi_env env, + napi_callback_info info) { + napi_value out; + napi_value ret; + NODE_API_CALL(env, napi_get_reference_value(env, dangling_ref, &out)); + + if (out == nullptr) { + // If the napi_ref has been invalidated, delete it. + NODE_API_CALL(env, napi_delete_reference(env, dangling_ref)); + NODE_API_CALL(env, napi_get_boolean(env, true, &ret)); + } else { + // The dangling napi_ref is still valid. + NODE_API_CALL(env, napi_get_boolean(env, false, &ret)); + } + return ret; +} + EXTERN_C_START napi_value Init(napi_env env, napi_value exports) { MyObject::Init(env, exports); + + napi_property_descriptor descriptors[] = { + DECLARE_NODE_API_PROPERTY("objectWrapDanglingReference", + ObjectWrapDanglingReference), + DECLARE_NODE_API_PROPERTY("objectWrapDanglingReferenceTest", + ObjectWrapDanglingReferenceTest), + }; + + NODE_API_CALL( + env, + napi_define_properties(env, + exports, + sizeof(descriptors) / sizeof(*descriptors), + descriptors)); + return exports; } EXTERN_C_END diff --git a/test/js-native-api/6_object_wrap/test-object-wrap-ref.js b/test/js-native-api/6_object_wrap/test-object-wrap-ref.js new file mode 100644 index 00000000000000..81832c195c1890 --- /dev/null +++ b/test/js-native-api/6_object_wrap/test-object-wrap-ref.js @@ -0,0 +1,13 @@ +// Flags: --expose-gc + +'use strict'; +const common = require('../../common'); +const addon = require(`./build/${common.buildType}/6_object_wrap`); + +(function scope() { + addon.objectWrapDanglingReference({}); +})(); + +common.gcUntil('object-wrap-ref', () => { + return addon.objectWrapDanglingReferenceTest(); +});