From 660516d22aea173c1fb9db8876629548d32b4958 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 24 Jun 2022 13:51:09 -0700 Subject: [PATCH] fix(network): make allHeaders wait until all headers are available (#15094) fix(network): make allHeaders wait until all header are available Before, calling `allHeaders()` from `page.on('request')` would yield provisional headers instead. With these changes: - In Firefox, all headers are available immediately. - In Chromium, all headers are available upon requestWillBeSentExtraInfo. - In WebKit, all headers are available upon responseReceived. - In all browsers, intercepted requests use "provisional" headers as all headers, since there is no network stack to change the headers. Drive-by: migrated Chromium to `hasExtraInfo` flags that simplifies the logic quite a bit. --- .../src/server/chromium/crNetworkManager.ts | 120 +++++------------- .../src/server/firefox/ffNetworkManager.ts | 4 + .../playwright-core/src/server/network.ts | 53 ++++---- .../server/webkit/wkInterceptableRequest.ts | 5 +- .../src/server/webkit/wkPage.ts | 6 + tests/page/page-network-request.spec.ts | 24 ++++ 6 files changed, 94 insertions(+), 118 deletions(-) diff --git a/packages/playwright-core/src/server/chromium/crNetworkManager.ts b/packages/playwright-core/src/server/chromium/crNetworkManager.ts index fa1d830ee5..ad8f394f7d 100644 --- a/packages/playwright-core/src/server/chromium/crNetworkManager.ts +++ b/packages/playwright-core/src/server/chromium/crNetworkManager.ts @@ -54,7 +54,6 @@ 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)), @@ -119,8 +118,6 @@ export class CRNetworkManager { } _onRequestWillBeSent(workerFrame: frames.Frame | undefined, event: Protocol.Network.requestWillBeSentPayload) { - this._responseExtraInfoTracker.requestWillBeSent(event); - // Request interception doesn't happen for data URLs with Network Service. if (this._protocolRequestInterceptionEnabled && !event.request.url.startsWith('data:')) { const requestId = event.requestId; @@ -136,10 +133,6 @@ export class CRNetworkManager { } } - _onRequestServedFromCache(event: Protocol.Network.requestServedFromCachePayload) { - this._responseExtraInfoTracker.requestServedFromCache(event); - } - _onRequestWillBeSentExtraInfo(event: Protocol.Network.requestWillBeSentExtraInfoPayload) { this._responseExtraInfoTracker.requestWillBeSentExtraInfo(event); } @@ -160,13 +153,6 @@ export class CRNetworkManager { } _onRequestPaused(workerFrame: frames.Frame | undefined, event: Protocol.Fetch.requestPausedPayload) { - if (!event.responseStatusCode && !event.responseErrorReason) { - // Request intercepted, deliver signal to the tracker. - const request = this._requestIdToRequest.get(event.networkId!); - if (request) - this._responseExtraInfoTracker.requestPaused(request.request, event); - } - if (!event.networkId) { // Fetch without networkId means that request was not recongnized by inspector, and // it will never receive Network.requestWillBeSent. Most likely, this is an internal request @@ -198,7 +184,7 @@ export class CRNetworkManager { const request = this._requestIdToRequest.get(requestWillBeSentEvent.requestId); // If we connect late to the target, we could have missed the requestWillBeSent event. if (request) { - this._handleRequestRedirect(request, requestWillBeSentEvent.redirectResponse, requestWillBeSentEvent.timestamp); + this._handleRequestRedirect(request, requestWillBeSentEvent.redirectResponse, requestWillBeSentEvent.timestamp, requestWillBeSentEvent.redirectHasExtraInfo); redirectedFrom = request; } } @@ -266,10 +252,16 @@ export class CRNetworkManager { redirectedFrom }); this._requestIdToRequest.set(requestWillBeSentEvent.requestId, request); + if (requestPausedEvent && !requestPausedEvent.responseStatusCode && !requestPausedEvent.responseErrorReason) { + // We will not receive extra info when intercepting the request. + // Use the headers from the Fetch.requestPausedPayload and release the allHeaders() + // right away, so that client can call it from the route handler. + request.request.setRawRequestHeaders(headersObjectToArray(requestPausedEvent.request.headers, '\n')); + } this._page._frameManager.requestStarted(request.request, route || undefined); } - _createResponse(request: InterceptableRequest, responsePayload: Protocol.Network.Response): network.Response { + _createResponse(request: InterceptableRequest, responsePayload: Protocol.Network.Response, hasExtraInfo: boolean): network.Response { const getResponseBody = async () => { const contentLengthHeader = Object.entries(responsePayload.headers).find(header => header[0].toLowerCase() === 'content-length'); const expectedLength = contentLengthHeader ? +contentLengthHeader[1] : undefined; @@ -332,12 +324,18 @@ export class CRNetworkManager { validFrom: responsePayload?.securityDetails?.validFrom, validTo: responsePayload?.securityDetails?.validTo, }); - this._responseExtraInfoTracker.processResponse(request._requestId, response, request.wasFulfilled()); + if (hasExtraInfo) { + this._responseExtraInfoTracker.processResponse(request._requestId, response); + } else { + // Use "provisional" headers as "raw" ones. + response.request().setRawRequestHeaders(null); + response.setRawResponseHeaders(null); + } return response; } - _handleRequestRedirect(request: InterceptableRequest, responsePayload: Protocol.Network.Response, timestamp: number) { - const response = this._createResponse(request, responsePayload); + _handleRequestRedirect(request: InterceptableRequest, responsePayload: Protocol.Network.Response, timestamp: number, hasExtraInfo: boolean) { + const response = this._createResponse(request, responsePayload, hasExtraInfo); response._requestFinished((timestamp - request._timestamp) * 1000); this._requestIdToRequest.delete(request._requestId); if (request._interceptionId) @@ -351,13 +349,11 @@ export class CRNetworkManager { } _onResponseReceived(event: Protocol.Network.responseReceivedPayload) { - this._responseExtraInfoTracker.responseReceived(event); - const request = this._requestIdToRequest.get(event.requestId); // FileUpload sends a response without a matching request. if (!request) return; - const response = this._createResponse(request, event.response); + const response = this._createResponse(request, event.response, event.hasExtraInfo); this._page._frameManager.requestReceivedResponse(response); } @@ -484,16 +480,11 @@ class InterceptableRequest { request = request._redirectedFrom; return request._route; } - - wasFulfilled() { - return this._routeForRedirectChain()?._wasFulfilled || false; - } } class RouteImpl implements network.RouteDelegate { private readonly _client: CRSession; private _interceptionId: string; - _wasFulfilled = false; constructor(client: CRSession, interceptionId: string) { this._client = client; @@ -513,7 +504,6 @@ class RouteImpl implements network.RouteDelegate { } async fulfill(response: types.NormalizedFulfillResponse) { - this._wasFulfilled = true; const body = response.isBase64 ? response.body : Buffer.from(response.body).toString('base64'); const responseHeaders = splitSetCookieHeader(response.headers); @@ -573,70 +563,39 @@ const errorReasons: { [reason: string]: Protocol.Network.ErrorReason } = { type RequestInfo = { requestId: string, - requestWillBeSentExtraInfo: Protocol.Network.requestWillBeSentExtraInfoPayload[], - responseReceivedExtraInfo: Protocol.Network.responseReceivedExtraInfoPayload[], + // Events are replaced with "undefined" to avoid updating the same headers twice. + requestWillBeSentExtraInfo: (Protocol.Network.requestWillBeSentExtraInfoPayload | undefined)[], + responseReceivedExtraInfo: (Protocol.Network.responseReceivedExtraInfoPayload | undefined)[], + // Note: we only put the responses that expect extra info in this list. + // Since the order of responses and extraInfo events is the same, each response + // will get a pair of matching request/response extraInfo events in this list. responses: network.Response[], loadingFinished?: Protocol.Network.loadingFinishedPayload, loadingFailed?: Protocol.Network.loadingFailedPayload, - sawResponseWithoutConnectionId: boolean - requestServedFromCache: boolean; }; // This class aligns responses with response headers from extra info: // - Network.requestWillBeSent, Network.responseReceived, Network.loadingFinished/loadingFailed are // dispatched using one channel. -// - Network.requestWillBeSentExtraInfo and Network.responseReceivedExtraInfo are dispatches on +// - Network.requestWillBeSentExtraInfo and Network.responseReceivedExtraInfo are dispatched on // another channel. Those channels are not associated, so events come in random order. // // This class will associate responses with the new headers. These extra info headers will become // available to client reliably upon requestfinished event only. It consumes CDP // signals on one end and processResponse(network.Response) signals on the other hands. It then makes -// sure that responses have all the extra headers in place by the time request finises. +// sure that responses have all the extra headers in place by the time request finishes. // // The shape of the instrumentation API is deliberately following the CDP, so that it -// what clear what is called when and what this means to the tracker without extra +// is clear what is called when and what this means to the tracker without extra // documentation. class ResponseExtraInfoTracker { private _requests = new Map(); - requestWillBeSent(event: Protocol.Network.requestWillBeSentPayload) { - const info = this._requests.get(event.requestId); - if (info && event.redirectResponse) - this._innerResponseReceived(info, event.redirectResponse); - else - this._getOrCreateEntry(event.requestId); - } - requestWillBeSentExtraInfo(event: Protocol.Network.requestWillBeSentExtraInfoPayload) { const info = this._getOrCreateEntry(event.requestId); info.requestWillBeSentExtraInfo.push(event); this._patchHeaders(info, info.requestWillBeSentExtraInfo.length - 1); - } - - requestServedFromCache(event: Protocol.Network.requestServedFromCachePayload) { - const info = this._getOrCreateEntry(event.requestId); - info.requestServedFromCache = true; - } - - responseReceived(event: Protocol.Network.responseReceivedPayload) { - const info = this._requests.get(event.requestId); - if (!info) - return; - this._innerResponseReceived(info, event.response); - } - - requestPaused(request: network.Request, event: Protocol.Fetch.requestPausedPayload) { - // requestWillBeSentExtraInfo is not being called when interception - // is enabled. But interception is mutually exclusive with the redirects. - // So we can use the headers from the Fetch.requestPausedPayload immediately. - request.setRawRequestHeaders(headersObjectToArray(event.request.headers, '\n')); - } - - private _innerResponseReceived(info: RequestInfo, response: Protocol.Network.Response) { - if (!response.connectionId) { - // Starting with this response we no longer can guarantee that response and extra info correspond to the same index. - info.sawResponseWithoutConnectionId = true; - } + this._checkFinished(info); } responseReceivedExtraInfo(event: Protocol.Network.responseReceivedExtraInfoPayload) { @@ -646,19 +605,8 @@ class ResponseExtraInfoTracker { this._checkFinished(info); } - processResponse(requestId: string, response: network.Response, wasFulfilled: boolean) { - // We are not interested in ExtraInfo tracking for fulfilled requests, our Blink - // headers are the ones that contain fulfilled headers. - if (wasFulfilled) { - this._stopTracking(requestId); - return; - } - - const info = this._requests.get(requestId); - if (!info || info.sawResponseWithoutConnectionId) - return; - if (!info.requestServedFromCache) - response.setWillReceiveExtraHeaders(); + processResponse(requestId: string, response: network.Response) { + const info = this._getOrCreateEntry(requestId); info.responses.push(response); this._patchHeaders(info, info.responses.length - 1); } @@ -687,8 +635,6 @@ class ResponseExtraInfoTracker { requestWillBeSentExtraInfo: [], responseReceivedExtraInfo: [], responses: [], - sawResponseWithoutConnectionId: false, - requestServedFromCache: false }; this._requests.set(requestId, info); } @@ -698,12 +644,15 @@ class ResponseExtraInfoTracker { private _patchHeaders(info: RequestInfo, index: number) { const response = info.responses[index]; const requestExtraInfo = info.requestWillBeSentExtraInfo[index]; - if (response && requestExtraInfo) + if (response && requestExtraInfo) { response.request().setRawRequestHeaders(headersObjectToArray(requestExtraInfo.headers, '\n')); + info.requestWillBeSentExtraInfo[index] = undefined; + } const responseExtraInfo = info.responseReceivedExtraInfo[index]; if (response && responseExtraInfo) { response.setRawResponseHeaders(headersObjectToArray(responseExtraInfo.headers, '\n')); response.request().responseSize.responseHeadersSize = responseExtraInfo.headersText?.length || 0; + info.responseReceivedExtraInfo[index] = undefined; } } @@ -713,7 +662,6 @@ class ResponseExtraInfoTracker { if (info.responses.length <= info.responseReceivedExtraInfo.length) { // We have extra info for each response. - // We could have more extra infos because we stopped collecting responses at some point. this._stopTracking(info.requestId); return; } diff --git a/packages/playwright-core/src/server/firefox/ffNetworkManager.ts b/packages/playwright-core/src/server/firefox/ffNetworkManager.ts index 5e6a1d16d8..3d4df80af6 100644 --- a/packages/playwright-core/src/server/firefox/ffNetworkManager.ts +++ b/packages/playwright-core/src/server/firefox/ffNetworkManager.ts @@ -113,6 +113,8 @@ export class FFNetworkManager { validFrom: event?.securityDetails?.validFrom, validTo: event?.securityDetails?.validTo, }); + // "raw" headers are the same as "provisional" headers in Firefox. + response.setRawResponseHeaders(null); this._page._frameManager.requestReceivedResponse(response); } @@ -194,6 +196,8 @@ class InterceptableRequest { postDataBuffer = Buffer.from(payload.postData, 'base64'); this.request = new network.Request(frame, redirectedFrom ? redirectedFrom.request : null, payload.navigationId, payload.url, internalCauseToResourceType[payload.internalCause] || causeToResourceType[payload.cause] || 'other', payload.method, postDataBuffer, payload.headers); + // "raw" headers are the same as "provisional" headers in Firefox. + this.request.setRawRequestHeaders(null); } _finalRequest(): InterceptableRequest { diff --git a/packages/playwright-core/src/server/network.ts b/packages/playwright-core/src/server/network.ts index c5edcd15d9..1995a31571 100644 --- a/packages/playwright-core/src/server/network.ts +++ b/packages/playwright-core/src/server/network.ts @@ -103,7 +103,7 @@ export class Request extends SdkObject { private _postData: Buffer | null; readonly _headers: types.HeadersArray; private _headersMap = new Map(); - private _rawRequestHeadersPromise: ManualPromise | undefined; + private _rawRequestHeadersPromise = new ManualPromise(); private _frame: frames.Frame; private _waitForResponsePromise = new ManualPromise(); _responseEndTiming = -1; @@ -131,6 +131,8 @@ 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 { @@ -157,22 +159,13 @@ export class Request extends SdkObject { return this._headersMap.get(name); } - setWillReceiveExtraHeaders() { - if (!this._rawRequestHeadersPromise) - this._rawRequestHeadersPromise = new ManualPromise(); - } - - setRawRequestHeaders(headers: types.HeadersArray) { - if (!this._rawRequestHeadersPromise) - this._rawRequestHeadersPromise = new ManualPromise(); - this._rawRequestHeadersPromise!.resolve(headers); + // "null" means no raw headers available - we'll use provisional headers as raw headers. + setRawRequestHeaders(headers: types.HeadersArray | null) { + if (!this._rawRequestHeadersPromise.isDone()) + this._rawRequestHeadersPromise.resolve(headers || this._headers); } async rawRequestHeaders(): Promise { - return this._rawRequestHeadersPromise || Promise.resolve(this._headers); - } - - rawRequestHeadersPromise(): Promise | undefined { return this._rawRequestHeadersPromise; } @@ -222,7 +215,7 @@ export class Request extends SdkObject { headersSize += this.method().length; headersSize += (new URL(this.url())).pathname.length; headersSize += 8; // httpVersion - const headers = this.rawRequestHeadersPromise() ? await this.rawRequestHeadersPromise()! : this._headers; + const headers = await this.rawRequestHeaders(); for (const header of headers) headersSize += header.name.length + header.value.length + 4; // 4 = ': ' + '\r\n' return headersSize; @@ -344,11 +337,11 @@ export type RemoteAddr = { }; export type SecurityDetails = { - protocol?: string; - subjectName?: string; - issuer?: string; - validFrom?: number; - validTo?: number; + protocol?: string; + subjectName?: string; + issuer?: string; + validFrom?: number; + validTo?: number; }; export class Response extends SdkObject { @@ -364,7 +357,7 @@ export class Response extends SdkObject { private _timing: ResourceTiming; private _serverAddrPromise = new ManualPromise(); private _securityDetailsPromise = new ManualPromise(); - private _rawResponseHeadersPromise: ManualPromise | undefined; + private _rawResponseHeadersPromise = new ManualPromise(); private _httpVersion: string | undefined; private _fromServiceWorker: boolean; @@ -393,6 +386,9 @@ 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(); } @@ -422,18 +418,13 @@ export class Response extends SdkObject { } async rawResponseHeaders(): Promise { - return this._rawResponseHeadersPromise || Promise.resolve(this._headers); + return this._rawResponseHeadersPromise; } - setWillReceiveExtraHeaders() { - this._request.setWillReceiveExtraHeaders(); - this._rawResponseHeadersPromise = new ManualPromise(); - } - - setRawResponseHeaders(headers: types.HeadersArray) { - if (!this._rawResponseHeadersPromise) - this._rawResponseHeadersPromise = new ManualPromise(); - this._rawResponseHeadersPromise!.resolve(headers); + // "null" means no raw headers available - we'll use provisional headers as raw headers. + setRawResponseHeaders(headers: types.HeadersArray | null) { + if (!this._rawResponseHeadersPromise.isDone()) + this._rawResponseHeadersPromise.resolve(headers || this._headers); } timing(): ResourceTiming { diff --git a/packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts b/packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts index 9cd3db1f3d..ffdd423a5d 100644 --- a/packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts +++ b/packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts @@ -88,7 +88,10 @@ export class WKInterceptableRequest { responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1, }; const setCookieSeparator = process.platform === 'darwin' ? ',' : '\n'; - return new network.Response(this.request, responsePayload.status, responsePayload.statusText, headersObjectToArray(responsePayload.headers, ',', setCookieSeparator), timing, getResponseBody, responsePayload.source === 'service-worker'); + 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); + return response; } } diff --git a/packages/playwright-core/src/server/webkit/wkPage.ts b/packages/playwright-core/src/server/webkit/wkPage.ts index e40beff66d..cda372860f 100644 --- a/packages/playwright-core/src/server/webkit/wkPage.ts +++ b/packages/playwright-core/src/server/webkit/wkPage.ts @@ -1034,6 +1034,9 @@ export class WKPage implements PageDelegate { session.sendMayFail('Network.interceptRequestWithError', { errorType: 'Cancellation', requestId: event.requestId }); return; } + // There is no point in waiting for the raw headers in Network.responseReceived when intercepting. + // Use provisional headers as raw headers, so that client can call allHeaders() from the route handler. + request.request.setRawRequestHeaders(null); if (!request._route) { // Intercepted, although we do not intend to allow interception. // Just continue. @@ -1055,6 +1058,9 @@ export class WKPage implements PageDelegate { 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); diff --git a/tests/page/page-network-request.spec.ts b/tests/page/page-network-request.spec.ts index 2e9e91c4bf..fccdb77a48 100644 --- a/tests/page/page-network-request.spec.ts +++ b/tests/page/page-network-request.spec.ts @@ -93,6 +93,30 @@ it('should get the same headers as the server', async ({ page, server, browserNa expect(headers).toEqual(serverRequest.headers); }); +it('should not return allHeaders() until they are available', async ({ page, server, browserName, platform }) => { + it.fail(browserName === 'webkit' && platform === 'win32', 'Curl does not show accept-encoding and accept-language'); + + let requestHeadersPromise; + page.on('request', request => requestHeadersPromise = request.allHeaders()); + let responseHeadersPromise; + page.on('response', response => responseHeadersPromise = response.allHeaders()); + + let serverRequest; + server.setRoute('/empty.html', async (request, response) => { + serverRequest = request; + response.writeHead(200, { 'foo': 'bar' }); + await new Promise(f => setTimeout(f, 3000)); + response.end('done'); + }); + + await page.goto(server.PREFIX + '/empty.html'); + const requestHeaders = await requestHeadersPromise; + expect(requestHeaders).toEqual(serverRequest.headers); + + const responseHeaders = await responseHeadersPromise; + expect(responseHeaders['foo']).toBe('bar'); +}); + it('should get the same headers as the server CORS', async ({ page, server, browserName, platform }) => { it.fail(browserName === 'webkit' && platform === 'win32', 'Curl does not show accept-encoding and accept-language');