src: remove unnecessary Reset() calls

bnoordhuis · MylesBorins · commit f89f659dcf03 · 2018-03-07T09:30:22.000-08:00
The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. Backport-PR-URL: #19185 PR-URL: #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/src/async_wrap.cc b/src/async_wrap.cc
@@ -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;
 }
 
diff --git a/src/base_object-inl.h b/src/base_object-inl.h
@@ -42,11 +42,6 @@ inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
 }
 
 
-inline BaseObject::~BaseObject() {
-  CHECK(persistent_handle_.IsEmpty());
-}
-
-
 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();
 }
 
 
diff --git a/src/base_object.h b/src/base_object.h
@@ -34,7 +34,7 @@ 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.
diff --git a/src/cares_wrap.cc b/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.
diff --git a/src/env-inl.h b/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_;
diff --git a/src/env.cc b/src/env.cc
@@ -300,8 +300,6 @@ void Environment::RunAndClearNativeImmediates() {
       v8::TryCatch try_catch(isolate());
       for (auto it = list.begin(); it != list.end(); ++it) {
         it->cb_(this, it->data_);
-        if (it->keep_alive_)
-          it->keep_alive_->Reset();
         if (it->refed_)
           ref_count++;
         if (UNLIKELY(try_catch.HasCaught())) {
diff --git a/src/handle_wrap.cc b/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;
 }
 
diff --git a/src/handle_wrap.h b/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;
diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
@@ -84,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);
   }
@@ -111,7 +107,6 @@ class JSBindingsConnection : public AsyncWrap {
     delegate_.Disconnect();
     if (!persistent().IsEmpty()) {
       ClearWrap(object());
-      persistent().Reset();
     }
     delete this;
   }
diff --git a/src/module_wrap.cc b/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)) {
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -31,12 +31,6 @@ 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;
   node::Persistent<v8::Value> last_exception;
   node::Persistent<v8::ObjectTemplate> wrap_template;
@@ -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;
@@ -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);
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -102,7 +102,6 @@ class CallbackInfo {
                       FreeCallback callback,
                       char* data,
                       void* hint);
-  ~CallbackInfo();
   Persistent<ArrayBuffer> persistent_;
   FreeCallback const callback_;
   char* const data_;
@@ -146,11 +145,6 @@ CallbackInfo::CallbackInfo(Isolate* isolate,
 }
 
 
-CallbackInfo::~CallbackInfo() {
-  persistent_.Reset();
-}
-
-
 void CallbackInfo::WeakCallback(
     const WeakCallbackInfo<CallbackInfo>& data) {
   CallbackInfo* self = data.GetParameter();
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -109,11 +109,6 @@ ContextifyContext::ContextifyContext(
 }
 
 
-ContextifyContext::~ContextifyContext() {
-  context_.Reset();
-}
-
-
 // This is an object that just keeps an internal pointer to this
 // ContextifyContext.  It's passed to the NamedPropertyHandler.  If we
 // pass the main JavaScript context object we're embedded in, then the
@@ -1157,11 +1152,6 @@ class ContextifyScript : public BaseObject {
       : BaseObject(env, object) {
     MakeWeak<ContextifyScript>(this);
   }
-
-
-  ~ContextifyScript() override {
-    script_.Reset();
-  }
 };
 
 
diff --git a/src/node_contextify.h b/src/node_contextify.h
@@ -21,7 +21,6 @@ class ContextifyContext {
   ContextifyContext(Environment* env,
                     v8::Local<v8::Object> sandbox_obj,
                     v8::Local<v8::Object> options_obj);
-  ~ContextifyContext();
 
   v8::Local<v8::Value> CreateDataWrapper(Environment* env);
   v8::Local<v8::Context> CreateV8Context(Environment* env,
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -2826,7 +2826,6 @@ void SSLWrap<Base>::CertCbDone(const FunctionCallbackInfo<Value>& args) {
   if (cons->HasInstance(ctx)) {
     SecureContext* sc;
     ASSIGN_OR_RETURN_UNWRAP(&sc, ctx.As<Object>());
-    w->sni_context_.Reset();
     w->sni_context_.Reset(env->isolate(), ctx);
 
     int rv;
@@ -5580,7 +5579,6 @@ class PBKDF2Request : public AsyncWrap {
     keylen_ = 0;
 
     ClearWrap(object());
-    persistent().Reset();
   }
 
   uv_work_t* work_req() {
@@ -5747,7 +5745,6 @@ class RandomBytesRequest : public AsyncWrap {
 
   ~RandomBytesRequest() override {
     ClearWrap(object());
-    persistent().Reset();
   }
 
   uv_work_t* work_req() {
diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -225,14 +225,6 @@ class SSLWrap {
       SSL_SESSION_free(next_sess_);
       next_sess_ = nullptr;
     }
-
-#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
-    sni_context_.Reset();
-#endif
-
-#ifdef NODE__HAVE_TLSEXT_STATUS_CB
-    ocsp_response_.Reset();
-#endif  // NODE__HAVE_TLSEXT_STATUS_CB
   }
 
   inline SSL* ssl() const { return ssl_; }
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -278,8 +278,6 @@ Http2Session::Http2Settings::Http2Settings(
 Http2Session::Http2Settings::~Http2Settings() {
   if (!object().IsEmpty())
     ClearWrap(object());
-  persistent().Reset();
-  CHECK(persistent().IsEmpty());
 }
 
 // Generates a Buffer that contains the serialized payload of a SETTINGS
@@ -535,8 +533,6 @@ Http2Session::~Http2Session() {
   CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0);
   if (!object().IsEmpty())
     ClearWrap(object());
-  persistent().Reset();
-  CHECK(persistent().IsEmpty());
   DEBUG_HTTP2SESSION(this, "freeing nghttp2 session");
   nghttp2_session_del(session_);
 }
@@ -1766,8 +1762,6 @@ Http2Stream::~Http2Stream() {
 
   if (!object().IsEmpty())
     ClearWrap(object());
-  persistent().Reset();
-  CHECK(persistent().IsEmpty());
 }
 
 // Notify the Http2Stream that a new block of HEADERS is being processed.
@@ -2784,8 +2778,6 @@ Http2Session::Http2Ping::Http2Ping(
 Http2Session::Http2Ping::~Http2Ping() {
   if (!object().IsEmpty())
     ClearWrap(object());
-  persistent().Reset();
-  CHECK(persistent().IsEmpty());
 }
 
 void Http2Session::Http2Ping::Send(uint8_t* payload) {
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
@@ -157,7 +157,6 @@ class Parser : public AsyncWrap, public StreamListener {
 
   ~Parser() override {
     ClearWrap(object());
-    persistent().Reset();
   }
 
 
diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h
@@ -25,7 +25,6 @@ template <typename T>
 ReqWrap<T>::~ReqWrap() {
   CHECK_EQ(req_.data, this);  // Assert that someone has called Dispatched().
   CHECK_EQ(false, persistent().IsEmpty());
-  persistent().Reset();
 }
 
 template <typename T>
diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc
@@ -174,11 +174,6 @@ TCPWrap::TCPWrap(Environment* env, Local<Object> object, ProviderType provider)
 }
 
 
-TCPWrap::~TCPWrap() {
-  CHECK(persistent().IsEmpty());
-}
-
-
 void TCPWrap::SetNoDelay(const FunctionCallbackInfo<Value>& args) {
   TCPWrap* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap,
diff --git a/src/tcp_wrap.h b/src/tcp_wrap.h
@@ -55,7 +55,6 @@ class TCPWrap : public ConnectionWrap<TCPWrap, uv_tcp_t> {
 
   TCPWrap(Environment* env, v8::Local<v8::Object> object,
           ProviderType provider);
-  ~TCPWrap();
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SetNoDelay(const v8::FunctionCallbackInfo<v8::Value>& args);
diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
@@ -86,12 +86,7 @@ TLSWrap::TLSWrap(Environment* env,
 TLSWrap::~TLSWrap() {
   enc_in_ = nullptr;
   enc_out_ = nullptr;
-
   sc_ = nullptr;
-
-#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
-  sni_context_.Reset();
-#endif  // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
 }
 
 
@@ -852,7 +847,6 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) {
     return SSL_TLSEXT_ERR_NOACK;
   }
 
-  p->sni_context_.Reset();
   p->sni_context_.Reset(env->isolate(), ctx);
 
   SecureContext* sc = Unwrap<SecureContext>(ctx.As<Object>());

Original file line number	Diff line number	Diff line change
`@@ -426,8 +426,6 @@ void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo<DestroyParam>& info) {`
`426`	`426`	`if (val->IsFalse()) {`
`427`	`427`	`AsyncWrap::EmitDestroy(env, p->asyncId);`
`428`	`428`	`}`
`429`		`- p->target.Reset();`
`430`		`- p->propBag.Reset();`
`431`	`429`	`delete p;`
`432`	`430`	`}`
`433`	`431`
Original file line number	Diff line number	Diff line change
`@@ -42,11 +42,6 @@ inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)`
`42`	`42`	`}`
`43`	`43`
`44`	`44`
`45`		`-inline BaseObject::~BaseObject() {`
`46`		`- CHECK(persistent_handle_.IsEmpty());`
`47`		`-}`
`48`		`-`
`49`		`-`
`50`	`45`	`inline Persistent<v8::Object>& BaseObject::persistent() {`
`51`	`46`	`return persistent_handle_;`
`52`	`47`	`}`
`@@ -65,8 +60,7 @@ inline Environment* BaseObject::env() const {`
`65`	`60`	`template <typename Type>`
`66`	`61`	`inline void BaseObject::WeakCallback(`
`67`	`62`	`const v8::WeakCallbackInfo<Type>& data) {`
`68`		`- std::unique_ptr<Type> self(data.GetParameter());`
`69`		`- self->persistent().Reset();`
	`63`	`+ delete data.GetParameter();`
`70`	`64`	`}`
`71`	`65`
`72`	`66`
Original file line number	Diff line number	Diff line change
`@@ -598,7 +598,6 @@ class QueryWrap : public AsyncWrap {`
`598`	`598`	`~QueryWrap() override {`
`599`	`599`	`CHECK_EQ(false, persistent().IsEmpty());`
`600`	`600`	`ClearWrap(object());`
`601`		`- persistent().Reset();`
`602`	`601`	`}`
`603`	`602`
`604`	`603`	`// Subclasses should implement the appropriate Send method.`
Original file line number	Diff line number	Diff line change
`@@ -84,10 +84,6 @@ class JSBindingsConnection : public AsyncWrap {`
`84`	`84`	`inspector->Connect(&delegate_);`
`85`	`85`	`}`
`86`	`86`
`87`		`- ~JSBindingsConnection() override {`
`88`		`- callback_.Reset();`
`89`		`- }`
`90`		`-`
`91`	`87`	`void OnMessage(Local<Value> value) {`
`92`	`88`	`MakeCallback(callback_.Get(env()->isolate()), 1, &value);`
`93`	`89`	`}`
`@@ -111,7 +107,6 @@ class JSBindingsConnection : public AsyncWrap {`
`111`	`107`	`delegate_.Disconnect();`
`112`	`108`	`if (!persistent().IsEmpty()) {`
`113`	`109`	`ClearWrap(object());`
`114`		`- persistent().Reset();`
`115`	`110`	`}`
`116`	`111`	`delete this;`
`117`	`112`	`}`
Original file line number	Diff line number	Diff line change
`@@ -59,9 +59,6 @@ ModuleWrap::~ModuleWrap() {`
`59`	`59`	`break;`
`60`	`60`	`}`
`61`	`61`	`}`
`62`		`-`
`63`		`- module_.Reset();`
`64`		`- context_.Reset();`
`65`	`62`	`}`
`66`	`63`
`67`	`64`	`void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {`
`@@ -215,8 +212,6 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {`
`215`	`212`	`module->InstantiateModule(context, ModuleWrap::ResolveCallback);`
`216`	`213`
`217`	`214`	`// clear resolve cache on instantiate`
`218`		`- for (auto& entry : obj->resolve_cache_)`
`219`		`- entry.second.Reset();`
`220`	`215`	`obj->resolve_cache_.clear();`
`221`	`216`
`222`	`217`	`if (!ok.FromMaybe(false)) {`