From 7e6104f1bb63a07885b08f277c3b708caa6b8a23 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 01:29:07 +0200 Subject: src: introduce custom smart pointers for `BaseObject`s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Referring to `BaseObject` instances using standard C++ smart pointers can interfere with BaseObject’s own cleanup mechanisms (explicit delete, delete-on-GC and delete-on-cleanup). Introducing custom smart pointers allows referring to `BaseObject`s safely while keeping those mechanisms intact. Refs: https://github.com/nodejs/quic/pull/141 Refs: https://github.com/nodejs/quic/pull/149 Reviewed-By: James M Snell PR-URL: https://github.com/nodejs/node/pull/30374 Refs: https://github.com/nodejs/quic/pull/165 Reviewed-By: David Carlier --- src/base_object-inl.h | 215 +++++++++++++++++++++++++++++++++++++++++++++-- src/base_object.h | 111 ++++++++++++++++++++++-- src/env-inl.h | 8 ++ src/env.cc | 6 ++ src/env.h | 8 ++ src/handle_wrap.cc | 14 ++- src/handle_wrap.h | 3 +- src/memory_tracker-inl.h | 8 ++ src/memory_tracker.h | 8 ++ 9 files changed, 354 insertions(+), 27 deletions(-) (limited to 'src') diff --git a/src/base_object-inl.h b/src/base_object-inl.h index af69084f4a..4fc9210b39 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -32,16 +32,25 @@ namespace node { BaseObject::BaseObject(Environment* env, v8::Local object) - : persistent_handle_(env->isolate(), object), - env_(env) { + : persistent_handle_(env->isolate(), object), env_(env) { CHECK_EQ(false, object.IsEmpty()); CHECK_GT(object->InternalFieldCount(), 0); object->SetAlignedPointerInInternalField(0, static_cast(this)); - env_->AddCleanupHook(DeleteMe, static_cast(this)); + env->AddCleanupHook(DeleteMe, static_cast(this)); + env->modify_base_object_count(1); } BaseObject::~BaseObject() { - RemoveCleanupHook(); + env()->modify_base_object_count(-1); + env()->RemoveCleanupHook(DeleteMe, static_cast(this)); + + if (UNLIKELY(has_pointer_data())) { + PointerData* metadata = pointer_data(); + CHECK_EQ(metadata->strong_ptr_count, 0); + metadata->self = nullptr; + if (metadata->weak_ptr_count == 0) + delete metadata; + } if (persistent_handle_.IsEmpty()) { // This most likely happened because the weak callback below cleared it. @@ -49,7 +58,7 @@ BaseObject::~BaseObject() { } { - v8::HandleScope handle_scope(env_->isolate()); + v8::HandleScope handle_scope(env()->isolate()); object()->SetAlignedPointerInInternalField(0, nullptr); } } @@ -58,20 +67,25 @@ void BaseObject::RemoveCleanupHook() { env_->RemoveCleanupHook(DeleteMe, static_cast(this)); } +void BaseObject::Detach() { + CHECK_GT(pointer_data()->strong_ptr_count, 0); + pointer_data()->is_detached = true; +} + v8::Global& BaseObject::persistent() { return persistent_handle_; } v8::Local BaseObject::object() const { - return PersistentToLocal::Default(env_->isolate(), persistent_handle_); + return PersistentToLocal::Default(env()->isolate(), persistent_handle_); } v8::Local BaseObject::object(v8::Isolate* isolate) const { v8::Local handle = object(); DCHECK_EQ(handle->CreationContext()->GetIsolate(), isolate); - DCHECK_EQ(env_->isolate(), isolate); + DCHECK_EQ(env()->isolate(), isolate); return handle; } @@ -80,7 +94,6 @@ Environment* BaseObject::env() const { return env_; } - BaseObject* BaseObject::FromJSObject(v8::Local obj) { CHECK_GT(obj->InternalFieldCount(), 0); return static_cast(obj->GetAlignedPointerFromInternalField(0)); @@ -94,20 +107,34 @@ T* BaseObject::FromJSObject(v8::Local object) { void BaseObject::MakeWeak() { + if (has_pointer_data()) { + pointer_data()->wants_weak_jsobj = true; + if (pointer_data()->strong_ptr_count > 0) return; + } + persistent_handle_.SetWeak( this, [](const v8::WeakCallbackInfo& data) { - std::unique_ptr obj(data.GetParameter()); + BaseObject* obj = data.GetParameter(); // Clear the persistent handle so that ~BaseObject() doesn't attempt // to mess with internal fields, since the JS object may have // transitioned into an invalid state. // Refs: https://github.com/nodejs/node/issues/18897 obj->persistent_handle_.Reset(); + CHECK_IMPLIES(obj->has_pointer_data(), + obj->pointer_data()->strong_ptr_count == 0); + obj->OnGCCollect(); }, v8::WeakCallbackType::kParameter); } +void BaseObject::OnGCCollect() { + delete this; +} void BaseObject::ClearWeak() { + if (has_pointer_data()) + pointer_data()->wants_weak_jsobj = false; + persistent_handle_.ClearWeak(); } @@ -141,6 +168,176 @@ void BaseObject::InternalFieldSet(v8::Local property, info.This()->SetInternalField(Field, value); } +bool BaseObject::has_pointer_data() const { + return pointer_data_ != nullptr; +} + +BaseObject::PointerData* BaseObject::pointer_data() { + if (!has_pointer_data()) { + PointerData* metadata = new PointerData(); + metadata->wants_weak_jsobj = persistent_handle_.IsWeak(); + metadata->self = this; + pointer_data_ = metadata; + } + CHECK(has_pointer_data()); + return pointer_data_; +} + +void BaseObject::decrease_refcount() { + CHECK(has_pointer_data()); + PointerData* metadata = pointer_data(); + CHECK_GT(metadata->strong_ptr_count, 0); + unsigned int new_refcount = --metadata->strong_ptr_count; + if (new_refcount == 0) { + if (metadata->is_detached) { + delete this; + } else if (metadata->wants_weak_jsobj && !persistent_handle_.IsEmpty()) { + MakeWeak(); + } + } +} + +void BaseObject::increase_refcount() { + unsigned int prev_refcount = pointer_data()->strong_ptr_count++; + if (prev_refcount == 0 && !persistent_handle_.IsEmpty()) + persistent_handle_.ClearWeak(); +} + +template +BaseObject::PointerData* +BaseObjectPtrImpl::pointer_data() const { + if (kIsWeak) { + return data_.pointer_data; + } else { + if (get_base_object() == nullptr) return nullptr; + return get_base_object()->pointer_data(); + } +} + +template +BaseObject* BaseObjectPtrImpl::get_base_object() const { + if (kIsWeak) { + if (pointer_data() == nullptr) return nullptr; + return pointer_data()->self; + } else { + return data_.target; + } +} + +template +BaseObjectPtrImpl::~BaseObjectPtrImpl() { + if (get() == nullptr) return; + if (kIsWeak) { + if (--pointer_data()->weak_ptr_count == 0 && + pointer_data()->self == nullptr) { + delete pointer_data(); + } + } else { + get()->decrease_refcount(); + } +} + +template +BaseObjectPtrImpl::BaseObjectPtrImpl() { + data_.target = nullptr; +} + +template +BaseObjectPtrImpl::BaseObjectPtrImpl(T* target) + : BaseObjectPtrImpl() { + if (target == nullptr) return; + if (kIsWeak) { + data_.pointer_data = target->pointer_data(); + CHECK_NOT_NULL(pointer_data()); + pointer_data()->weak_ptr_count++; + } else { + data_.target = target; + CHECK_NOT_NULL(pointer_data()); + get()->increase_refcount(); + } +} + +template +template +BaseObjectPtrImpl::BaseObjectPtrImpl( + const BaseObjectPtrImpl& other) + : BaseObjectPtrImpl(other.get()) {} + +template +BaseObjectPtrImpl::BaseObjectPtrImpl(const BaseObjectPtrImpl& other) + : BaseObjectPtrImpl(other.get()) {} + +template +template +BaseObjectPtrImpl& BaseObjectPtrImpl::operator=( + const BaseObjectPtrImpl& other) { + if (other.get() == get()) return *this; + this->~BaseObjectPtrImpl(); + return *new (this) BaseObjectPtrImpl(other); +} + +template +BaseObjectPtrImpl& BaseObjectPtrImpl::operator=( + const BaseObjectPtrImpl& other) { + if (other.get() == get()) return *this; + this->~BaseObjectPtrImpl(); + return *new (this) BaseObjectPtrImpl(other); +} + +template +BaseObjectPtrImpl::BaseObjectPtrImpl(BaseObjectPtrImpl&& other) + : data_(other.data_) { + if (kIsWeak) + other.data_.target = nullptr; + else + other.data_.pointer_data = nullptr; +} + +template +BaseObjectPtrImpl& BaseObjectPtrImpl::operator=( + BaseObjectPtrImpl&& other) { + if (&other == this) return *this; + this->~BaseObjectPtrImpl(); + return *new (this) BaseObjectPtrImpl(std::move(other)); +} + +template +void BaseObjectPtrImpl::reset(T* ptr) { + *this = BaseObjectPtrImpl(ptr); +} + +template +T* BaseObjectPtrImpl::get() const { + return static_cast(get_base_object()); +} + +template +T& BaseObjectPtrImpl::operator*() const { + return *get(); +} + +template +T* BaseObjectPtrImpl::operator->() const { + return get(); +} + +template +BaseObjectPtrImpl::operator bool() const { + return get() != nullptr; +} + +template +BaseObjectPtr MakeBaseObject(Args&&... args) { + return BaseObjectPtr(new T(std::forward(args)...)); +} + +template +BaseObjectPtr MakeDetachedBaseObject(Args&&... args) { + BaseObjectPtr target = MakeBaseObject(std::forward(args)...); + target->Detach(); + return target; +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/base_object.h b/src/base_object.h index 0b202cd3a5..38afe11a76 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -31,6 +31,8 @@ namespace node { class Environment; +template +class BaseObjectPtrImpl; class BaseObject : public MemoryRetainer { public: @@ -62,10 +64,12 @@ class BaseObject : public MemoryRetainer { static inline T* FromJSObject(v8::Local object); // Make the `v8::Global` a weak reference and, `delete` this object once - // the JS object has been garbage collected. + // the JS object has been garbage collected and there are no (strong) + // BaseObjectPtr references to it. inline void MakeWeak(); - // Undo `MakeWeak()`, i.e. turn this into a strong reference. + // Undo `MakeWeak()`, i.e. turn this into a strong reference that is a GC + // root and will not be touched by the garbage collector. inline void ClearWeak(); // Utility to create a FunctionTemplate with one internal field (used for @@ -86,11 +90,15 @@ class BaseObject : public MemoryRetainer { // This is a bit of a hack. See the override in async_wrap.cc for details. virtual bool IsDoneInitializing() const; + // Can be used to avoid this object keepling itself alive as a GC root + // indefinitely, for example when this object is owned and deleted by another + // BaseObject once that is torn down. This can only be called when there is + // a BaseObjectPtr to this object. + inline void Detach(); + protected: - // Can be used to avoid the automatic object deletion when the Environment - // exits, for example when this object is owned and deleted by another - // BaseObject at that point. - inline void RemoveCleanupHook(); + inline void RemoveCleanupHook(); // TODO(addaleax): Remove. + virtual inline void OnGCCollect(); private: v8::Local WrappedObject() const override; @@ -103,12 +111,44 @@ class BaseObject : public MemoryRetainer { // refer to `doc/guides/node-postmortem-support.md` friend int GenDebugSymbols(); friend class CleanupHookCallback; + template + friend class BaseObjectPtrImpl; v8::Global persistent_handle_; + + // Metadata that is associated with this BaseObject if there are BaseObjectPtr + // or BaseObjectWeakPtr references to it. + // This object is deleted when the BaseObject itself is destroyed, and there + // are no weak references to it. + struct PointerData { + // Number of BaseObjectPtr instances that refer to this object. If this + // is non-zero, the BaseObject is always a GC root and will not be destroyed + // during cleanup until the count drops to zero again. + unsigned int strong_ptr_count = 0; + // Number of BaseObjectWeakPtr instances that refer to this object. + unsigned int weak_ptr_count = 0; + // Indicates whether MakeWeak() has been called. + bool wants_weak_jsobj = false; + // Indicates whether Detach() has been called. If that is the case, this + // object will be destryoed once the strong pointer count drops to zero. + bool is_detached = false; + // Reference to the original BaseObject. This is used by weak pointers. + BaseObject* self = nullptr; + }; + + inline bool has_pointer_data() const; + // This creates a PointerData struct if none was associated with this + // BaseObject before. + inline PointerData* pointer_data(); + + // Functions that adjust the strong pointer count. + inline void decrease_refcount(); + inline void increase_refcount(); + Environment* env_; + PointerData* pointer_data_ = nullptr; }; - // Global alias for FromJSObject() to avoid churn. template inline T* Unwrap(v8::Local obj) { @@ -124,6 +164,63 @@ inline T* Unwrap(v8::Local obj) { return __VA_ARGS__; \ } while (0) +// Implementation of a generic strong or weak pointer to a BaseObject. +// If strong, this will keep the target BaseObject alive regardless of other +// circumstances such das GC or Environment cleanup. +// If weak, destruction behaviour is not affected, but the pointer will be +// reset to nullptr once the BaseObject is destroyed. +// The API matches std::shared_ptr closely. +template +class BaseObjectPtrImpl final { + public: + inline BaseObjectPtrImpl(); + inline ~BaseObjectPtrImpl(); + inline explicit BaseObjectPtrImpl(T* target); + + // Copy and move constructors. Note that the templated version is not a copy + // or move constructor in the C++ sense of the word, so an identical + // untemplated version is provided. + template + inline BaseObjectPtrImpl(const BaseObjectPtrImpl& other); + inline BaseObjectPtrImpl(const BaseObjectPtrImpl& other); + template + inline BaseObjectPtrImpl& operator=(const BaseObjectPtrImpl& other); + inline BaseObjectPtrImpl& operator=(const BaseObjectPtrImpl& other); + inline BaseObjectPtrImpl(BaseObjectPtrImpl&& other); + inline BaseObjectPtrImpl& operator=(BaseObjectPtrImpl&& other); + + inline void reset(T* ptr = nullptr); + inline T* get() const; + inline T& operator*() const; + inline T* operator->() const; + inline operator bool() const; + + private: + union { + BaseObject* target; // Used for strong pointers. + BaseObject::PointerData* pointer_data; // Used for weak pointers. + } data_; + + inline BaseObject* get_base_object() const; + inline BaseObject::PointerData* pointer_data() const; +}; + +template +using BaseObjectPtr = BaseObjectPtrImpl; +template +using BaseObjectWeakPtr = BaseObjectPtrImpl; + +// Create a BaseObject instance and return a pointer to it. +// This variant leaves the object as a GC root by default. +template +inline BaseObjectPtr MakeBaseObject(Args&&... args); +// Create a BaseObject instance and return a pointer to it. +// This variant detaches the object by default, meaning that the caller fully +// owns it, and once the last BaseObjectPtr to it is destroyed, the object +// itself is also destroyed. +template +inline BaseObjectPtr MakeDetachedBaseObject(Args&&... args); + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/env-inl.h b/src/env-inl.h index ee170b0715..c361c8fa63 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -1151,6 +1151,14 @@ void Environment::ForEachBaseObject(T&& iterator) { } } +void Environment::modify_base_object_count(int64_t delta) { + base_object_count_ += delta; +} + +int64_t Environment::base_object_count() const { + return base_object_count_; +} + bool AsyncRequest::is_stopped() const { return stopped_.load(); } diff --git a/src/env.cc b/src/env.cc index c8704cb3be..5f9a6acb46 100644 --- a/src/env.cc +++ b/src/env.cc @@ -431,6 +431,8 @@ Environment::~Environment() { addon.Close(); } } + + CHECK_EQ(base_object_count(), 0); } void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { @@ -1088,6 +1090,10 @@ AsyncRequest::~AsyncRequest() { // Not really any better place than env.cc at this moment. void BaseObject::DeleteMe(void* data) { BaseObject* self = static_cast(data); + if (self->has_pointer_data() && + self->pointer_data()->strong_ptr_count > 0) { + return self->Detach(); + } delete self; } diff --git a/src/env.h b/src/env.h index 5f5188d49d..a0fc496cce 100644 --- a/src/env.h +++ b/src/env.h @@ -1216,6 +1216,12 @@ class Environment : public MemoryRetainer { inline AsyncRequest* thread_stopper() { return &thread_stopper_; } + // The BaseObject count is a debugging helper that makes sure that there are + // no memory leaks caused by BaseObjects staying alive longer than expected + // (in particular, no circular BaseObjectPtr references). + inline void modify_base_object_count(int64_t delta); + inline int64_t base_object_count() const; + #if HAVE_INSPECTOR void set_coverage_connection( std::unique_ptr connection); @@ -1427,6 +1433,8 @@ class Environment : public MemoryRetainer { uint64_t cleanup_hook_counter_ = 0; bool started_cleanup_ = false; + int64_t base_object_count_ = 0; + // A custom async abstraction (a pair of async handle and a state variable) // Used by embedders to shutdown running Node instance. AsyncRequest thread_stopper_; diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index fc84ca19bb..f3a3555754 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -84,14 +84,8 @@ void HandleWrap::Close(Local close_callback) { } -void HandleWrap::MakeWeak() { - persistent().SetWeak( - this, - [](const v8::WeakCallbackInfo& data) { - HandleWrap* handle_wrap = data.GetParameter(); - handle_wrap->persistent().Reset(); - handle_wrap->Close(); - }, v8::WeakCallbackType::kParameter); +void HandleWrap::OnGCCollect() { + Close(); } @@ -122,7 +116,9 @@ HandleWrap::HandleWrap(Environment* env, void HandleWrap::OnClose(uv_handle_t* handle) { - std::unique_ptr wrap { static_cast(handle->data) }; + BaseObjectPtr wrap { static_cast(handle->data) }; + wrap->Detach(); + Environment* env = wrap->env(); HandleScope scope(env->isolate()); Context::Scope context_scope(env->context()); diff --git a/src/handle_wrap.h b/src/handle_wrap.h index fbcea4ae44..612874aa2e 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -76,14 +76,13 @@ class HandleWrap : public AsyncWrap { static v8::Local GetConstructorTemplate( Environment* env); - void MakeWeak(); // This hides BaseObject::MakeWeak() - protected: HandleWrap(Environment* env, v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); virtual void OnClose() {} + void OnGCCollect() final; void MarkAsInitialized(); void MarkAsUninitialized(); diff --git a/src/memory_tracker-inl.h b/src/memory_tracker-inl.h index 14635aaf5e..938aba1a7a 100644 --- a/src/memory_tracker-inl.h +++ b/src/memory_tracker-inl.h @@ -119,6 +119,14 @@ void MemoryTracker::TrackField(const char* edge_name, TrackField(edge_name, value.get(), node_name); } +template +void MemoryTracker::TrackField(const char* edge_name, + const BaseObjectPtrImpl& value, + const char* node_name) { + if (value.get() == nullptr) return; + TrackField(edge_name, value.get(), node_name); +} + template void MemoryTracker::TrackField(const char* edge_name, const T& value, diff --git a/src/memory_tracker.h b/src/memory_tracker.h index 3bcbe97c99..7e39da5ecf 100644 --- a/src/memory_tracker.h +++ b/src/memory_tracker.h @@ -30,6 +30,8 @@ namespace node { class MemoryTracker; class MemoryRetainerNode; +template +class BaseObjectPtrImpl; namespace crypto { class NodeBIO; @@ -138,11 +140,17 @@ class MemoryTracker { inline void TrackField(const char* edge_name, const std::unique_ptr& value, const char* node_name = nullptr); + template inline void TrackField(const char* edge_name, const std::shared_ptr& value, const char* node_name = nullptr); + template + void TrackField(const char* edge_name, + const BaseObjectPtrImpl& value, + const char* node_name = nullptr); + // For containers, the elements will be graphed as grandchildren nodes // if the container is not empty. // By default, we assume the parent count the stack size of the container -- cgit v1.2.3