From c43faca452945672669208cbdeae1df9fe2f9ead Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 11 Mar 2025 19:58:57 +0100 Subject: [PATCH 1/2] test: use validateByRetainingPath in heapdump tests This makes sure that the tests are run on actual heap snapshots and prints out missing paths when it cannot be found, which makes failures easier to debug, and removes the unnecessary requirement for BaseObjects to be root - which would make the heap snapshot containment view rather noisy and is not conceptually correct, since they are actually held by the BaseObjectList held by the realms. --- test/common/heap.js | 21 ++++- test/pummel/test-heapdump-dns.js | 40 +++++---- test/pummel/test-heapdump-env.js | 34 ++++---- test/pummel/test-heapdump-fs-promise.js | 32 +++++--- test/pummel/test-heapdump-http2.js | 98 ++++++++++------------- test/pummel/test-heapdump-inspector.js | 47 ++++------- test/pummel/test-heapdump-shadow-realm.js | 29 +++---- test/pummel/test-heapdump-tls.js | 32 +++++--- test/pummel/test-heapdump-urlpattern.js | 31 ++++--- test/pummel/test-heapdump-vm-script.js | 4 +- test/pummel/test-heapdump-worker.js | 31 ++++--- test/pummel/test-heapdump-zlib.js | 43 +++++----- 12 files changed, 238 insertions(+), 204 deletions(-) diff --git a/test/common/heap.js b/test/common/heap.js index bec3b3208c0295..e4e02ded36ac56 100644 --- a/test/common/heap.js +++ b/test/common/heap.js @@ -221,6 +221,7 @@ function validateSnapshotNodes(...args) { * A alternative heap snapshot validator that can be used to verify cppgc-managed nodes. * Modified from * https://chromium.googlesource.com/v8/v8/+/b00e995fb212737802810384ba2b868d0d92f7e5/test/unittests/heap/cppgc-js/unified-heap-snapshot-unittest.cc#134 + * @param {object[]} nodes Snapshot nodes returned by createJSHeapSnapshot() or a subset filtered from it. * @param {string} rootName Name of the root node. Typically a class name used to filter all native nodes with * this name. For cppgc-managed objects, this is typically the name configured by * SET_CPPGC_NAME() prefixed with an additional "Node /" prefix e.g. @@ -231,12 +232,12 @@ 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) { - const nodes = createJSHeapSnapshot(); +function validateByRetainingPathFromNodes(nodes, rootName, retainingPath, allowEmpty = false) { let haystack = nodes.filter((n) => n.name === rootName && n.type !== 'string'); for (let i = 0; i < retainingPath.length; ++i) { @@ -269,6 +270,9 @@ function findByRetainingPath(rootName, retainingPath) { } if (newHaystack.length === 0) { + if (allowEmpty) { + return []; + } const format = (val) => util.inspect(val, { breakLength: 128, depth: 3 }); console.error('#'); console.error('# Retaining path to search for:'); @@ -282,6 +286,7 @@ function findByRetainingPath(rootName, retainingPath) { } assert.fail(`Could not find target edge ${format(expected)} in the heap snapshot.`); + } haystack = newHaystack; @@ -321,9 +326,19 @@ function getHeapSnapshotOptionTests() { }; } +/** + * Similar to @see {validateByRetainingPathFromNodes} but creates the snapshot from scratch. + */ +function validateByRetainingPath(...args) { + const nodes = createJSHeapSnapshot(); + return validateByRetainingPathFromNodes(nodes, ...args); +} + module.exports = { recordState, validateSnapshotNodes, - findByRetainingPath, + validateByRetainingPath, + validateByRetainingPathFromNodes, getHeapSnapshotOptionTests, + createJSHeapSnapshot, }; diff --git a/test/pummel/test-heapdump-dns.js b/test/pummel/test-heapdump-dns.js index 24b0b9bff15f87..cd1a650772b533 100644 --- a/test/pummel/test-heapdump-dns.js +++ b/test/pummel/test-heapdump-dns.js @@ -1,19 +1,31 @@ -// Flags: --expose-internals 'use strict'; +// This tests heap snapshot integration of dns ChannelWrap. + require('../common'); -const { validateSnapshotNodes } = require('../common/heap'); +const { validateByRetainingPath } = require('../common/heap'); +const assert = require('assert'); + +// Before dns is loaded, no ChannelWrap should be created. +{ + const nodes = validateByRetainingPath('Node / ChannelWrap', []); + assert.strictEqual(nodes.length, 0); +} -validateSnapshotNodes('Node / ChannelWrap', []); const dns = require('dns'); -validateSnapshotNodes('Node / ChannelWrap', [{}]); + +// Right after dns is loaded, a ChannelWrap should be created for the default +// resolver, but it has no task list. +{ + validateByRetainingPath('Node / ChannelWrap', [ + { node_name: 'ChannelWrap', edge_name: 'native_to_javascript' }, + ]); +} + dns.resolve('localhost', () => {}); -validateSnapshotNodes('Node / ChannelWrap', [ - { - children: [ - { node_name: 'Node / NodeAresTask::List', edge_name: 'task_list' }, - // `Node / ChannelWrap` (C++) -> `ChannelWrap` (JS) - { node_name: 'ChannelWrap', edge_name: 'native_to_javascript' }, - ], - detachedness: 2, - }, -]); + +// After dns resolution, the ChannelWrap of the default resolver has the task list. +{ + validateByRetainingPath('Node / ChannelWrap', [ + { node_name: 'Node / NodeAresTask::List', edge_name: 'task_list' }, + ]); +} diff --git a/test/pummel/test-heapdump-env.js b/test/pummel/test-heapdump-env.js index 4027a2b8a7b057..bb1679095b5b1f 100644 --- a/test/pummel/test-heapdump-env.js +++ b/test/pummel/test-heapdump-env.js @@ -1,4 +1,3 @@ -// Flags: --expose-internals 'use strict'; // This tests that Environment is tracked in heap snapshots. @@ -6,19 +5,24 @@ // test-heapdump-*.js files. require('../common'); -const { validateSnapshotNodes } = require('../common/heap'); +const { createJSHeapSnapshot, validateByRetainingPathFromNodes } = require('../common/heap'); -validateSnapshotNodes('Node / Environment', [{ - children: [ - { node_name: 'Node / CleanupQueue', edge_name: 'cleanup_queue' }, - { node_name: 'Node / IsolateData', edge_name: 'isolate_data' }, - { node_name: 'Node / PrincipalRealm', edge_name: 'principal_realm' }, - ], -}]); +const nodes = createJSHeapSnapshot(); -validateSnapshotNodes('Node / PrincipalRealm', [{ - children: [ - { node_name: 'process', edge_name: 'process_object' }, - { node_name: 'Node / BaseObjectList', edge_name: 'base_object_list' }, - ], -}]); +const envs = validateByRetainingPathFromNodes(nodes, 'Node / Environment', []); +validateByRetainingPathFromNodes(envs, 'Node / Environment', [ + { node_name: 'Node / CleanupQueue', edge_name: 'cleanup_queue' }, +]); +validateByRetainingPathFromNodes(envs, 'Node / Environment', [ + { node_name: 'Node / IsolateData', edge_name: 'isolate_data' }, +]); + +const realms = validateByRetainingPathFromNodes(envs, 'Node / Environment', [ + { node_name: 'Node / PrincipalRealm', edge_name: 'principal_realm' }, +]); +validateByRetainingPathFromNodes(realms, 'Node / PrincipalRealm', [ + { node_name: 'process', edge_name: 'process_object' }, +]); +validateByRetainingPathFromNodes(realms, 'Node / PrincipalRealm', [ + { node_name: 'Node / BaseObjectList', edge_name: 'base_object_list' }, +]); diff --git a/test/pummel/test-heapdump-fs-promise.js b/test/pummel/test-heapdump-fs-promise.js index 10ee087ae7fc23..429359e1a6be72 100644 --- a/test/pummel/test-heapdump-fs-promise.js +++ b/test/pummel/test-heapdump-fs-promise.js @@ -1,16 +1,26 @@ -// Flags: --expose-internals 'use strict'; + +// This tests heap snapshot integration of fs promise. + require('../common'); -const { validateSnapshotNodes } = require('../common/heap'); +const { validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap'); const fs = require('fs').promises; +const assert = require('assert'); + +// Before fs promise is used, no FSReqPromise should be created. +{ + const nodes = validateByRetainingPath('Node / FSReqPromise', []); + assert.strictEqual(nodes.length, 0); +} -validateSnapshotNodes('Node / FSReqPromise', []); fs.stat(__filename); -validateSnapshotNodes('Node / FSReqPromise', [ - { - children: [ - { node_name: 'FSReqPromise', edge_name: 'native_to_javascript' }, - { node_name: 'Node / AliasedFloat64Array', edge_name: 'stats_field_array' }, - ], - }, -]); + +{ + const nodes = validateByRetainingPath('Node / FSReqPromise', []); + validateByRetainingPathFromNodes(nodes, 'Node / FSReqPromise', [ + { node_name: 'FSReqPromise', edge_name: 'native_to_javascript' }, + ]); + validateByRetainingPathFromNodes(nodes, 'Node / FSReqPromise', [ + { node_name: 'Node / AliasedFloat64Array', edge_name: 'stats_field_array' }, + ]); +} diff --git a/test/pummel/test-heapdump-http2.js b/test/pummel/test-heapdump-http2.js index c9218723792a1f..9a60a238b49762 100644 --- a/test/pummel/test-heapdump-http2.js +++ b/test/pummel/test-heapdump-http2.js @@ -1,15 +1,21 @@ -// Flags: --expose-internals 'use strict'; + +// This tests heap snapshot integration of http2. + const common = require('../common'); -const { recordState } = require('../common/heap'); +const { createJSHeapSnapshot, validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap'); +const assert = require('assert'); + if (!common.hasCrypto) common.skip('missing crypto'); const http2 = require('http2'); +// Before http2 is used, no Http2Session or Http2Streamshould be created. { - const state = recordState(); - state.validateSnapshotNodes('Node / Http2Session', []); - state.validateSnapshotNodes('Node / Http2Stream', []); + const sessions = validateByRetainingPath('Node / Http2Session', []); + assert.strictEqual(sessions.length, 0); + const streams = validateByRetainingPath('Node / Http2Stream', []); + assert.strictEqual(streams.length, 0); } const server = http2.createServer(); @@ -21,63 +27,45 @@ server.listen(0, () => { const req = client.request(); req.on('response', common.mustCall(() => { - const state = recordState(); + const nodes = createJSHeapSnapshot(); // `Node / Http2Stream` (C++) -> Http2Stream (JS) - state.validateSnapshotNodes('Node / Http2Stream', [ - { - children: [ - // current_headers and/or queue could be empty - { node_name: 'Http2Stream', edge_name: 'native_to_javascript' }, - ], - }, - ], { loose: true }); + validateByRetainingPathFromNodes(nodes, 'Node / Http2Stream', [ + // current_headers and/or queue could be empty + { node_name: 'Http2Stream', edge_name: 'native_to_javascript' }, + ]); // `Node / FileHandle` (C++) -> FileHandle (JS) - state.validateSnapshotNodes('Node / FileHandle', [ - { - children: [ - { node_name: 'FileHandle', edge_name: 'native_to_javascript' }, - // current_headers could be empty - ], - }, - ], { loose: true }); - state.validateSnapshotNodes('Node / TCPSocketWrap', [ - { - children: [ - { node_name: 'TCP', edge_name: 'native_to_javascript' }, - ], - }, - ], { loose: true }); - state.validateSnapshotNodes('Node / TCPServerWrap', [ - { - children: [ - { node_name: 'TCP', edge_name: 'native_to_javascript' }, - ], - }, - ], { loose: true }); + validateByRetainingPathFromNodes(nodes, 'Node / FileHandle', [ + { node_name: 'FileHandle', edge_name: 'native_to_javascript' }, + // current_headers could be empty + ]); + validateByRetainingPathFromNodes(nodes, 'Node / TCPSocketWrap', [ + { node_name: 'TCP', edge_name: 'native_to_javascript' }, + ]); + + validateByRetainingPathFromNodes(nodes, 'Node / TCPServerWrap', [ + { node_name: 'TCP', edge_name: 'native_to_javascript' }, + ]); + // `Node / StreamPipe` (C++) -> StreamPipe (JS) - state.validateSnapshotNodes('Node / StreamPipe', [ - { - children: [ - { node_name: 'StreamPipe', edge_name: 'native_to_javascript' }, - ], - }, + validateByRetainingPathFromNodes(nodes, 'Node / StreamPipe', [ + { node_name: 'StreamPipe', edge_name: 'native_to_javascript' }, ]); + // `Node / Http2Session` (C++) -> Http2Session (JS) - state.validateSnapshotNodes('Node / Http2Session', [ - { - children: [ - { node_name: 'Http2Session', edge_name: 'native_to_javascript' }, - { node_name: 'Node / nghttp2_memory', edge_name: 'nghttp2_memory' }, - { - node_name: 'Node / streams', edge_name: 'streams', - }, - // outstanding_pings, outgoing_buffers, outgoing_storage, - // pending_rst_streams could be empty - ], - }, - ], { loose: true }); + const sessions = validateByRetainingPathFromNodes(nodes, 'Node / Http2Session', []); + validateByRetainingPathFromNodes(sessions, 'Node / Http2Session', [ + { node_name: 'Http2Session', edge_name: 'native_to_javascript' }, + ]); + validateByRetainingPathFromNodes(sessions, 'Node / Http2Session', [ + { node_name: 'Node / nghttp2_memory', edge_name: 'nghttp2_memory' }, + ]); + validateByRetainingPathFromNodes(sessions, 'Node / Http2Session', [ + { node_name: 'Node / streams', edge_name: 'streams' }, + ]); + // outstanding_pings, outgoing_buffers, outgoing_storage, + // pending_rst_streams could be empty })); req.resume(); diff --git a/test/pummel/test-heapdump-inspector.js b/test/pummel/test-heapdump-inspector.js index 53950490e34b90..70ae8e9fc173aa 100644 --- a/test/pummel/test-heapdump-inspector.js +++ b/test/pummel/test-heapdump-inspector.js @@ -1,44 +1,29 @@ -// Flags: --expose-internals 'use strict'; -const common = require('../common'); +// This tests heap snapshot integration of inspector. +const common = require('../common'); +const assert = require('assert'); common.skipIfInspectorDisabled(); -const { validateSnapshotNodes } = require('../common/heap'); +const { validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap'); const inspector = require('inspector'); -const snapshotNode = { - children: [ - { node_name: 'Node / InspectorSession', edge_name: 'session' }, - ], -}; - -// Starts with no JSBindingsConnection (or 1 if coverage enabled). +// Starts with no JSBindingsConnection. { - const expected = []; - if (process.env.NODE_V8_COVERAGE) { - expected.push(snapshotNode); - } - validateSnapshotNodes('Node / JSBindingsConnection', expected); + const nodes = validateByRetainingPath('Node / JSBindingsConnection', []); + assert.strictEqual(nodes.length, 0); } -// JSBindingsConnection should be added. +// JSBindingsConnection should be added once inspector session is created. { const session = new inspector.Session(); session.connect(); - const expected = [ - { - children: [ - { node_name: 'Node / InspectorSession', edge_name: 'session' }, - { node_name: 'Connection', edge_name: 'native_to_javascript' }, - (edge) => edge.name === 'callback' && - (edge.to.type === undefined || // embedded graph - edge.to.type === 'closure'), // snapshot - ], - }, - ]; - if (process.env.NODE_V8_COVERAGE) { - expected.push(snapshotNode); - } - validateSnapshotNodes('Node / JSBindingsConnection', expected); + + const nodes = validateByRetainingPath('Node / JSBindingsConnection', []); + validateByRetainingPathFromNodes(nodes, 'Node / JSBindingsConnection', [ + { node_name: 'Node / InspectorSession', edge_name: 'session' }, + ]); + validateByRetainingPathFromNodes(nodes, 'Node / JSBindingsConnection', [ + { node_name: 'Connection', edge_name: 'native_to_javascript' }, + ]); } diff --git a/test/pummel/test-heapdump-shadow-realm.js b/test/pummel/test-heapdump-shadow-realm.js index 1482fe6b6fcb8b..66c32986b4c8cd 100644 --- a/test/pummel/test-heapdump-shadow-realm.js +++ b/test/pummel/test-heapdump-shadow-realm.js @@ -1,9 +1,16 @@ -// Flags: --experimental-shadow-realm --expose-internals +// Flags: --experimental-shadow-realm +// This tests heap snapshot integration of ShadowRealm + 'use strict'; require('../common'); -const { validateSnapshotNodes } = require('../common/heap'); +const { validateByRetainingPath } = require('../common/heap'); +const assert = require('assert'); -validateSnapshotNodes('Node / ShadowRealm', []); +// Before shadow realm is created, no ShadowRealm should be captured. +{ + const nodes = validateByRetainingPath('Node / ShadowRealm', []); + assert.strictEqual(nodes.length, 0); +} let realm; let counter = 0; @@ -23,19 +30,9 @@ function createRealms() { } function validateHeap() { - validateSnapshotNodes('Node / Environment', [ - { - children: [ - { node_name: 'Node / shadow_realms', edge_name: 'shadow_realms' }, - ], - }, - ]); - validateSnapshotNodes('Node / shadow_realms', [ - { - children: [ - { node_name: 'Node / ShadowRealm' }, - ], - }, + validateByRetainingPath('Node / Environment', [ + { node_name: 'Node / shadow_realms', edge_name: 'shadow_realms' }, + { node_name: 'Node / ShadowRealm' }, ]); } diff --git a/test/pummel/test-heapdump-tls.js b/test/pummel/test-heapdump-tls.js index b1bf6aac37a441..a40cbe31dee425 100644 --- a/test/pummel/test-heapdump-tls.js +++ b/test/pummel/test-heapdump-tls.js @@ -1,15 +1,20 @@ -// Flags: --expose-internals 'use strict'; +// This tests heap snapshot integration of tls sockets. const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const { validateSnapshotNodes } = require('../common/heap'); +const { validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap'); +const assert = require('assert'); const net = require('net'); const tls = require('tls'); -validateSnapshotNodes('Node / TLSWrap', []); +// Before tls is used, no TLSWrap should be created. +{ + const nodes = validateByRetainingPath('Node / TLSWrap', []); + assert.strictEqual(nodes.length, 0); +} const server = net.createServer(common.mustCall((c) => { c.end(); @@ -21,15 +26,16 @@ const server = net.createServer(common.mustCall((c) => { })); c.write('hello'); - validateSnapshotNodes('Node / TLSWrap', [ - { - children: [ - { node_name: 'Node / NodeBIO', edge_name: 'enc_out' }, - { node_name: 'Node / NodeBIO', edge_name: 'enc_in' }, - // `Node / TLSWrap` (C++) -> `TLSWrap` (JS) - { node_name: 'TLSWrap', edge_name: 'native_to_javascript' }, - // pending_cleartext_input could be empty - ], - }, + const nodes = validateByRetainingPath('Node / TLSWrap', []); + validateByRetainingPathFromNodes(nodes, 'Node / TLSWrap', [ + { node_name: 'Node / NodeBIO', edge_name: 'enc_out' }, + ]); + validateByRetainingPathFromNodes(nodes, 'Node / TLSWrap', [ + { node_name: 'Node / NodeBIO', edge_name: 'enc_in' }, + ]); + validateByRetainingPathFromNodes(nodes, 'Node / TLSWrap', [ + // `Node / TLSWrap` (C++) -> `TLSWrap` (JS) + { node_name: 'TLSWrap', edge_name: 'native_to_javascript' }, + // pending_cleartext_input could be empty ]); })); diff --git a/test/pummel/test-heapdump-urlpattern.js b/test/pummel/test-heapdump-urlpattern.js index f903feae85b999..05ac827e2508e7 100644 --- a/test/pummel/test-heapdump-urlpattern.js +++ b/test/pummel/test-heapdump-urlpattern.js @@ -1,24 +1,23 @@ -// Flags: --expose-internals 'use strict'; +// This tests heap snapshot integration of URLPattern. require('../common'); -const { validateSnapshotNodes } = require('../common/heap'); +const { validateByRetainingPath } = require('../common/heap'); const { URLPattern } = require('node:url'); +const assert = require('assert'); + +// Before url pattern is used, no URLPattern should be created. +{ + const nodes = validateByRetainingPath('Node / URLPattern', []); + assert.strictEqual(nodes.length, 0); +} -validateSnapshotNodes('Node / URLPattern', []); const urlPattern = new URLPattern('https://example.com/:id'); -validateSnapshotNodes('Node / URLPattern', [ - { - children: [ - { node_name: 'Node / ada::url_pattern', edge_name: 'url_pattern' }, - ], - }, -]); -validateSnapshotNodes('Node / ada::url_pattern', [ - { - children: [ - { node_name: 'Node / ada::url_pattern_component', edge_name: 'protocol_component' }, - ], - }, + +// After url pattern is used, a URLPattern should be created. +validateByRetainingPath('Node / URLPattern', [ + { node_name: 'Node / ada::url_pattern', edge_name: 'url_pattern' }, + // ada::url_pattern references ada::url_pattern_component. + { node_name: 'Node / ada::url_pattern_component', edge_name: 'protocol_component' }, ]); // Use `urlPattern`. diff --git a/test/pummel/test-heapdump-vm-script.js b/test/pummel/test-heapdump-vm-script.js index b8ca96b34d2f8f..e7878a7803878e 100644 --- a/test/pummel/test-heapdump-vm-script.js +++ b/test/pummel/test-heapdump-vm-script.js @@ -1,10 +1,10 @@ 'use strict'; require('../common'); -const { findByRetainingPath } = require('../common/heap'); +const { validateByRetainingPath } = require('../common/heap'); const source = 'const foo = 123'; const script = require('vm').createScript(source); -findByRetainingPath('Node / ContextifyScript', [ +validateByRetainingPath('Node / ContextifyScript', [ { node_name: '(shared function info)' }, // This is the UnboundScript referenced by ContextifyScript. { edge_name: 'script' }, { edge_name: 'source', node_type: 'string', node_name: source }, diff --git a/test/pummel/test-heapdump-worker.js b/test/pummel/test-heapdump-worker.js index 72f2e66fc89c18..740bf97cb81181 100644 --- a/test/pummel/test-heapdump-worker.js +++ b/test/pummel/test-heapdump-worker.js @@ -1,16 +1,27 @@ -// Flags: --expose-internals 'use strict'; + +// This tests heap snapshot integration of worker. + require('../common'); -const { validateSnapshotNodes } = require('../common/heap'); +const { validateByRetainingPath } = require('../common/heap'); const { Worker } = require('worker_threads'); +const assert = require('assert'); + +// Before worker is used, no MessagePort should be created. +{ + const nodes = validateByRetainingPath('Node / Worker', []); + assert.strictEqual(nodes.length, 0); +} -validateSnapshotNodes('Node / Worker', []); const worker = new Worker('setInterval(() => {}, 100);', { eval: true }); -validateSnapshotNodes('Node / MessagePort', [ - { - children: [ - { node_name: 'Node / MessagePortData', edge_name: 'data' }, - ], - }, -], { loose: true }); +// When a worker is alive, a Worker instance and its message port should be captured. +{ + validateByRetainingPath('Node / Worker', [ + { node_name: 'Worker', edge_name: 'native_to_javascript' }, + { node_name: 'MessagePort', edge_name: 'messagePort' }, + { node_name: 'Node / MessagePort', edge_name: 'javascript_to_native' }, + { node_name: 'Node / MessagePortData', edge_name: 'data' }, + ]); +} + worker.terminate(); diff --git a/test/pummel/test-heapdump-zlib.js b/test/pummel/test-heapdump-zlib.js index 9d3a2322068f1b..9f11fbcadad306 100644 --- a/test/pummel/test-heapdump-zlib.js +++ b/test/pummel/test-heapdump-zlib.js @@ -1,28 +1,35 @@ -// Flags: --expose-internals 'use strict'; +// This tests heap snapshot integration of zlib stream. + const common = require('../common'); -const { validateSnapshotNodes } = require('../common/heap'); +const assert = require('assert'); +const { validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap'); const zlib = require('zlib'); -validateSnapshotNodes('Node / ZlibStream', []); +// Before zlib stream is created, no ZlibStream should be created. +{ + const nodes = validateByRetainingPath('Node / ZlibStream', []); + assert.strictEqual(nodes.length, 0); +} const gzip = zlib.createGzip(); -validateSnapshotNodes('Node / ZlibStream', [ - { - children: [ - { node_name: 'Zlib', edge_name: 'native_to_javascript' }, - // No entry for memory because zlib memory is initialized lazily. - ], - }, -]); +// After zlib stream is created, a ZlibStream should be created. +{ + const streams = validateByRetainingPath('Node / ZlibStream', []); + validateByRetainingPathFromNodes(streams, 'Node / ZlibStream', [ + { node_name: 'Zlib', edge_name: 'native_to_javascript' }, + ]); + // No entry for memory because zlib memory is initialized lazily. + const withMemory = validateByRetainingPathFromNodes(streams, 'Node / ZlibStream', [ + { node_name: 'Node / zlib_memory', edge_name: 'zlib_memory' }, + ], true); + assert.strictEqual(withMemory.length, 0); +} + +// After zlib stream is written, zlib_memory should be created. gzip.write('hello world', common.mustCall(() => { - validateSnapshotNodes('Node / ZlibStream', [ - { - children: [ - { node_name: 'Zlib', edge_name: 'native_to_javascript' }, - { node_name: 'Node / zlib_memory', edge_name: 'zlib_memory' }, - ], - }, + validateByRetainingPath('Node / ZlibStream', [ + { node_name: 'Node / zlib_memory', edge_name: 'zlib_memory' }, ]); })); From 11115da19618f4ec0eb298354a5964d44ec03b58 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 11 Mar 2025 20:02:17 +0100 Subject: [PATCH 2/2] src: annotate BaseObjects in the heap snapshots correctly This fixes two issues in the BaseObject views in the heap snapshots: 1. BaseObjects are not conceptually roots when the environment and the realms are also showing up in the heap snapshot. Rather, they should be considered being held alive by the BaseObjectList in the realms, which are in turn held alive by Environment. The actual root from the containment view should be the Environment instead. 2. The concept of DOM detaching does not really apply to Node.js wrappers, and it's confusing to connect that with the weakness or detachment (native weakness) of BaseObjects. To avoid the confusion, just restore to the default detachedness for them. --- src/base_object-inl.h | 5 ----- src/base_object.cc | 4 ---- src/base_object.h | 3 --- 3 files changed, 12 deletions(-) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 6f731b17fe0b84..cc60ddddb037e0 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -125,11 +125,6 @@ bool BaseObject::IsWeakOrDetached() const { return pd->wants_weak_jsobj || pd->is_detached; } -v8::EmbedderGraph::Node::Detachedness BaseObject::GetDetachedness() const { - return IsWeakOrDetached() ? v8::EmbedderGraph::Node::Detachedness::kDetached - : v8::EmbedderGraph::Node::Detachedness::kUnknown; -} - template void BaseObject::InternalFieldGet( const v8::FunctionCallbackInfo& args) { diff --git a/src/base_object.cc b/src/base_object.cc index 593fce4382ab3a..404e2aa8c88d0c 100644 --- a/src/base_object.cc +++ b/src/base_object.cc @@ -161,10 +161,6 @@ Local BaseObject::WrappedObject() const { return object(); } -bool BaseObject::IsRootNode() const { - return !persistent_handle_.IsWeak(); -} - bool BaseObject::IsNotIndicativeOfMemoryLeakAtExit() const { return IsWeakOrDetached(); } diff --git a/src/base_object.h b/src/base_object.h index f5b791187ec4d9..da49eacb647649 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -105,8 +105,6 @@ class BaseObject : public MemoryRetainer { // to it anymore. inline bool IsWeakOrDetached() const; - inline v8::EmbedderGraph::Node::Detachedness GetDetachedness() const override; - // Utility to create a FunctionTemplate with one internal field (used for // the `BaseObject*` pointer) and a constructor that initializes that field // to `nullptr`. @@ -192,7 +190,6 @@ class BaseObject : public MemoryRetainer { private: v8::Local WrappedObject() const override; - bool IsRootNode() const override; void DeleteMe(); // persistent_handle_ needs to be at a fixed offset from the start of the