diff --git a/node.gyp b/node.gyp index 9ae69e01df8390..810cea8c241359 100644 --- a/node.gyp +++ b/node.gyp @@ -1100,6 +1100,7 @@ 'test/cctest/node_test_fixture.h', 'test/cctest/test_aliased_buffer.cc', 'test/cctest/test_base64.cc', + 'test/cctest/test_base_object_ptr.cc', 'test/cctest/test_node_postmortem_metadata.cc', 'test/cctest/test_environment.cc', 'test/cctest/test_linked_binding.cc', diff --git a/src/base_object-inl.h b/src/base_object-inl.h index af69084f4a5595..4fc9210b39bf89 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 0b202cd3a51324..38afe11a761e26 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 ee170b071503f8..c361c8fa63ee49 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 c8704cb3be6dce..5f9a6acb461f97 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 5f5188d49d6052..a0fc496cce6599 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 fc84ca19bb2517..f3a355575412a2 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 fbcea4ae4487f5..612874aa2efb4f 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 14635aaf5e84c8..938aba1a7a8d11 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 3bcbe97c99c4e2..7e39da5ecf6de9 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 diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index f6b80c860c1f58..ac0701d0942666 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -105,9 +105,9 @@ class NodeTestFixture : public ::testing::Test { } void TearDown() override { + platform->DrainTasks(isolate_); isolate_->Exit(); isolate_->Dispose(); - platform->DrainTasks(isolate_); platform->UnregisterIsolate(isolate_); isolate_ = nullptr; } diff --git a/test/cctest/test_base_object_ptr.cc b/test/cctest/test_base_object_ptr.cc new file mode 100644 index 00000000000000..18e27edba8cd53 --- /dev/null +++ b/test/cctest/test_base_object_ptr.cc @@ -0,0 +1,176 @@ +#include "gtest/gtest.h" +#include "node.h" +#include "base_object-inl.h" +#include "node_test_fixture.h" + +using node::BaseObject; +using node::BaseObjectPtr; +using node::BaseObjectWeakPtr; +using node::Environment; +using node::MakeBaseObject; +using node::MakeDetachedBaseObject; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::Object; + +class BaseObjectPtrTest : public EnvironmentTestFixture {}; + +class DummyBaseObject : public BaseObject { + public: + DummyBaseObject(Environment* env, Local obj) : BaseObject(env, obj) {} + + static Local MakeJSObject(Environment* env) { + return BaseObject::MakeLazilyInitializedJSTemplate(env) + ->GetFunction(env->context()).ToLocalChecked() + ->NewInstance(env->context()).ToLocalChecked(); + } + + static BaseObjectPtr NewDetached(Environment* env) { + Local obj = MakeJSObject(env); + return MakeDetachedBaseObject(env, obj); + } + + static BaseObjectPtr New(Environment* env) { + Local obj = MakeJSObject(env); + return MakeBaseObject(env, obj); + } + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(DummyBaseObject) + SET_SELF_SIZE(DummyBaseObject) +}; + +TEST_F(BaseObjectPtrTest, ScopedDetached) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + EXPECT_EQ(env->base_object_count(), 0); + { + BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); + EXPECT_EQ(env->base_object_count(), 1); + } + EXPECT_EQ(env->base_object_count(), 0); +} + +TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + BaseObjectWeakPtr weak_ptr; + + EXPECT_EQ(env->base_object_count(), 0); + { + BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); + weak_ptr = ptr; + EXPECT_EQ(env->base_object_count(), 1); + } + EXPECT_EQ(weak_ptr.get(), nullptr); + EXPECT_EQ(env->base_object_count(), 0); +} + +TEST_F(BaseObjectPtrTest, Undetached) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + node::AddEnvironmentCleanupHook(isolate_, [](void* arg) { + EXPECT_EQ(static_cast(arg)->base_object_count(), 0); + }, env); + + BaseObjectPtr ptr = DummyBaseObject::New(env); + EXPECT_EQ(env->base_object_count(), 1); +} + +TEST_F(BaseObjectPtrTest, GCWeak) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + BaseObjectWeakPtr weak_ptr; + + { + const HandleScope handle_scope(isolate_); + BaseObjectPtr ptr = DummyBaseObject::New(env); + weak_ptr = ptr; + ptr->MakeWeak(); + + EXPECT_EQ(env->base_object_count(), 1); + EXPECT_EQ(weak_ptr.get(), ptr.get()); + EXPECT_EQ(weak_ptr->persistent().IsWeak(), false); + + ptr.reset(); + } + + EXPECT_EQ(env->base_object_count(), 1); + EXPECT_NE(weak_ptr.get(), nullptr); + EXPECT_EQ(weak_ptr->persistent().IsWeak(), true); + + v8::V8::SetFlagsFromString("--expose-gc"); + isolate_->RequestGarbageCollectionForTesting(Isolate::kFullGarbageCollection); + + EXPECT_EQ(env->base_object_count(), 0); + EXPECT_EQ(weak_ptr.get(), nullptr); +} + +TEST_F(BaseObjectPtrTest, Moveable) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); + EXPECT_EQ(env->base_object_count(), 1); + BaseObjectWeakPtr weak_ptr { ptr }; + EXPECT_EQ(weak_ptr.get(), ptr.get()); + + BaseObjectPtr ptr2 = std::move(ptr); + EXPECT_EQ(weak_ptr.get(), ptr2.get()); + EXPECT_EQ(ptr.get(), nullptr); + + BaseObjectWeakPtr weak_ptr2 = std::move(weak_ptr); + EXPECT_EQ(weak_ptr2.get(), ptr2.get()); + EXPECT_EQ(weak_ptr.get(), nullptr); + EXPECT_EQ(env->base_object_count(), 1); + + ptr2.reset(); + + EXPECT_EQ(weak_ptr2.get(), nullptr); + EXPECT_EQ(env->base_object_count(), 0); +} + +TEST_F(BaseObjectPtrTest, NestedClasses) { + class ObjectWithPtr : public BaseObject { + public: + ObjectWithPtr(Environment* env, Local obj) : BaseObject(env, obj) {} + + BaseObjectPtr ptr1; + BaseObjectPtr ptr2; + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(ObjectWithPtr) + SET_SELF_SIZE(ObjectWithPtr) + }; + + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + node::AddEnvironmentCleanupHook(isolate_, [](void* arg) { + EXPECT_EQ(static_cast(arg)->base_object_count(), 0); + }, env); + + ObjectWithPtr* obj = + new ObjectWithPtr(env, DummyBaseObject::MakeJSObject(env)); + obj->ptr1 = DummyBaseObject::NewDetached(env); + obj->ptr2 = DummyBaseObject::New(env); + + EXPECT_EQ(env->base_object_count(), 3); +} diff --git a/test/cctest/test_node_postmortem_metadata.cc b/test/cctest/test_node_postmortem_metadata.cc index f33d40eb5c23fe..3fb67ecbca265e 100644 --- a/test/cctest/test_node_postmortem_metadata.cc +++ b/test/cctest/test_node_postmortem_metadata.cc @@ -93,14 +93,13 @@ TEST_F(DebugSymbolsTest, BaseObjectPersistentHandle) { v8::Local object = obj_templ->NewInstance(env.context()).ToLocalChecked(); - DummyBaseObject obj(*env, object); + node::BaseObjectPtr obj = + node::MakeDetachedBaseObject(*env, object); - auto expected = reinterpret_cast(&obj.persistent()); - auto calculated = reinterpret_cast(&obj) + + auto expected = reinterpret_cast(&obj->persistent()); + auto calculated = reinterpret_cast(obj.get()) + nodedbg_offset_BaseObject__persistent_handle___v8_Persistent_v8_Object; EXPECT_EQ(expected, calculated); - - obj.persistent().Reset(); // ~BaseObject() expects an empty handle. }