diff --git a/src/cppgc_helpers-inl.h b/src/cppgc_helpers-inl.h index 26cf107602f3..4db197d694f8 100644 --- a/src/cppgc_helpers-inl.h +++ b/src/cppgc_helpers-inl.h @@ -11,7 +11,6 @@ namespace node { template void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local obj) { CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount); - ptr->realm_ = realm; v8::Isolate* isolate = realm->isolate(); ptr->traced_reference_ = v8::TracedReference(isolate, obj); // Note that ptr must be of concrete type T in Wrap. @@ -23,7 +22,7 @@ void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local obj) { realm->isolate_data()->embedder_id_for_cppgc(), EmbedderDataTag::kEmbedderType); obj->SetAlignedPointerInInternalField(kSlot, ptr, EmbedderDataTag::kDefault); - realm->TrackCppgcWrapper(ptr); + ptr->list_node_ = realm->TrackCppgcWrapper(ptr); } template @@ -49,17 +48,26 @@ T* CppgcMixin::Unwrap(v8::Local obj) { } v8::Local CppgcMixin::object() const { - return traced_reference_.Get(realm_->isolate()); + return traced_reference_.Get(realm()->isolate()); } Environment* CppgcMixin::env() const { - return realm_->env(); + return realm()->env(); +} + +Realm* CppgcMixin::realm() const { + return list_node_ == nullptr ? nullptr : list_node_->realm; +} + +void CppgcMixin::Finalize() { + Realm* current_realm = realm(); + if (current_realm == nullptr) return; + this->Clean(current_realm); + list_node_->realm = nullptr; } CppgcMixin::~CppgcMixin() { - if (realm_ != nullptr) { - realm_->set_should_purge_empty_cppgc_wrappers(true); - } + delete list_node_; } } // namespace node diff --git a/src/cppgc_helpers.cc b/src/cppgc_helpers.cc index 7c557a822d20..9424904c0ea8 100644 --- a/src/cppgc_helpers.cc +++ b/src/cppgc_helpers.cc @@ -1,14 +1,14 @@ -#include "cppgc_helpers.h" -#include "env-inl.h" +#include "cppgc_helpers.h" // NOLINT(build/include_inline) +#include "cppgc_helpers-inl.h" namespace node { void CppgcWrapperList::Cleanup() { - for (auto node : *this) { - CppgcMixin* ptr = node->persistent.Get(); - if (ptr != nullptr) { - ptr->Finalize(); - } + while (!IsEmpty()) { + CppgcWrapperListNode* node = PopFront(); + CppgcMixin* wrapper = node->persistent.Get(); + if (wrapper != nullptr) wrapper->Finalize(); + node->realm = nullptr; } } @@ -23,18 +23,4 @@ void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const { } } } - -void CppgcWrapperList::PurgeEmpty() { - for (auto weak_it = begin(); weak_it != end();) { - CppgcWrapperListNode* node = *weak_it; - auto next_it = ++weak_it; - // The underlying cppgc wrapper has already been garbage collected. - // Remove it from the list. - if (!node->persistent) { - node->persistent.Clear(); - delete node; - } - weak_it = next_it; - } -} } // namespace node diff --git a/src/cppgc_helpers.h b/src/cppgc_helpers.h index fe6300a5d5d2..f1363d5da78f 100644 --- a/src/cppgc_helpers.h +++ b/src/cppgc_helpers.h @@ -47,9 +47,9 @@ class CppgcWrapperListNode; * cleanup relies on a living Node.js `Realm`, it should implement a * pattern like this: * - * ~MyWrap() { this->Destroy(); } + * ~MyWrap() { this->Finalize(); } * void Clean(Realm* env) override { - * // Do cleanup that relies on a living Environemnt. + * // Do cleanup that relies on a living Realm. * } */ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer { @@ -68,7 +68,7 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer { inline v8::Local object() const; inline Environment* env() const; - inline Realm* realm() const { return realm_; } + inline Realm* realm() const; inline v8::Local object(v8::Isolate* isolate) const { return traced_reference_.Get(isolate); } @@ -95,11 +95,7 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer { // destructor. Outside of Finalize(), subclasses should avoid calling // into JavaScript or perform any operation that can trigger garbage // collection during the destruction. - void Finalize() { - if (realm_ == nullptr) return; - this->Clean(realm_); - realm_ = nullptr; - } + inline void Finalize(); // The default implementation of Clean() is a no-op. If subclasses wish // to perform cleanup that require a living Realm, they should @@ -110,10 +106,8 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer { inline ~CppgcMixin(); - friend class CppgcWrapperListNode; - private: - Realm* realm_ = nullptr; + CppgcWrapperListNode* list_node_ = nullptr; v8::TracedReference traced_reference_; }; diff --git a/src/node_realm-inl.h b/src/node_realm-inl.h index 394ece5a8ace..62f77384a387 100644 --- a/src/node_realm-inl.h +++ b/src/node_realm-inl.h @@ -133,11 +133,13 @@ void Realm::TrackBaseObject(BaseObject* bo) { ++base_object_count_; } -CppgcWrapperListNode::CppgcWrapperListNode(CppgcMixin* ptr) : persistent(ptr) {} +CppgcWrapperListNode::CppgcWrapperListNode(Realm* realm, CppgcMixin* wrapper) + : realm(realm), persistent(wrapper) {} -void Realm::TrackCppgcWrapper(CppgcMixin* handle) { - DCHECK_EQ(handle->realm(), this); - cppgc_wrapper_list_.PushFront(new CppgcWrapperListNode(handle)); +CppgcWrapperListNode* Realm::TrackCppgcWrapper(CppgcMixin* handle) { + CppgcWrapperListNode* node = new CppgcWrapperListNode(this, handle); + cppgc_wrapper_list_.PushFront(node); + return node; } void Realm::UntrackBaseObject(BaseObject* bo) { @@ -146,7 +148,7 @@ void Realm::UntrackBaseObject(BaseObject* bo) { } bool Realm::PendingCleanup() const { - return !base_object_list_.IsEmpty(); + return !base_object_list_.IsEmpty() || !cppgc_wrapper_list_.IsEmpty(); } } // namespace node diff --git a/src/node_realm.cc b/src/node_realm.cc index d2459d4eeb33..91bb530dfd94 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -10,8 +10,6 @@ namespace node { using v8::Context; using v8::EscapableHandleScope; -using v8::GCCallbackFlags; -using v8::GCType; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -25,26 +23,11 @@ Realm::Realm(Environment* env, v8::Local context, Kind kind) : env_(env), isolate_(Isolate::GetCurrent()), kind_(kind) { context_.Reset(isolate_, context); env->AssignToContext(context, this, ContextInfo("")); - // The environment can also purge empty wrappers in the check callback, - // though that may be a bit excessive depending on usage patterns. - // For now using the GC epilogue is adequate. - isolate_->AddGCEpilogueCallback(PurgeEmptyCppgcWrappers, this); } Realm::~Realm() { - isolate_->RemoveGCEpilogueCallback(PurgeEmptyCppgcWrappers, this); CHECK_EQ(base_object_count_, 0); -} - -void Realm::PurgeEmptyCppgcWrappers(Isolate* isolate, - GCType type, - GCCallbackFlags flags, - void* data) { - Realm* realm = static_cast(data); - if (realm->should_purge_empty_cppgc_wrappers_) { - realm->cppgc_wrapper_list_.PurgeEmpty(); - realm->should_purge_empty_cppgc_wrappers_ = false; - } + CHECK(cppgc_wrapper_list_.IsEmpty()); } void Realm::MemoryInfo(MemoryTracker* tracker) const { diff --git a/src/node_realm.h b/src/node_realm.h index 690beaf1a1aa..8bba26f50a92 100644 --- a/src/node_realm.h +++ b/src/node_realm.h @@ -27,16 +27,16 @@ using BindingDataStore = static_cast(BindingDataType::kBindingDataTypeCount)>; /** - * This is a wrapper around a weak persistent of CppgcMixin, used in the - * CppgcWrapperList to avoid accessing already garbage collected CppgcMixins. + * Owned by a CppgcMixin and linked into its Realm's list until the Realm + * cleans up and clears `realm`. The Realm only calls into wrappers the GC + * still considers alive (the weak persistent); a collected wrapper whose + * destructor runs later sees `realm == nullptr` instead of a freed Realm. */ class CppgcWrapperListNode { public: - explicit inline CppgcWrapperListNode(CppgcMixin* ptr); - inline explicit operator bool() const { return !persistent; } - inline CppgcMixin* operator->() const { return persistent.Get(); } - inline CppgcMixin* operator*() const { return persistent.Get(); } + inline CppgcWrapperListNode(Realm* realm, CppgcMixin* wrapper); + Realm* realm; cppgc::WeakPersistent persistent; // Used by ContainerOf in the ListNode implementation for fast manipulation of // CppgcWrapperList. @@ -53,7 +53,6 @@ class CppgcWrapperList public MemoryRetainer { public: void Cleanup(); - void PurgeEmpty(); SET_MEMORY_INFO_NAME(CppgcWrapperList) SET_SELF_SIZE(CppgcWrapperList) @@ -148,7 +147,7 @@ class Realm : public MemoryRetainer { // Base object count created after the bootstrap of the realm. inline int64_t base_object_created_after_bootstrap() const; - inline void TrackCppgcWrapper(CppgcMixin* handle); + inline CppgcWrapperListNode* TrackCppgcWrapper(CppgcMixin* handle); inline CppgcWrapperList* cppgc_wrapper_list() { return &cppgc_wrapper_list_; } #define V(PropertyName, TypeName) \ @@ -164,14 +163,6 @@ class Realm : public MemoryRetainer { // it's only used for tests. std::vector builtins_in_snapshot; - // This used during the destruction of cppgc wrappers to inform a GC epilogue - // callback to clean up the weak persistents used to track cppgc wrappers if - // the wrappers are already garbage collected to prevent holding on to - // excessive useless persistents. - inline void set_should_purge_empty_cppgc_wrappers(bool value) { - should_purge_empty_cppgc_wrappers_ = value; - } - protected: ~Realm(); @@ -181,17 +172,11 @@ class Realm : public MemoryRetainer { // Shorthand for isolate pointer. v8::Isolate* isolate_; v8::Global context_; - bool should_purge_empty_cppgc_wrappers_ = false; #define V(PropertyName, TypeName) v8::Global PropertyName##_; PER_REALM_STRONG_PERSISTENT_VALUES(V) #undef V - static void PurgeEmptyCppgcWrappers(v8::Isolate* isolate, - v8::GCType type, - v8::GCCallbackFlags flags, - void* data); - private: void InitializeContext(v8::Local context, const RealmSerializeInfo* realm_info); diff --git a/test/cctest/test_cppgc.cc b/test/cctest/test_cppgc.cc index 2f586617bd6c..478098154665 100644 --- a/test/cctest/test_cppgc.cc +++ b/test/cctest/test_cppgc.cc @@ -3,8 +3,11 @@ #include #include #include +#include #include #include +#include "cppgc_helpers-inl.h" +#include "node_realm-inl.h" #include "node_test_fixture.h" // This tests that Node.js can work with an existing CppHeap. @@ -106,3 +109,114 @@ TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) { // heap can be reclaimed. So just check at least some of them are traced. EXPECT_GT(CppGCed::kTraceCount, 0); } + +class CppgcTest : public EnvironmentTestFixture { + protected: + // Above the external memory hard limit, so V8 runs a full GC synchronously + // and leaves sweeping (and cppgc destructors) for later. + static constexpr size_t kExternalMemoryPressure = size_t{8} << 30; + + void CollectGarbageLeavingSweepingPending() { + v8::ExternalMemoryAccounter pressure; + pressure.Increase(isolate_, kExternalMemoryPressure); + pressure.Decrease(isolate_, kExternalMemoryPressure); + } + + void FinishSweeping() { + isolate_->LowMemoryNotification(); + platform->DrainTasks(isolate_); + } +}; + +using node::CppgcMixin; + +class RealmBoundWrap final : CPPGC_MIXIN(RealmBoundWrap) { + public: + SET_CPPGC_NAME(RealmBoundWrap) + DEFAULT_CPPGC_TRACE() + SET_NO_MEMORY_INFO() + + static node::Realm* live_realm; + static int clean_count; + static int clean_with_dead_realm_count; + static int destructor_count; + + RealmBoundWrap(node::Environment* env, v8::Local object) { + CppgcMixin::Wrap(this, env, object); + } + ~RealmBoundWrap() { + Finalize(); + destructor_count++; + } + void Clean(node::Realm* realm) override { + clean_count++; + if (realm != live_realm) clean_with_dead_realm_count++; + } +}; + +node::Realm* RealmBoundWrap::live_realm = nullptr; +int RealmBoundWrap::clean_count = 0; +int RealmBoundWrap::clean_with_dead_realm_count = 0; +int RealmBoundWrap::destructor_count = 0; + +TEST_F(CppgcTest, CleanIsNotCalledWithFreedRealm) { + constexpr int kCount = 32; + { + const v8::HandleScope handle_scope(isolate_); + Env env{handle_scope, Argv()}; + RealmBoundWrap::live_realm = (*env)->principal_realm(); + + v8::Local ctor = v8::FunctionTemplate::New(isolate_); + ctor->InstanceTemplate()->SetInternalFieldCount( + node::CppgcMixin::kInternalFieldCount); + v8::Local fn = + ctor->GetFunction(env.context()).ToLocalChecked(); + { + v8::HandleScope inner_scope(isolate_); + for (int i = 0; i <= kCount; i++) { + v8::Local obj = + fn->NewInstance(env.context()).ToLocalChecked(); + cppgc::MakeGarbageCollected( + (*env)->cppgc_allocation_handle(), *env, obj); + if (i < kCount) continue; + env.context() + ->Global() + ->Set(env.context(), + v8::String::NewFromUtf8Literal(isolate_, "kept"), + obj) + .Check(); + } + } + + CollectGarbageLeavingSweepingPending(); + EXPECT_LT(RealmBoundWrap::destructor_count, kCount); + } + RealmBoundWrap::live_realm = nullptr; + FinishSweeping(); + + EXPECT_GE(RealmBoundWrap::clean_count, 1); + EXPECT_EQ(RealmBoundWrap::clean_with_dead_realm_count, 0); +} + +TEST_F(CppgcTest, WrappersAliveAtFreeEnvironmentDoNotLeak) { + const v8::HandleScope handle_scope(isolate_); + Env env{handle_scope, Argv()}; + node::LoadEnvironment(*env, + "globalThis.script = new (require('vm').Script)('1');" + "globalThis.context = require('vm').createContext();") + .ToLocalChecked(); +} + +TEST_F(CppgcTest, VmScriptCollectedBeforeFreeEnvironmentSweptAfter) { + { + const v8::HandleScope handle_scope(isolate_); + Env env{handle_scope, Argv()}; + node::LoadEnvironment(*env, + "const { Script } = require('vm');" + "for (let i = 0; i < 64; i++) new Script('1');" + "undefined;") + .ToLocalChecked(); + CollectGarbageLeavingSweepingPending(); + } + FinishSweeping(); +}