Skip to content

Commit

Permalink
cherry-pick(#24145): fix(snapshots): match resources by method (#24147)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dgozman authored Jul 11, 2023
1 parent 1e8e8b4 commit 74ec8c2
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 12 deletions.
6 changes: 6 additions & 0 deletions packages/playwright-core/src/server/har/harTracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
12 changes: 8 additions & 4 deletions packages/trace-viewer/src/snapshotRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions packages/trace-viewer/src/snapshotServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
5 changes: 5 additions & 0 deletions packages/trace-viewer/src/snapshotStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
2 changes: 1 addition & 1 deletion packages/trace-viewer/src/sw.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
2 changes: 2 additions & 0 deletions packages/trace-viewer/src/traceModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ export class TraceModel {

this.contextEntries.push(contextEntry);
}

this._snapshotStorage!.finalize();
}

async hasEntry(filename: string): Promise<boolean> {
Expand Down
4 changes: 2 additions & 2 deletions tests/library/snapshotter.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand Down Expand Up @@ -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; }');
});

Expand Down
20 changes: 17 additions & 3 deletions tests/library/trace-viewer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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) }');
Expand All @@ -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)');
});

0 comments on commit 74ec8c2

Please sign in to comment.