src: prevent persistent handle resource leaks

bnoordhuis · bnoordhuis · commit 992703f2b50f · 2018-02-21T15:24:18.000+01:00
Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
diff --git a/node.gyp b/node.gyp
@@ -371,9 +371,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',
diff --git a/src/async_wrap.cc b/src/async_wrap.cc
@@ -409,8 +409,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;
 };
 
 
diff --git a/src/base_object-inl.h b/src/base_object-inl.h
@@ -47,7 +47,7 @@ inline BaseObject::~BaseObject() {
 }
 
 
-inline v8::Persistent<v8::Object>& BaseObject::persistent() {
+inline Persistent<v8::Object>& BaseObject::persistent() {
   return persistent_handle_;
 }
 
diff --git a/src/base_object.h b/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 {
@@ -39,12 +40,7 @@ class BaseObject {
   // 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_;
 };
 
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -550,8 +550,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);
diff --git a/src/env.h b/src/env.h
@@ -847,7 +847,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_;
@@ -858,8 +858,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
 
diff --git a/src/inspector_agent.cc b/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;
 
diff --git a/src/inspector_agent.h b/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
diff --git a/src/inspector_js_api.cc b/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;
 
diff --git a/src/module_wrap.h b/src/module_wrap.h
@@ -62,11 +62,11 @@ class ModuleWrap : public BaseObject {
   static ModuleWrap* GetFromModule(node::Environment*, v8::Local<v8::Module>);
 
 
-  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
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -38,10 +38,10 @@ struct napi_env__ {
     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 +274,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 +360,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,
@@ -470,7 +470,7 @@ class Reference : private Finalizer {
     }
   }
 
-  v8::Persistent<v8::Value> _persistent;
+  node::Persistent<v8::Value> _persistent;
   uint32_t _refcount;
   bool _delete_self;
 };
@@ -846,8 +846,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);
 
@@ -3493,10 +3493,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);
 }
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -78,7 +78,6 @@ using v8::Local;
 using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Object;
-using v8::Persistent;
 using v8::String;
 using v8::Uint32Array;
 using v8::Uint8Array;
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -48,7 +48,6 @@ using v8::NamedPropertyHandlerConfiguration;
 using v8::Nothing;
 using v8::Object;
 using v8::ObjectTemplate;
-using v8::Persistent;
 using v8::PropertyAttribute;
 using v8::PropertyCallbackInfo;
 using v8::PropertyDescriptor;
diff --git a/src/node_contextify.h b/src/node_contextify.h
@@ -15,7 +15,7 @@ class ContextifyContext {
   enum { kSandboxObjectIndex = 1 };
 
   Environment* const env_;
-  v8::Persistent<v8::Context> context_;
+  Persistent<v8::Context> context_;
 
  public:
   ContextifyContext(Environment* env,
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -100,7 +100,6 @@ using v8::MaybeLocal;
 using v8::Null;
 using v8::Object;
 using v8::ObjectTemplate;
-using v8::Persistent;
 using v8::PropertyAttribute;
 using v8::ReadOnly;
 using v8::Signature;
diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -353,11 +353,11 @@ class SSLWrap {
   ClientHelloParser hello_parser_;
 
 #ifdef NODE__HAVE_TLSEXT_STATUS_CB
-  v8::Persistent<v8::Object> ocsp_response_;
+  Persistent<v8::Object> ocsp_response_;
 #endif  // NODE__HAVE_TLSEXT_STATUS_CB
 
 #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
-  v8::Persistent<v8::Value> sni_context_;
+  Persistent<v8::Value> sni_context_;
 #endif
 
   friend class SecureContext;
diff --git a/src/node_file.h b/src/node_file.h
@@ -14,7 +14,6 @@ using v8::HandleScope;
 using v8::Local;
 using v8::MaybeLocal;
 using v8::Object;
-using v8::Persistent;
 using v8::Promise;
 using v8::Undefined;
 using v8::Value;
diff --git a/src/node_internals.h b/src/node_internals.h
@@ -25,6 +25,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "node.h"
+#include "node_persistent.h"
 #include "util-inl.h"
 #include "env-inl.h"
 #include "uv.h"
@@ -215,7 +216,7 @@ class Environment;
 template <class TypeName>
 inline v8::Local<TypeName> PersistentToLocal(
     v8::Isolate* isolate,
-    const v8::Persistent<TypeName>& persistent);
+    const Persistent<TypeName>& persistent);
 
 // Creates a new context with Node.js-specific tweaks.  Currently, it removes
 // the `v8BreakIterator` property from the global `Intl` object if present.
diff --git a/src/node_persistent.h b/src/node_persistent.h
@@ -0,0 +1,30 @@
+#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>>;
+
+}  // namespace node
+
+#endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+
+#endif  // SRC_NODE_PERSISTENT_H_
diff --git a/src/node_zlib.cc b/src/node_zlib.cc
@@ -46,7 +46,6 @@ using v8::HandleScope;
 using v8::Local;
 using v8::Number;
 using v8::Object;
-using v8::Persistent;
 using v8::String;
 using v8::Uint32Array;
 using v8::Value;
diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
@@ -225,8 +225,8 @@ inline StreamWriteResult StreamBase::Write(
   return StreamWriteResult { async, err, req_wrap };
 }
 
-template<typename OtherBase, bool kResetPersistent>
-SimpleShutdownWrap<OtherBase, kResetPersistent>::SimpleShutdownWrap(
+template <typename OtherBase>
+SimpleShutdownWrap<OtherBase>::SimpleShutdownWrap(
     StreamBase* stream,
     v8::Local<v8::Object> req_wrap_obj)
   : ShutdownWrap(stream, req_wrap_obj),
@@ -236,23 +236,18 @@ SimpleShutdownWrap<OtherBase, kResetPersistent>::SimpleShutdownWrap(
   Wrap(req_wrap_obj, static_cast<AsyncWrap*>(this));
 }
 
-template<typename OtherBase, bool kResetPersistent>
-SimpleShutdownWrap<OtherBase, kResetPersistent>::~SimpleShutdownWrap() {
+template <typename OtherBase>
+SimpleShutdownWrap<OtherBase>::~SimpleShutdownWrap() {
   ClearWrap(static_cast<AsyncWrap*>(this)->object());
-  if (kResetPersistent) {
-    auto& persistent = static_cast<AsyncWrap*>(this)->persistent();
-    CHECK_EQ(persistent.IsEmpty(), false);
-    persistent.Reset();
-  }
 }
 
 inline ShutdownWrap* StreamBase::CreateShutdownWrap(
     v8::Local<v8::Object> object) {
   return new SimpleShutdownWrap<AsyncWrap>(this, object);
 }
 
-template<typename OtherBase, bool kResetPersistent>
-SimpleWriteWrap<OtherBase, kResetPersistent>::SimpleWriteWrap(
+template <typename OtherBase>
+SimpleWriteWrap<OtherBase>::SimpleWriteWrap(
     StreamBase* stream,
     v8::Local<v8::Object> req_wrap_obj)
   : WriteWrap(stream, req_wrap_obj),
@@ -262,14 +257,9 @@ SimpleWriteWrap<OtherBase, kResetPersistent>::SimpleWriteWrap(
   Wrap(req_wrap_obj, static_cast<AsyncWrap*>(this));
 }
 
-template<typename OtherBase, bool kResetPersistent>
-SimpleWriteWrap<OtherBase, kResetPersistent>::~SimpleWriteWrap() {
+template <typename OtherBase>
+SimpleWriteWrap<OtherBase>::~SimpleWriteWrap() {
   ClearWrap(static_cast<AsyncWrap*>(this)->object());
-  if (kResetPersistent) {
-    auto& persistent = static_cast<AsyncWrap*>(this)->persistent();
-    CHECK_EQ(persistent.IsEmpty(), false);
-    persistent.Reset();
-  }
 }
 
 inline WriteWrap* StreamBase::CreateWriteWrap(
diff --git a/src/stream_base.h b/src/stream_base.h
diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc
diff --git a/src/util-inl.h b/src/util-inl.h
diff --git a/src/util.h b/src/util.h

Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ inline BaseObject::~BaseObject() {`
`47`	`47`	`}`
`48`	`48`
`49`	`49`
`50`		`-inline v8::Persistent<v8::Object>& BaseObject::persistent() {`
	`50`	`+inline Persistent<v8::Object>& BaseObject::persistent() {`
`51`	`51`	`return persistent_handle_;`
`52`	`52`	`}`
`53`	`53`