node-api: generalize finalizer second pass callback

Author: legendecas

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',

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 <typename... Args>
 Reference::Reference(napi_env env, v8::Local<v8::Value> value, Args&&... args)
     : RefBase(env, std::forward<Args>(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<v8::Value> 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<v8::Value> Reference::Get() {
-  if (_persistent.IsEmpty()) {
+  if (persistent_.IsEmpty()) {
     return v8::Local<v8::Value>();
   } else {
-    return v8::Local<v8::Value>::New(_env->isolate, _persistent);
+    return v8::Local<v8::Value>::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<SecondPassCallParameterRef>& 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<SecondPassCallParameterRef>& 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<Reference>& 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<v8impl::Reference*>(ref));
+  delete reinterpret_cast<v8impl::Reference*>(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 =