RPP: fix lcp image request <> navigation relation

The below bug was reported where the LCP Insight was using the wrong
image request for the LCP event. This was because the
LargestImagePaintHandler tied events to the current navigation, but if
the page being loaded has multiple frames, the "current navigation"
isn't that helpful and instead we need to track "current navigation per
frame" in order to not have data from frame A get mixed up with data
from frame B.

The bug below is caused by the "current navigation" becoming the loading
of an iframe, which happens before the actual LCP image is loaded. That
means we mistakenly relate the actual LCP image to an entirely different
and unrelated navigation.

The fix here is to track the current navigation per frame. Then when we
look up and relate the LCP Image request to the navigation, we aren't
mistakenly mixing requests across different frames.

I also landed an example in the stories repo
ChromeDevTools/performance-stories#38 to give us
a test case for this and used it in the unit test.

Fixed: 384000716
Change-Id: Ica187428545ec1d783ea3a050d0e837ad6f3f729
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6091385
Commit-Queue: Andres Olivares <[email protected]>
Auto-Submit: Jack Franklin <[email protected]>
Commit-Queue: Jack Franklin <[email protected]>
Reviewed-by: Andres Olivares <[email protected]>

Author: jackfranklin

front_end/models/trace/handlers/LargestImagePaintHandler.test.ts

@@ -3,10 +3,11 @@
 // found in the LICENSE file.
 
 import type * as Protocol from '../../../generated/protocol.js';
+import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
 import {TraceLoader} from '../../../testing/TraceLoader.js';
 import * as Trace from '../trace.js';
 
-describe('LargestImagePaintHandler', function() {
+describeWithEnvironment('LargestImagePaintHandler', function() {
   beforeEach(async () => {
     Trace.Handlers.ModelHandlers.LargestImagePaint.reset();
   });
@@ -24,4 +25,21 @@ describe('LargestImagePaintHandler', function() {
     assert.strictEqual(imageForLCP?.args.data?.DOMNodeId, 10 as Protocol.DOM.BackendNodeId);
     assert.strictEqual(imageForLCP?.args.data?.imageUrl, 'https://via.placeholder.com/2000.jpg');
   });
+
+  it('is able to identify the LCP image request for each navigation', async function() {
+    // The handler depends on Meta + Network requests, let's just execute
+    // all of them rather than call them individually.
+    const {parsedTrace} = await TraceLoader.traceEngine(this, 'lcp-multiple-frames.json.gz');
+
+    const {mainFrameNavigations} = parsedTrace.Meta;
+    // There is only one main frame navigation in this trace.
+    assert.lengthOf(mainFrameNavigations, 1);
+    const mainNavigation = mainFrameNavigations.at(0);
+    assert.isOk(mainNavigation);
+
+    const {lcpRequestByNavigation} = parsedTrace.LargestImagePaint;
+    const lcpRequest = lcpRequestByNavigation.get(mainNavigation);
+    assert.isOk(lcpRequest);
+    assert.strictEqual(lcpRequest.args.data.url, 'https://placehold.co/1000.jpg');
+  });
 });

front_end/models/trace/handlers/LargestImagePaintHandler.ts

@@ -21,26 +21,32 @@ import type {HandlerName} from './types.js';
  * `LargestContentfulPaint::Candidate` will have. So, when we find an image
  * paint candidate, we can store it, keying it on the node ID.
  * Then, when it comes to finding the network request for an LCP image, we can
- *
  * use the nodeId from the LCP candidate to find the image candidate. That image
  * candidate also contains a `imageUrl` property, which will have the full URL
  * to the image.
  **/
 const imageByDOMNodeId = new Map<Protocol.DOM.BackendNodeId, Types.Events.LargestImagePaintCandidate>();
 const lcpRequestByNavigation = new Map<Types.Events.NavigationStart|null, Types.Events.SyntheticNetworkRequest>();
 const lcpPaintEventByNavigation = new Map<Types.Events.NavigationStart|null, Types.Events.LargestImagePaintCandidate>();
-let currentNavigation: Types.Events.NavigationStart|null;
+
+/**
+ * We track the latest navigation that happened because we want to relate each
+ * LargestImagePaintCandidate to the navigation it occurred after, but we have
+ * to track navigations per-frame to avoid us reading a navigation from one
+ * frame and treating it as the latest navigation across all frames.
+ */
+const lastNavigationPerFrame = new Map<string, Types.Events.NavigationStart>();
 
 export function reset(): void {
   imageByDOMNodeId.clear();
   lcpRequestByNavigation.clear();
   lcpPaintEventByNavigation.clear();
-  currentNavigation = null;
+  lastNavigationPerFrame.clear();
 }
 
 export function handleEvent(event: Types.Events.Event): void {
-  if (Types.Events.isNavigationStart(event)) {
-    currentNavigation = event;
+  if (Types.Events.isNavigationStart(event) && event.args.frame) {
+    lastNavigationPerFrame.set(event.args.frame, event);
     return;
   }
 
@@ -53,7 +59,13 @@ export function handleEvent(event: Types.Events.Event): void {
   }
 
   imageByDOMNodeId.set(event.args.data.DOMNodeId, event);
-  lcpPaintEventByNavigation.set(currentNavigation, event);
+
+  // Now relate this LargestImagePaintCandidate event to the last navigation
+  // that occurred within the same frame.
+  const navigationForEvent = lastNavigationPerFrame.get(event.args.frame);
+  if (navigationForEvent) {
+    lcpPaintEventByNavigation.set(navigationForEvent, event);
+  }
 }
 
 export async function finalize(): Promise<void> {

front_end/panels/timeline/fixtures/traces/BUILD.gn

@@ -46,6 +46,7 @@ copy_to_gen("traces") {
     "large-recalc-style.json.gz",
     "lcp-images-rasterizer.json.gz",
     "lcp-images.json.gz",
+    "lcp-multiple-frames.json.gz",
     "lcp-web-font.json.gz",
     "load-simple.json.gz",
     "mainWasm_profile.json.gz",

front_end/panels/timeline/fixtures/traces/README.md

@@ -228,3 +228,14 @@ Generate from a recording of [this HTML file](https://gist.github.com/adamraine/
 4. Reload the page
 5. Click the button once
 6. End recording
+
+### lcp-multiple-frames
+
+Generated from [lcp-iframes story](https://github.com/ChromeDevTools/performance-stories/tree/main/lcp-iframes).
+
+Contains a page load that has two frames (main frame + iframe). There are two images loaded in each:
+
+- the iframe loads placeholder.co/50.jpg and placeholder.co/2000.jpg
+- the main frame loads placeholder.co/100.jpg and placeholder.co/1000.jpg
+
+This trace is used to verify the fix for a bug [crbug.com/384000716] where we incorrectly associated image requests to the wrong navigation when calculating the LCP image.