src: track cppgc wrappers with a list in Realm

This allows us to perform cleanups of cppgc wrappers that rely on a living Realm during Realm shutdown. Otherwise the cleanup may happen during object destruction, which can be triggered by GC after Realm shutdown, leading to invalid access to Realm. The general pattern for this type of non-trivial destruction is designed to be: ``` class MyWrap final : CPPGC_MIXIN(MyWrap) { public: ~MyWrap() { this->Finalize(); } void Clean(Realm* realm) 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. } } ``` 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. PR-URL: #56534 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Author: joyeecheung

node.gyp

@@ -205,6 +205,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

@@ -1131,6 +1131,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 `Realm`, it should implement a
+pattern like this:
+
+```cpp
+ ~MyWrap() { this->Finalize(); }
+ void Clean(Realm* env) override {
+ // Do cleanup that relies on a living Realm.
+ }
+```
+
 `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
@@ -1285,16 +1296,76 @@ 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
 by the JavaScript land (for example, by being returned by a method and
 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.
+
+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. 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
 
@@ -1421,6 +1492,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
@@ -1432,6 +1504,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,65 @@
+#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();
+}
+
+CppgcMixin::~CppgcMixin() {
+ if (realm_ != nullptr) {
+ realm_->set_should_purge_empty_cppgc_wrappers(true);
+ }
+}
+
+} // namespace node
+
+#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+
+#endif // SRC_CPPGC_HELPERS_INL_H_

src/cppgc_helpers.cc

@@ -0,0 +1,37 @@
+#include "cppgc_helpers.h"
+#include "env-inl.h"
+
+namespace node {
+
+void CppgcWrapperList::Cleanup() {
+ for (auto node : *this) {
+ CppgcMixin* ptr = node->persistent.Get();
+ if (ptr != nullptr) {
+ ptr->Finalize();
+ }
+ }
+}
+
+void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const {
+ 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,14 +6,19 @@
 #include <type_traits> // std::remove_reference
 #include "cppgc/garbage-collected.h"
 #include "cppgc/name-provider.h"
-#include "env.h"
+#include "cppgc/persistent.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 CppgcWrapperListNode;
+
 /**
  * This is a helper mixin with a BaseObject-like interface to help
  * implementing wrapper objects managed by V8's cppgc (Oilpan) library.
@@ -25,20 +30,29 @@ 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 `Realm`, it should implement a
+ * pattern like this:
+ *
+ * ~MyWrap() { this->Destroy(); }
+ * void Clean(Realm* env) override {
+ * // Do cleanup that relies on a living Environemnt.
+ * }
  */
-class CppgcMixin : public cppgc::GarbageCollectedMixin {
+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.
@@ -48,48 +62,58 @@ 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);
- }
+ 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 Realm* realm() const { return realm_; }
+ 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_);
  }
 
+ // 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 Finalize() {
+ if (realm_ == nullptr) return;
+ this->Clean(realm_);
+ realm_ = nullptr;
+ }
+
+ // 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) {}
+
+ inline ~CppgcMixin();
+
+ friend class CppgcWrapperListNode;
+
  private:
- Environment* env_;
+ Realm* realm_ = nullptr;
  v8::TracedReference<v8::Object> traced_reference_;
 };
 
@@ -105,7 +129,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