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

addaleax · targos · commit 33702913b113 · 2019-04-30T08:17:33.000+02:00
`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: #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>
diff --git a/node.gyp b/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',
diff --git a/src/async_wrap.cc b/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;
-  Persistent<Object> propBag;
+  Global<Object> target;
+  Global<Object> propBag;
 };
 
 void AsyncWrap::WeakCallback(const WeakCallbackInfo<DestroyParam>& info) {
diff --git a/src/base_object-inl.h b/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_;
 }
 
diff --git a/src/base_object.h b/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_;
 };
 
diff --git a/src/env.h b/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
diff --git a/src/heap_utils.cc b/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 {
diff --git a/src/inspector_agent.cc b/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());
diff --git a/src/inspector_agent.h b/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_;
-  node::Persistent<v8::Function> disable_async_hook_function_;
+  v8::Global<v8::Function> enable_async_hook_function_;
+  v8::Global<v8::Function> disable_async_hook_function_;
 };
 
 }  // namespace inspector
diff --git a/src/inspector_js_api.cc b/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) {
diff --git a/src/js_native_api_v8_internals.h b/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;
 
diff --git a/src/memory_tracker-inl.h b/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_));
 }
diff --git a/src/memory_tracker.h b/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,
diff --git a/src/module_wrap.cc b/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,
diff --git a/src/module_wrap.h b/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_;
-  Persistent<v8::String> url_;
+  v8::Global<v8::Module> module_;
+  v8::Global<v8::String> url_;
   bool linked_ = false;
-  std::unordered_map<std::string, Persistent<v8::Promise>> resolve_cache_;
-  Persistent<v8::Context> context_;
+  std::unordered_map<std::string, v8::Global<v8::Promise>> resolve_cache_;
+  v8::Global<v8::Context> context_;
   uint32_t id_;
 };
 
diff --git a/src/node_buffer.cc b/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_;
diff --git a/src/node_contextify.h b/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_;
 };
 
diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -321,8 +321,8 @@ class SSLWrap {
 
   ClientHelloParser hello_parser_;
 
-  Persistent<v8::ArrayBufferView> ocsp_response_;
-  Persistent<v8::Value> sni_context_;
+  v8::Global<v8::ArrayBufferView> ocsp_response_;
+  v8::Global<v8::Value> sni_context_;
 
   friend class SecureContext;
 };
diff --git a/src/node_file.h b/src/node_file.h
@@ -451,8 +451,8 @@ class FileHandle : public AsyncWrap, public StreamBase {
     CloseReq& operator=(const CloseReq&&) = delete;
 
    private:
-    Persistent<Promise> promise_{};
-    Persistent<Value> ref_{};
+    v8::Global<Promise> promise_{};
+    v8::Global<Value> ref_{};
   };
 
   // Asynchronous close
diff --git a/src/node_internals.h b/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"
diff --git a/src/node_persistent.h b/src/node_persistent.h
diff --git a/src/node_util.cc b/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) {
diff --git a/src/node_zlib.cc b/src/node_zlib.cc
diff --git a/src/util.h b/src/util.h

Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ BaseObject::~BaseObject() {`
`56`	`56`	`}`
`57`	`57`
`58`	`58`
`59`		`-Persistent<v8::Object>& BaseObject::persistent() {`
	`59`	`+v8::Global<v8::Object>& BaseObject::persistent() {`
`60`	`60`	`return persistent_handle_;`
`61`	`61`	`}`
`62`	`62`