From f5311cee9bb00380336f9eb1859e4e67a673e74f Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Tue, 21 Dec 2021 11:19:34 -0800 Subject: [PATCH 1/2] feat: show detail on leaking dom nodes Fixes #7 --- src/analyzeDomNodes.js | 30 +++++ src/analyzeEventListeners.js | 14 +++ src/domNodes.js | 10 -- src/eventListeners.js | 79 ++++++------- src/format.js | 23 +++- src/getNodeDescriptors.js | 14 +++ src/heapsnapshots.js | 23 +--- src/index.js | 161 +++++++++++++++------------ test/spec/domNodes.test.mjs | 30 +++++ test/spec/eventListeners.test.mjs | 15 +++ test/www/recyclesDomNodes/index.html | 44 ++++++++ 11 files changed, 289 insertions(+), 154 deletions(-) create mode 100644 src/analyzeDomNodes.js delete mode 100644 src/domNodes.js create mode 100644 src/getNodeDescriptors.js create mode 100644 test/www/recyclesDomNodes/index.html diff --git a/src/analyzeDomNodes.js b/src/analyzeDomNodes.js new file mode 100644 index 0000000..9ec4df6 --- /dev/null +++ b/src/analyzeDomNodes.js @@ -0,0 +1,30 @@ +function createDescriptionToCounts (nodes) { + const map = new Map() + for (const { description } of nodes) { + const count = map.get(description) || 0 + map.set(description, count + 1) + } + return map +} + +export function analyzeDomNodes (nodesBefore, nodesAfter, numIterations) { + const result = [] + + const descriptionToCountsBefore = createDescriptionToCounts(nodesBefore) + const descriptionToCountsAfter = createDescriptionToCounts(nodesAfter) + + descriptionToCountsAfter.forEach((countAfter, description) => { + const countBefore = descriptionToCountsBefore.get(description) || 0 + const delta = countAfter - countBefore + if (delta > 0) { + result.push({ + description, + before: countBefore, + after: countAfter, + delta, + deltaPerIteration: delta / numIterations + }) + } + }) + return result +} diff --git a/src/analyzeEventListeners.js b/src/analyzeEventListeners.js index 6408dd0..c8110a8 100644 --- a/src/analyzeEventListeners.js +++ b/src/analyzeEventListeners.js @@ -85,3 +85,17 @@ export function analyzeEventListeners (startListenersSummary, endListenersSummar return sortBy(result, ['type']) } + +export function calculateEventListenersSummary (eventListenersStart, eventListenersEnd, numIterations) { + const before = sum(eventListenersStart.map(({ listeners }) => listeners.length)) + const after = sum(eventListenersEnd.map(({ listeners }) => listeners.length)) + const delta = after - before + const deltaPerIteration = delta / numIterations + + return { + before, + after, + delta, + deltaPerIteration + } +} diff --git a/src/domNodes.js b/src/domNodes.js deleted file mode 100644 index cd8e83c..0000000 --- a/src/domNodes.js +++ /dev/null @@ -1,10 +0,0 @@ -import { getAllDomNodes } from './browser/getAllDomNodes.js' - -export async function countDomNodes (page) { - return (await page.evaluate(` - (function () { - ${getAllDomNodes} - return getAllDomNodes().length - })() - `)) -} diff --git a/src/eventListeners.js b/src/eventListeners.js index 2f1aedc..98098a3 100644 --- a/src/eventListeners.js +++ b/src/eventListeners.js @@ -1,59 +1,52 @@ import { v4 as uuidV4 } from 'uuid' import { omit, pick } from './util.js' +import { getNodeDescriptors } from './getNodeDescriptors.js' import { getAllDomNodes } from './browser/getAllDomNodes.js' // via https://stackoverflow.com/a/67030384 -export async function getEventListeners (page) { +export async function getDomNodesAndListeners (page, cdpSession) { const objectGroup = uuidV4() - const cdpSession = await page.target().createCDPSession() - try { - const { result: { objectId } } = await cdpSession.send('Runtime.evaluate', { - expression: ` + const { result: { objectId } } = await cdpSession.send('Runtime.evaluate', { + expression: ` (function () { ${getAllDomNodes} return [...getAllDomNodes(), window, document] })() `, - objectGroup - }) - // Using the returned remote object ID, actually get the list of descriptors - const { result } = await cdpSession.send('Runtime.getProperties', { objectId }) - - const arrayProps = Object.fromEntries(result.map(_ => ([_.name, _.value]))) - - const length = arrayProps.length.value - - const nodes = [] - - for (let i = 0; i < length; i++) { - nodes.push(arrayProps[i]) + objectGroup + }) + const nodeDescriptors = await getNodeDescriptors(cdpSession, objectId) + + const listenersWithNodes = [] + + // scrub the objects for external consumption, remove unnecessary stuff like objectId + const cleanNode = node => pick(node, ['className', 'description']) + const cleanListener = listener => ({ + // originalHandler seems to contain the same information as handler + ...omit(listener, ['backendNodeId', 'originalHandler']), + handler: omit(listener.handler, ['objectId']) + }) + + for (const node of nodeDescriptors) { + const { objectId } = node + + const { listeners } = await cdpSession.send('DOMDebugger.getEventListeners', { objectId }) + + if (listeners.length) { + listenersWithNodes.push({ + node: cleanNode(node), + listeners: listeners.map(cleanListener) + }) } + } - const nodesWithListeners = [] - - for (const node of nodes) { - const { objectId } = node - - const { listeners } = await cdpSession.send('DOMDebugger.getEventListeners', { objectId }) - - if (listeners.length) { - nodesWithListeners.push({ - node: pick(node, ['className', 'description']), - listeners: listeners.map(listener => { - return { - // originalHandler seems to contain the same information as handler - // as for objectId, these are useless since we will just release the object group anyway - ...omit(listener, ['backendNodeId', 'originalHandler']), - handler: omit(listener.handler, ['objectId']) - } - }) - }) - } - } + await cdpSession.send('Runtime.releaseObjectGroup', { objectGroup }) + + // don't include the window/document objects in the list of dom nodes + const returnNodes = nodeDescriptors.slice(0, nodeDescriptors.length - 2).map(cleanNode) - await cdpSession.send('Runtime.releaseObjectGroup', { objectGroup }) - return nodesWithListeners - } finally { - await cdpSession.detach() + return { + nodes: returnNodes, + listeners: listenersWithNodes } } diff --git a/src/format.js b/src/format.js index b1d2f04..1defac2 100644 --- a/src/format.js +++ b/src/format.js @@ -23,7 +23,7 @@ ${markdownTable(tableData)} `.trim() + '\n\n' } -function formatLeakingEventListeners (listenerSummaries) { +function formatLeakingEventListeners (listenerSummaries, eventListenersSummary) { const tableData = [[ 'Event', '# added', @@ -41,18 +41,29 @@ function formatLeakingEventListeners (listenerSummaries) { ]) } return ` -Leaking event listeners: +Leaking event listeners (+${eventListenersSummary.deltaPerIteration} total): ${markdownTable(tableData)} `.trim() + '\n\n' } function formatLeakingDomNodes (domNodes) { + const tableData = [[ + 'Description', + '# added' + ]] + + for (const { description, deltaPerIteration } of domNodes.nodes) { + tableData.push([ + description, + deltaPerIteration + ]) + } return ` -Leaking DOM nodes: +Leaking DOM nodes (+${domNodes.deltaPerIteration} total): -DOM size grew by ${domNodes.deltaPerIteration} node(s) - `.trim() + '\n\n' +${markdownTable(tableData)} + `.trim() + '\n\n' } function formatLeakingCollections (leakingCollections) { @@ -91,7 +102,7 @@ export function formatResult ({ test, result }) { leakTables += formatLeakingObjects(result.leaks.objects) } if (result.leaks.eventListeners.length) { - leakTables += formatLeakingEventListeners(result.leaks.eventListeners) + leakTables += formatLeakingEventListeners(result.leaks.eventListeners, result.leaks.eventListenersSummary) } if (result.leaks.domNodes.delta > 0) { leakTables += formatLeakingDomNodes(result.leaks.domNodes) diff --git a/src/getNodeDescriptors.js b/src/getNodeDescriptors.js new file mode 100644 index 0000000..fb1dab1 --- /dev/null +++ b/src/getNodeDescriptors.js @@ -0,0 +1,14 @@ +// Given an array of dom nodes, get their descriptors +export async function getNodeDescriptors (cdpSession, objectId) { + // via https://stackoverflow.com/a/67030384 + const { result } = await cdpSession.send('Runtime.getProperties', { objectId }) + + const arrayProps = Object.fromEntries(result.map(_ => ([_.name, _.value]))) + const length = arrayProps.length.value + const descriptors = [] + + for (let i = 0; i < length; i++) { + descriptors.push(arrayProps[i]) + } + return descriptors +} diff --git a/src/heapsnapshots.js b/src/heapsnapshots.js index 799e0da..9b5b71d 100644 --- a/src/heapsnapshots.js +++ b/src/heapsnapshots.js @@ -4,9 +4,8 @@ import cryptoRandomString from 'crypto-random-string' import { createReadStream, createWriteStream } from 'fs' import * as HeapSnapshotWorker from './thirdparty/devtools/heap_snapshot_worker/heap_snapshot_worker.js' -export async function takeHeapSnapshot (page) { +export async function takeHeapSnapshot (page, cdpSession) { const filename = path.join(tempDir, `fuite-${cryptoRandomString({ length: 8, type: 'alphanumeric' })}.heapsnapshot`) - const cdpSession = await page.target().createCDPSession() let writeStream const writeStreamPromise = new Promise((resolve, reject) => { writeStream = createWriteStream(filename, { encoding: 'utf8' }) @@ -30,7 +29,6 @@ export async function takeHeapSnapshot (page) { }) await heapProfilerPromise - await cdpSession.detach() writeStream.close() await writeStreamPromise return filename @@ -64,22 +62,3 @@ export async function createHeapSnapshotModel (filename) { return (await loader.buildSnapshot()) } - -export async function takeThrowawayHeapSnapshot (page) { - const cdpSession = await page.target().createCDPSession() - const heapProfilerPromise = new Promise(resolve => { - cdpSession.on('HeapProfiler.reportHeapSnapshotProgress', ({ finished }) => { - if (finished) { - resolve() - } - }) - }) - await cdpSession.send('HeapProfiler.enable') - await cdpSession.send('HeapProfiler.collectGarbage') - await cdpSession.send('HeapProfiler.takeHeapSnapshot', { - reportProgress: true - }) - - await heapProfilerPromise - await cdpSession.detach() -} diff --git a/src/index.js b/src/index.js index d928699..552b3e2 100644 --- a/src/index.js +++ b/src/index.js @@ -2,13 +2,13 @@ import puppeteer from 'puppeteer' import * as defaultScenario from './defaultScenario.js' import { takeHeapSnapshot } from './heapsnapshots.js' import { waitForPageIdle } from './puppeteerUtil.js' -import { getEventListeners } from './eventListeners.js' +import { getDomNodesAndListeners } from './eventListeners.js' import fs from 'fs/promises' import { analyzeHeapSnapshots } from './analyzeHeapsnapshots.js' -import { analyzeEventListeners } from './analyzeEventListeners.js' -import { countDomNodes } from './domNodes.js' +import { analyzeEventListeners, calculateEventListenersSummary } from './analyzeEventListeners.js' import { findLeakingCollections, startTrackingCollections } from './collections.js' import ora from 'ora' +import { analyzeDomNodes } from './analyzeDomNodes.js' export const DEFAULT_ITERATIONS = 7 @@ -81,6 +81,15 @@ function massagePageUrl (pageUrl) { return pageUrl } +async function runWithCdpSession (page, runnable) { + const cdpSession = await page.target().createCDPSession() + try { + return (await runnable(cdpSession)) + } finally { + await cdpSession.detach() + } +} + export async function * findLeaks (pageUrl, options = {}) { const { scenario, numIterations, progress, debug, heapsnapshot, browser @@ -93,83 +102,89 @@ export async function * findLeaks (pageUrl, options = {}) { return (await runOnFreshPage(browser, pageUrl, setup, async page => { await iteration(page, test.data) // one throwaway iteration to avoid measuring one-time setup costs onProgress('Taking start snapshot...') - const weakMap = await startTrackingCollections(page) - const eventListenersStart = await getEventListeners(page) - const domNodesCountStart = await countDomNodes(page) - const startSnapshotFilename = await takeHeapSnapshot(page) - if (debug) { - // Point in time before running any iterations - debugger // eslint-disable-line no-debugger - } - for (let i = 0; i < numIterations; i++) { - onProgress(`Iteration ${i + 1}/${numIterations}...`) - await iteration(page, test.data) - } - onProgress('Taking end snapshot...') - const endSnapshotFilename = await takeHeapSnapshot(page) - const domNodesCountEnd = await countDomNodes(page) - const eventListenersEnd = await getEventListeners(page) - const leakingCollections = await findLeakingCollections(page, weakMap, numIterations, debug) - if (debug) { - // Point in time after running iterations - debugger // eslint-disable-line no-debugger - } + return (await runWithCdpSession(page, async cdpSession => { + const weakMap = await startTrackingCollections(page) + const { nodes: domNodesStart, listeners: eventListenersStart } = await getDomNodesAndListeners(page, cdpSession) + const startSnapshotFilename = await takeHeapSnapshot(page, cdpSession) + if (debug) { + // Point in time before running any iterations + debugger // eslint-disable-line no-debugger + } + for (let i = 0; i < numIterations; i++) { + onProgress(`Iteration ${i + 1}/${numIterations}...`) + await iteration(page, test.data) + } + onProgress('Taking end snapshot...') + const endSnapshotFilename = await takeHeapSnapshot(page, cdpSession) + const { nodes: domNodesEnd, listeners: eventListenersEnd } = await getDomNodesAndListeners(page, cdpSession) + const leakingCollections = await findLeakingCollections(page, weakMap, numIterations, debug) + if (debug) { + // Point in time after running iterations + debugger // eslint-disable-line no-debugger + } - onProgress('Analyzing snapshots...') - const { leakingObjects, startStatistics, endStatistics } = await analyzeHeapSnapshots( - startSnapshotFilename, endSnapshotFilename, numIterations - ) - const leakingListeners = analyzeEventListeners(eventListenersStart, eventListenersEnd, numIterations) - const domNodesCountDelta = domNodesCountEnd - domNodesCountStart - const delta = endStatistics.total - startStatistics.total - const deltaPerIteration = Math.round(delta / numIterations) - const leaksDetected = Boolean( - deltaPerIteration > 0 && ( - leakingObjects.length || - leakingListeners.length || - domNodesCountDelta > 0 || - leakingCollections.length + onProgress('Analyzing snapshots...') + const { leakingObjects, startStatistics, endStatistics } = await analyzeHeapSnapshots( + startSnapshotFilename, endSnapshotFilename, numIterations + ) + const leakingListeners = analyzeEventListeners(eventListenersStart, eventListenersEnd, numIterations) + const eventListenersSummary = calculateEventListenersSummary(eventListenersStart, eventListenersEnd, numIterations) + const leakingDomNodes = analyzeDomNodes(domNodesStart, domNodesEnd, numIterations) + const delta = endStatistics.total - startStatistics.total + const deltaPerIteration = Math.round(delta / numIterations) + const leaksDetected = Boolean( + deltaPerIteration > 0 && ( + leakingObjects.length || + leakingListeners.length || + leakingDomNodes.length || + leakingCollections.length + ) ) - ) - - const result = { - delta, - deltaPerIteration, - numIterations, - leaks: { - detected: leaksDetected, - objects: leakingObjects, - eventListeners: leakingListeners, - domNodes: { - delta: domNodesCountDelta, - deltaPerIteration: domNodesCountDelta / numIterations + + const result = { + delta, + deltaPerIteration, + numIterations, + leaks: { + detected: leaksDetected, + objects: leakingObjects, + eventListeners: leakingListeners, + eventListenersSummary, // eventListenersSummary is a separate object for backwards compat + domNodes: { + // delta and deltaPerIteration are redundant, but for backwards compat + delta: domNodesEnd.length - domNodesStart.length, + deltaPerIteration: (domNodesEnd.length - domNodesStart.length) / numIterations, + nodes: leakingDomNodes + }, + collections: leakingCollections }, - collections: leakingCollections - }, - before: { - statistics: startStatistics, - eventListeners: eventListenersStart, - domNodes: { - count: domNodesCountStart - } - }, - after: { - statistics: endStatistics, - eventListeners: eventListenersEnd, - domNodes: { - count: domNodesCountEnd + before: { + statistics: startStatistics, + eventListeners: eventListenersStart, + domNodes: { + count: domNodesStart.length, // domNodes.count is redundant, but for backwards compat + nodes: domNodesStart + } + }, + after: { + statistics: endStatistics, + eventListeners: eventListenersEnd, + domNodes: { + count: domNodesEnd.length, // domNodes.count is redundant, but for backwards compat + nodes: domNodesEnd + } } } - } - if (heapsnapshot) { - result.before.heapsnapshot = startSnapshotFilename - result.after.heapsnapshot = endSnapshotFilename - } else { - await Promise.all([fs.rm(startSnapshotFilename), fs.rm(endSnapshotFilename)]) - } + if (heapsnapshot) { + result.before.heapsnapshot = startSnapshotFilename + result.after.heapsnapshot = endSnapshotFilename + } else { + await Promise.all([fs.rm(startSnapshotFilename), fs.rm(endSnapshotFilename)]) + } - return { test, result } + return { test, result } + })) })) } diff --git a/test/spec/domNodes.test.mjs b/test/spec/domNodes.test.mjs index fe4a09c..f45ada7 100644 --- a/test/spec/domNodes.test.mjs +++ b/test/spec/domNodes.test.mjs @@ -14,6 +14,36 @@ describe('dom nodes', () => { ]) const result = results[0].result expect(result.leaks.detected).to.equal(true) + expect(result.leaks.domNodes.delta).to.equal(3) expect(result.leaks.domNodes.deltaPerIteration).to.equal(1) + + expect(result.before.domNodes.count).to.equal(14) + expect(result.after.domNodes.count).to.equal(17) + + expect(result.leaks.domNodes.nodes).to.deep.equal([ + { + description: 'div', + before: 2, + after: 5, + delta: 3, + deltaPerIteration: 1 + } + ] + ) + }) + + it('the tool does not leak dom nodes itself', async () => { + const results = await asyncIterableToArray(findLeaks('http://localhost:3000/test/www/recyclesDomNodes/', { + iterations: 3 + })) + + expect(results.length).to.equal(1) + expect(results.map(_ => ({ href: _.test.data.href }))).to.deep.equal([ + { href: 'about' } + ]) + const result = results[0].result + expect(result.leaks.detected).to.equal(false) + expect(result.leaks.domNodes.deltaPerIteration).to.equal(0) + expect(result.leaks.eventListeners).to.deep.equal([]) }) }) diff --git a/test/spec/eventListeners.test.mjs b/test/spec/eventListeners.test.mjs index a877cac..16b32d9 100644 --- a/test/spec/eventListeners.test.mjs +++ b/test/spec/eventListeners.test.mjs @@ -15,6 +15,13 @@ describe('event listeners', () => { const result = results[0].result expect(result.leaks.detected).to.equal(true) + expect(result.leaks.eventListenersSummary).to.deep.equal({ + before: 9, + after: 21, + delta: 12, + deltaPerIteration: 4 + }) + expect(result.leaks.eventListeners).to.deep.equal([ { type: 'click', @@ -151,6 +158,14 @@ describe('event listeners', () => { const result = results[0].result expect(result.leaks.detected).to.equal(true) + expect(result.leaks.eventListenersSummary).to.deep.equal({ + before: 7, + after: 10, + delta: 3, + deltaPerIteration: 1 + } + ) + expect(result.leaks.eventListeners).to.deep.equal([ { type: 'resize', diff --git a/test/www/recyclesDomNodes/index.html b/test/www/recyclesDomNodes/index.html new file mode 100644 index 0000000..a5f794d --- /dev/null +++ b/test/www/recyclesDomNodes/index.html @@ -0,0 +1,44 @@ + + + + + + + + + + \ No newline at end of file From 0f94c1e821f48c6fe8f78e4c37ee2f88713e0fbe Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Tue, 21 Dec 2021 13:38:04 -0800 Subject: [PATCH 2/2] fix: refactor --- src/eventListeners.js | 4 ++-- src/{getNodeDescriptors.js => getDescriptors.js} | 4 ++-- src/index.js | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) rename src/{getNodeDescriptors.js => getDescriptors.js} (75%) diff --git a/src/eventListeners.js b/src/eventListeners.js index 98098a3..6f71d36 100644 --- a/src/eventListeners.js +++ b/src/eventListeners.js @@ -1,6 +1,6 @@ import { v4 as uuidV4 } from 'uuid' import { omit, pick } from './util.js' -import { getNodeDescriptors } from './getNodeDescriptors.js' +import { getDescriptors } from './getDescriptors.js' import { getAllDomNodes } from './browser/getAllDomNodes.js' // via https://stackoverflow.com/a/67030384 @@ -15,7 +15,7 @@ export async function getDomNodesAndListeners (page, cdpSession) { `, objectGroup }) - const nodeDescriptors = await getNodeDescriptors(cdpSession, objectId) + const nodeDescriptors = await getDescriptors(cdpSession, objectId) const listenersWithNodes = [] diff --git a/src/getNodeDescriptors.js b/src/getDescriptors.js similarity index 75% rename from src/getNodeDescriptors.js rename to src/getDescriptors.js index fb1dab1..ba79e2b 100644 --- a/src/getNodeDescriptors.js +++ b/src/getDescriptors.js @@ -1,5 +1,5 @@ -// Given an array of dom nodes, get their descriptors -export async function getNodeDescriptors (cdpSession, objectId) { +// Given an array of objects, get their descriptors +export async function getDescriptors (cdpSession, objectId) { // via https://stackoverflow.com/a/67030384 const { result } = await cdpSession.send('Runtime.getProperties', { objectId }) diff --git a/src/index.js b/src/index.js index 552b3e2..f1323d1 100644 --- a/src/index.js +++ b/src/index.js @@ -151,7 +151,6 @@ export async function * findLeaks (pageUrl, options = {}) { eventListeners: leakingListeners, eventListenersSummary, // eventListenersSummary is a separate object for backwards compat domNodes: { - // delta and deltaPerIteration are redundant, but for backwards compat delta: domNodesEnd.length - domNodesStart.length, deltaPerIteration: (domNodesEnd.length - domNodesStart.length) / numIterations, nodes: leakingDomNodes