From 2a14bf39f718b3171f752f3b01899147927c4c11 Mon Sep 17 00:00:00 2001 From: legendecas Date: Fri, 5 Aug 2022 15:04:39 +0800 Subject: [PATCH] node-api: generalize finalizer second pass callback --- node.gyp | 1 - src/js_native_api_v8.cc | 229 ++++++++------------------- src/js_native_api_v8.h | 153 +++++++----------- src/node_api.cc | 93 ++++++----- src/node_api_internals.h | 4 + test/cctest/test_js_native_api_v8.cc | 102 ------------ 6 files changed, 185 insertions(+), 397 deletions(-) delete mode 100644 test/cctest/test_js_native_api_v8.cc diff --git a/node.gyp b/node.gyp index 6efd75dce48ef4..fc0fd3542d86d0 100644 --- a/node.gyp +++ b/node.gyp @@ -987,7 +987,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 db0cd6eb01e4a6..1fb62bdf0403dc 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -191,10 +191,10 @@ inline static napi_status ConcludeDeferred(napi_env env, enum UnwrapAction { KeepWrap, RemoveWrap }; -inline static napi_status Unwrap(napi_env env, - napi_value js_object, - void** result, - UnwrapAction action) { +inline napi_status Unwrap(napi_env env, + napi_value js_object, + void** result, + UnwrapAction action) { NAPI_PREAMBLE(env); CHECK_ARG(env, js_object); if (action == KeepWrap) { @@ -220,7 +220,7 @@ inline static napi_status Unwrap(napi_env env, if (action == RemoveWrap) { CHECK(obj->DeletePrivate(context, NAPI_PRIVATE_KEY(context, wrapper)) .FromJust()); - Reference::Delete(reference); + delete reference; } return GET_RETURN_STATUS(env); @@ -466,11 +466,20 @@ RefBase::RefBase(napi_env env, void* finalize_data, void* finalize_hint) : Finalizer(env, finalize_callback, finalize_data, finalize_hint), - _refcount(initial_refcount), - _delete_self(delete_self) { + refcount_(initial_refcount), + delete_self_(delete_self) { 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, @@ -485,105 +494,62 @@ RefBase* RefBase::New(napi_env env, 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() { + napi_finalize finalize_callback = finalize_callback_; + bool delete_self = delete_self_; + + // 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 the flag `delete_self`. + // 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 self-destructive, userland code should delete it. + // Now delete it if it is self-destructive. + if (delete_self) { + 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, @@ -600,15 +566,6 @@ Reference* Reference::New(napi_env env, 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() { uint32_t refcount = RefBase::Ref(); if (refcount == 1) { @@ -627,25 +584,20 @@ 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); + RefBase::Finalize(); } // ClearWeak is marking the Reference so that the gc should not @@ -654,72 +606,25 @@ void Reference::Finalize(bool is_env_teardown) { // 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; + if (!persistent_.IsEmpty()) { + persistent_.ClearWeak(); } } // 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 @@ -2515,7 +2420,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); } @@ -3205,7 +3110,7 @@ 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 = diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index ac6e3f187b3078..c461e0f79ddf7e 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -1,12 +1,11 @@ #ifndef SRC_JS_NATIVE_API_V8_H_ #define SRC_JS_NATIVE_API_V8_H_ -// This file needs to be compatible with C compilers. #include // NOLINT(modernize-deprecated-headers) #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 { @@ -14,7 +13,7 @@ class RefTracker { public: RefTracker() {} virtual ~RefTracker() {} - virtual void Finalize(bool isEnvTeardown) {} + virtual void Finalize() {} typedef RefTracker RefList; @@ -40,7 +39,7 @@ class RefTracker { static void FinalizeAll(RefList* list) { while (list->next_ != nullptr) { - list->next_->Finalize(true); + list->next_->Finalize(); } } @@ -49,12 +48,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); } virtual ~napi_env__() { FinalizeAll(); } @@ -94,13 +93,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); + } + void FinalizeAll() { // First we must finalize those references that have `napi_finalizer` // callbacks. The reason is that addons might store other references which @@ -118,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; @@ -125,47 +139,18 @@ struct napi_env__ { void* instance_data = nullptr; }; -// 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; -}; - -static inline napi_status napi_clear_last_error(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; return napi_ok; } -static inline napi_status napi_set_last_error(napi_env env, - napi_status error_code, - uint32_t engine_error_code = 0, - void* engine_reserved = nullptr) { +inline napi_status napi_set_last_error(napi_env env, + napi_status error_code, + uint32_t engine_error_code = 0, + void* engine_reserved = nullptr) { env->last_error.error_code = error_code; env->last_error.engine_error_code = engine_error_code; env->last_error.engine_reserved = engine_reserved; @@ -275,6 +260,12 @@ static inline napi_status napi_set_last_error(napi_env env, #define CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, maybe, status) \ RETURN_STATUS_IF_FALSE_WITH_PREAMBLE((env), !((maybe).IsEmpty()), (status)) +#define STATUS_CALL(call) \ + do { \ + napi_status status = (call); \ + if (status != napi_ok) return status; \ + } while (0) + namespace v8impl { //=== Conversion between V8 Handles and napi_value ======================== @@ -296,49 +287,35 @@ 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_; } 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 { @@ -355,8 +332,8 @@ class TryCatch : public v8::TryCatch { napi_env _env; }; -// Wrapper around v8impl::Persistent that implements reference counting. -class RefBase : protected Finalizer, RefTracker { +// Wrapper around Finalizer that implements reference counting. +class RefBase : public Finalizer, public RefTracker { protected: RefBase(napi_env env, uint32_t initial_refcount, @@ -372,26 +349,23 @@ class RefBase : protected Finalizer, RefTracker { 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(); protected: - void Finalize(bool is_env_teardown = false) override; + void Finalize() override; private: - uint32_t _refcount; - bool _delete_self; + uint32_t refcount_; + bool delete_self_; }; +// Wrapper around v8impl::Persistent. class Reference : public RefBase { - using SecondPassCallParameterRef = Reference*; - protected: template Reference(napi_env env, v8::Local value, Args&&... args); @@ -411,30 +385,19 @@ class Reference : public RefBase { v8::Local Get(); protected: - void Finalize(bool is_env_teardown = false) override; + void Finalize() override; private: + static void WeakCallback(const v8::WeakCallbackInfo& data); + void ClearWeak(); void SetWeak(); - static void FinalizeCallback( - const v8::WeakCallbackInfo& data); - static void SecondPassCallback( - const v8::WeakCallbackInfo& data); - - v8impl::Persistent _persistent; - SecondPassCallParameterRef* _secondPassParameter; - bool _secondPassScheduled; + v8impl::Persistent persistent_; FRIEND_TEST(JsNativeApiV8Test, Reference); }; } // end of namespace v8impl -#define STATUS_CALL(call) \ - do { \ - napi_status status = (call); \ - if (status != napi_ok) return status; \ - } while (0) - #endif // SRC_JS_NATIVE_API_V8_H_ diff --git a/src/node_api.cc b/src/node_api.cc index 08352b0a584b54..48d272609b7126 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -26,6 +26,7 @@ node_napi_env__::node_napi_env__(v8::Local context, node_napi_env__::~node_napi_env__() { destructing = true; + DrainFinalizerQueue(); FinalizeAll(); } @@ -46,26 +47,34 @@ 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); + if (!finalization_scheduled) { + 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) { @@ -105,23 +114,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 same 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(); } }; static inline napi_env NewEnv(v8::Local context, @@ -370,10 +396,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(); } @@ -952,12 +975,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 6478520fd55da6..8957ce81a1dfe7 100644 --- a/src/node_api_internals.h +++ b/src/node_api_internals.h @@ -20,6 +20,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); @@ -31,6 +34,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