[LiveMetrics] Release objects to prevent memory leaks

The CDP client will retain objects in memory until they are explicitly
released. This CL releases objects so that they don't cause any
memory leaks.

The frontend link to the elements panel needs to be created before the
object is released so this CL also moves the logic for creating node
links into LiveMetrics.ts.

This CL also uses WeakRef in the injected code to prevent memory leaks
caused by the permanent node list.

Bug: 376777343
Change-Id: I809ae85802842c480f38a75c13097b575ee64f97
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6098570
Reviewed-by: Connor Clark <[email protected]>
Commit-Queue: Adam Raine <[email protected]>

Author: Adam Raine

front_end/models/live-metrics/LiveMetrics.ts

@@ -159,8 +159,7 @@ export class LiveMetrics extends Common.ObjectWrapper.ObjectWrapper<EventTypes>
    * DOM nodes can't be sent over a runtime binding, so we have to retrieve
    * them separately.
    */
-  async #resolveDomNode(index: number, executionContextId: Protocol.Runtime.ExecutionContextId):
-      Promise<SDK.DOMModel.DOMNode|null> {
+  async #resolveNodeRef(index: number, executionContextId: Protocol.Runtime.ExecutionContextId): Promise<NodeRef|null> {
     if (!this.#target) {
       return null;
     }
@@ -184,15 +183,21 @@ export class LiveMetrics extends Common.ObjectWrapper.ObjectWrapper<EventTypes>
       return null;
     }
 
-    const remoteObject = runtimeModel.createRemoteObject(result);
-    return domModel.pushObjectAsNodeToFrontend(remoteObject);
-  }
+    let remoteObject;
+    try {
+      remoteObject = runtimeModel.createRemoteObject(result);
+      const node = await domModel.pushObjectAsNodeToFrontend(remoteObject);
+      if (!node) {
+        return null;
+      }
 
-  async #refreshNode(domModel: SDK.DOMModel.DOMModel, node: SDK.DOMModel.DOMNode):
-      Promise<SDK.DOMModel.DOMNode|undefined> {
-    const backendNodeId = node.backendNodeId();
-    const nodes = await domModel.pushNodesByBackendIdsToFrontend(new Set([backendNodeId]));
-    return nodes?.get(backendNodeId) || undefined;
+      const link = await Common.Linkifier.Linkifier.linkify(node);
+      return {node, link};
+    } catch {
+      return null;
+    } finally {
+      remoteObject?.release();
+    }
   }
 
   #sendStatusUpdate(): void {
@@ -221,24 +226,29 @@ export class LiveMetrics extends Common.ObjectWrapper.ObjectWrapper<EventTypes>
   async #onDocumentUpdate(event: Common.EventTarget.EventTargetEvent<SDK.DOMModel.DOMModel>): Promise<void> {
     const domModel = event.data;
 
-    const allLayoutAffectedNodes = this.#layoutShifts.flatMap(shift => shift.affectedNodes);
-    const toRefresh: Array<{node?: SDK.DOMModel.DOMNode}> =
-        [this.#lcpValue || {}, ...this.#interactions.values(), ...allLayoutAffectedNodes];
+    const toRefresh = [
+      this.#lcpValue?.nodeRef,
+      ...this.#interactions.values().map(i => i.nodeRef),
+      ...this.#layoutShifts.flatMap(shift => shift.affectedNodeRefs),
+    ].filter((nodeRef): nodeRef is NodeRef => Boolean(nodeRef));
 
-    const allPromises = toRefresh.map(item => {
-      const node = item.node;
-      if (node === undefined) {
+    const idsToRefresh = new Set(toRefresh.map(nodeRef => nodeRef.node.backendNodeId()));
+    const nodes = await domModel.pushNodesByBackendIdsToFrontend(idsToRefresh);
+    if (!nodes) {
+      return;
+    }
+
+    const allPromises = toRefresh.map(async nodeRef => {
+      const refreshedNode = nodes.get(nodeRef.node.backendNodeId());
+
+      // It is possible for the refreshed node to be undefined even though it was defined previously.
+      // We should keep the affected nodes consistent from the user perspective, so we will just keep the stale node instead of removing it.
+      if (!refreshedNode) {
         return;
       }
 
-      return this.#refreshNode(domModel, node).then(refreshedNode => {
-        // In theory, it is possible for `node` to be undefined even though it was defined previously.
-        // We should keep the affected nodes consistent from the user perspective, so we will just keep the stale node instead of removing it.
-        // This is unlikely to happen in practice.
-        if (refreshedNode) {
-          item.node = refreshedNode;
-        }
-      });
+      nodeRef.node = refreshedNode;
+      nodeRef.link = await Common.Linkifier.Linkifier.linkify(refreshedNode);
     });
 
     await Promise.all(allPromises);
@@ -257,9 +267,9 @@ export class LiveMetrics extends Common.ObjectWrapper.ObjectWrapper<EventTypes>
           warnings,
         };
         if (webVitalsEvent.nodeIndex !== undefined) {
-          const node = await this.#resolveDomNode(webVitalsEvent.nodeIndex, executionContextId);
-          if (node) {
-            lcpEvent.node = node;
+          const nodeRef = await this.#resolveNodeRef(webVitalsEvent.nodeIndex, executionContextId);
+          if (nodeRef) {
+            lcpEvent.nodeRef = nodeRef;
           }
         }
 
@@ -321,26 +331,25 @@ export class LiveMetrics extends Common.ObjectWrapper.ObjectWrapper<EventTypes>
         }
 
         if (webVitalsEvent.nodeIndex !== undefined) {
-          const node = await this.#resolveDomNode(webVitalsEvent.nodeIndex, executionContextId);
+          const node = await this.#resolveNodeRef(webVitalsEvent.nodeIndex, executionContextId);
           if (node) {
-            interaction.node = node;
+            interaction.nodeRef = node;
           }
         }
         break;
       }
       case 'LayoutShift': {
         const nodePromises = webVitalsEvent.affectedNodeIndices.map(nodeIndex => {
-          return this.#resolveDomNode(nodeIndex, executionContextId);
+          return this.#resolveNodeRef(nodeIndex, executionContextId);
         });
 
-        const affectedNodes = (await Promise.all(nodePromises))
-                                  .filter((node): node is SDK.DOMModel.DOMNode => Boolean(node))
-                                  .map(node => ({node}));
+        const affectedNodes =
+            (await Promise.all(nodePromises)).filter((nodeRef): nodeRef is NodeRef => Boolean(nodeRef));
 
         const layoutShift: LayoutShift = {
           score: webVitalsEvent.score,
           uniqueLayoutShiftId: webVitalsEvent.uniqueLayoutShiftId,
-          affectedNodes,
+          affectedNodeRefs: affectedNodes,
         };
         this.#layoutShifts.push(layoutShift);
         break;
@@ -572,9 +581,14 @@ export interface MetricValue {
   warnings?: string[];
 }
 
+export interface NodeRef {
+  node: SDK.DOMModel.DOMNode;
+  link: Node;
+}
+
 export interface LCPValue extends MetricValue {
   phases: Spec.LCPPhases;
-  node?: SDK.DOMModel.DOMNode;
+  nodeRef?: NodeRef;
 }
 
 export interface INPValue extends MetricValue {
@@ -589,7 +603,7 @@ export interface CLSValue extends MetricValue {
 export interface LayoutShift {
   score: number;
   uniqueLayoutShiftId: Spec.UniqueLayoutShiftId;
-  affectedNodes: Array<{node: SDK.DOMModel.DOMNode}>;
+  affectedNodeRefs: NodeRef[];
 }
 
 export interface Interaction {
@@ -601,7 +615,7 @@ export interface Interaction {
   nextPaintTime: number;
   phases: Spec.INPPhases;
   longAnimationFrameTimings: Spec.PerformanceLongAnimationFrameTimingJSON[];
-  node?: SDK.DOMModel.DOMNode;
+  nodeRef?: NodeRef;
 }
 
 export interface StatusEvent {

front_end/models/live-metrics/web-vitals-injected/web-vitals-injected.ts

@@ -77,11 +77,11 @@ function sendEventToDevTools(event: Spec.WebVitalsEvent): void {
   window[Spec.EVENT_BINDING_NAME](payload);
 }
 
-const nodeList: Node[] = [];
+const nodeList: WeakRef<Node>[] = [];
 
 function establishNodeIndex(node: Node): number {
   const index = nodeList.length;
-  nodeList.push(node);
+  nodeList.push(new WeakRef(node));
   return index;
 }
 
@@ -95,7 +95,7 @@ function establishNodeIndex(node: Node): number {
  * for the specified index.
  */
 window.getNodeForIndex = (index: number): Node|undefined => {
-  return nodeList[index];
+  return nodeList[index].deref();
 };
 
 function limitScripts(loafs: Spec.PerformanceLongAnimationFrameTimingJSON[]):

front_end/panels/timeline/components/LiveMetricsView.test.ts

@@ -398,9 +398,9 @@ describeWithMockConnection('LiveMetricsView', () => {
       },
       interactions: new Map(),
       layoutShifts: [
-        {score: 0.05, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
-        {score: 0.1, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-2'},
-        {score: 0.01, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-3'},
+        {score: 0.05, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
+        {score: 0.1, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-2'},
+        {score: 0.01, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-3'},
       ],
     });
     await coordinator.done();
@@ -430,9 +430,9 @@ describeWithMockConnection('LiveMetricsView', () => {
       },
       interactions: new Map(),
       layoutShifts: [
-        {score: 0.05, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
-        {score: 0.1, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-2'},
-        {score: 0.01, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-3'},
+        {score: 0.05, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
+        {score: 0.1, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-2'},
+        {score: 0.01, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-3'},
       ],
     });
     await coordinator.done();
@@ -449,9 +449,9 @@ describeWithMockConnection('LiveMetricsView', () => {
       },
       interactions: new Map(),
       layoutShifts: [
-        {score: 0.05, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
-        {score: 0.1, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-2'},
-        {score: 0.01, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-3'},
+        {score: 0.05, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
+        {score: 0.1, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-2'},
+        {score: 0.01, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-3'},
       ],
     });
     await coordinator.done();
@@ -586,7 +586,7 @@ describeWithMockConnection('LiveMetricsView', () => {
         },
       ]),
       layoutShifts: [
-        {score: 0.1, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
+        {score: 0.1, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
       ],
     });
     await coordinator.done();
@@ -643,7 +643,7 @@ describeWithMockConnection('LiveMetricsView', () => {
         },
       ]),
       layoutShifts: [
-        {score: 0.1, affectedNodes: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
+        {score: 0.1, affectedNodeRefs: [], uniqueLayoutShiftId: 'layout-shift-1-1'},
       ],
     });
     await coordinator.done();

front_end/panels/timeline/components/LiveMetricsView.ts

@@ -32,8 +32,7 @@ import type {MetricCardData} from './MetricCard.js';
 import metricValueStyles from './metricValueStyles.css.js';
 import {CLS_THRESHOLDS, INP_THRESHOLDS, renderMetricValue} from './Utils.js';
 
-const {html, nothing, Directives} = LitHtml;
-const {until} = Directives;
+const {html, nothing} = LitHtml;
 
 type DeviceOption = CrUXManager.DeviceScope|'AUTO';
 
@@ -409,7 +408,7 @@ export class LiveMetricsView extends LegacyWrapper.LegacyWrapper.WrappableCompon
 
   #renderLcpCard(): LitHtml.LitTemplate {
     const fieldData = this.#cruxManager.getSelectedFieldMetricData('largest_contentful_paint');
-    const node = this.#lcpValue?.node;
+    const nodeLink = this.#lcpValue?.nodeRef?.link;
     const phases = this.#lcpValue?.phases;
 
     // clang-format off
@@ -428,10 +427,10 @@ export class LiveMetricsView extends LegacyWrapper.LegacyWrapper.WrappableCompon
           [i18nString(UIStrings.elementRenderDelay), phases.elementRenderDelay],
         ],
       } as MetricCardData}>
-        ${node ? html`
+        ${nodeLink ? html`
             <div class="related-info" slot="extra-info">
               <span class="related-info-label">${i18nString(UIStrings.lcpElement)}</span>
-              <span class="related-info-link">${until(Common.Linkifier.Linkifier.linkify(node))}</span>
+              <span class="related-info-link">${nodeLink}</span>
             </div>
           `
           : nothing}
@@ -906,8 +905,7 @@ export class LiveMetricsView extends LegacyWrapper.LegacyWrapper.WrappableCompon
                       html`<span class="interaction-inp-chip" title=${i18nString(UIStrings.inpInteraction)}>INP</span>`
                     : nothing}
                   </span>
-                  <span class="interaction-node">${
-                    interaction.node && until(Common.Linkifier.Linkifier.linkify(interaction.node))}</span>
+                  <span class="interaction-node">${interaction.nodeRef?.link}</span>
                   ${isP98Excluded ? html`<devtools-icon
                     class="interaction-info"
                     name="info"
@@ -1011,8 +1009,8 @@ export class LiveMetricsView extends LegacyWrapper.LegacyWrapper.WrappableCompon
             <li id=${layoutShift.uniqueLayoutShiftId} class="log-item layout-shift" tabindex="-1">
               <div class="layout-shift-score">Layout shift score: ${metricValue}</div>
               <div class="layout-shift-nodes">
-                ${layoutShift.affectedNodes.map(({node}) => html`
-                  <div class="layout-shift-node">${until(Common.Linkifier.Linkifier.linkify(node))}</div>
+                ${layoutShift.affectedNodeRefs.map(({link}) => html`
+                  <div class="layout-shift-node">${link}</div>
                 `)}
               </div>
             </li>