src: track cppgc wrappers with CppgcWrapperQueue

Author: joyeecheung

src/README.md

@@ -1041,6 +1041,17 @@ class MyWrap final : CPPGC_MIXIN(MyWrap) {
 }
 ```
 
+If the wrapper needs to perform cleanups when it's destroyed and that
+cleanup relies on a living Node.js `Environment`, it should implement a
+pattern like this:
+
+```c++
+  ~MyWrap() { this->Clean(); }
+  void CleanEnvResource(Environment* env) override {
+     // Do cleanup that relies on a living Environemnt.
+  }
+```
+
 `cppgc::GarbageCollected` types are expected to implement a
 `void Trace(cppgc::Visitor* visitor) const` method. When they are the
 final class in the hierarchy, this method must be marked `final`. For
@@ -1195,6 +1206,8 @@ referrer->Set(
 ).ToLocalChecked();
 ```
 
+#### Lifetime and cleanups of cppgc-managed objects
+
 Typically, a newly created cppgc-managed wrapper object should be held alive
 by the JavaScript land (for example, by being returned by a method and
 staying alive in a closure). Long-lived cppgc objects can also
@@ -1206,6 +1219,27 @@ it, this can happen at any time after the garbage collector notices that
 it's no longer reachable and before the V8 isolate is torn down.
 See the [Oilpan documentation in Chromium][] for more details.
 
+If the cppgc-managed objects does not need to perform any special cleanup,
+it's fine to use the default destructor. If it needs to perform only trivial
+cleanup that only affects its own members without calling into JS, potentially
+triggering garbage collection or relying on a living `Environment`, then it's
+fine to just implement the trivial cleanup in the destructor directly. If it
+needs to do any substantial cleanup that involves a living `Environment`, because
+the destructor can be called at any time by the garbage collection, even after
+the `Environment` is already gone, it must implement the cleanup with this pattern:
+
+```c++
+  ~MyWrap() { this->Clean(); }
+  void CleanEnvResource(Environment* env) override {
+     // Do cleanup that relies on a living Environemnt. This would be
+     // called by CppgcMixin::Clean() first during Environment shutdown,
+     // while the Environment is still alive. If the destructor calls
+     // Clean() again later during garbage collection that happens after
+     // Environment shutdown, CleanEnvResource() would be skipped, preventing
+     // invalid access to the Environment.
+  }
+```
+
 ### Callback scopes
 
 The public `CallbackScope` and the internally used `InternalCallbackScope`

src/cppgc_helpers.h

@@ -25,20 +25,30 @@ namespace node {
  * with V8's GC scheduling.
  *
  * A cppgc-managed native wrapper should look something like this, note
- * that per cppgc rules, CPPGC_MIXIN(Klass) must be at the left-most
+ * that per cppgc rules, CPPGC_MIXIN(MyWrap) must be at the left-most
  * position in the hierarchy (which ensures cppgc::GarbageCollected
  * is at the left-most position).
  *
- * class Klass final : CPPGC_MIXIN(Klass) {
+ * class MyWrap final : CPPGC_MIXIN(MyWrap) {
  *  public:
- *   SET_CPPGC_NAME(Klass)  // Sets the heap snapshot name to "Node / Klass"
+ *   SET_CPPGC_NAME(MyWrap)  // Sets the heap snapshot name to "Node / MyWrap"
  *   void Trace(cppgc::Visitor* visitor) const final {
  *     CppgcMixin::Trace(visitor);
  *     visitor->Trace(...);  // Trace any additional owned traceable data
  *   }
  * }
+ *
+ * If the wrapper needs to perform cleanups when it's destroyed and that
+ * cleanup relies on a living Node.js `Environment`, it should implement a
+ * pattern like this:
+ *
+ *   ~MyWrap() { this->Clean(); }
+ *   void CleanEnvResource(Environment* env) override {
+ *     // Do cleanup that relies on a living Environemnt.
+ *   }
  */
-class CppgcMixin : public cppgc::GarbageCollectedMixin {
+class CppgcMixin : public cppgc::GarbageCollectedMixin,
+                   public CppgcWrapperListNode {
  public:
   // To help various callbacks access wrapper objects with different memory
   // management, cppgc-managed objects share the same layout as BaseObjects.
@@ -58,6 +68,7 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin {
     obj->SetAlignedPointerInInternalField(
         kEmbedderType, env->isolate_data()->embedder_id_for_cppgc());
     obj->SetAlignedPointerInInternalField(kSlot, ptr);
+    env->cppgc_wrapper_list()->PushFront(ptr);
   }
 
   v8::Local<v8::Object> object() const {
@@ -88,8 +99,29 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin {
     visitor->Trace(traced_reference_);
   }
 
+  // This implements CppgcWrapperListNode::Clean and is run for all the
+  // remaining Cppgc wrappers tracked in the Environment during Environment
+  // shutdown. The destruction of the wrappers would happen later, when the
+  // final garbage collection is triggered when CppHeap is torn down as part of
+  // the Isolate teardown. If subclasses of CppgcMixin wish to perform cleanups
+  // that depend on the Environment during destruction, they should implment it
+  // in a CleanEnvResource() override, and then call this->Clean() from their
+  // destructor. Outside of CleanEnvResource(), subclasses should avoid calling
+  // into JavaScript or perform any operation that can trigger garbage
+  // collection during the destruction.
+  void Clean() override {
+    if (env_ == nullptr) return;
+    this->CleanEnvResource(env_);
+    env_ = nullptr;
+  }
+
+  // The default implementation of CleanEnvResource() is a no-op. Subclasses
+  // should override it to perform cleanup that require a living Environment,
+  // instead of doing these cleanups directly in the destructor.
+  virtual void CleanEnvResource(Environment* env) {}
+
  private:
-  Environment* env_;
+  Environment* env_ = nullptr;
   v8::TracedReference<v8::Object> traced_reference_;
 };
 

src/env.cc

@@ -1290,6 +1290,8 @@ void Environment::RunCleanup() {
     CleanupHandles();
   }
 
+  for (CppgcWrapperListNode* handle : cppgc_wrapper_list_) handle->Clean();
+
   for (const int fd : unmanaged_fds_) {
     uv_fs_t close_req;
     uv_fs_close(nullptr, &close_req, fd, nullptr);

src/env.h

@@ -615,6 +615,12 @@ class Cleanable {
   friend class Environment;
 };
 
+class CppgcWrapperListNode {
+ public:
+  virtual void Clean() = 0;
+  ListNode<CppgcWrapperListNode> wrapper_list_node;
+};
+
 /**
  * Environment is a per-isolate data structure that represents an execution
  * environment. Each environment has a principal realm. An environment can
@@ -703,6 +709,7 @@ class Environment final : public MemoryRetainer {
                        Realm* realm,
                        const ContextInfo& info);
   void UnassignFromContext(v8::Local<v8::Context> context);
+  void UntrackContext(v8::Local<v8::Context> context);
   void TrackShadowRealm(shadow_realm::ShadowRealm* realm);
   void UntrackShadowRealm(shadow_realm::ShadowRealm* realm);
 
@@ -910,12 +917,18 @@ class Environment final : public MemoryRetainer {
   typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
   typedef ListHead<ReqWrapBase, &ReqWrapBase::req_wrap_queue_> ReqWrapQueue;
   typedef ListHead<Cleanable, &Cleanable::cleanable_queue_> CleanableQueue;
+  typedef ListHead<CppgcWrapperListNode,
+                   &CppgcWrapperListNode::wrapper_list_node>
+      CppgcWrapperList;
 
   inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
   inline CleanableQueue* cleanable_queue() {
     return &cleanable_queue_;
   }
   inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }
+  inline CppgcWrapperList* cppgc_wrapper_list() {
+    return &cppgc_wrapper_list_;
+  }
 
   // https://w3c.github.io/hr-time/#dfn-time-origin
   inline uint64_t time_origin() {
@@ -1084,7 +1097,6 @@ class Environment final : public MemoryRetainer {
                                                      v8::Local<v8::Value>),
                          const char* errmsg);
   void TrackContext(v8::Local<v8::Context> context);
-  void UntrackContext(v8::Local<v8::Context> context);
 
   std::list<binding::DLib> loaded_addons_;
   v8::Isolate* const isolate_;
@@ -1203,6 +1215,8 @@ class Environment final : public MemoryRetainer {
   CleanableQueue cleanable_queue_;
   HandleWrapQueue handle_wrap_queue_;
   ReqWrapQueue req_wrap_queue_;
+  CppgcWrapperList cppgc_wrapper_list_;
+
   int handle_cleanup_waiting_ = 0;
   int request_waiting_ = 0;