src: track cppgc wrappers with CppgcWrapperList in Environment

This allows us to perform cleanups of cppgc wrappers that rely
on a living Environment during Environment shutdown. Otherwise
the cleanup may happen during object destruction, which can
be triggered by GC after Enivronment shutdown, leading to invalid
access to Environment.

The general pattern for this type of non-trivial destruction is
designed to be:

```
class MyWrap final : CPPGC_MIXIN(MyWrap) {
 public:
  ~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.
  }
}
```

In addition, this allows us to trace external memory held by the wrappers
in the heap snapshots if we add synthethic edges between the wrappers
and other nodes in the embdder graph callback, or to perform snapshot
serialization for them.

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;