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(); +});