diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 54bcf2e9715a0a..2aeb96a17a599a 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -314,16 +314,16 @@ class RefBase : protected Finalizer, RefTracker { }; class Reference : public RefBase { + using SecondPassCallParameterRef = Reference*; + protected: template - Reference(napi_env env, - v8::Local value, - Args&&... args) + Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), - _persistent(env->isolate, value) { + _persistent(env->isolate, value), + _secondPassParameter(new SecondPassCallParameterRef(this)) { if (RefCount() == 0) { - _persistent.SetWeak( - this, FinalizeCallback, v8::WeakCallbackType::kParameter); + SetWeak(); } } @@ -344,10 +344,19 @@ class Reference : public RefBase { finalize_hint); } + virtual ~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 (_secondPassParameter != nullptr) { + delete _secondPassParameter; + } + } + inline uint32_t Ref() { uint32_t refcount = RefBase::Ref(); if (refcount == 1) { - _persistent.ClearWeak(); + ClearWeak(); } return refcount; } @@ -356,8 +365,7 @@ class Reference : public RefBase { uint32_t old_refcount = RefCount(); uint32_t refcount = RefBase::Unref(); if (old_refcount == 1 && refcount == 0) { - _persistent.SetWeak( - this, FinalizeCallback, v8::WeakCallbackType::kParameter); + SetWeak(); } return refcount; } @@ -377,10 +385,11 @@ class Reference : public RefBase { // 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 - // `Finalize()`, so let's reset the persistent here if nothing is - // keeping it alive. - if (is_env_teardown && _persistent.IsWeak()) { - _persistent.ClearWeak(); + // `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(); } // Chain up to perform the rest of the finalization. @@ -388,6 +397,35 @@ class Reference : public RefBase { } private: + // 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 + // secheduled no Finalization will be run. + inline void ClearWeak() { + if (!_persistent.IsEmpty()) { + _persistent.ClearWeak(); + } + if (_secondPassParameter != nullptr) { + *_secondPassParameter = nullptr; + } + } + + // Mark the reference as weak and eligible for collection + // by the gc. + inline void SetWeak() { + if (_secondPassParameter == nullptr) { + // This means that the Reference has already been processed + // by the second pass calback, so its already been Finalized, do + // nothing + return; + } + _persistent.SetWeak( + _secondPassParameter, FinalizeCallback, + v8::WeakCallbackType::kParameter); + *_secondPassParameter = this; + } + // 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 @@ -395,21 +433,47 @@ class Reference : public RefBase { // 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. - static void FinalizeCallback(const v8::WeakCallbackInfo& data) { - Reference* reference = data.GetParameter(); + static void 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->_secondPassParameter = nullptr; data.SetSecondPassCallback(SecondPassCallback); } - static void SecondPassCallback(const v8::WeakCallbackInfo& data) { - data.GetParameter()->Finalize(); + // 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 seperately allocated parameter instead + // of a parameter within the Reference object itself. This is what + // is stored in _secondPassParameter and it is alocated in the + // constructor for the Reference. + static void 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->Finalize(); } bool env_teardown_finalize_started_ = false; v8impl::Persistent _persistent; + SecondPassCallParameterRef* _secondPassParameter; }; enum UnwrapAction {