wip: crypto: use cppgc to manage Hash

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

@@ -1112,6 +1112,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
@@ -1266,16 +1277,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. 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
 
@@ -1402,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
@@ -1412,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.
@@ -25,20 +29,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,49 +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);
-  }
+  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_);
   }
 
+  // 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. Subclasses
+  // should override it to perform cleanup that require a living Realm,
+  // instead of doing these cleanups directly in the destructor.
+  virtual void Clean(Realm* realm) {}
+
+  friend class CppgcWrapperList;
+
  private:
-  Environment* env_;
+  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
@@ -105,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/crypto/crypto_hash.cc

@@ -1,6 +1,8 @@
 #include "crypto/crypto_hash.h"
 #include "async_wrap-inl.h"
 #include "base_object-inl.h"
+#include "cppgc/allocation.h"
+#include "cppgc_helpers-inl.h"
 #include "env-inl.h"
 #include "memory_tracker-inl.h"
 #include "string_bytes.h"
@@ -31,14 +33,23 @@ using v8::Object;
 using v8::Uint32;
 using v8::Value;
 
+#ifdef ASSIGN_OR_RETURN_UNWRAP
+#undef ASSIGN_OR_RETURN_UNWRAP
+#endif
+
+#define ASSIGN_OR_RETURN_UNWRAP ASSIGN_OR_RETURN_UNWRAP_CPPGC
 namespace crypto {
-Hash::Hash(Environment* env, Local<Object> wrap) : BaseObject(env, wrap) {
-  MakeWeak();
+Hash::Hash(Environment* env, Local<Object> wrap) {
+  CppgcMixin::Wrap(this, env, wrap);
+}
+
+void Hash::Trace(cppgc::Visitor* visitor) const {
+  CppgcMixin::Trace(visitor);
 }
 
 void Hash::MemoryInfo(MemoryTracker* tracker) const {
-  tracker->TrackFieldWithSize("mdctx", mdctx_ ? kSizeOf_EVP_MD_CTX : 0);
-  tracker->TrackFieldWithSize("md", digest_ ? md_len_ : 0);
+  tracker->TrackFieldWithSize("mdctx", mdctx_ ? kSizeOf_EVP_MD_CTX : 0, "EVP_MD_CTX");
+  tracker->TrackFieldWithSize("md", digest_ ? md_len_ : 0, "ByteSource");
 }
 
 #if OPENSSL_VERSION_MAJOR >= 3
@@ -322,7 +333,8 @@ void Hash::New(const FunctionCallbackInfo<Value>& args) {
     xof_md_len = Just<unsigned int>(args[1].As<Uint32>()->Value());
   }
 
-  Hash* hash = new Hash(env, args.This());
+  Hash* hash = cppgc::MakeGarbageCollected<Hash>(
+      env->isolate()->GetCppHeap()->GetAllocationHandle(), env, args.This());
   if (md == nullptr || !hash->HashInit(md, xof_md_len)) {
     return ThrowCryptoError(env, ERR_get_error(),
                             "Digest method not supported");