crypto: use cppgc to manage Hash

Author: joyeecheung

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");

src/crypto/crypto_hash.h

@@ -3,7 +3,7 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
-#include "base_object.h"
+#include "cppgc_helpers.h"
 #include "crypto/crypto_keys.h"
 #include "crypto/crypto_util.h"
 #include "env.h"
@@ -12,29 +12,30 @@
 
 namespace node {
 namespace crypto {
-class Hash final : public BaseObject {
+
+class Hash final : CPPGC_MIXIN(Hash) {
  public:
+  SET_CPPGC_NAME(Hash)
+  void Trace(cppgc::Visitor* visitor) const final;
+  void MemoryInfo(MemoryTracker* tracker) const override;
+
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
   static void RegisterExternalReferences(ExternalReferenceRegistry* registry);
 
-  void MemoryInfo(MemoryTracker* tracker) const override;
-  SET_MEMORY_INFO_NAME(Hash)
-  SET_SELF_SIZE(Hash)
-
   bool HashInit(const EVP_MD* md, v8::Maybe<unsigned int> xof_md_len);
   bool HashUpdate(const char* data, size_t len);
 
   static void GetHashes(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void GetCachedAliases(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void OneShotDigest(const v8::FunctionCallbackInfo<v8::Value>& args);
 
+  Hash(Environment* env, v8::Local<v8::Object> wrap);
+
  protected:
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void HashUpdate(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void HashDigest(const v8::FunctionCallbackInfo<v8::Value>& args);
 
-  Hash(Environment* env, v8::Local<v8::Object> wrap);
-
  private:
   ncrypto::EVPMDCtxPointer mdctx_{};
   unsigned int md_len_ = 0;

src/crypto/crypto_util.h

@@ -4,6 +4,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "async_wrap.h"
+#include "cppgc_helpers.h"
 #include "env.h"
 #include "node_errors.h"
 #include "node_external_reference.h"
@@ -74,7 +75,12 @@ void Decode(const v8::FunctionCallbackInfo<v8::Value>& args,
             void (*callback)(T*, const v8::FunctionCallbackInfo<v8::Value>&,
                              const char*, size_t)) {
   T* ctx;
-  ASSIGN_OR_RETURN_UNWRAP(&ctx, args.This());
+  if constexpr (std::is_base_of_v<BaseObject, T>) {
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.This());
+  } else {
+    ctx = CppgcMixin::Unwrap<T>(args.This());
+    if (ctx == nullptr) return;
+  }
 
   if (args[0]->IsString()) {
     StringBytes::InlineDecoder decoder;

test/common/heap.js

@@ -231,11 +231,13 @@ function validateSnapshotNodes(...args) {
  *   node_type?: string,
  *   edge_type?: string,
  * }]} retainingPath The retaining path specification to search from the root nodes.
+ * @param {boolean} allowEmpty Whether the function should fail if no matching nodes can be found.
+ *
  * @returns {[object]} All the leaf nodes matching the retaining path specification. If none can be found,
  *                     logs the nodes found in the last matching step of the path (if any), and throws an
  *                     assertion error.
  */
-function findByRetainingPath(rootName, retainingPath) {
+function findByRetainingPath(rootName, retainingPath, allowEmpty = false) {
   const nodes = createJSHeapSnapshot();
   let haystack = nodes.filter((n) => n.name === rootName && n.type !== 'string');
 
@@ -269,19 +271,23 @@ function findByRetainingPath(rootName, retainingPath) {
     }
 
     if (newHaystack.length === 0) {
-      const format = (val) => util.inspect(val, { breakLength: 128, depth: 3 });
-      console.error('#');
-      console.error('# Retaining path to search for:');
-      for (let j = 0; j < retainingPath.length; ++j) {
-        console.error(`# - '${format(retainingPath[j])}'${i === j ? '\t<--- not found' : ''}`);
-      }
-      console.error('#\n');
-      console.error('# Nodes found in the last step include:');
-      for (let j = 0; j < haystack.length; ++j) {
-        console.error(`# - '${format(haystack[j])}`);
+      if (allowEmpty) {
+        return [];
+      } else {
+        const format = (val) => util.inspect(val, { breakLength: 128, depth: 3 });
+        console.error('#');
+        console.error('# Retaining path to search for:');
+        for (let j = 0; j < retainingPath.length; ++j) {
+          console.error(`# - '${format(retainingPath[j])}'${i === j ? '\t<--- not found' : ''}`);
+        }
+        console.error('#\n');
+        console.error('# Nodes found in the last step include:');
+        for (let j = 0; j < haystack.length; ++j) {
+          console.error(`# - '${format(haystack[j])}`);
+        }
+  
+        assert.fail(`Could not find target edge ${format(expected)} in the heap snapshot.`);
       }
-
-      assert.fail(`Could not find target edge ${format(expected)} in the heap snapshot.`);
     }
 
     haystack = newHaystack;
@@ -326,4 +332,5 @@ module.exports = {
   validateSnapshotNodes,
   findByRetainingPath,
   getHeapSnapshotOptionTests,
+  createJSHeapSnapshot,
 };

test/pummel/test-heapdump-hash.js

@@ -0,0 +1,33 @@
+'use strict';
+require('../common');
+const { findByRetainingPath, createJSHeapSnapshot } = require('../common/heap');
+const { createHash } = require('crypto');
+const assert = require('assert');
+
+// In case the bootstrap process creates any Hash objects, capture a snapshot first
+// and save the initial length.
+const originalNodes = findByRetainingPath('Node / Hash', [
+  { edge_name: 'mdctx' },
+], true);
+
+const count = 5;
+const arr = [];
+for (let i = 0; i < count; ++i) {
+  arr.push(createHash('sha1'));
+}
+
+const nodesWithCtx = findByRetainingPath('Node / Hash', [
+  { edge_name: 'mdctx', node_name: 'Node / EVP_MD_CTX' },
+]);
+
+assert.strictEqual(nodesWithCtx.length - originalNodes.length, count);
+
+for (let i = 0; i < count; ++i) {
+  arr[i].update('test').digest('hex');
+}
+
+const nodesWithDigest = findByRetainingPath('Node / Hash', [
+  { edge_name: 'md', node_name: 'Node / ByteSource' },
+]);
+
+assert.strictEqual(nodesWithDigest.length - originalNodes.length, count);