From ff2647cfa31a01665b541d5ed310b298b60b65c9 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 29 Jun 2022 18:11:22 -0700 Subject: [PATCH] fix(network): remove races from sizes calculation (#15208) - Do not resolve raw headers upon `loadingFinished`, since they may still come later in `responseReceivedExtraInfo`. - Introduce separate promises for `encodedBodySize`, `transferSize` and `responseHeadersSize`. - Make sure we resolve each of them either with data available from the browser, or a fallback calculation. - Set raw response headers for redirects on WebKit. - Do not stall on cached responses in Chromium, they have erroneously set `hasExtraInfo` flag. - Use `transferSize` that is available in Firefox protocol. --- .../src/server/chromium/crNetworkManager.ts | 47 ++++++++++---- .../src/server/firefox/ffNetworkManager.ts | 10 ++- .../src/server/har/harTracer.ts | 3 +- .../playwright-core/src/server/network.ts | 62 +++++++++++-------- .../server/webkit/wkInterceptableRequest.ts | 13 ++++ .../src/server/webkit/wkPage.ts | 28 ++++----- 6 files changed, 107 insertions(+), 56 deletions(-) diff --git a/packages/playwright-core/src/server/chromium/crNetworkManager.ts b/packages/playwright-core/src/server/chromium/crNetworkManager.ts index ad8f394f7d..540d6e649f 100644 --- a/packages/playwright-core/src/server/chromium/crNetworkManager.ts +++ b/packages/playwright-core/src/server/chromium/crNetworkManager.ts @@ -54,6 +54,7 @@ export class CRNetworkManager { eventsHelper.addEventListener(session, 'Fetch.authRequired', this._onAuthRequired.bind(this)), eventsHelper.addEventListener(session, 'Network.requestWillBeSent', this._onRequestWillBeSent.bind(this, workerFrame)), eventsHelper.addEventListener(session, 'Network.requestWillBeSentExtraInfo', this._onRequestWillBeSentExtraInfo.bind(this)), + eventsHelper.addEventListener(session, 'Network.requestServedFromCache', this._onRequestServedFromCache.bind(this)), eventsHelper.addEventListener(session, 'Network.responseReceived', this._onResponseReceived.bind(this)), eventsHelper.addEventListener(session, 'Network.responseReceivedExtraInfo', this._onResponseReceivedExtraInfo.bind(this)), eventsHelper.addEventListener(session, 'Network.loadingFinished', this._onLoadingFinished.bind(this)), @@ -133,6 +134,10 @@ export class CRNetworkManager { } } + _onRequestServedFromCache(event: Protocol.Network.requestServedFromCachePayload) { + this._responseExtraInfoTracker.requestServedFromCache(event); + } + _onRequestWillBeSentExtraInfo(event: Protocol.Network.requestWillBeSentExtraInfoPayload) { this._responseExtraInfoTracker.requestWillBeSentExtraInfo(event); } @@ -324,18 +329,14 @@ export class CRNetworkManager { validFrom: responsePayload?.securityDetails?.validFrom, validTo: responsePayload?.securityDetails?.validTo, }); - if (hasExtraInfo) { - this._responseExtraInfoTracker.processResponse(request._requestId, response); - } else { - // Use "provisional" headers as "raw" ones. - response.request().setRawRequestHeaders(null); - response.setRawResponseHeaders(null); - } + this._responseExtraInfoTracker.processResponse(request._requestId, response, hasExtraInfo); return response; } _handleRequestRedirect(request: InterceptableRequest, responsePayload: Protocol.Network.Response, timestamp: number, hasExtraInfo: boolean) { const response = this._createResponse(request, responsePayload, hasExtraInfo); + response.setTransferSize(null); + response.setEncodedBodySize(null); response._requestFinished((timestamp - request._timestamp) * 1000); this._requestIdToRequest.delete(request._requestId); if (request._interceptionId) @@ -372,8 +373,8 @@ export class CRNetworkManager { // event from protocol. @see https://crbug.com/883475 const response = request.request._existingResponse(); if (response) { - request.request.responseSize.transferSize = event.encodedDataLength; - request.request.responseSize.encodedBodySize = event.encodedDataLength - request.request.responseSize.responseHeadersSize; + response.setTransferSize(event.encodedDataLength); + response.responseHeadersSize().then(size => response.setEncodedBodySize(event.encodedDataLength - size)); response._requestFinished(helper.secondsToRoundishMillis(event.timestamp - request._timestamp)); } this._requestIdToRequest.delete(request._requestId); @@ -406,8 +407,11 @@ export class CRNetworkManager { if (!request) return; const response = request.request._existingResponse(); - if (response) + if (response) { + response.setTransferSize(null); + response.setEncodedBodySize(null); response._requestFinished(helper.secondsToRoundishMillis(event.timestamp - request._timestamp)); + } this._requestIdToRequest.delete(request._requestId); if (request._interceptionId) this._attemptedAuthentications.delete(request._interceptionId); @@ -572,6 +576,7 @@ type RequestInfo = { responses: network.Response[], loadingFinished?: Protocol.Network.loadingFinishedPayload, loadingFailed?: Protocol.Network.loadingFailedPayload, + servedFromCache?: boolean, }; // This class aligns responses with response headers from extra info: @@ -598,6 +603,11 @@ class ResponseExtraInfoTracker { this._checkFinished(info); } + requestServedFromCache(event: Protocol.Network.requestServedFromCachePayload) { + const info = this._getOrCreateEntry(event.requestId); + info.servedFromCache = true; + } + responseReceivedExtraInfo(event: Protocol.Network.responseReceivedExtraInfoPayload) { const info = this._getOrCreateEntry(event.requestId); info.responseReceivedExtraInfo.push(event); @@ -605,8 +615,19 @@ class ResponseExtraInfoTracker { this._checkFinished(info); } - processResponse(requestId: string, response: network.Response) { - const info = this._getOrCreateEntry(requestId); + processResponse(requestId: string, response: network.Response, hasExtraInfo: boolean) { + let info = this._requests.get(requestId); + // Cached responses have erroneous "hasExtraInfo" flag. + // https://bugs.chromium.org/p/chromium/issues/detail?id=1340398 + if (!hasExtraInfo || info?.servedFromCache) { + // Use "provisional" headers as "raw" ones. + response.request().setRawRequestHeaders(null); + response.setResponseHeadersSize(null); + response.setRawResponseHeaders(null); + return; + } + + info = this._getOrCreateEntry(requestId); info.responses.push(response); this._patchHeaders(info, info.responses.length - 1); } @@ -650,8 +671,8 @@ class ResponseExtraInfoTracker { } const responseExtraInfo = info.responseReceivedExtraInfo[index]; if (response && responseExtraInfo) { + response.setResponseHeadersSize(responseExtraInfo.headersText?.length || 0); response.setRawResponseHeaders(headersObjectToArray(responseExtraInfo.headers, '\n')); - response.request().responseSize.responseHeadersSize = responseExtraInfo.headersText?.length || 0; info.responseReceivedExtraInfo[index] = undefined; } } diff --git a/packages/playwright-core/src/server/firefox/ffNetworkManager.ts b/packages/playwright-core/src/server/firefox/ffNetworkManager.ts index 3d4df80af6..2f4bf7be6b 100644 --- a/packages/playwright-core/src/server/firefox/ffNetworkManager.ts +++ b/packages/playwright-core/src/server/firefox/ffNetworkManager.ts @@ -115,6 +115,8 @@ export class FFNetworkManager { }); // "raw" headers are the same as "provisional" headers in Firefox. response.setRawResponseHeaders(null); + // Headers size are not available in Firefox. + response.setResponseHeadersSize(null); this._page._frameManager.requestReceivedResponse(response); } @@ -123,7 +125,8 @@ export class FFNetworkManager { if (!request) return; const response = request.request._existingResponse()!; - request.request.responseSize.transferSize = event.transferSize; + response.setTransferSize(event.transferSize); + response.setEncodedBodySize(event.encodedBodySize); // Keep redirected requests in the map for future reference as redirectedFrom. const isRedirected = response.status() >= 300 && response.status() <= 399; @@ -145,8 +148,11 @@ export class FFNetworkManager { return; this._requests.delete(request._id); const response = request.request._existingResponse(); - if (response) + if (response) { + response.setTransferSize(null); + response.setEncodedBodySize(null); response._requestFinished(-1); + } request.request._setFailureText(event.errorCode); this._page._frameManager.requestFailed(request.request, event.errorCode === 'NS_BINDING_ABORTED'); } diff --git a/packages/playwright-core/src/server/har/harTracer.ts b/packages/playwright-core/src/server/har/harTracer.ts index c9eb736d52..5b06804546 100644 --- a/packages/playwright-core/src/server/har/harTracer.ts +++ b/packages/playwright-core/src/server/har/harTracer.ts @@ -310,8 +310,7 @@ export class HarTracer { this._addBarrier(page, response.sizes().then(sizes => { harEntry.response.bodySize = sizes.responseBodySize; harEntry.response.headersSize = sizes.responseHeadersSize; - // Fallback for WebKit by calculating it manually - harEntry.response._transferSize = response.request().responseSize.transferSize || (sizes.responseHeadersSize + sizes.responseBodySize); + harEntry.response._transferSize = sizes.transferSize; harEntry.request.headersSize = sizes.requestHeadersSize; compressionCalculationBarrier?.setEncodedBodySize(sizes.responseBodySize); })); diff --git a/packages/playwright-core/src/server/network.ts b/packages/playwright-core/src/server/network.ts index 1995a31571..708933eb10 100644 --- a/packages/playwright-core/src/server/network.ts +++ b/packages/playwright-core/src/server/network.ts @@ -84,12 +84,6 @@ export function stripFragmentFromUrl(url: string): string { return url.substring(0, url.indexOf('#')); } -type ResponseSize = { - encodedBodySize: number; - transferSize: number; - responseHeadersSize: number; -}; - export class Request extends SdkObject { private _response: Response | null = null; private _redirectedFrom: Request | null; @@ -107,7 +101,6 @@ export class Request extends SdkObject { private _frame: frames.Frame; private _waitForResponsePromise = new ManualPromise(); _responseEndTiming = -1; - readonly responseSize: ResponseSize = { encodedBodySize: 0, transferSize: 0, responseHeadersSize: 0 }; constructor(frame: frames.Frame, redirectedFrom: Request | null, documentId: string | undefined, url: string, resourceType: string, method: string, postData: Buffer | null, headers: types.HeadersArray) { @@ -131,8 +124,6 @@ export class Request extends SdkObject { _setFailureText(failureText: string) { this._failureText = failureText; this._waitForResponsePromise.resolve(null); - // If we didn't get raw headers, declare them equal to provisional. - this.setRawRequestHeaders(null); } url(): string { @@ -329,6 +320,7 @@ export type ResourceSizes = { requestHeadersSize: number, responseBodySize: number, responseHeadersSize: number, + transferSize: number, }; export type RemoteAddr = { @@ -360,6 +352,9 @@ export class Response extends SdkObject { private _rawResponseHeadersPromise = new ManualPromise(); private _httpVersion: string | undefined; private _fromServiceWorker: boolean; + private _encodedBodySizePromise = new ManualPromise(); + private _transferSizePromise = new ManualPromise(); + private _responseHeadersSizePromise = new ManualPromise(); constructor(request: Request, status: number, statusText: string, headers: types.HeadersArray, timing: ResourceTiming, getResponseBodyCallback: GetResponseBodyCallback, fromServiceWorker: boolean, httpVersion?: string) { super(request.frame(), 'response'); @@ -386,9 +381,6 @@ export class Response extends SdkObject { } _requestFinished(responseEndTiming: number) { - // If we didn't get raw headers, declare them equal to provisional. - this.setRawResponseHeaders(null); - this._request.setRawRequestHeaders(null); this._request._responseEndTiming = Math.max(responseEndTiming, this._timing.responseStart); this._finishedPromise.resolve(); } @@ -427,6 +419,18 @@ export class Response extends SdkObject { this._rawResponseHeadersPromise.resolve(headers || this._headers); } + setTransferSize(size: number | null) { + this._transferSizePromise.resolve(size); + } + + setEncodedBodySize(size: number | null) { + this._encodedBodySizePromise.resolve(size); + } + + setResponseHeadersSize(size: number | null) { + this._responseHeadersSizePromise.resolve(size); + } + timing(): ResourceTiming { return this._timing; } @@ -472,39 +476,47 @@ export class Response extends SdkObject { return this._fromServiceWorker; } - private async _responseHeadersSize(): Promise { - if (this._request.responseSize.responseHeadersSize) - return this._request.responseSize.responseHeadersSize; + async responseHeadersSize(): Promise { + const availableSize = await this._responseHeadersSizePromise; + if (availableSize !== null) + return availableSize; + + // Fallback to calculating it manually. let headersSize = 4; // 4 = 2 spaces + 2 line breaks (HTTP/1.1 200 Ok\r\n) headersSize += 8; // httpVersion; headersSize += 3; // statusCode; headersSize += this.statusText().length; - const headers = await this._bestEffortResponseHeaders(); + const headers = await this._rawResponseHeadersPromise; for (const header of headers) headersSize += header.name.length + header.value.length + 4; // 4 = ': ' + '\r\n' headersSize += 2; // '\r\n' return headersSize; } - private async _bestEffortResponseHeaders(): Promise { - return this._rawResponseHeadersPromise ? await this._rawResponseHeadersPromise : this._headers; - } - async sizes(): Promise { - await this._finishedPromise; const requestHeadersSize = await this._request.requestHeadersSize(); - const responseHeadersSize = await this._responseHeadersSize(); - let { encodedBodySize } = this._request.responseSize; - if (!encodedBodySize) { - const headers = await this._bestEffortResponseHeaders(); + const responseHeadersSize = await this.responseHeadersSize(); + + let encodedBodySize = await this._encodedBodySizePromise; + if (encodedBodySize === null) { + // Fallback to calculating it manually. + const headers = await this._rawResponseHeadersPromise; const contentLength = headers.find(h => h.name.toLowerCase() === 'content-length')?.value; encodedBodySize = contentLength ? +contentLength : 0; } + + let transferSize = await this._transferSizePromise; + if (transferSize === null) { + // Fallback to calculating it manually. + transferSize = responseHeadersSize + encodedBodySize; + } + return { requestBodySize: this._request.bodySize(), requestHeadersSize, responseBodySize: encodedBodySize, responseHeadersSize, + transferSize, }; } } diff --git a/packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts b/packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts index ffdd423a5d..c987b461b7 100644 --- a/packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts +++ b/packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts @@ -89,8 +89,21 @@ export class WKInterceptableRequest { }; const setCookieSeparator = process.platform === 'darwin' ? ',' : '\n'; const response = new network.Response(this.request, responsePayload.status, responsePayload.statusText, headersObjectToArray(responsePayload.headers, ',', setCookieSeparator), timing, getResponseBody, responsePayload.source === 'service-worker'); + // No raw response headers in WebKit, use "provisional" ones. response.setRawResponseHeaders(null); + // Transfer size is not available in WebKit. + response.setTransferSize(null); + + if (responsePayload.requestHeaders && Object.keys(responsePayload.requestHeaders).length) { + const headers = { ...responsePayload.requestHeaders }; + if (!headers['host']) + headers['Host'] = new URL(this.request.url()).host; + this.request.setRawRequestHeaders(headersObjectToArray(headers)); + } else { + // No raw headers avaialable, use provisional ones. + this.request.setRawRequestHeaders(null); + } return response; } } diff --git a/packages/playwright-core/src/server/webkit/wkPage.ts b/packages/playwright-core/src/server/webkit/wkPage.ts index cda372860f..2d8bc872a8 100644 --- a/packages/playwright-core/src/server/webkit/wkPage.ts +++ b/packages/playwright-core/src/server/webkit/wkPage.ts @@ -18,7 +18,7 @@ import path from 'path'; import { PNG, jpegjs } from '../../utilsBundle'; import { splitErrorMessage } from '../../utils/stackTrace'; -import { assert, createGuid, debugAssert, headersArrayToObject, headersObjectToArray } from '../../utils'; +import { assert, createGuid, debugAssert, headersArrayToObject } from '../../utils'; import { hostPlatform } from '../../utils/hostPlatform'; import type * as accessibility from '../accessibility'; import * as dialog from '../dialog'; @@ -1022,6 +1022,8 @@ export class WKPage implements PageDelegate { const response = request.createResponse(responsePayload); response._securityDetailsFinished(); response._serverAddrFinished(); + response.setResponseHeadersSize(null); + response.setEncodedBodySize(null); response._requestFinished(responsePayload.timing ? helper.secondsToRoundishMillis(timestamp - request._timestamp) : -1); this._requestIdToRequest.delete(request._requestId); this._page._frameManager.requestReceivedResponse(response); @@ -1053,15 +1055,6 @@ export class WKPage implements PageDelegate { return; this._requestIdToResponseReceivedPayloadEvent.set(request._requestId, event); const response = request.createResponse(event.response); - if (event.response.requestHeaders && Object.keys(event.response.requestHeaders).length) { - const headers = { ...event.response.requestHeaders }; - if (!headers['host']) - headers['Host'] = new URL(request.request.url()).host; - request.request.setRawRequestHeaders(headersObjectToArray(headers)); - } else { - // No raw headers avaialable, use provisional ones. - request.request.setRawRequestHeaders(null); - } this._page._frameManager.requestReceivedResponse(response); if (response.status() === 204) { @@ -1094,12 +1087,13 @@ export class WKPage implements PageDelegate { }); if (event.metrics?.protocol) response._setHttpVersion(event.metrics.protocol); - if (event.metrics?.responseBodyBytesReceived) - request.request.responseSize.encodedBodySize = event.metrics.responseBodyBytesReceived; - if (event.metrics?.responseHeaderBytesReceived) - request.request.responseSize.responseHeadersSize = event.metrics.responseHeaderBytesReceived; + response.setEncodedBodySize(event.metrics?.responseBodyBytesReceived ?? null); + response.setResponseHeadersSize(event.metrics?.responseHeaderBytesReceived ?? null); response._requestFinished(helper.secondsToRoundishMillis(event.timestamp - request._timestamp)); + } else { + // Use provisional headers if we didn't have the response with raw headers. + request.request.setRawRequestHeaders(null); } this._requestIdToResponseReceivedPayloadEvent.delete(request._requestId); @@ -1113,11 +1107,17 @@ export class WKPage implements PageDelegate { // @see https://crbug.com/750469 if (!request) return; + const response = request.request._existingResponse(); if (response) { response._serverAddrFinished(); response._securityDetailsFinished(); + response.setResponseHeadersSize(null); + response.setEncodedBodySize(null); response._requestFinished(helper.secondsToRoundishMillis(event.timestamp - request._timestamp)); + } else { + // Use provisional headers if we didn't have the response with raw headers. + request.request.setRawRequestHeaders(null); } this._requestIdToRequest.delete(request._requestId); request.request._setFailureText(event.errorText);