fixup! src: track cppgc wrappers with a list in Realm

Author: joyeecheung

src/cppgc_helpers-inl.h

@@ -52,6 +52,12 @@ Environment* CppgcMixin::env() const {
   return realm_->env();
 }
 
+CppgcMixin::~CppgcMixin() {
+  if (realm_ != nullptr) {
+    realm_->set_should_purge_empty_cppgc_wrappers(true);
+  }
+}
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

src/cppgc_helpers.cc

@@ -4,15 +4,34 @@
 namespace node {
 
 void CppgcWrapperList::Cleanup() {
-  for (auto handle : *this) {
-    handle->Finalize();
+  for (auto node : *this) {
+    CppgcMixin* ptr = node->persistent.Get();
+    if (ptr != nullptr) {
+      ptr->Finalize();
+    }
   }
 }
 
 void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const {
-  for (auto handle : *this) {
-    tracker->Track(handle);
+  for (auto node : *this) {
+    CppgcMixin* ptr = node->persistent.Get();
+    if (ptr != nullptr) {
+      tracker->Track(ptr);
+    }
   }
 }
 
+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

src/cppgc_helpers.h

@@ -6,6 +6,7 @@
 #include <type_traits>  // std::remove_reference
 #include "cppgc/garbage-collected.h"
 #include "cppgc/name-provider.h"
+#include "cppgc/persistent.h"
 #include "memory_tracker.h"
 #include "util.h"
 #include "v8-cppgc.h"
@@ -16,7 +17,7 @@ namespace node {
 
 class Environment;
 class Realm;
-class CppgcWrapperList;
+class CppgcWrapperListNode;
 
 /**
  * This is a helper mixin with a BaseObject-like interface to help
@@ -100,17 +101,20 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
     realm_ = nullptr;
   }
 
-  // The default implementation of Clean() is a no-op. Subclasses
-  // should override it to perform cleanup that require a living Realm,
-  // instead of doing these cleanups directly in the destructor.
+  // The default implementation of Clean() is a no-op. If subclasses wish
+  // to perform cleanup that require a living Realm, they should
+  // should put the cleanups in a Clean() override, and call this->Finalize()
+  // in the destructor, instead of doing those cleanups directly in the
+  // destructor.
   virtual void Clean(Realm* realm) {}
 
-  friend class CppgcWrapperList;
+  inline ~CppgcMixin();
+
+  friend class CppgcWrapperListNode;
 
  private:
   Realm* realm_ = nullptr;
   v8::TracedReference<v8::Object> traced_reference_;
-  ListNode<CppgcMixin> wrapper_list_node_;
 };
 
 // If the class doesn't have additional owned traceable data, use this macro to

src/node_realm-inl.h

@@ -130,9 +130,11 @@ void Realm::TrackBaseObject(BaseObject* bo) {
   ++base_object_count_;
 }
 
+CppgcWrapperListNode::CppgcWrapperListNode(CppgcMixin* ptr) : persistent(ptr) {}
+
 void Realm::TrackCppgcWrapper(CppgcMixin* handle) {
   DCHECK_EQ(handle->realm(), this);
-  cppgc_wrapper_list_.PushFront(handle);
+  cppgc_wrapper_list_.PushFront(new CppgcWrapperListNode(handle));
 }
 
 void Realm::UntrackBaseObject(BaseObject* bo) {

src/node_realm.cc

@@ -10,7 +10,10 @@ namespace node {
 
 using v8::Context;
 using v8::EscapableHandleScope;
+using v8::GCCallbackFlags;
+using v8::GCType;
 using v8::HandleScope;
+using v8::Isolate;
 using v8::Local;
 using v8::MaybeLocal;
 using v8::Object;
@@ -22,12 +25,28 @@ Realm::Realm(Environment* env, v8::Local<v8::Context> context, Kind kind)
     : env_(env), isolate_(context->GetIsolate()), 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<Realm*>(data);
+  if (realm->should_purge_empty_cppgc_wrappers_) {
+    realm->cppgc_wrapper_list_.PurgeEmpty();
+    realm->should_purge_empty_cppgc_wrappers_ = false;
+  }
+}
+
 void Realm::MemoryInfo(MemoryTracker* tracker) const {
 #define V(PropertyName, TypeName)                                              \
   tracker->TrackField(#PropertyName, PropertyName());

src/node_realm.h

@@ -26,11 +26,34 @@ using BindingDataStore =
     std::array<BaseObjectWeakPtr<BaseObject>,
                static_cast<size_t>(BindingDataType::kBindingDataTypeCount)>;
 
+/**
+ * This is a wrapper around a weak persistent of CppgcMixin, used in the
+ * CppgcWrapperList to avoid accessing already garbage collected CppgcMixins.
+ */
+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(); }
+
+  cppgc::WeakPersistent<CppgcMixin> persistent;
+  // Used by ContainerOf in the ListNode implementation for fast manipulation of
+  // CppgcWrapperList.
+  ListNode<CppgcWrapperListNode> wrapper_list_node;
+};
+
+/**
+ * A per-realm list of weak persistent of cppgc wrappers, which implements
+ * iterations that require iterate over cppgc wrappers created by Node.js.
+ */
 class CppgcWrapperList
-    : public ListHead<CppgcMixin, &CppgcMixin::wrapper_list_node_>,
+    : public ListHead<CppgcWrapperListNode,
+                      &CppgcWrapperListNode::wrapper_list_node>,
       public MemoryRetainer {
  public:
   void Cleanup();
+  void PurgeEmpty();
 
   SET_MEMORY_INFO_NAME(CppgcWrapperList)
   SET_SELF_SIZE(CppgcWrapperList)
@@ -141,6 +164,14 @@ class Realm : public MemoryRetainer {
   // it's only used for tests.
   std::vector<std::string> 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();
 
@@ -150,11 +181,17 @@ class Realm : public MemoryRetainer {
   // Shorthand for isolate pointer.
   v8::Isolate* isolate_;
   v8::Global<v8::Context> context_;
+  bool should_purge_empty_cppgc_wrappers_ = false;
 
 #define V(PropertyName, TypeName) v8::Global<TypeName> 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<v8::Context> context,
                          const RealmSerializeInfo* realm_info);