src: free placeholder nodes for cppgc wrappers in MemoryTracker

`MemoryTracker::AddNode(const CppgcMixin*)` allocates a
`MemoryRetainerNode` that only stands in for the wrapper's JS node while
its `MemoryInfo()` runs; unlike the other node kinds it is not handed
to the `EmbedderGraph`, and nothing freed it. Every heap snapshot (or
other `BuildEmbedderGraph` call) leaked one node per live `vm.Script`
or `vm` context.

Keep the placeholders in the tracker and free them with it, and make
the already-seen path in `Track(const CppgcMixin*)` add its edge to the
wrapper's JS node like the first visit does, so the graph never refers
to a placeholder.

Refs: #56534
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

Author: codebytere

src/memory_tracker-inl.h

@@ -300,7 +300,7 @@ void MemoryTracker::Track(const CppgcMixin* retainer, const char* edge_name) {
   auto it = seen_.find(retainer);
   if (it != seen_.end()) {
     if (CurrentNode() != nullptr) {
-      AddEdge(CurrentNode(), it->second, edge_name);
+      AddEdge(CurrentNode(), it->second->JSWrapperNode(), edge_name);
     }
     return;  // It has already been tracked, no need to call MemoryInfo again
   }
@@ -357,6 +357,11 @@ inline void MemoryTracker::TraitTrackInline(const T& retainer,
       -(static_cast<int>(MemoryRetainerTraits<T>::SelfSize(retainer))));
 }
 
+MemoryTracker::MemoryTracker(v8::Isolate* isolate, v8::EmbedderGraph* graph)
+    : isolate_(isolate), graph_(graph) {}
+
+MemoryTracker::~MemoryTracker() = default;
+
 v8::EmbedderGraph::Node* MemoryTracker::CurrentNode() const {
   if (node_stack_.empty()) return nullptr;
   MemoryRetainerNode* n = node_stack_.top();
@@ -373,7 +378,8 @@ MemoryRetainerNode* MemoryTracker::AddNode(const CppgcMixin* retainer,
     return it->second;
   }
 
-  MemoryRetainerNode* n = new MemoryRetainerNode(this, retainer);
+  cppgc_nodes_.push_back(std::make_unique<MemoryRetainerNode>(this, retainer));
+  MemoryRetainerNode* n = cppgc_nodes_.back().get();
   seen_[retainer] = n;
   if (CurrentNode() != nullptr) {
     AddEdge(CurrentNode(), n->JSWrapperNode(), edge_name);

src/memory_tracker.h

@@ -8,10 +8,12 @@
 #include <uv.h>
 
 #include <limits>
+#include <memory>
 #include <queue>
 #include <stack>
 #include <string>
 #include <unordered_map>
+#include <vector>
 
 namespace v8 {
 class BackingStore;
@@ -294,9 +296,8 @@ class MemoryTracker {
   inline v8::EmbedderGraph* graph() { return graph_; }
   inline v8::Isolate* isolate() { return isolate_; }
 
-  inline explicit MemoryTracker(v8::Isolate* isolate,
-                                v8::EmbedderGraph* graph)
-    : isolate_(isolate), graph_(graph) {}
+  inline explicit MemoryTracker(v8::Isolate* isolate, v8::EmbedderGraph* graph);
+  inline ~MemoryTracker();
 
   // Can be passed to Track() if it is not desirable
   // to create a strong edge between nodes, i.e. when
@@ -334,6 +335,8 @@ class MemoryTracker {
   v8::EmbedderGraph* graph_;
   std::stack<MemoryRetainerNode*> node_stack_;
   NodeMap seen_;
+  // Placeholder nodes for cppgc wrappers; the graph only owns their JS nodes.
+  std::vector<std::unique_ptr<MemoryRetainerNode>> cppgc_nodes_;
 };
 
 }  // namespace node

test/cctest/test_environment.cc

@@ -411,6 +411,19 @@ TEST_F(EnvironmentTest, CollectExternalReferencesFromSeveralThreads) {
   EXPECT_EQ(node::SnapshotBuilder::CollectExternalReferences().back(), 0);
 }
 
+TEST_F(EnvironmentTest, HeapSnapshotWithCppgcWrappersDoesNotLeak) {
+  const v8::HandleScope handle_scope(isolate_);
+  const Argv argv;
+  Env env{handle_scope, argv};
+  node::LoadEnvironment(*env,
+                        "const vm = require('vm');"
+                        "globalThis.script = new vm.Script('1');"
+                        "globalThis.context = vm.createContext();")
+      .ToLocalChecked();
+  node::heap::HeapSnapshotPointer snapshot{
+      isolate_->GetHeapProfiler()->TakeHeapSnapshot()};
+}
+
 TEST_F(EnvironmentTest, NoEnvironmentSanity) {
   const v8::HandleScope handle_scope(isolate_);
   v8::Local<v8::Context> context = v8::Context::New(isolate_);