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.

Author: legendecas

node.gyp

@@ -998,7 +998,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

@@ -218,7 +218,7 @@ inline 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);
@@ -464,11 +464,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,
@@ -483,105 +492,65 @@ 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() {
+  bool delete_self = delete_self_;
+  // Swap out the field finalize_callback so that it can not be accidentally
+  // called more than once.
+  napi_finalize finalize_callback = finalize_callback_;
+  finalize_callback_ = nullptr;
+
+  // 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,
@@ -598,24 +567,25 @@ 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() {
+  // 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 +595,36 @@ 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);
-}
-
-// 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<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 +2422,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 +3112,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 =