From 74ec8c243a52a56196fd18bb54e26046d89d1d48 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 10 Jul 2023 22:10:57 -0700 Subject: [PATCH] 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. --- .../src/server/har/harTracer.ts | 6 ++++++ packages/trace-viewer/src/snapshotRenderer.ts | 12 +++++++---- packages/trace-viewer/src/snapshotServer.ts | 4 ++-- packages/trace-viewer/src/snapshotStorage.ts | 5 +++++ packages/trace-viewer/src/sw.ts | 2 +- packages/trace-viewer/src/traceModel.ts | 2 ++ tests/library/snapshotter.spec.ts | 4 ++-- tests/library/trace-viewer.spec.ts | 20 ++++++++++++++++--- 8 files changed, 43 insertions(+), 12 deletions(-) diff --git a/packages/playwright-core/src/server/har/harTracer.ts b/packages/playwright-core/src/server/har/harTracer.ts index 36f7d8ede267b..4a322b4aa4226 100644 --- a/packages/playwright-core/src/server/har/harTracer.ts +++ b/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(), diff --git a/packages/trace-viewer/src/snapshotRenderer.ts b/packages/trace-viewer/src/snapshotRenderer.ts index 223d553c7be61..da725fe8f1a3f 100644 --- a/packages/trace-viewer/src/snapshotRenderer.ts +++ b/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,9 +134,11 @@ 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; @@ -142,7 +146,7 @@ export class SnapshotRenderer { } } - if (result) { + if (result && method.toUpperCase() === 'GET') { // Patch override if necessary. for (const o of snapshot.resourceOverrides) { if (url === o.url && o.sha1) { diff --git a/packages/trace-viewer/src/snapshotServer.ts b/packages/trace-viewer/src/snapshotServer.ts index 4dd673ed885ea..f1820d9df8e7d 100644 --- a/packages/trace-viewer/src/snapshotServer.ts +++ b/packages/trace-viewer/src/snapshotServer.ts @@ -65,11 +65,11 @@ export class SnapshotServer { }); } - async serveResource(requestUrlAlternatives: string[], snapshotUrl: string): Promise { + async serveResource(requestUrlAlternatives: string[], method: string, snapshotUrl: string): Promise { 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; } diff --git a/packages/trace-viewer/src/snapshotStorage.ts b/packages/trace-viewer/src/snapshotStorage.ts index a341ebf38d75f..9f4aea60c2a53 100644 --- a/packages/trace-viewer/src/snapshotStorage.ts +++ b/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)); + } } diff --git a/packages/trace-viewer/src/sw.ts b/packages/trace-viewer/src/sw.ts index 52257be2cce2c..13c21d745ed7a 100644 --- a/packages/trace-viewer/src/sw.ts +++ b/packages/trace-viewer/src/sw.ts @@ -142,7 +142,7 @@ async function doFetch(event: FetchEvent): Promise { 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() { diff --git a/packages/trace-viewer/src/traceModel.ts b/packages/trace-viewer/src/traceModel.ts index 3518a3ab80391..a4c323ffbe51f 100644 --- a/packages/trace-viewer/src/traceModel.ts +++ b/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 { diff --git a/tests/library/snapshotter.spec.ts b/tests/library/snapshotter.spec.ts index a06d8bb612eb1..0d19b811e7c7d 100644 --- a/tests/library/snapshotter.spec.ts +++ b/tests/library/snapshotter.spec.ts @@ -51,7 +51,7 @@ it.describe('snapshots', () => { }); await page.setContent(''); 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; }'); }); diff --git a/tests/library/trace-viewer.spec.ts b/tests/library/trace-viewer.spec.ts index 825ae96900965..55c9176a0c275 100644 --- a/tests/library/trace-viewer.spec.ts +++ b/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 = ` +
Hello
`; let reloadStartedCallback = () => {}; const reloadStartedPromise = new Promise(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)'); });