cherry-pick(#24145): fix(snapshots): match resources by method (#24147)

Fixes #24144.

Previously, we only matched by url, which confuses GET and HEAD requests
where the latter is usually zero-sized.

Also make sure that resources are sorted by their monotonicTime, since
that's not always the case in the trace file, where they are sorted by
the "response body retrieved" time.

Author: dgozman

packages/playwright-core/src/server/har/harTracer.ts

@@ -437,6 +437,12 @@ export class HarTracer {
     const pageEntry = this._createPageEntryIfNeeded(page);
     const request = response.request();
 
+    // Prefer "response received" time over "request sent" time
+    // for the purpose of matching requests that were used in a particular snapshot.
+    // Note that both snapshot time and request time are taken here in the Node process.
+    if (this._options.includeTraceInfo)
+      harEntry._monotonicTime = monotonicTime();
+
     harEntry.response = {
       status: response.status(),
       statusText: response.statusText(),

packages/trace-viewer/src/snapshotRenderer.ts

@@ -112,17 +112,19 @@ export class SnapshotRenderer {
     return { html, pageId: snapshot.pageId, frameId: snapshot.frameId, index: this._index };
   }
 
-  resourceByUrl(url: string): ResourceSnapshot | undefined {
+  resourceByUrl(url: string, method: string): ResourceSnapshot | undefined {
     const snapshot = this._snapshot;
     let result: ResourceSnapshot | undefined;
 
     // First try locating exact resource belonging to this frame.
     for (const resource of this._resources) {
+      // Only use resources that received response before the snapshot.
+      // Note that both snapshot time and request time are taken in the same Node process.
       if (typeof resource._monotonicTime === 'number' && resource._monotonicTime >= snapshot.timestamp)
         break;
       if (resource._frameref !== snapshot.frameId)
         continue;
-      if (resource.request.url === url) {
+      if (resource.request.url === url && resource.request.method === method) {
         // Pick the last resource with matching url - most likely it was used
         // at the time of snapshot, not the earlier aborted resource with the same url.
         result = resource;
@@ -132,17 +134,19 @@ export class SnapshotRenderer {
     if (!result) {
       // Then fall back to resource with this URL to account for memory cache.
       for (const resource of this._resources) {
+        // Only use resources that received response before the snapshot.
+        // Note that both snapshot time and request time are taken in the same Node process.
         if (typeof resource._monotonicTime === 'number' && resource._monotonicTime >= snapshot.timestamp)
           break;
-        if (resource.request.url === url) {
+        if (resource.request.url === url && resource.request.method === method) {
           // Pick the last resource with matching url - most likely it was used
           // at the time of snapshot, not the earlier aborted resource with the same url.
           result = resource;
         }
       }
     }
 
-    if (result) {
+    if (result && method.toUpperCase() === 'GET') {
       // Patch override if necessary.
       for (const o of snapshot.resourceOverrides) {
         if (url === o.url && o.sha1) {

packages/trace-viewer/src/snapshotServer.ts

@@ -65,11 +65,11 @@ export class SnapshotServer {
     });
   }
 
-  async serveResource(requestUrlAlternatives: string[], snapshotUrl: string): Promise<Response> {
+  async serveResource(requestUrlAlternatives: string[], method: string, snapshotUrl: string): Promise<Response> {
     let resource: ResourceSnapshot | undefined;
     const snapshot = this._snapshotIds.get(snapshotUrl)!;
     for (const requestUrl of requestUrlAlternatives) {
-      resource = snapshot?.resourceByUrl(removeHash(requestUrl));
+      resource = snapshot?.resourceByUrl(removeHash(requestUrl), method);
       if (resource)
         break;
     }

packages/trace-viewer/src/snapshotStorage.ts

@@ -56,4 +56,9 @@ export class SnapshotStorage {
   snapshotsForTest() {
     return [...this._frameSnapshots.keys()];
   }
+
+  finalize() {
+    // Resources are not necessarily sorted in the trace file, so sort them now.
+    this._resources.sort((a, b) => (a._monotonicTime || 0) - (b._monotonicTime || 0));
+  }
 }

packages/trace-viewer/src/sw.ts

@@ -142,7 +142,7 @@ async function doFetch(event: FetchEvent): Promise<Response> {
   const lookupUrls = [request.url];
   if (isDeployedAsHttps && request.url.startsWith('https://'))
     lookupUrls.push(request.url.replace(/^https/, 'http'));
-  return snapshotServer.serveResource(lookupUrls, snapshotUrl);
+  return snapshotServer.serveResource(lookupUrls, request.method, snapshotUrl);
 }
 
 async function gc() {

packages/trace-viewer/src/traceModel.ts

@@ -100,6 +100,8 @@ export class TraceModel {
 
       this.contextEntries.push(contextEntry);
     }
+
+    this._snapshotStorage!.finalize();
   }
 
   async hasEntry(filename: string): Promise<boolean> {

tests/library/snapshotter.spec.ts

@@ -51,7 +51,7 @@ it.describe('snapshots', () => {
     });
     await page.setContent('<link rel="stylesheet" href="style.css"><button>Hello</button>');
     const snapshot = await snapshotter.captureSnapshot(toImpl(page), 'call@1', 'snapshot@call@1');
-    const resource = snapshot.resourceByUrl(`http://localhost:${server.PORT}/style.css`);
+    const resource = snapshot.resourceByUrl(`http://localhost:${server.PORT}/style.css`, 'GET');
     expect(resource).toBeTruthy();
   });
 
@@ -124,7 +124,7 @@ it.describe('snapshots', () => {
 
     await page.evaluate(() => { (document.styleSheets[0].cssRules[0] as any).style.color = 'blue'; });
     const snapshot2 = await snapshotter.captureSnapshot(toImpl(page), 'call@2', 'snapshot@call@2');
-    const resource = snapshot2.resourceByUrl(`http://localhost:${server.PORT}/style.css`);
+    const resource = snapshot2.resourceByUrl(`http://localhost:${server.PORT}/style.css`, 'GET');
     expect((await snapshotter.resourceContentForTest(resource.response.content._sha1)).toString()).toBe('button { color: blue; }');
   });
 

tests/library/trace-viewer.spec.ts

@@ -851,7 +851,7 @@ test('should open trace-1.31', async ({ showTraceViewer }) => {
   await expect(snapshot.locator('[__playwright_target__]')).toHaveText(['Submit']);
 });
 
-test('should prefer later resource request', async ({ page, server, runAndTrace }) => {
+test('should prefer later resource request with the same method', async ({ page, server, runAndTrace }) => {
   const html = `
     <body>
       <script>
@@ -862,13 +862,22 @@ test('should prefer later resource request', async ({ page, server, runAndTrace
 
         if (!window.location.href.includes('reloaded'))
           window.location.href = window.location.href + '?reloaded';
+        else
+          link.onload = () => fetch('style.css', { method: 'HEAD' });
       </script>
+      <div>Hello</div>
     </body>
   `;
 
   let reloadStartedCallback = () => {};
   const reloadStartedPromise = new Promise<void>(f => reloadStartedCallback = f);
   server.setRoute('/style.css', async (req, res) => {
+    if (req.method === 'HEAD') {
+      res.statusCode = 200;
+      res.end('');
+      return;
+    }
+
     // Make sure reload happens before style arrives.
     await reloadStartedPromise;
     res.end('body { background-color: rgb(123, 123, 123) }');
@@ -880,8 +889,13 @@ test('should prefer later resource request', async ({ page, server, runAndTrace
   });
 
   const traceViewer = await runAndTrace(async () => {
+    const headRequest = page.waitForRequest(req => req.url() === server.PREFIX + '/style.css' && req.method() === 'HEAD');
     await page.goto(server.PREFIX + '/index.html');
+    await headRequest;
+    await page.locator('div').click();
   });
-  const frame = await traceViewer.snapshotFrame('page.goto');
-  await expect(frame.locator('body')).toHaveCSS('background-color', 'rgb(123, 123, 123)');
+  const frame1 = await traceViewer.snapshotFrame('page.goto');
+  await expect(frame1.locator('body')).toHaveCSS('background-color', 'rgb(123, 123, 123)');
+  const frame2 = await traceViewer.snapshotFrame('locator.click');
+  await expect(frame2.locator('body')).toHaveCSS('background-color', 'rgb(123, 123, 123)');
 });