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 iterate over the wrappers 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.

src/README.md

@@ -1041,6 +1041,26 @@ 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:
+
+```cpp
+  ~MyWrap() { this->Clean(); }
+  void CleanEnvResource(Environment* env) override {
+     // Do cleanup that relies on a living Environemnt.
+  }
+```
+
+If the cleanup also needs to access the V8 heap, it should include this
+in the private section of its class body (the `CPPGC_USING_PRE_FINALIZER`
+macro is in the [`cppgc/prefinalizer.h` header][]):
+
+```cpp
+ private:
+  CPPGC_USING_PRE_FINALIZER(MyWrap, Clean);
+```
+
 `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 +1215,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 +1228,51 @@ 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.
 
+When a cppgc-managed object is no longer reachable in the heap, its destructor
+will be invoked by the garbage collection, which can happen after the `Environment`
+is already gone, or after any object it references is gone. To ensure safety,
+the cleanups of a cppgc-managed object should adhere to different patterns,
+depending on what it needs to do:
+
+1. If it does not need to do any non-trivial, nor does its members, just use
+   the default destructor.
+2. If the cleanup relies on a living `Environment`, the class should use this
+   pattern in its class body:
+
+   ```cpp
+   ~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.
+   }
+   ```
+3. If the cleanup relies on access to the V8 heap, including using any V8
+   handles, in addition to 2, it should use the `CPPGC_USING_PRE_FINALIZER`
+   macro (from the [`cppgc/prefinalizer.h` header][]) in the private
+   section of its class body:
+
+   ```cpp
+    private:
+     CPPGC_USING_PRE_FINALIZER(MyWrap, Clean);
+   ```
+
+Both the destructor and the pre-finalizer are always called on the thread
+in which the object is created.
+
+It's worth noting that the use of pre-finalizers would have a negative impact
+on the garbage collection performance as V8 needs to scan all of them during
+each sweeping. If the object is expected to be created frequently in large
+amounts in the application, it's better to avoid access to the V8 heap in its
+cleanup to avoid having to use a pre-finalizer.
+
+For more information about the cleanup of cppgc-managed objects and
+what can be done in a pre-finalizer, see the [cppgc documentation][] and
+the [`cppgc/prefinalizer.h` header][].
+
 ### Callback scopes
 
 The public `CallbackScope` and the internally used `InternalCallbackScope`
@@ -1331,6 +1398,7 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
 [`async_hooks` module]: https://nodejs.org/api/async_hooks.html
 [`async_wrap.h`]: async_wrap.h
 [`base_object.h`]: base_object.h
+[`cppgc/prefinalizer.h` header]: ../deps/v8/include/cppgc/prefinalizer.h
 [`handle_wrap.h`]: handle_wrap.h
 [`memory_tracker.h`]: memory_tracker.h
 [`req_wrap.h`]: req_wrap.h
@@ -1341,6 +1409,7 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
 [`vm` module]: https://nodejs.org/api/vm.html
 [binding function]: #binding-functions
 [cleanup hooks]: #cleanup-hooks
+[cppgc documentation]: ../deps/v8/include/cppgc/README.md
 [event loop]: #event-loop
 [exception handling]: #exception-handling
 [fast API calls]: ../doc/contributing/adding-v8-fast-api.md

src/cppgc_helpers.h

@@ -25,20 +25,38 @@ 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.
+ *   }
+ *
+ * If the cleanup also needs access to the V8 heap, including using V8
+ * handles, it should include this in the private section of the
+ * class body (the `CPPGC_USING_PRE_FINALIZER` macro is in the
+ * `cppgc/prefinalizer.h` header):
+ *
+ *  private:
+ *   CPPGC_USING_PRE_FINALIZER(MyWrap, Clean);
  */
-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 +76,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 +107,32 @@ 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 implement
+  // it n a CleanEnvResource() override, and then call this->Clean() from their
+  // destructor. If they wish to access the V8 heap in the cleanup, they should
+  // also use CPPGC_USING_PRE_FINALIZER(MyWrap, Clean); in the private section
+  // of the class.
+  // 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
@@ -910,12 +916,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() {
@@ -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;