From 82981a80919c4c30aee3accb06c07a2f8869c145 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Thu, 25 Jan 2024 16:54:42 -0800 Subject: [PATCH] fix: interception id not found error in route.continue (#29180) We stopped catching all exceptions in https://github.com/microsoft/playwright/pull/28539 in hope that we'll get loadingFailed even before Fetch.continue/fulfill command's error. Turns out this is racy and may fail if the test cancels the request while we are continuing it. The following test could in theory reproduce it if stars align and the timing is good: ```js it('page.continue on canceled request', async ({ page }) => { let resolveRoute; const routePromise = new Promise(f => resolveRoute = f); await page.route('http://test.com/x', resolveRoute); const evalPromise = page.evaluate(async () => { const abortController = new AbortController(); (window as any).abortController = abortController; return fetch('http://test.com/x', { signal: abortController.signal }).catch(e => 'cancelled'); }); const route = await routePromise; void page.evaluate(() => (window as any).abortController.abort()); await new Promise(f => setTimeout(f, 10)); await route.continue(); const req = await evalPromise; expect(req).toBe('cancelled'); }); ``` Fixes https://github.com/microsoft/playwright/issues/29123 --- .../src/server/chromium/crNetworkManager.ts | 41 ++++++++++++----- .../playwright-core/src/server/network.ts | 45 ++++--------------- 2 files changed, 37 insertions(+), 49 deletions(-) diff --git a/packages/playwright-core/src/server/chromium/crNetworkManager.ts b/packages/playwright-core/src/server/chromium/crNetworkManager.ts index ad07d24eacd65..af9cb8fc82dee 100644 --- a/packages/playwright-core/src/server/chromium/crNetworkManager.ts +++ b/packages/playwright-core/src/server/chromium/crNetworkManager.ts @@ -28,6 +28,7 @@ import type * as types from '../types'; import type { CRPage } from './crPage'; import { assert, headersObjectToArray } from '../../utils'; import type { CRServiceWorker } from './crServiceWorker'; +import { isProtocolError } from '../protocolError'; type SessionInfo = { session: CRSession; @@ -571,34 +572,50 @@ class RouteImpl implements network.RouteDelegate { method: overrides.method, postData: overrides.postData ? overrides.postData.toString('base64') : undefined }; - await this._session.send('Fetch.continueRequest', this._alreadyContinuedParams); + await catchDisallowedErrors(async () => { + await this._session.send('Fetch.continueRequest', this._alreadyContinuedParams); + }); } async fulfill(response: types.NormalizedFulfillResponse) { const body = response.isBase64 ? response.body : Buffer.from(response.body).toString('base64'); const responseHeaders = splitSetCookieHeader(response.headers); - await this._session.send('Fetch.fulfillRequest', { - requestId: this._interceptionId!, - responseCode: response.status, - responsePhrase: network.STATUS_TEXTS[String(response.status)], - responseHeaders, - body, + await catchDisallowedErrors(async () => { + await this._session.send('Fetch.fulfillRequest', { + requestId: this._interceptionId!, + responseCode: response.status, + responsePhrase: network.STATUS_TEXTS[String(response.status)], + responseHeaders, + body, + }); }); } async abort(errorCode: string = 'failed') { const errorReason = errorReasons[errorCode]; assert(errorReason, 'Unknown error code: ' + errorCode); - // In certain cases, protocol will return error if the request was already canceled - // or the page was closed. We should tolerate these errors. - await this._session._sendMayFail('Fetch.failRequest', { - requestId: this._interceptionId!, - errorReason + await catchDisallowedErrors(async () => { + await this._session.send('Fetch.failRequest', { + requestId: this._interceptionId!, + errorReason + }); }); } } +// In certain cases, protocol will return error if the request was already canceled +// or the page was closed. We should tolerate these errors but propagate other. +async function catchDisallowedErrors(callback: () => Promise) { + try { + return await callback(); + } catch (e) { + if (isProtocolError(e) && e.message.includes('Invalid http status code or phrase')) + throw e; + } +} + + function splitSetCookieHeader(headers: types.HeadersArray): types.HeadersArray { const index = headers.findIndex(({ name }) => name.toLowerCase() === 'set-cookie'); if (index === -1) diff --git a/packages/playwright-core/src/server/network.ts b/packages/playwright-core/src/server/network.ts index dc2fa2ea3c01d..83b0093a3886e 100644 --- a/packages/playwright-core/src/server/network.ts +++ b/packages/playwright-core/src/server/network.ts @@ -134,18 +134,6 @@ export class Request extends SdkObject { this._waitForResponsePromise.resolve(null); } - async _waitForRequestFailure() { - const response = await this._waitForResponsePromise; - // If response is null it was a failure an we are done. - if (!response) - return; - await response._finishedPromise; - if (this.failure()) - return; - // If request finished without errors, we stall. - await new Promise(() => {}); - } - _setOverrides(overrides: types.NormalizedContinueOverrides) { this._overrides = overrides; this._updateHeadersMap(); @@ -270,13 +258,7 @@ export class Route extends SdkObject { async abort(errorCode: string = 'failed') { this._startHandling(); this._request._context.emit(BrowserContext.Events.RequestAborted, this._request); - await Promise.race([ - this._delegate.abort(errorCode), - // If the request is already cancelled by the page before we handle the route, - // we'll receive loading failed event and will ignore route handling error. - this._request._waitForRequestFailure() - ]); - + await this._delegate.abort(errorCode); this._endHandling(); } @@ -304,17 +286,12 @@ export class Route extends SdkObject { const headers = [...(overrides.headers || [])]; this._maybeAddCorsHeaders(headers); this._request._context.emit(BrowserContext.Events.RequestFulfilled, this._request); - await Promise.race([ - this._delegate.fulfill({ - status: overrides.status || 200, - headers, - body, - isBase64, - }), - // If the request is already cancelled by the page before we handle the route, - // we'll receive loading failed event and will ignore route handling error. - this._request._waitForRequestFailure() - ]); + await this._delegate.fulfill({ + status: overrides.status || 200, + headers, + body: body!, + isBase64, + }); this._endHandling(); } @@ -347,13 +324,7 @@ export class Route extends SdkObject { this._request._setOverrides(overrides); if (!overrides.isFallback) this._request._context.emit(BrowserContext.Events.RequestContinued, this._request); - await Promise.race([ - this._delegate.continue(this._request, overrides), - // If the request is already cancelled by the page before we handle the route, - // we'll receive loading failed event and will ignore route handling error. - this._request._waitForRequestFailure() - ]); - + await this._delegate.continue(this._request, overrides); this._endHandling(); }