src: prefer v8::Global over node::Persistent

`v8::Global` is essentially a nicer variant of `node::Persistent` that,
in addition to reset-on-destroy, also implements move semantics.

This commit makes the necessary replacements, removes
`node::Persistent` and (now-)unnecessary inclusions of the
`node_persistent.h` header, and makes some of the functions that
take Persistents as arguments more generic so that they work with all
`v8::PersistentBase` flavours.

PR-URL: https://github.com/nodejs/node/pull/27287
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

node.gyp

@@ -606,7 +606,6 @@
         'src/node_options-inl.h',
         'src/node_perf.h',
         'src/node_perf_common.h',
-        'src/node_persistent.h',
         'src/node_platform.h',
         'src/node_process.h',
         'src/node_revert.h',

src/async_wrap.cc

@@ -35,6 +35,7 @@ using v8::EscapableHandleScope;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
@@ -346,8 +347,8 @@ class DestroyParam {
  public:
   double asyncId;
   Environment* env;
-  Persistent<Object> target;
+  Global<Object> target;
-  Persistent<Object> propBag;
+  Global<Object> propBag;
 };
 
 void AsyncWrap::WeakCallback(const WeakCallbackInfo<DestroyParam>& info) {

src/base_object-inl.h

@@ -56,7 +56,7 @@ BaseObject::~BaseObject() {
 }
 
 
-Persistent<v8::Object>& BaseObject::persistent() {
+v8::Global<v8::Object>& BaseObject::persistent() {
   return persistent_handle_;
 }
 

src/base_object.h

@@ -24,7 +24,6 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
-#include "node_persistent.h"
 #include "memory_tracker-inl.h"
 #include "v8.h"
 #include <type_traits>  // std::remove_reference
@@ -50,7 +49,7 @@ class BaseObject : public MemoryRetainer {
   // is associated with the passed Isolate in debug mode.
   inline v8::Local<v8::Object> object(v8::Isolate* isolate) const;
 
-  inline Persistent<v8::Object>& persistent();
+  inline v8::Global<v8::Object>& persistent();
 
   inline Environment* env() const;
 
@@ -62,7 +61,7 @@ class BaseObject : public MemoryRetainer {
   template <typename T>
   static inline T* FromJSObject(v8::Local<v8::Object> object);
 
-  // Make the `Persistent` a weak reference and, `delete` this object once
+  // Make the `v8::Global` a weak reference and, `delete` this object once
   // the JS object has been garbage collected.
   inline void MakeWeak();
 
@@ -96,7 +95,7 @@ class BaseObject : public MemoryRetainer {
   friend int GenDebugSymbols();
   friend class CleanupHookCallback;
 
-  Persistent<v8::Object> persistent_handle_;
+  v8::Global<v8::Object> persistent_handle_;
   Environment* env_;
 };
 

src/env.h

@@ -929,7 +929,7 @@ class Environment : public MemoryRetainer {
   std::unordered_map<uint32_t, contextify::ContextifyScript*>
       id_to_script_map;
   std::unordered_set<CompileFnEntry*> compile_fn_entries;
-  std::unordered_map<uint32_t, Persistent<v8::Function>> id_to_function_map;
+  std::unordered_map<uint32_t, v8::Global<v8::Function>> id_to_function_map;
 
   inline uint32_t get_next_module_id();
   inline uint32_t get_next_script_id();
@@ -1292,7 +1292,7 @@ class Environment : public MemoryRetainer {
   template <typename T>
   void ForEachBaseObject(T&& iterator);
 
-#define V(PropertyName, TypeName) Persistent<TypeName> PropertyName ## _;
+#define V(PropertyName, TypeName) v8::Global<TypeName> PropertyName ## _;
   ENVIRONMENT_STRONG_PERSISTENT_VALUES(V)
   ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
 #undef V

src/heap_utils.cc

@@ -8,6 +8,7 @@ using v8::EmbedderGraph;
 using v8::EscapableHandleScope;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::HandleScope;
 using v8::HeapSnapshot;
 using v8::Isolate;
@@ -56,7 +57,7 @@ class JSGraphJSNode : public EmbedderGraph::Node {
   };
 
  private:
-  Persistent<Value> persistent_;
+  Global<Value> persistent_;
 };
 
 class JSGraph : public EmbedderGraph {

src/inspector_agent.cc

@@ -34,6 +34,7 @@ using node::FatalError;
 
 using v8::Context;
 using v8::Function;
+using v8::Global;
 using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
@@ -834,7 +835,7 @@ void Agent::DisableAsyncHook() {
 }
 
 void Agent::ToggleAsyncHook(Isolate* isolate,
-                            const node::Persistent<Function>& fn) {
+                            const Global<Function>& fn) {
   CHECK(parent_env_->has_run_bootstrapping_code());
   HandleScope handle_scope(isolate);
   CHECK(!fn.IsEmpty());

src/inspector_agent.h

@@ -7,7 +7,6 @@
 #endif
 
 #include "node_options-inl.h"
-#include "node_persistent.h"
 #include "v8.h"
 
 #include <cstddef>
@@ -114,7 +113,7 @@ class Agent {
 
  private:
   void ToggleAsyncHook(v8::Isolate* isolate,
-                       const node::Persistent<v8::Function>& fn);
+                       const v8::Global<v8::Function>& fn);
 
   node::Environment* parent_env_;
   // Encapsulates majority of the Inspector functionality
@@ -133,8 +132,8 @@ class Agent {
 
   bool pending_enable_async_hook_ = false;
   bool pending_disable_async_hook_ = false;
-  node::Persistent<v8::Function> enable_async_hook_function_;
+  v8::Global<v8::Function> enable_async_hook_function_;
-  node::Persistent<v8::Function> disable_async_hook_function_;
+  v8::Global<v8::Function> disable_async_hook_function_;
 };
 
 }  // namespace inspector

src/inspector_js_api.cc

@@ -15,6 +15,7 @@ using v8::Context;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
@@ -116,7 +117,7 @@ class JSBindingsConnection : public AsyncWrap {
 
  private:
   std::unique_ptr<InspectorSession> session_;
-  Persistent<Function> callback_;
+  Global<Function> callback_;
 };
 
 static bool InspectorEnabled(Environment* env) {

src/js_native_api_v8_internals.h

@@ -29,7 +29,7 @@
 namespace v8impl {
 
 template <typename T>
-using Persistent = node::Persistent<T>;
+using Persistent = v8::Global<T>;
 
 using PersistentToLocal = node::PersistentToLocal;
 

src/memory_tracker-inl.h

@@ -184,9 +184,9 @@ void MemoryTracker::TrackField(const char* edge_name,
   TrackField(edge_name, value.Get(isolate_));
 }
 
-template <typename T, typename Traits>
+template <typename T>
 void MemoryTracker::TrackField(const char* edge_name,
-                               const v8::Persistent<T, Traits>& value,
+                               const v8::PersistentBase<T>& value,
                                const char* node_name) {
   TrackField(edge_name, value.Get(isolate_));
 }

src/memory_tracker.h

@@ -90,9 +90,9 @@ class CleanupHookCallback;
  *     NonPointerRetainerClass non_pointer_retainer;
  *     InternalClass internal_member_;
  *     std::vector<uv_async_t> vector_;
- *     node::Persistent<Object> target_;
+ *     v8::Global<Object> target_;
  *
- *     node::Persistent<Object> wrapped_;
+ *     v8::Global<Object> wrapped_;
  * }
  *
  * This creates the following graph:
@@ -185,9 +185,9 @@ class MemoryTracker {
   void TrackField(const char* edge_name,
                   const v8::Eternal<T>& value,
                   const char* node_name);
-  template <typename T, typename Traits>
+  template <typename T>
   inline void TrackField(const char* edge_name,
-                         const v8::Persistent<T, Traits>& value,
+                         const v8::PersistentBase<T>& value,
                          const char* node_name = nullptr);
   template <typename T>
   inline void TrackField(const char* edge_name,

src/module_wrap.cc

@@ -25,6 +25,7 @@ using v8::Context;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::HandleScope;
 using v8::Integer;
 using v8::IntegrityLevel;
@@ -611,7 +612,7 @@ Maybe<const PackageConfig*> GetPackageConfig(Environment* env,
       env->exports_string()).ToLocal(&exports_v) &&
       (exports_v->IsObject() || exports_v->IsString() ||
       exports_v->IsBoolean())) {
-    Persistent<Value> exports;
+    Global<Value> exports;
     exports.Reset(env->isolate(), exports_v);
 
     auto entry = env->package_json_cache.emplace(path,

src/module_wrap.h

@@ -75,11 +75,11 @@ class ModuleWrap : public BaseObject {
       v8::Local<v8::Module> referrer);
   static ModuleWrap* GetFromModule(node::Environment*, v8::Local<v8::Module>);
 
-  Persistent<v8::Module> module_;
+  v8::Global<v8::Module> module_;
-  Persistent<v8::String> url_;
+  v8::Global<v8::String> url_;
   bool linked_ = false;
-  std::unordered_map<std::string, Persistent<v8::Promise>> resolve_cache_;
+  std::unordered_map<std::string, v8::Global<v8::Promise>> resolve_cache_;
-  Persistent<v8::Context> context_;
+  v8::Global<v8::Context> context_;
   uint32_t id_;
 };
 

src/node_buffer.cc

@@ -62,6 +62,7 @@ using v8::ArrayBufferView;
 using v8::Context;
 using v8::EscapableHandleScope;
 using v8::FunctionCallbackInfo;
+using v8::Global;
 using v8::Integer;
 using v8::Isolate;
 using v8::Just;
@@ -99,7 +100,7 @@ class CallbackInfo {
                       FreeCallback callback,
                       char* data,
                       void* hint);
-  Persistent<ArrayBuffer> persistent_;
+  Global<ArrayBuffer> persistent_;
   FreeCallback const callback_;
   char* const data_;
   void* const hint_;

src/node_contextify.h

@@ -100,7 +100,7 @@ class ContextifyContext {
       uint32_t index,
       const v8::PropertyCallbackInfo<v8::Boolean>& args);
   Environment* const env_;
-  Persistent<v8::Context> context_;
+  v8::Global<v8::Context> context_;
 };
 
 class ContextifyScript : public BaseObject {
@@ -129,7 +129,7 @@ class ContextifyScript : public BaseObject {
   inline uint32_t id() { return id_; }
 
  private:
-  node::Persistent<v8::UnboundScript> script_;
+  v8::Global<v8::UnboundScript> script_;
   uint32_t id_;
 };
 

src/node_crypto.h

@@ -321,8 +321,8 @@ class SSLWrap {
 
   ClientHelloParser hello_parser_;
 
-  Persistent<v8::ArrayBufferView> ocsp_response_;
+  v8::Global<v8::ArrayBufferView> ocsp_response_;
-  Persistent<v8::Value> sni_context_;
+  v8::Global<v8::Value> sni_context_;
 
   friend class SecureContext;
 };

src/node_file.h

@@ -451,8 +451,8 @@ class FileHandle : public AsyncWrap, public StreamBase {
     CloseReq& operator=(const CloseReq&&) = delete;
 
    private:
-    Persistent<Promise> promise_{};
+    v8::Global<Promise> promise_{};
-    Persistent<Value> ref_{};
+    v8::Global<Value> ref_{};
   };
 
   // Asynchronous close

src/node_internals.h

@@ -28,7 +28,6 @@
 #include "node.h"
 #include "node_binding.h"
 #include "node_mutex.h"
-#include "node_persistent.h"
 #include "tracing/trace_event.h"
 #include "util-inl.h"
 #include "uv.h"

src/node_persistent.h

@@ -1,66 +0,0 @@
-#ifndef SRC_NODE_PERSISTENT_H_
-#define SRC_NODE_PERSISTENT_H_
-
-#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
-
-#include "v8.h"
-
-namespace node {
-
-template <typename T>
-struct ResetInDestructorPersistentTraits {
-  static const bool kResetInDestructor = true;
-  template <typename S, typename M>
-  // Disallow copy semantics by leaving this unimplemented.
-  inline static void Copy(
-      const v8::Persistent<S, M>&,
-      v8::Persistent<T, ResetInDestructorPersistentTraits<T>>*);
-};
-
-// v8::Persistent does not reset the object slot in its destructor.  That is
-// acknowledged as a flaw in the V8 API and expected to change in the future
-// but for now node::Persistent is the easier and safer alternative.
-template <typename T>
-using Persistent = v8::Persistent<T, ResetInDestructorPersistentTraits<T>>;
-
-class PersistentToLocal {
- public:
-  // If persistent.IsWeak() == false, then do not call persistent.Reset()
-  // while the returned Local<T> is still in scope, it will destroy the
-  // reference to the object.
-  template <class TypeName>
-  static inline v8::Local<TypeName> Default(
-      v8::Isolate* isolate,
-      const Persistent<TypeName>& persistent) {
-    if (persistent.IsWeak()) {
-      return PersistentToLocal::Weak(isolate, persistent);
-    } else {
-      return PersistentToLocal::Strong(persistent);
-    }
-  }
-
-  // Unchecked conversion from a non-weak Persistent<T> to Local<T>,
-  // use with care!
-  //
-  // Do not call persistent.Reset() while the returned Local<T> is still in
-  // scope, it will destroy the reference to the object.
-  template <class TypeName>
-  static inline v8::Local<TypeName> Strong(
-      const Persistent<TypeName>& persistent) {
-    return *reinterpret_cast<v8::Local<TypeName>*>(
-        const_cast<Persistent<TypeName>*>(&persistent));
-  }
-
-  template <class TypeName>
-  static inline v8::Local<TypeName> Weak(
-      v8::Isolate* isolate,
-      const Persistent<TypeName>& persistent) {
-    return v8::Local<TypeName>::New(isolate, persistent);
-  }
-};
-
-}  // namespace node
-
-#endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
-
-#endif  // SRC_NODE_PERSISTENT_H_

src/node_util.cc

@@ -13,6 +13,7 @@ using v8::Context;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::IndexFilter;
 using v8::Integer;
 using v8::Isolate;
@@ -189,7 +190,7 @@ class WeakReference : public BaseObject {
   SET_NO_MEMORY_INFO()
 
  private:
-  Persistent<Object> target_;
+  Global<Object> target_;
 };
 
 static void GuessHandleType(const FunctionCallbackInfo<Value>& args) {

src/node_zlib.cc

@@ -47,6 +47,7 @@ using v8::Context;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::HandleScope;
 using v8::Int32;
 using v8::Integer;
@@ -526,7 +527,7 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork {
   bool closed_ = false;
   unsigned int refs_ = 0;
   uint32_t* write_result_ = nullptr;
-  Persistent<Function> write_js_callback_;
+  Global<Function> write_js_callback_;
   std::atomic<ssize_t> unreported_allocations_{0};
   size_t zlib_memory_ = 0;
 

src/util.h

@@ -24,7 +24,6 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
-#include "node_persistent.h"
 #include "v8.h"
 
 #include <cassert>
@@ -669,6 +668,44 @@ class SlicedArguments : public MaybeStackBuffer<v8::Local<v8::Value>> {
       const v8::FunctionCallbackInfo<v8::Value>& args, size_t start = 0);
 };
 
+// Convert a v8::PersistentBase, e.g. v8::Global, to a Local, with an extra
+// optimization for strong persistent handles.
+class PersistentToLocal {
+ public:
+  // If persistent.IsWeak() == false, then do not call persistent.Reset()
+  // while the returned Local<T> is still in scope, it will destroy the
+  // reference to the object.
+  template <class TypeName>
+  static inline v8::Local<TypeName> Default(
+      v8::Isolate* isolate,
+      const v8::PersistentBase<TypeName>& persistent) {
+    if (persistent.IsWeak()) {
+      return PersistentToLocal::Weak(isolate, persistent);
+    } else {
+      return PersistentToLocal::Strong(persistent);
+    }
+  }
+
+  // Unchecked conversion from a non-weak Persistent<T> to Local<T>,
+  // use with care!
+  //
+  // Do not call persistent.Reset() while the returned Local<T> is still in
+  // scope, it will destroy the reference to the object.
+  template <class TypeName>
+  static inline v8::Local<TypeName> Strong(
+      const v8::PersistentBase<TypeName>& persistent) {
+    return *reinterpret_cast<v8::Local<TypeName>*>(
+        const_cast<v8::PersistentBase<TypeName>*>(&persistent));
+  }
+
+  template <class TypeName>
+  static inline v8::Local<TypeName> Weak(
+      v8::Isolate* isolate,
+      const v8::PersistentBase<TypeName>& persistent) {
+    return v8::Local<TypeName>::New(isolate, persistent);
+  }
+};
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS