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

Author: joyeecheung

node.gyp

@@ -206,6 +206,7 @@
       'src/connect_wrap.h',
       'src/connection_wrap.h',
       'src/cppgc_helpers.h',
+      'src/cppgc_helpers.cc',
       'src/dataqueue/queue.h',
       'src/debug_utils.h',
       'src/debug_utils-inl.h',

src/README.md

@@ -1113,13 +1113,13 @@ 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
+cleanup relies on a living Node.js `Realm`, it should implement a
 pattern like this:
 
 ```cpp
-  ~MyWrap() { this->Clean(); }
-  void CleanEnvResource(Environment* env) override {
-     // Do cleanup that relies on a living Environemnt.
+  ~MyWrap() { this->Finalize(); }
+  void Clean(Realm* env) override {
+     // Do cleanup that relies on a living Realm.
   }
 ```
 
@@ -1277,6 +1277,17 @@ referrer->Set(
 ).ToLocalChecked();
 ```
 
+#### Creating references between cppgc-managed objects and `BaseObject`s
+
+This is currently unsupported with the existing helpers. If this has
+to be done, new helpers must be implemented first. Consult the cppgc
+headers when trying to implement it.
+
+Another way to work around it is to always do the migration bottom-to-top.
+If a cppgc-managed object needs to reference a `BaseObject`, convert
+that `BaseObject` to be cppgc-managed first, and then use `cppgc::Member`
+to create the references.
+
 #### Lifetime and cleanups of cppgc-managed objects
 
 Typically, a newly created cppgc-managed wrapper object should be held alive
@@ -1285,31 +1296,57 @@ staying alive in a closure). Long-lived cppgc objects can also
 be held alive from C++ using persistent handles (see
 `deps/v8/include/cppgc/persistent.h`) or as members of other living
 cppgc-managed objects (see `deps/v8/include/cppgc/member.h`) if necessary.
-Its destructor will be called when no other objects from the V8 heap reference
-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:
 
-```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.
-  }
-```
+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 `Realm`
+is already gone, or after any object it references is gone. It is therefore
+unsafe to invoke V8 APIs directly in the destructors. 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 cleanup, nor does its members, just use
+   the default destructor. Note that cleanup of `v8::TracedReference` and
+   `cppgc::Member` are already handled automatically by V8 so if they are all the
+   non-trivial members the class has, this case applies.
+2. If the cleanup relies on a living `Realm`, but does not need to access V8
+   APIs, the class should use this pattern in its class body:
+
+   ```cpp
+   ~MyWrap() { this->Finalize(); }
+   void Clean(Realm* env) override {
+     // Do cleanup that relies on a living Realm. This would be
+     // called by CppgcMixin::Finalize() first during Realm shutdown,
+     // while the Realm is still alive. If the destructor calls
+     // Finalize() again later during garbage collection that happens after
+     // Realm shutdown, Clean() would be skipped, preventing
+     // invalid access to the Realm.
+   }
+   ```
+
+   If implementers want to call `Finalize()` from `Clean()` again, they
+   need to make sure that calling `Clean()` recursively is safe.
+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, Finalize);
+   ```
+
+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
 
@@ -1436,6 +1473,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
@@ -1446,6 +1484,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-inl.h

@@ -0,0 +1,59 @@
+#ifndef SRC_CPPGC_HELPERS_INL_H_
+#define SRC_CPPGC_HELPERS_INL_H_
+
+#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+
+#include "cppgc_helpers.h"
+#include "env-inl.h"
+
+namespace node {
+
+template <typename T>
+void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local<v8::Object> obj) {
+  CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount);
+  ptr->realm_ = realm;
+  v8::Isolate* isolate = realm->isolate();
+  ptr->traced_reference_ = v8::TracedReference<v8::Object>(isolate, obj);
+  // Note that ptr must be of concrete type T in Wrap.
+  v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(isolate, obj, ptr);
+  // Keep the layout consistent with BaseObjects.
+  obj->SetAlignedPointerInInternalField(
+      kEmbedderType, realm->isolate_data()->embedder_id_for_cppgc());
+  obj->SetAlignedPointerInInternalField(kSlot, ptr);
+  realm->TrackCppgcWrapper(ptr);
+}
+
+template <typename T>
+void CppgcMixin::Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj) {
+  Wrap(ptr, env->principal_realm(), obj);
+}
+
+template <typename T>
+T* CppgcMixin::Unwrap(v8::Local<v8::Object> obj) {
+  // We are not using v8::Object::Unwrap currently because that requires
+  // access to isolate which the ASSIGN_OR_RETURN_UNWRAP macro that we'll shim
+  // with ASSIGN_OR_RETURN_UNWRAP_GC doesn't take, and we also want a
+  // signature consistent with BaseObject::Unwrap() to avoid churn. Since
+  // cppgc-managed objects share the same layout as BaseObjects, just unwrap
+  // from the pointer in the internal field, which should be valid as long as
+  // the object is still alive.
+  if (obj->InternalFieldCount() != T::kInternalFieldCount) {
+    return nullptr;
+  }
+  T* ptr = static_cast<T*>(obj->GetAlignedPointerFromInternalField(T::kSlot));
+  return ptr;
+}
+
+v8::Local<v8::Object> CppgcMixin::object() const {
+  return traced_reference_.Get(realm_->isolate());
+}
+
+Environment* CppgcMixin::env() const {
+  return realm_->env();
+}
+
+}  // namespace node
+
+#endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+
+#endif  // SRC_CPPGC_HELPERS_INL_H_

src/cppgc_helpers.cc

@@ -0,0 +1,18 @@
+#include "cppgc_helpers.h"
+#include "env-inl.h"
+
+namespace node {
+
+void CppgcWrapperList::Cleanup() {
+  for (auto handle : *this) {
+    handle->Finalize();
+  }
+}
+
+void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const {
+  for (auto handle : *this) {
+    tracker->Track(handle);
+  }
+}
+
+}  // namespace node

src/cppgc_helpers.h

@@ -6,14 +6,18 @@
 #include <type_traits>  // std::remove_reference
 #include "cppgc/garbage-collected.h"
 #include "cppgc/name-provider.h"
-#include "env.h"
 #include "memory_tracker.h"
+#include "util.h"
 #include "v8-cppgc.h"
 #include "v8-sandbox.h"
 #include "v8.h"
 
 namespace node {
 
+class Environment;
+class Realm;
+class CppgcWrapperList;
+
 /**
  * This is a helper mixin with a BaseObject-like interface to help
  * implementing wrapper objects managed by V8's cppgc (Oilpan) library.
@@ -39,16 +43,15 @@ namespace node {
  * }
  *
  * 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
+ * cleanup relies on a living Node.js `Realm`, it should implement a
  * pattern like this:
  *
- *   ~MyWrap() { this->Clean(); }
- *   void CleanEnvResource(Environment* env) override {
+ *   ~MyWrap() { this->Destroy(); }
+ *   void Clean(Realm* env) override {
  *     // Do cleanup that relies on a living Environemnt.
  *   }
  */
-class CppgcMixin : public cppgc::GarbageCollectedMixin,
-                   public CppgcWrapperListNode {
+class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
  public:
   // To help various callbacks access wrapper objects with different memory
   // management, cppgc-managed objects share the same layout as BaseObjects.
@@ -58,71 +61,55 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin,
   // invoked from the child class constructor, per cppgc::GarbageCollectedMixin
   // rules.
   template <typename T>
-  static void Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj) {
-    CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount);
-    ptr->env_ = env;
-    v8::Isolate* isolate = env->isolate();
-    ptr->traced_reference_ = v8::TracedReference<v8::Object>(isolate, obj);
-    v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(isolate, obj, ptr);
-    // Keep the layout consistent with BaseObjects.
-    obj->SetAlignedPointerInInternalField(
-        kEmbedderType, env->isolate_data()->embedder_id_for_cppgc());
-    obj->SetAlignedPointerInInternalField(kSlot, ptr);
-    env->cppgc_wrapper_list()->PushFront(ptr);
-  }
+  static inline void Wrap(T* ptr, Realm* realm, v8::Local<v8::Object> obj);
+  template <typename T>
+  static inline void Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj);
 
-  v8::Local<v8::Object> object() const {
-    return traced_reference_.Get(env_->isolate());
+  inline v8::Local<v8::Object> object() const;
+  inline Environment* env() const;
+  inline v8::Local<v8::Object> object(v8::Isolate* isolate) const {
+    return traced_reference_.Get(isolate);
   }
 
-  Environment* env() const { return env_; }
-
   template <typename T>
-  static T* Unwrap(v8::Local<v8::Object> obj) {
-    // We are not using v8::Object::Unwrap currently because that requires
-    // access to isolate which the ASSIGN_OR_RETURN_UNWRAP macro that we'll shim
-    // with ASSIGN_OR_RETURN_UNWRAP_GC doesn't take, and we also want a
-    // signature consistent with BaseObject::Unwrap() to avoid churn. Since
-    // cppgc-managed objects share the same layout as BaseObjects, just unwrap
-    // from the pointer in the internal field, which should be valid as long as
-    // the object is still alive.
-    if (obj->InternalFieldCount() != T::kInternalFieldCount) {
-      return nullptr;
-    }
-    T* ptr = static_cast<T*>(obj->GetAlignedPointerFromInternalField(T::kSlot));
-    return ptr;
-  }
+  static inline T* Unwrap(v8::Local<v8::Object> obj);
 
   // Subclasses are expected to invoke CppgcMixin::Trace() in their own Trace()
   // methods.
   void Trace(cppgc::Visitor* visitor) const override {
     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
+  // TODO(joyeecheung): use ObjectSizeTrait;
+  inline size_t SelfSize() const override { return sizeof(*this); }
+  inline bool IsCppgcWrapper() const override { return true; }
+
+  // This is run for all the remaining Cppgc wrappers tracked in the Realm
+  // during Realm 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 Realm during destruction, they should implment
+  // it in a Clean() override, and then call this->Finalize() from their
+  // destructor. Outside of Finalize(), 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;
+  void Finalize() {
+    if (realm_ == nullptr) return;
+    this->Clean(realm_);
+    realm_ = nullptr;
   }
 
-  // The default implementation of CleanEnvResource() is a no-op. Subclasses
-  // should override it to perform cleanup that require a living Environment,
+  // 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.
-  virtual void CleanEnvResource(Environment* env) {}
+  virtual void Clean(Realm* realm) {}
+
+  friend class CppgcWrapperList;
 
  private:
-  Environment* env_ = nullptr;
+  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
@@ -137,7 +124,8 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin,
 #define SET_CPPGC_NAME(Klass)                                                  \
   inline const char* GetHumanReadableName() const final {                      \
     return "Node / " #Klass;                                                   \
-  }
+  }                                                                            \
+  inline const char* MemoryInfoName() const override { return #Klass; }
 
 /**
  * Similar to ASSIGN_OR_RETURN_UNWRAP() but works on cppgc-managed types

src/env.cc

@@ -1302,8 +1302,6 @@ 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);