[v9.x] backport 18656 + 19002

node.gyp

@@ -359,9 +359,10 @@
         'src/node_internals.h',
         'src/node_javascript.h',
         'src/node_mutex.h',
-        'src/node_platform.h',
         'src/node_perf.h',
         'src/node_perf_common.h',
+        'src/node_persistent.h',
+        'src/node_platform.h',
         'src/node_root_certs.h',
         'src/node_version.h',
         'src/node_watchdog.h',

src/async_wrap.cc

@@ -410,8 +410,8 @@ static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
 class DestroyParam {
  public:
   double asyncId;
-  v8::Persistent<Object> target;
-  v8::Persistent<Object> propBag;
+  Persistent<Object> target;
+  Persistent<Object> propBag;
 };
 
 
@@ -426,8 +426,6 @@ void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo<DestroyParam>& info) {
   if (val->IsFalse()) {
     AsyncWrap::EmitDestroy(env, p->asyncId);
   }
-  p->target.Reset();
-  p->propBag.Reset();
   delete p;
 }
 

src/base_object-inl.h

@@ -42,12 +42,7 @@ inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
 }
 
 
-inline BaseObject::~BaseObject() {
-  CHECK(persistent_handle_.IsEmpty());
-}
-
-
-inline v8::Persistent<v8::Object>& BaseObject::persistent() {
+inline Persistent<v8::Object>& BaseObject::persistent() {
   return persistent_handle_;
 }
 
@@ -65,8 +60,7 @@ inline Environment* BaseObject::env() const {
 template <typename Type>
 inline void BaseObject::WeakCallback(
     const v8::WeakCallbackInfo<Type>& data) {
-  std::unique_ptr<Type> self(data.GetParameter());
-  self->persistent().Reset();
+  delete data.GetParameter();
 }
 
 

src/base_object.h

@@ -24,6 +24,7 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
+#include "node_persistent.h"
 #include "v8.h"
 
 namespace node {
@@ -33,18 +34,13 @@ class Environment;
 class BaseObject {
  public:
   inline BaseObject(Environment* env, v8::Local<v8::Object> handle);
-  inline virtual ~BaseObject();
+  virtual ~BaseObject() = default;
 
   // Returns the wrapped object.  Returns an empty handle when
   // persistent.IsEmpty() is true.
   inline v8::Local<v8::Object> object();
 
-  // The parent class is responsible for calling .Reset() on destruction
-  // when the persistent handle is strong because there is no way for
-  // BaseObject to know when the handle goes out of scope.
-  // Weak handles have been reset by the time the destructor runs but
-  // calling .Reset() again is harmless.
-  inline v8::Persistent<v8::Object>& persistent();
+  inline Persistent<v8::Object>& persistent();
 
   inline Environment* env() const;
 
@@ -71,7 +67,7 @@ class BaseObject {
   // position of members in memory are predictable. For more information please
   // refer to `doc/guides/node-postmortem-support.md`
   friend int GenDebugSymbols();
-  v8::Persistent<v8::Object> persistent_handle_;
+  Persistent<v8::Object> persistent_handle_;
   Environment* env_;
 };
 

src/cares_wrap.cc

@@ -598,7 +598,6 @@ class QueryWrap : public AsyncWrap {
   ~QueryWrap() override {
     CHECK_EQ(false, persistent().IsEmpty());
     ClearWrap(object());
-    persistent().Reset();
   }
 
   // Subclasses should implement the appropriate Send method.

src/env-inl.h

@@ -357,9 +357,6 @@ inline Environment::~Environment() {
 
   context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex,
                                              nullptr);
-#define V(PropertyName, TypeName) PropertyName ## _.Reset();
-  ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
-#undef V
 
   delete[] heap_statistics_buffer_;
   delete[] heap_space_statistics_buffer_;
@@ -541,8 +538,8 @@ void Environment::CreateImmediate(native_immediate_callback cb,
   native_immediate_callbacks_.push_back({
     cb,
     data,
-    std::unique_ptr<v8::Persistent<v8::Object>>(obj.IsEmpty() ?
-        nullptr : new v8::Persistent<v8::Object>(isolate_, obj)),
+    std::unique_ptr<Persistent<v8::Object>>(obj.IsEmpty() ?
+        nullptr : new Persistent<v8::Object>(isolate_, obj)),
     ref
   });
   immediate_info()->count_inc(1);

src/env.cc

@@ -296,13 +296,24 @@ void Environment::RunAndClearNativeImmediates() {
     size_t ref_count = 0;
     std::vector<NativeImmediateCallback> list;
     native_immediate_callbacks_.swap(list);
-    for (const auto& cb : list) {
-      cb.cb_(this, cb.data_);
-      if (cb.keep_alive_)
-        cb.keep_alive_->Reset();
-      if (cb.refed_)
-        ref_count++;
-    }
+    auto drain_list = [&]() {
+      v8::TryCatch try_catch(isolate());
+      for (auto it = list.begin(); it != list.end(); ++it) {
+        it->cb_(this, it->data_);
+        if (it->refed_)
+          ref_count++;
+        if (UNLIKELY(try_catch.HasCaught())) {
+          FatalException(isolate(), try_catch);
+          // Bail out, remove the already executed callbacks from list
+          // and set up a new TryCatch for the other pending callbacks.
+          std::move_backward(it, list.end(), list.begin() + (list.end() - it));
+          list.resize(list.end() - it);
+          return true;
+        }
+      }
+      return false;
+    };
+    while (drain_list()) {}
 
 #ifdef DEBUG
     CHECK_GE(immediate_info()->count(), count);

src/env.h

@@ -800,7 +800,7 @@ class Environment {
   struct NativeImmediateCallback {
     native_immediate_callback cb_;
     void* data_;
-    std::unique_ptr<v8::Persistent<v8::Object>> keep_alive_;
+    std::unique_ptr<Persistent<v8::Object>> keep_alive_;
     bool refed_;
   };
   std::vector<NativeImmediateCallback> native_immediate_callbacks_;
@@ -811,8 +811,7 @@ class Environment {
                              v8::Local<v8::Promise> promise,
                              v8::Local<v8::Value> parent);
 
-#define V(PropertyName, TypeName)                                             \
-  v8::Persistent<TypeName> PropertyName ## _;
+#define V(PropertyName, TypeName) Persistent<TypeName> PropertyName ## _;
   ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
 #undef V
 

src/handle_wrap.cc

@@ -98,11 +98,6 @@ HandleWrap::HandleWrap(Environment* env,
 }
 
 
-HandleWrap::~HandleWrap() {
-  CHECK(persistent().IsEmpty());
-}
-
-
 void HandleWrap::OnClose(uv_handle_t* handle) {
   HandleWrap* wrap = static_cast<HandleWrap*>(handle->data);
   Environment* env = wrap->env();
@@ -120,7 +115,6 @@ void HandleWrap::OnClose(uv_handle_t* handle) {
     wrap->MakeCallback(env->onclose_string(), 0, nullptr);
 
   ClearWrap(wrap->object());
-  wrap->persistent().Reset();
   delete wrap;
 }
 

src/handle_wrap.h

@@ -75,7 +75,6 @@ class HandleWrap : public AsyncWrap {
              v8::Local<v8::Object> object,
              uv_handle_t* handle,
              AsyncWrap::ProviderType provider);
-  ~HandleWrap() override;
 
  private:
   friend class Environment;

src/inspector_agent.cc

@@ -30,7 +30,6 @@ using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
 using v8::Object;
-using v8::Persistent;
 using v8::String;
 using v8::Value;
 

src/inspector_agent.h

@@ -97,7 +97,7 @@ class Agent {
 
  private:
   void ToggleAsyncHook(v8::Isolate* isolate,
-                       const v8::Persistent<v8::Function>& fn);
+                       const Persistent<v8::Function>& fn);
 
   node::Environment* parent_env_;
   std::unique_ptr<NodeInspectorClient> client_;
@@ -109,8 +109,8 @@ class Agent {
 
   bool pending_enable_async_hook_;
   bool pending_disable_async_hook_;
-  v8::Persistent<v8::Function> enable_async_hook_function_;
-  v8::Persistent<v8::Function> disable_async_hook_function_;
+  Persistent<v8::Function> enable_async_hook_function_;
+  Persistent<v8::Function> disable_async_hook_function_;
 };
 
 }  // namespace inspector

src/inspector_js_api.cc

@@ -19,7 +19,6 @@ using v8::Local;
 using v8::MaybeLocal;
 using v8::NewStringType;
 using v8::Object;
-using v8::Persistent;
 using v8::String;
 using v8::Value;
 
@@ -85,10 +84,6 @@ class JSBindingsConnection : public AsyncWrap {
     inspector->Connect(&delegate_);
   }
 
-  ~JSBindingsConnection() override {
-    callback_.Reset();
-  }
-
   void OnMessage(Local<Value> value) {
     MakeCallback(callback_.Get(env()->isolate()), 1, &value);
   }
@@ -112,7 +107,6 @@ class JSBindingsConnection : public AsyncWrap {
     delegate_.Disconnect();
     if (!persistent().IsEmpty()) {
       ClearWrap(object());
-      persistent().Reset();
     }
     delete this;
   }

src/module_wrap.cc

@@ -59,9 +59,6 @@ ModuleWrap::~ModuleWrap() {
       break;
     }
   }
-
-  module_.Reset();
-  context_.Reset();
 }
 
 void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
@@ -215,8 +212,6 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {
       module->InstantiateModule(context, ModuleWrap::ResolveCallback);
 
   // clear resolve cache on instantiate
-  for (auto& entry : obj->resolve_cache_)
-    entry.second.Reset();
   obj->resolve_cache_.clear();
 
   if (!ok.FromMaybe(false)) {

src/module_wrap.h

@@ -49,11 +49,11 @@ class ModuleWrap : public BaseObject {
       v8::Local<v8::String> specifier,
       v8::Local<v8::Module> referrer);
 
-  v8::Persistent<v8::Module> module_;
-  v8::Persistent<v8::String> url_;
+  Persistent<v8::Module> module_;
+  Persistent<v8::String> url_;
   bool linked_ = false;
-  std::unordered_map<std::string, v8::Persistent<v8::Promise>> resolve_cache_;
-  v8::Persistent<v8::Context> context_;
+  std::unordered_map<std::string, Persistent<v8::Promise>> resolve_cache_;
+  Persistent<v8::Context> context_;
 };
 
 }  // namespace loader

src/node_api.cc

@@ -31,17 +31,11 @@ struct napi_env__ {
       : isolate(_isolate),
         last_error(),
         loop(_loop) {}
-  ~napi_env__() {
-    last_exception.Reset();
-    wrap_template.Reset();
-    function_data_template.Reset();
-    accessor_data_template.Reset();
-  }
   v8::Isolate* isolate;
-  v8::Persistent<v8::Value> last_exception;
-  v8::Persistent<v8::ObjectTemplate> wrap_template;
-  v8::Persistent<v8::ObjectTemplate> function_data_template;
-  v8::Persistent<v8::ObjectTemplate> accessor_data_template;
+  node::Persistent<v8::Value> last_exception;
+  node::Persistent<v8::ObjectTemplate> wrap_template;
+  node::Persistent<v8::ObjectTemplate> function_data_template;
+  node::Persistent<v8::ObjectTemplate> accessor_data_template;
   napi_extended_error_info last_error;
   int open_handle_scopes = 0;
   int open_callback_scopes = 0;
@@ -274,13 +268,13 @@ static_assert(sizeof(v8::Local<v8::Value>) == sizeof(napi_value),
   "Cannot convert between v8::Local<v8::Value> and napi_value");
 
 static
-napi_deferred JsDeferredFromV8Persistent(v8::Persistent<v8::Value>* local) {
+napi_deferred JsDeferredFromNodePersistent(node::Persistent<v8::Value>* local) {
   return reinterpret_cast<napi_deferred>(local);
 }
 
 static
-v8::Persistent<v8::Value>* V8PersistentFromJsDeferred(napi_deferred local) {
-  return reinterpret_cast<v8::Persistent<v8::Value>*>(local);
+node::Persistent<v8::Value>* NodePersistentFromJsDeferred(napi_deferred local) {
+  return reinterpret_cast<node::Persistent<v8::Value>*>(local);
 }
 
 static
@@ -360,7 +354,7 @@ class Finalizer {
   void* _finalize_hint;
 };
 
-// Wrapper around v8::Persistent that implements reference counting.
+// Wrapper around node::Persistent that implements reference counting.
 class Reference : private Finalizer {
  private:
   Reference(napi_env env,
@@ -381,16 +375,6 @@ class Reference : private Finalizer {
     }
   }
 
-  ~Reference() {
-    // The V8 Persistent class currently does not reset in its destructor:
-    // see NonCopyablePersistentTraits::kResetInDestructor = false.
-    // (Comments there claim that might change in the future.)
-    // To avoid memory leaks, it is better to reset at this time, however
-    // care must be taken to avoid attempting this after the Isolate has
-    // shut down, for example via a static (atexit) destructor.
-    _persistent.Reset();
-  }
-
  public:
   void* Data() {
     return _finalize_data;
@@ -470,7 +454,7 @@ class Reference : private Finalizer {
     }
   }
 
-  v8::Persistent<v8::Value> _persistent;
+  node::Persistent<v8::Value> _persistent;
   uint32_t _refcount;
   bool _delete_self;
 };
@@ -846,8 +830,8 @@ napi_status ConcludeDeferred(napi_env env,
   CHECK_ARG(env, result);
 
   v8::Local<v8::Context> context = env->isolate->GetCurrentContext();
-  v8::Persistent<v8::Value>* deferred_ref =
-      V8PersistentFromJsDeferred(deferred);
+  node::Persistent<v8::Value>* deferred_ref =
+      NodePersistentFromJsDeferred(deferred);
   v8::Local<v8::Value> v8_deferred =
       v8::Local<v8::Value>::New(env->isolate, *deferred_ref);
 
@@ -857,7 +841,6 @@ napi_status ConcludeDeferred(napi_env env,
       v8_resolver->Resolve(context, v8impl::V8LocalValueFromJsValue(result)) :
       v8_resolver->Reject(context, v8impl::V8LocalValueFromJsValue(result));
 
-  deferred_ref->Reset();
   delete deferred_ref;
 
   RETURN_STATUS_IF_FALSE(env, success.FromMaybe(false), napi_generic_failure);
@@ -3493,10 +3476,10 @@ napi_status napi_create_promise(napi_env env,
   CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure);
 
   auto v8_resolver = maybe.ToLocalChecked();
-  auto v8_deferred = new v8::Persistent<v8::Value>();
+  auto v8_deferred = new node::Persistent<v8::Value>();
   v8_deferred->Reset(env->isolate, v8_resolver);
 
-  *deferred = v8impl::JsDeferredFromV8Persistent(v8_deferred);
+  *deferred = v8impl::JsDeferredFromNodePersistent(v8_deferred);
   *promise = v8impl::JsValueFromV8LocalValue(v8_resolver->GetPromise());
   return GET_RETURN_STATUS(env);
 }