deps,src,test: deflake test-diagnostics-channel-memory-leak

common.gypi

@@ -36,7 +36,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.17',
+    'v8_embedder_string': '-node.18',
 
     ##### V8 defaults for Node.js #####
 

deps/v8/include/v8-profiler.h

@@ -921,12 +921,22 @@ class V8_EXPORT EmbedderGraph {
   virtual ~EmbedderGraph() = default;
 };
 
+class QueryObjectPredicate {
+ public:
+  virtual ~QueryObjectPredicate() = default;
+  virtual bool Filter(v8::Local<v8::Object> object) = 0;
+};
+
 /**
  * Interface for controlling heap profiling. Instance of the
  * profiler can be retrieved using v8::Isolate::GetHeapProfiler.
  */
 class V8_EXPORT HeapProfiler {
  public:
+  void QueryObjects(v8::Local<v8::Context> context,
+                    QueryObjectPredicate* predicate,
+                    std::vector<v8::Global<v8::Object>>* objects);
+
   enum SamplingFlags {
     kSamplingNoFlags = 0,
     kSamplingForceGC = 1 << 0,

deps/v8/src/api/api.cc

@@ -10917,6 +10917,16 @@ int HeapProfiler::GetSnapshotCount() {
   return reinterpret_cast<i::HeapProfiler*>(this)->GetSnapshotsCount();
 }
 
+void HeapProfiler::QueryObjects(Local<Context> v8_context,
+                                QueryObjectPredicate* predicate,
+                                std::vector<Global<Object>>* objects) {
+  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_context->GetIsolate());
+  i::HeapProfiler* profiler = reinterpret_cast<i::HeapProfiler*>(this);
+  DCHECK_EQ(isolate, profiler->isolate());
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
+  profiler->QueryObjects(Utils::OpenHandle(*v8_context), predicate, objects);
+}
+
 const HeapSnapshot* HeapProfiler::GetHeapSnapshot(int index) {
   return reinterpret_cast<const HeapSnapshot*>(
       reinterpret_cast<i::HeapProfiler*>(this)->GetSnapshot(index));

deps/v8/src/debug/debug-interface.cc

@@ -1211,15 +1211,6 @@ v8::MaybeLocal<v8::Value> EvaluateGlobalForTesting(
   RETURN_ESCAPED(result);
 }
 
-void QueryObjects(v8::Local<v8::Context> v8_context,
-                  QueryObjectPredicate* predicate,
-                  std::vector<v8::Global<v8::Object>>* objects) {
-  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_context->GetIsolate());
-  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
-  isolate->heap_profiler()->QueryObjects(Utils::OpenHandle(*v8_context),
-                                         predicate, objects);
-}
-
 void GlobalLexicalScopeNames(v8::Local<v8::Context> v8_context,
                              std::vector<v8::Global<v8::String>>* names) {
   i::Handle<i::Context> context = Utils::OpenHandle(*v8_context);

deps/v8/src/debug/debug-interface.h

@@ -530,16 +530,6 @@ class V8_EXPORT_PRIVATE StackTraceIterator {
                                              bool throw_on_side_effect) = 0;
 };
 
-class QueryObjectPredicate {
- public:
-  virtual ~QueryObjectPredicate() = default;
-  virtual bool Filter(v8::Local<v8::Object> object) = 0;
-};
-
-void QueryObjects(v8::Local<v8::Context> context,
-                  QueryObjectPredicate* predicate,
-                  std::vector<v8::Global<v8::Object>>* objects);
-
 void GlobalLexicalScopeNames(v8::Local<v8::Context> context,
                              std::vector<v8::Global<v8::String>>* names);
 

deps/v8/src/inspector/v8-debugger.cc

@@ -8,6 +8,7 @@
 #include "include/v8-context.h"
 #include "include/v8-function.h"
 #include "include/v8-microtask-queue.h"
+#include "include/v8-profiler.h"
 #include "include/v8-util.h"
 #include "src/inspector/inspected-context.h"
 #include "src/inspector/protocol/Protocol.h"
@@ -38,7 +39,7 @@ void cleanupExpiredWeakPointers(Map& map) {
   }
 }
 
-class MatchPrototypePredicate : public v8::debug::QueryObjectPredicate {
+class MatchPrototypePredicate : public v8::QueryObjectPredicate {
  public:
   MatchPrototypePredicate(V8InspectorImpl* inspector,
                           v8::Local<v8::Context> context,
@@ -994,7 +995,7 @@ v8::Local<v8::Array> V8Debugger::queryObjects(v8::Local<v8::Context> context,
   v8::Isolate* isolate = context->GetIsolate();
   std::vector<v8::Global<v8::Object>> v8_objects;
   MatchPrototypePredicate predicate(m_inspector, context, prototype);
-  v8::debug::QueryObjects(context, &predicate, &v8_objects);
+  isolate->GetHeapProfiler()->QueryObjects(context, &predicate, &v8_objects);
 
   v8::MicrotasksScope microtasksScope(context,
                                       v8::MicrotasksScope::kDoNotRunMicrotasks);

deps/v8/src/profiler/heap-profiler.cc

@@ -247,7 +247,7 @@ Heap* HeapProfiler::heap() const { return ids_->heap(); }
 Isolate* HeapProfiler::isolate() const { return heap()->isolate(); }
 
 void HeapProfiler::QueryObjects(Handle<Context> context,
-                                debug::QueryObjectPredicate* predicate,
+                                v8::QueryObjectPredicate* predicate,
                                 std::vector<v8::Global<v8::Object>>* objects) {
   // We need a stack marker here to allow deterministic passes over the stack.
   // The garbage collection and the two object heap iterators should scan the

deps/v8/src/profiler/heap-profiler.h

@@ -30,7 +30,7 @@ class StringsStorage;
 // generate consistent IDs for moved objects.
 class HeapProfilerNativeMoveListener {
  public:
-  HeapProfilerNativeMoveListener(HeapProfiler* profiler)
+  explicit HeapProfilerNativeMoveListener(HeapProfiler* profiler)
       : profiler_(profiler) {}
   HeapProfilerNativeMoveListener(const HeapProfilerNativeMoveListener& other) =
       delete;
@@ -116,8 +116,7 @@ class HeapProfiler : public HeapObjectAllocationTracker {
 
   Isolate* isolate() const;
 
-  void QueryObjects(Handle<Context> context,
-                    debug::QueryObjectPredicate* predicate,
+  void QueryObjects(Handle<Context> context, QueryObjectPredicate* predicate,
                     std::vector<v8::Global<v8::Object>>* objects);
   void set_native_move_listener(
       std::unique_ptr<HeapProfilerNativeMoveListener> listener) {

deps/v8/test/cctest/test-heap-profiler.cc

@@ -30,6 +30,7 @@
 #include <ctype.h>
 
 #include <memory>
+#include <vector>
 
 #include "include/v8-function.h"
 #include "include/v8-json.h"
@@ -4064,6 +4065,67 @@ TEST(SamplingHeapProfilerSampleDuringDeopt) {
   heap_profiler->StopSamplingHeapProfiler();
 }
 
+namespace {
+class TestQueryObjectPredicate : public v8::QueryObjectPredicate {
+ public:
+  TestQueryObjectPredicate(v8::Local<v8::Context> context,
+                           v8::Local<v8::Symbol> symbol)
+      : context_(context), symbol_(symbol) {}
+
+  bool Filter(v8::Local<v8::Object> object) override {
+    return object->HasOwnProperty(context_, symbol_).FromMaybe(false);
+  }
+
+ private:
+  v8::Local<v8::Context> context_;
+  v8::Local<v8::Symbol> symbol_;
+};
+
+class IncludeAllQueryObjectPredicate : public v8::QueryObjectPredicate {
+ public:
+  IncludeAllQueryObjectPredicate() {}
+  bool Filter(v8::Local<v8::Object> object) override { return true; }
+};
+}  // anonymous namespace
+
+TEST(QueryObjects) {
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Context> context = env.local();
+
+  v8::Local<v8::Symbol> sym =
+      v8::Symbol::New(isolate, v8_str("query_object_test"));
+  context->Global()->Set(context, v8_str("test_symbol"), sym).Check();
+  v8::Local<v8::Value> arr = CompileRun(R"(
+      const arr = [];
+      for (let i = 0; i < 10; ++i) {
+        arr.push({[test_symbol]: true});
+      }
+      arr;
+    )");
+  context->Global()->Set(context, v8_str("arr"), arr).Check();
+  v8::HeapProfiler* heap_profiler = isolate->GetHeapProfiler();
+
+  {
+    TestQueryObjectPredicate predicate(context, sym);
+    std::vector<v8::Global<v8::Object>> out;
+    heap_profiler->QueryObjects(context, &predicate, &out);
+
+    CHECK_EQ(out.size(), 10);
+    for (size_t i = 0; i < out.size(); ++i) {
+      CHECK(out[i].Get(isolate)->HasOwnProperty(context, sym).FromMaybe(false));
+    }
+  }
+
+  {
+    IncludeAllQueryObjectPredicate predicate;
+    std::vector<v8::Global<v8::Object>> out;
+    heap_profiler->QueryObjects(context, &predicate, &out);
+    CHECK_GE(out.size(), 10);
+  }
+}
+
 TEST(WeakReference) {
   v8::Isolate* isolate = CcTest::isolate();
   i::Isolate* i_isolate = CcTest::i_isolate();

src/heap_utils.cc

@@ -474,6 +474,39 @@ void TriggerHeapSnapshot(const FunctionCallbackInfo<Value>& args) {
   return args.GetReturnValue().Set(filename_v);
 }
 
+class PrototypeChainHas : public v8::QueryObjectPredicate {
+ public:
+  PrototypeChainHas(Local<Context> context, Local<Object> search)
+      : context_(context), search_(search) {}
+
+  // What we can do in the filter can be quite limited, but looking up
+  // the prototype chain is something that the inspector console API
+  // queryObject() does so it is supported.
+  bool Filter(Local<Object> object) override {
+    for (Local<Value> proto = object->GetPrototype(); proto->IsObject();
+         proto = proto.As<Object>()->GetPrototype()) {
+      if (search_ == proto) return true;
+    }
+    return false;
+  }
+
+ private:
+  Local<Context> context_;
+  Local<Object> search_;
+};
+
+void CountObjectsWithPrototype(const FunctionCallbackInfo<Value>& args) {
+  CHECK_EQ(args.Length(), 1);
+  CHECK(args[0]->IsObject());
+  Local<Object> proto = args[0].As<Object>();
+  Isolate* isolate = args.GetIsolate();
+  Local<Context> context = isolate->GetCurrentContext();
+  PrototypeChainHas prototype_chain_has(context, proto);
+  std::vector<Global<Object>> out;
+  isolate->GetHeapProfiler()->QueryObjects(context, &prototype_chain_has, &out);
+  args.GetReturnValue().Set(static_cast<uint32_t>(out.size()));
+}
+
 void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
@@ -482,12 +515,15 @@ void Initialize(Local<Object> target,
   SetMethod(context, target, "triggerHeapSnapshot", TriggerHeapSnapshot);
   SetMethod(
       context, target, "createHeapSnapshotStream", CreateHeapSnapshotStream);
+  SetMethod(
+      context, target, "countObjectsWithPrototype", CountObjectsWithPrototype);
 }
 
 void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(BuildEmbedderGraph);
   registry->Register(TriggerHeapSnapshot);
   registry->Register(CreateHeapSnapshotStream);
+  registry->Register(CountObjectsWithPrototype);
 }
 
 }  // namespace heap

test/common/gc.js

@@ -75,7 +75,54 @@ async function runAndBreathe(fn, repeat, waitTime = 20) {
   }
 }
 
+/**
+ * This requires --expose-internals.
+ * This function can be used to check if an object factory leaks or not by
+ * iterating over the heap and count objects with the specified class
+ * (which is checked by looking up the prototype chain).
+ * @param {(i: number) => number} fn The factory receiving iteration count
+ *   and returning number of objects created. The return value should be
+ *   precise otherwise false negatives can be produced.
+ * @param {Function} klass The class whose object is used to count the objects
+ * @param {number} count Number of iterations that this check should be done
+ * @param {number} waitTime Optional breathing time for GC.
+ */
+async function checkIfCollectableByCounting(fn, klass, count, waitTime = 20) {
+  const { internalBinding } = require('internal/test/binding');
+  const { countObjectsWithPrototype } = internalBinding('heap_utils');
+  const { prototype, name } = klass;
+  const initialCount = countObjectsWithPrototype(prototype);
+  console.log(`Initial count of ${name}: ${initialCount}`);
+  let totalCreated = 0;
+  for (let i = 0; i < count; ++i) {
+    const created = await fn(i);
+    totalCreated += created;
+    console.log(`#${i}: created ${created} ${name}, total ${totalCreated}`);
+    await wait(waitTime);  // give GC some breathing room.
+    const currentCount = countObjectsWithPrototype(prototype);
+    const collected = totalCreated - (currentCount - initialCount);
+    console.log(`#${i}: counted ${currentCount} ${name}, collected ${collected}`);
+    if (collected > 0) {
+      console.log(`Detected ${collected} collected ${name}, finish early`);
+      return;
+    }
+  }
+
+  await wait(waitTime);  // give GC some breathing room.
+  const currentCount = countObjectsWithPrototype(prototype);
+  const collected = totalCreated - (currentCount - initialCount);
+  console.log(`Last count: counted ${currentCount} ${name}, collected ${collected}`);
+  // Some objects with the prototype can be collected.
+  if (collected > 0) {
+    console.log(`Detected ${collected} collected ${name}`);
+    return;
+  }
+
+  throw new Error(`${name} cannot be collected`);
+}
+
 module.exports = {
   checkIfCollectable,
   runAndBreathe,
+  checkIfCollectableByCounting,
 };

test/parallel/parallel.status

@@ -5,9 +5,6 @@ prefix parallel
 # sample-test                        : PASS,FLAKY
 
 [true] # This section applies to all platforms
-# https://github.com/nodejs/node/pull/50327
-# Currently there's no reliable way to test it.
-test-diagnostics-channel-memory-leak: SKIP
 
 [$system==win32]
 # https://github.com/nodejs/node/issues/41206

test/parallel/test-diagnostics-channel-memory-leak.js

@@ -1,24 +1,22 @@
-// Flags: --expose-gc
+// Flags: --expose-internals --max-old-space-size=16
 'use strict';
 
 // This test ensures that diagnostic channel references aren't leaked.
 
-require('../common');
-const { ok } = require('assert');
+const common = require('../common');
 
-const { subscribe, unsubscribe } = require('diagnostics_channel');
+const { subscribe, unsubscribe, Channel } = require('diagnostics_channel');
+const { checkIfCollectableByCounting } = require('../common/gc');
 
 function noop() {}
 
-const heapUsedBefore = process.memoryUsage().heapUsed;
-
-for (let i = 0; i < 1000; i++) {
-  subscribe(String(i), noop);
-  unsubscribe(String(i), noop);
-}
-
-global.gc();
-
-const heapUsedAfter = process.memoryUsage().heapUsed;
-
-ok(heapUsedBefore >= heapUsedAfter);
+const outer = 64;
+const inner = 256;
+checkIfCollectableByCounting((i) => {
+  for (let j = 0; j < inner; j++) {
+    const key = String(i * inner + j);
+    subscribe(key, noop);
+    unsubscribe(key, noop);
+  }
+  return inner;
+}, Channel, outer).then(common.mustCall());