From 8ff43adfce8a216713f0b09e981c78700d93ed71 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 26 Jan 2018 10:53:14 -0800 Subject: [PATCH 1/6] src: handle exceptions in env->SetImmediates PR-URL: https://github.com/nodejs/node/pull/18297 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina --- src/env.cc | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/env.cc b/src/env.cc index ca023125c2627c..50038b73160db7 100644 --- a/src/env.cc +++ b/src/env.cc @@ -296,13 +296,26 @@ void Environment::RunAndClearNativeImmediates() { size_t ref_count = 0; std::vector 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->keep_alive_) + it->keep_alive_->Reset(); + 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); From 9f426f784ad7b291d7c43b0bb17aa73592b8ed3d Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 21 Feb 2018 15:24:18 +0100 Subject: [PATCH 2/6] src: prevent persistent handle resource leaks 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: https://github.com/nodejs/node/pull/18656 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- node.gyp | 3 ++- src/async_wrap.cc | 4 ++-- src/base_object-inl.h | 2 +- src/base_object.h | 10 +++------- src/env-inl.h | 4 ++-- src/env.h | 5 ++--- src/inspector_agent.cc | 1 - src/inspector_agent.h | 6 +++--- src/inspector_js_api.cc | 1 - src/module_wrap.h | 8 ++++---- src/node_api.cc | 26 +++++++++++++------------- src/node_buffer.cc | 1 - src/node_contextify.cc | 1 - src/node_contextify.h | 2 +- src/node_crypto.cc | 1 - src/node_crypto.h | 4 ++-- src/node_internals.h | 3 ++- src/node_persistent.h | 30 ++++++++++++++++++++++++++++++ src/node_zlib.cc | 1 - src/stream_base-inl.h | 26 ++++++++------------------ src/stream_base.h | 4 ++-- src/stream_wrap.cc | 4 ++-- src/util-inl.h | 8 ++++---- src/util.h | 7 ++++--- 24 files changed, 87 insertions(+), 75 deletions(-) create mode 100644 src/node_persistent.h diff --git a/node.gyp b/node.gyp index 7da486ff6d8c7b..1f47e88a3132fe 100644 --- a/node.gyp +++ b/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', diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 93bd3d4864fd5d..f8e0992ee511b4 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -410,8 +410,8 @@ static void DisablePromiseHook(const FunctionCallbackInfo& args) { class DestroyParam { public: double asyncId; - v8::Persistent target; - v8::Persistent propBag; + Persistent target; + Persistent propBag; }; diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 900fc2b3edb9ca..6720bd6d886e1b 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -47,7 +47,7 @@ inline BaseObject::~BaseObject() { } -inline v8::Persistent& BaseObject::persistent() { +inline Persistent& BaseObject::persistent() { return persistent_handle_; } diff --git a/src/base_object.h b/src/base_object.h index 965683d029e43e..0640f91cbf5b88 100644 --- 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 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& persistent(); + inline Persistent& 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 persistent_handle_; + Persistent persistent_handle_; Environment* env_; }; diff --git a/src/env-inl.h b/src/env-inl.h index 0a2adae2bb0ade..5c0ff282f52829 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -541,8 +541,8 @@ void Environment::CreateImmediate(native_immediate_callback cb, native_immediate_callbacks_.push_back({ cb, data, - std::unique_ptr>(obj.IsEmpty() ? - nullptr : new v8::Persistent(isolate_, obj)), + std::unique_ptr>(obj.IsEmpty() ? + nullptr : new Persistent(isolate_, obj)), ref }); immediate_info()->count_inc(1); diff --git a/src/env.h b/src/env.h index 86612bf354d2b3..b08b767cb7cba1 100644 --- a/src/env.h +++ b/src/env.h @@ -800,7 +800,7 @@ class Environment { struct NativeImmediateCallback { native_immediate_callback cb_; void* data_; - std::unique_ptr> keep_alive_; + std::unique_ptr> keep_alive_; bool refed_; }; std::vector native_immediate_callbacks_; @@ -811,8 +811,7 @@ class Environment { v8::Local promise, v8::Local parent); -#define V(PropertyName, TypeName) \ - v8::Persistent PropertyName ## _; +#define V(PropertyName, TypeName) Persistent PropertyName ## _; ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 7ba1a144711524..e143d316d2e6fa 100644 --- 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 index 0555f5e18c2129..56fb407930fac5 100644 --- 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& fn); + const Persistent& fn); node::Environment* parent_env_; std::unique_ptr client_; @@ -109,8 +109,8 @@ class Agent { bool pending_enable_async_hook_; bool pending_disable_async_hook_; - v8::Persistent enable_async_hook_function_; - v8::Persistent disable_async_hook_function_; + Persistent enable_async_hook_function_; + Persistent disable_async_hook_function_; }; } // namespace inspector diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 428d8391f2581a..9a4857f1a10cab 100644 --- 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 index bedf665165c8f6..04f27decc2aeb4 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -49,11 +49,11 @@ class ModuleWrap : public BaseObject { v8::Local specifier, v8::Local referrer); - v8::Persistent module_; - v8::Persistent url_; + Persistent module_; + Persistent url_; bool linked_ = false; - std::unordered_map> resolve_cache_; - v8::Persistent context_; + std::unordered_map> resolve_cache_; + Persistent context_; }; } // namespace loader diff --git a/src/node_api.cc b/src/node_api.cc index 2c5f3066f728b1..7b0e110579b65b 100644 --- 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 last_exception; - v8::Persistent wrap_template; - v8::Persistent function_data_template; - v8::Persistent accessor_data_template; + node::Persistent last_exception; + node::Persistent wrap_template; + node::Persistent function_data_template; + node::Persistent 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) == sizeof(napi_value), "Cannot convert between v8::Local and napi_value"); static -napi_deferred JsDeferredFromV8Persistent(v8::Persistent* local) { +napi_deferred JsDeferredFromNodePersistent(node::Persistent* local) { return reinterpret_cast(local); } static -v8::Persistent* V8PersistentFromJsDeferred(napi_deferred local) { - return reinterpret_cast*>(local); +node::Persistent* NodePersistentFromJsDeferred(napi_deferred local) { + return reinterpret_cast*>(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 _persistent; + node::Persistent _persistent; uint32_t _refcount; bool _delete_self; }; @@ -846,8 +846,8 @@ napi_status ConcludeDeferred(napi_env env, CHECK_ARG(env, result); v8::Local context = env->isolate->GetCurrentContext(); - v8::Persistent* deferred_ref = - V8PersistentFromJsDeferred(deferred); + node::Persistent* deferred_ref = + NodePersistentFromJsDeferred(deferred); v8::Local v8_deferred = v8::Local::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(); + auto v8_deferred = new node::Persistent(); 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 index dff9d3c0995e02..d1983bde5ee354 100644 --- 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 index aac72514575695..d6b77c6145b2cb 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -50,7 +50,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 index c2b5b4dd9c93aa..33252e39abc378 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -15,7 +15,7 @@ class ContextifyContext { enum { kSandboxObjectIndex = 1 }; Environment* const env_; - v8::Persistent context_; + Persistent context_; public: ContextifyContext(Environment* env, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index eb62fc1bcdc315..5ac845c2b35be9 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -107,7 +107,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 index b866117f844358..1d663e0a75a866 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -354,11 +354,11 @@ class SSLWrap { ClientHelloParser hello_parser_; #ifdef NODE__HAVE_TLSEXT_STATUS_CB - v8::Persistent ocsp_response_; + Persistent ocsp_response_; #endif // NODE__HAVE_TLSEXT_STATUS_CB #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - v8::Persistent sni_context_; + Persistent sni_context_; #endif friend class SecureContext; diff --git a/src/node_internals.h b/src/node_internals.h index ced92da3216a28..af716c83997c78 100644 --- 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" @@ -214,7 +215,7 @@ class Environment; template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& 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 new file mode 100644 index 00000000000000..762842dd4bd373 --- /dev/null +++ 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 +struct ResetInDestructorPersistentTraits { + static const bool kResetInDestructor = true; + template + // Disallow copy semantics by leaving this unimplemented. + inline static void Copy( + const v8::Persistent&, + v8::Persistent>*); +}; + +// 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 +using Persistent = v8::Persistent>; + +} // 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 index 8ef4383e0355de..388630d507a433 100644 --- 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 index 008f9c01513bc0..81adf7a866b927 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -223,8 +223,8 @@ inline StreamWriteResult StreamBase::Write( return StreamWriteResult { async, err, req_wrap }; } -template -SimpleShutdownWrap::SimpleShutdownWrap( +template +SimpleShutdownWrap::SimpleShutdownWrap( StreamBase* stream, v8::Local req_wrap_obj) : ShutdownWrap(stream, req_wrap_obj), @@ -234,14 +234,9 @@ SimpleShutdownWrap::SimpleShutdownWrap( Wrap(req_wrap_obj, static_cast(this)); } -template -SimpleShutdownWrap::~SimpleShutdownWrap() { +template +SimpleShutdownWrap::~SimpleShutdownWrap() { ClearWrap(static_cast(this)->object()); - if (kResetPersistent) { - auto& persistent = static_cast(this)->persistent(); - CHECK_EQ(persistent.IsEmpty(), false); - persistent.Reset(); - } } inline ShutdownWrap* StreamBase::CreateShutdownWrap( @@ -249,8 +244,8 @@ inline ShutdownWrap* StreamBase::CreateShutdownWrap( return new SimpleShutdownWrap(this, object); } -template -SimpleWriteWrap::SimpleWriteWrap( +template +SimpleWriteWrap::SimpleWriteWrap( StreamBase* stream, v8::Local req_wrap_obj) : WriteWrap(stream, req_wrap_obj), @@ -260,14 +255,9 @@ SimpleWriteWrap::SimpleWriteWrap( Wrap(req_wrap_obj, static_cast(this)); } -template -SimpleWriteWrap::~SimpleWriteWrap() { +template +SimpleWriteWrap::~SimpleWriteWrap() { ClearWrap(static_cast(this)->object()); - if (kResetPersistent) { - auto& persistent = static_cast(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 index 59b8ee7b7221f0..8af05059f49e47 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -322,7 +322,7 @@ class StreamBase : public StreamResource { // `OtherBase` must have a constructor that matches the `AsyncWrap` // constructors’s (Environment*, Local, AsyncWrap::Provider) signature // and be a subclass of `AsyncWrap`. -template +template class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { public: SimpleShutdownWrap(StreamBase* stream, @@ -333,7 +333,7 @@ class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { size_t self_size() const override { return sizeof(*this); } }; -template +template class SimpleWriteWrap : public WriteWrap, public OtherBase { public: SimpleWriteWrap(StreamBase* stream, diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index e1df9edd39e151..27fe48d1165c75 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -264,8 +264,8 @@ void LibuvStreamWrap::SetBlocking(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(uv_stream_set_blocking(wrap->stream(), enable)); } -typedef SimpleShutdownWrap, false> LibuvShutdownWrap; -typedef SimpleWriteWrap, false> LibuvWriteWrap; +typedef SimpleShutdownWrap> LibuvShutdownWrap; +typedef SimpleWriteWrap> LibuvWriteWrap; ShutdownWrap* LibuvStreamWrap::CreateShutdownWrap(Local object) { return new LibuvShutdownWrap(this, object); diff --git a/src/util-inl.h b/src/util-inl.h index c5a25c91ffb088..d07cfea9227fbe 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -168,7 +168,7 @@ inline ContainerOfHelper ContainerOf(Inner Outer::*field, template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent) { + const Persistent& persistent) { if (persistent.IsWeak()) { return WeakPersistentToLocal(isolate, persistent); } else { @@ -178,15 +178,15 @@ inline v8::Local PersistentToLocal( template inline v8::Local StrongPersistentToLocal( - const v8::Persistent& persistent) { + const Persistent& persistent) { return *reinterpret_cast*>( - const_cast*>(&persistent)); + const_cast*>(&persistent)); } template inline v8::Local WeakPersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent) { + const Persistent& persistent) { return v8::Local::New(isolate, persistent); } diff --git a/src/util.h b/src/util.h index 21c566a4ca6cd6..7c679952d5fb1f 100644 --- a/src/util.h +++ b/src/util.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "node_persistent.h" #include "v8.h" #include @@ -218,7 +219,7 @@ inline ContainerOfHelper ContainerOf(Inner Outer::*field, template inline v8::Local PersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& persistent); // Unchecked conversion from a non-weak Persistent to Local, // use with care! @@ -227,12 +228,12 @@ inline v8::Local PersistentToLocal( // scope, it will destroy the reference to the object. template inline v8::Local StrongPersistentToLocal( - const v8::Persistent& persistent); + const Persistent& persistent); template inline v8::Local WeakPersistentToLocal( v8::Isolate* isolate, - const v8::Persistent& persistent); + const Persistent& persistent); // Convenience wrapper around v8::String::NewFromOneByte(). inline v8::Local OneByteString(v8::Isolate* isolate, From 097009a17f0ea2170cc8db8ec225d8f655223cda Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 6 Mar 2018 23:33:48 +0100 Subject: [PATCH 3/6] [squash] v9.x-specific fixups --- src/node_crypto.cc | 3 ++- src/node_crypto.h | 8 ++++---- src/node_file.cc | 6 ++++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 5ac845c2b35be9..a897f53cc10dc7 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3600,7 +3600,8 @@ void Connection::GetServername(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&conn, args.Holder()); if (conn->is_server() && !conn->servername_.IsEmpty()) { - args.GetReturnValue().Set(conn->servername_); + args.GetReturnValue().Set( + PersistentToLocal(args.GetIsolate(), conn->servername_)); } else { args.GetReturnValue().Set(false); } diff --git a/src/node_crypto.h b/src/node_crypto.h index 1d663e0a75a866..2f831b6812c282 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -380,13 +380,13 @@ class Connection : public AsyncWrap, public SSLWrap { void NewSessionDoneCb(); #ifndef OPENSSL_NO_NEXTPROTONEG - v8::Persistent npnProtos_; - v8::Persistent selectedNPNProto_; + Persistent npnProtos_; + Persistent selectedNPNProto_; #endif #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - v8::Persistent sniObject_; - v8::Persistent servername_; + Persistent sniObject_; + Persistent servername_; #endif size_t self_size() const override { return sizeof(*this); } diff --git a/src/node_file.cc b/src/node_file.cc index 9df13be5bd2dff..10655e54e54e90 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -366,7 +366,8 @@ class fs_req_wrap { After(uv_req); \ req_wrap = nullptr; \ } else { \ - args.GetReturnValue().Set(req_wrap->persistent()); \ + args.GetReturnValue().Set( \ + PersistentToLocal(env->isolate(), req_wrap->persistent())); \ } #define ASYNC_CALL(func, req, encoding, ...) \ @@ -1140,7 +1141,8 @@ static void WriteString(const FunctionCallbackInfo& args) { return; } - return args.GetReturnValue().Set(req_wrap->persistent()); + return args.GetReturnValue().Set( + PersistentToLocal(env->isolate(), req_wrap->persistent())); } From 3de8e578acafba6e7d33b4dd9a30e211b63e2bfd Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 21 Feb 2018 15:24:18 +0100 Subject: [PATCH 4/6] src: remove unnecessary Reset() calls The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. PR-URL: https://github.com/nodejs/node/pull/18656 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- src/async_wrap.cc | 2 -- src/base_object-inl.h | 8 +------- src/base_object.h | 2 +- src/cares_wrap.cc | 1 - src/env-inl.h | 3 --- src/env.cc | 2 -- src/handle_wrap.cc | 6 ------ src/handle_wrap.h | 1 - src/inspector_js_api.cc | 5 ----- src/module_wrap.cc | 5 ----- src/node_api.cc | 17 ----------------- src/node_buffer.cc | 6 ------ src/node_contextify.cc | 10 ---------- src/node_contextify.h | 1 - src/node_crypto.cc | 3 --- src/node_crypto.h | 8 -------- src/node_http2.cc | 8 -------- src/node_http_parser.cc | 1 - src/req_wrap-inl.h | 1 - src/tcp_wrap.cc | 5 ----- src/tcp_wrap.h | 1 - src/tls_wrap.cc | 6 ------ 22 files changed, 2 insertions(+), 100 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index f8e0992ee511b4..cd9f26d7782d46 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -426,8 +426,6 @@ void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo& 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 index 6720bd6d886e1b..51ef46599667df 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -42,11 +42,6 @@ inline BaseObject::BaseObject(Environment* env, v8::Local handle) } -inline BaseObject::~BaseObject() { - CHECK(persistent_handle_.IsEmpty()); -} - - inline Persistent& BaseObject::persistent() { return persistent_handle_; } @@ -65,8 +60,7 @@ inline Environment* BaseObject::env() const { template inline void BaseObject::WeakCallback( const v8::WeakCallbackInfo& data) { - std::unique_ptr self(data.GetParameter()); - self->persistent().Reset(); + delete data.GetParameter(); } diff --git a/src/base_object.h b/src/base_object.h index 0640f91cbf5b88..478499bbfeb5b2 100644 --- 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 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 index 165a8cda20618b..b8da20346692c3 100644 --- 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 index 5c0ff282f52829..f647f428c324ff 100644 --- 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 index 50038b73160db7..455f5980731f3f 100644 --- 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 index 7dcafa2ce6e842..a3b0209eb3121f 100644 --- 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(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 index 19fd36891a2fed..e7a335f5140253 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -75,7 +75,6 @@ class HandleWrap : public AsyncWrap { v8::Local 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 index 9a4857f1a10cab..1cced9420aea6c 100644 --- 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) { 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 index 24e73cc3e101c6..6393b4600940a9 100644 --- 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& args) { @@ -215,8 +212,6 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo& 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 index 7b0e110579b65b..63ce1d8e86955e 100644 --- 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 last_exception; node::Persistent 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 index d1983bde5ee354..f9a807602f612c 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -102,7 +102,6 @@ class CallbackInfo { FreeCallback callback, char* data, void* hint); - ~CallbackInfo(); Persistent persistent_; FreeCallback const callback_; char* const data_; @@ -146,11 +145,6 @@ CallbackInfo::CallbackInfo(Isolate* isolate, } -CallbackInfo::~CallbackInfo() { - persistent_.Reset(); -} - - void CallbackInfo::WeakCallback( const WeakCallbackInfo& data) { CallbackInfo* self = data.GetParameter(); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index d6b77c6145b2cb..f49a2362769bc2 100644 --- 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(this); } - - - ~ContextifyScript() override { - script_.Reset(); - } }; diff --git a/src/node_contextify.h b/src/node_contextify.h index 33252e39abc378..e6b7e0a9e080f2 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -21,7 +21,6 @@ class ContextifyContext { ContextifyContext(Environment* env, v8::Local sandbox_obj, v8::Local options_obj); - ~ContextifyContext(); v8::Local CreateDataWrapper(Environment* env); v8::Local CreateV8Context(Environment* env, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index a897f53cc10dc7..0f6212b81b954c 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2803,7 +2803,6 @@ void SSLWrap::CertCbDone(const FunctionCallbackInfo& args) { if (cons->HasInstance(ctx)) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, ctx.As()); - w->sni_context_.Reset(); w->sni_context_.Reset(env->isolate(), ctx); int rv; @@ -5562,7 +5561,6 @@ class PBKDF2Request : public AsyncWrap { keylen_ = 0; ClearWrap(object()); - persistent().Reset(); } uv_work_t* work_req() { @@ -5729,7 +5727,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 index 2f831b6812c282..f1efa811985681 100644 --- 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 index 5240ea8b5ca864..ed6bdec7bb896c 100644 --- 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 index d4044f8bbeea7b..207310f4068f43 100644 --- 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 index 4a7984e649c733..11b1389fa0e771 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -25,7 +25,6 @@ template ReqWrap::~ReqWrap() { CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched(). CHECK_EQ(false, persistent().IsEmpty()); - persistent().Reset(); } template diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index a0a58fb1b5cc8d..61b08217b8f129 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -174,11 +174,6 @@ TCPWrap::TCPWrap(Environment* env, Local object, ProviderType provider) } -TCPWrap::~TCPWrap() { - CHECK(persistent().IsEmpty()); -} - - void TCPWrap::SetNoDelay(const FunctionCallbackInfo& args) { TCPWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, diff --git a/src/tcp_wrap.h b/src/tcp_wrap.h index a7f6b1901981f6..2ab50f1fdcdfab 100644 --- a/src/tcp_wrap.h +++ b/src/tcp_wrap.h @@ -55,7 +55,6 @@ class TCPWrap : public ConnectionWrap { TCPWrap(Environment* env, v8::Local object, ProviderType provider); - ~TCPWrap(); static void New(const v8::FunctionCallbackInfo& args); static void SetNoDelay(const v8::FunctionCallbackInfo& args); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 18b56afece9e09..7ff49522438d65 100644 --- 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(ctx.As()); From 8c363cd48f7334ec10155b1c00b13e504c61881e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 21 Feb 2018 15:24:18 +0100 Subject: [PATCH 5/6] src: don't touch js object in Http2Session dtor Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. PR-URL: https://github.com/nodejs/node/pull/18656 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- src/node_http2.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index ed6bdec7bb896c..f1454aa11e58e9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -531,8 +531,6 @@ Http2Session::Http2Session(Environment* env, Http2Session::~Http2Session() { CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0); - if (!object().IsEmpty()) - ClearWrap(object()); DEBUG_HTTP2SESSION(this, "freeing nghttp2 session"); nghttp2_session_del(session_); } From 8827b71b552d291e5eafcbebce2529ec9e6e6eed Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 26 Feb 2018 15:23:25 +0100 Subject: [PATCH 6/6] http2: no stream destroy while its data is on the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a crash that occurred when a `Http2Stream` write is completed after it is already destroyed. Instead, don’t destroy the stream in that case and wait for GC to take over. PR-URL: https://github.com/nodejs/node/pull/19002 Fixes: https://github.com/nodejs/node/issues/18973 Reviewed-By: James M Snell Reviewed-By: Anatoli Papirovski --- src/node_http2.cc | 20 ++++-- src/node_http2.h | 3 + ...tp2-write-finishes-after-stream-destroy.js | 62 +++++++++++++++++++ 3 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-http2-write-finishes-after-stream-destroy.js diff --git a/src/node_http2.cc b/src/node_http2.cc index f1454aa11e58e9..7650969f8639ce 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1700,6 +1700,14 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { stream_buf_ = uv_buf_init(nullptr, 0); } +bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) { + for (const nghttp2_stream_write& wr : outgoing_buffers_) { + if (wr.req_wrap != nullptr && wr.req_wrap->stream() == stream) + return true; + } + return false; +} + // Every Http2Session session is tightly bound to a single i/o StreamBase // (typically a net.Socket or tls.TLSSocket). The lifecycle of the two is // tightly coupled with all data transfer between the two happening at the @@ -1753,13 +1761,11 @@ Http2Stream::Http2Stream( Http2Stream::~Http2Stream() { + DEBUG_HTTP2STREAM(this, "tearing down stream"); if (session_ != nullptr) { session_->RemoveStream(this); session_ = nullptr; } - - if (!object().IsEmpty()) - ClearWrap(object()); } // Notify the Http2Stream that a new block of HEADERS is being processed. @@ -1837,7 +1843,7 @@ inline void Http2Stream::Destroy() { Http2Stream* stream = static_cast(data); // Free any remaining outgoing data chunks here. This should be done // here because it's possible for destroy to have been called while - // we still have qeueued outbound writes. + // we still have queued outbound writes. while (!stream->queue_.empty()) { nghttp2_stream_write& head = stream->queue_.front(); if (head.req_wrap != nullptr) @@ -1845,7 +1851,11 @@ inline void Http2Stream::Destroy() { stream->queue_.pop(); } - delete stream; + // We can destroy the stream now if there are no writes for it + // already on the socket. Otherwise, we'll wait for the garbage collector + // to take care of cleaning up. + if (!stream->session()->HasWritesOnSocketForStream(stream)) + delete stream; }, this, this->object()); statistics_.end_time = uv_hrtime(); diff --git a/src/node_http2.h b/src/node_http2.h index 217c19c09287af..8f6662a0160bec 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -878,6 +878,9 @@ class Http2Session : public AsyncWrap, public StreamListener { // Removes a stream instance from this session inline void RemoveStream(Http2Stream* stream); + // Indicates whether there currently exist outgoing buffers for this stream. + bool HasWritesOnSocketForStream(Http2Stream* stream); + // Write data to the session inline ssize_t Write(const uv_buf_t* bufs, size_t nbufs); diff --git a/test/parallel/test-http2-write-finishes-after-stream-destroy.js b/test/parallel/test-http2-write-finishes-after-stream-destroy.js new file mode 100644 index 00000000000000..3b2dd4bcd4e548 --- /dev/null +++ b/test/parallel/test-http2-write-finishes-after-stream-destroy.js @@ -0,0 +1,62 @@ +// Flags: --expose-gc +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const makeDuplexPair = require('../common/duplexpair'); + +// Make sure the Http2Stream destructor works, since we don't clean the +// stream up like we would otherwise do. +process.on('exit', global.gc); + +{ + const { clientSide, serverSide } = makeDuplexPair(); + + let serverSideHttp2Stream; + let serverSideHttp2StreamDestroyed = false; + const server = http2.createServer(); + server.on('stream', common.mustCall((stream, headers) => { + serverSideHttp2Stream = stream; + stream.respond({ + 'content-type': 'text/html', + ':status': 200 + }); + + const originalWrite = serverSide._write; + serverSide._write = (buf, enc, cb) => { + if (serverSideHttp2StreamDestroyed) { + serverSide.destroy(); + serverSide.write = () => {}; + } else { + setImmediate(() => { + originalWrite.call(serverSide, buf, enc, () => setImmediate(cb)); + }); + } + }; + + // Enough data to fit into a single *session* window, + // not enough data to fit into a single *stream* window. + stream.write(Buffer.alloc(40000)); + })); + + server.emit('connection', serverSide); + + const client = http2.connect('http://localhost:80', { + createConnection: common.mustCall(() => clientSide) + }); + + const req = client.request({ ':path': '/' }); + + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 200); + })); + + req.on('data', common.mustCallAtLeast(() => { + if (!serverSideHttp2StreamDestroyed) { + serverSideHttp2Stream.destroy(); + serverSideHttp2StreamDestroyed = true; + } + })); +}