From 0c306df07b963229e316e6de5a434665cb2e19e4 Mon Sep 17 00:00:00 2001
From: Adam Raine <ad.st.raine@gmail.com>
Date: Wed, 3 Apr 2024 11:15:27 -0700
Subject: [PATCH 1/2] core(trace-elements): add sentry debugging for
 `impactedNodes`

---
 core/audits/layout-shifts.js                  |  2 +-
 core/gather/gatherers/trace-elements.js       | 35 +++++++++++++------
 .../gather/gatherers/trace-elements-test.js   |  7 ++++
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/core/audits/layout-shifts.js b/core/audits/layout-shifts.js
index d09da2722afb..2201ed185460 100644
--- a/core/audits/layout-shifts.js
+++ b/core/audits/layout-shifts.js
@@ -78,7 +78,7 @@ class LayoutShifts extends Audit {
       .slice(0, MAX_LAYOUT_SHIFTS);
     for (const event of topLayoutShiftEvents) {
       const biggestImpactNodeId = TraceElements.getBiggestImpactNodeForShiftEvent(
-        event.args.data.impacted_nodes || [], impactByNodeId);
+        event.args.data.impacted_nodes || [], impactByNodeId, event);
       const biggestImpactElement = traceElements.find(t => t.nodeId === biggestImpactNodeId);
 
       // Turn root causes into sub-items.
diff --git a/core/gather/gatherers/trace-elements.js b/core/gather/gatherers/trace-elements.js
index 6a939d30aadb..5773176183ba 100644
--- a/core/gather/gatherers/trace-elements.js
+++ b/core/gather/gatherers/trace-elements.js
@@ -88,19 +88,34 @@ class TraceElements extends BaseGatherer {
    *
    * @param {LH.Artifacts.TraceImpactedNode[]} impactedNodes
    * @param {Map<number, number>} impactByNodeId
+   * @param {import('../../lib/trace-engine.js').SaneSyntheticLayoutShift} event Only for debugging
    * @return {number|undefined}
    */
-  static getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId) {
-    let biggestImpactNodeId;
-    let biggestImpactNodeScore = Number.NEGATIVE_INFINITY;
-    for (const node of impactedNodes) {
-      const impactScore = impactByNodeId.get(node.node_id);
-      if (impactScore !== undefined && impactScore > biggestImpactNodeScore) {
-        biggestImpactNodeId = node.node_id;
-        biggestImpactNodeScore = impactScore;
+  static getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId, event) {
+    try {
+      let biggestImpactNodeId;
+      let biggestImpactNodeScore = Number.NEGATIVE_INFINITY;
+      for (const node of impactedNodes) {
+        const impactScore = impactByNodeId.get(node.node_id);
+        if (impactScore !== undefined && impactScore > biggestImpactNodeScore) {
+          biggestImpactNodeId = node.node_id;
+          biggestImpactNodeScore = impactScore;
+        }
       }
+      return biggestImpactNodeId;
+    } catch (err) {
+      // See https://github.com/GoogleChrome/lighthouse/issues/15870
+      // `impactedNodes` should always be an array here, but it can randomly be something else for
+      // currently unknown reasons. This exception handling will help us identify what
+      // `impactedNodes` really is and also prevent the error from being fatal.
+      Sentry.captureException(err, {
+        extra: {
+          impactedNodes,
+          event,
+        },
+      });
+      return;
     }
-    return biggestImpactNodeId;
   }
 
   /**
@@ -129,7 +144,7 @@ class TraceElements extends BaseGatherer {
         const nodeIds = [];
         const impactedNodes = event.args.data.impacted_nodes || [];
         const biggestImpactedNodeId =
-          this.getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId);
+          this.getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId, event);
         if (biggestImpactedNodeId !== undefined) {
           nodeIds.push(biggestImpactedNodeId);
         }
diff --git a/core/test/gather/gatherers/trace-elements-test.js b/core/test/gather/gatherers/trace-elements-test.js
index 67bdad011ed0..5457355df500 100644
--- a/core/test/gather/gatherers/trace-elements-test.js
+++ b/core/test/gather/gatherers/trace-elements-test.js
@@ -175,6 +175,13 @@ describe('Trace Elements gatherer - GetTopLayoutShiftElements', () => {
       {nodeId: 15}, // score: 0.1
     ]);
   });
+
+  describe('getBiggestImpactForShiftEvent', () => {
+    it('is non fatal if impactedNodes is not iterable', () => {
+      const result = TraceElementsGatherer.getBiggestImpactNodeForShiftEvent(1, new Map());
+      expect(result).toBeUndefined();
+    });
+  });
 });
 
 describe('Trace Elements gatherer - Animated Elements', () => {

From 7ca4c307e1d1a2a8988a3330e9481f8d098c324c Mon Sep 17 00:00:00 2001
From: Adam Raine <ad.st.raine@gmail.com>
Date: Fri, 5 Apr 2024 14:10:51 -0700
Subject: [PATCH 2/2] json string

---
 core/gather/gatherers/trace-elements.js | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/core/gather/gatherers/trace-elements.js b/core/gather/gatherers/trace-elements.js
index 5773176183ba..fb221e804538 100644
--- a/core/gather/gatherers/trace-elements.js
+++ b/core/gather/gatherers/trace-elements.js
@@ -108,10 +108,25 @@ class TraceElements extends BaseGatherer {
       // `impactedNodes` should always be an array here, but it can randomly be something else for
       // currently unknown reasons. This exception handling will help us identify what
       // `impactedNodes` really is and also prevent the error from being fatal.
+
+      // It's possible `impactedNodes` is not JSON serializable, so let's add more supplemental
+      // fields just in case.
+      const impactedNodesType = typeof impactedNodes;
+      const impactedNodesClassName = impactedNodes?.constructor?.name;
+
+      let impactedNodesJson;
+      let eventJson;
+      try {
+        impactedNodesJson = JSON.parse(JSON.stringify(impactedNodes));
+        eventJson = JSON.parse(JSON.stringify(event));
+      } catch {}
+
       Sentry.captureException(err, {
         extra: {
-          impactedNodes,
-          event,
+          impactedNodes: impactedNodesJson,
+          event: eventJson,
+          impactedNodesType,
+          impactedNodesClassName,
         },
       });
       return;