From a19bd95fff8d2e855b5ce542ea1df55e292b688f Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Thu, 7 Dec 2023 13:19:15 -0800 Subject: [PATCH 1/3] fix: throw if fuliflled with unsupported status code --- .../src/server/chromium/crNetworkManager.ts | 8 +--- .../playwright-core/src/server/network.ts | 39 ++++++++++++---- tests/page/page-request-continue.spec.ts | 26 +++++++++++ tests/page/page-request-fulfill.spec.ts | 46 +++++++++++++++++++ tests/page/page-route.spec.ts | 25 ++++++++++ 5 files changed, 130 insertions(+), 14 deletions(-) diff --git a/packages/playwright-core/src/server/chromium/crNetworkManager.ts b/packages/playwright-core/src/server/chromium/crNetworkManager.ts index a079bd409e1b3..ac22de36344f0 100644 --- a/packages/playwright-core/src/server/chromium/crNetworkManager.ts +++ b/packages/playwright-core/src/server/chromium/crNetworkManager.ts @@ -563,18 +563,14 @@ class RouteImpl implements network.RouteDelegate { method: overrides.method, postData: overrides.postData ? overrides.postData.toString('base64') : undefined }; - // 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.continueRequest', this._alreadyContinuedParams); + 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); - // 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.fulfillRequest', { + await this._session.send('Fetch.fulfillRequest', { requestId: this._interceptionId!, responseCode: response.status, responsePhrase: network.STATUS_TEXTS[String(response.status)], diff --git a/packages/playwright-core/src/server/network.ts b/packages/playwright-core/src/server/network.ts index fdf0573ac3683..e4ffb5e7134e0 100644 --- a/packages/playwright-core/src/server/network.ts +++ b/packages/playwright-core/src/server/network.ts @@ -258,7 +258,13 @@ export class Route extends SdkObject { async abort(errorCode: string = 'failed') { this._startHandling(); this._request._context.emit(BrowserContext.Events.RequestAborted, this._request); - await this._delegate.abort(errorCode); + 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 error and throw it here. + this._waitForFailure() + ]); + this._endHandling(); } @@ -286,15 +292,26 @@ export class Route extends SdkObject { const headers = [...(overrides.headers || [])]; this._maybeAddCorsHeaders(headers); this._request._context.emit(BrowserContext.Events.RequestFulfilled, this._request); - await this._delegate.fulfill({ - status: overrides.status || 200, - headers, - body, - isBase64, - }); + 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 error and throw it here. + this._waitForFailure() + ]); this._endHandling(); } + private async _waitForFailure() { + const response = await this._request.response(); + if (!response) + throw new Error(this._request._failureText || 'Request was cancelled'); + } + // See https://github.com/microsoft/playwright/issues/12929 private _maybeAddCorsHeaders(headers: NameValue[]) { const origin = this._request.headerValue('origin'); @@ -324,7 +341,13 @@ export class Route extends SdkObject { this._request._setOverrides(overrides); if (!overrides.isFallback) this._request._context.emit(BrowserContext.Events.RequestContinued, this._request); - await this._delegate.continue(this._request, overrides); + 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 error and throw it here. + this._waitForFailure() + ]); + this._endHandling(); } diff --git a/tests/page/page-request-continue.spec.ts b/tests/page/page-request-continue.spec.ts index 56a58ec63066d..0eef6de86ccf6 100644 --- a/tests/page/page-request-continue.spec.ts +++ b/tests/page/page-request-continue.spec.ts @@ -16,6 +16,7 @@ */ import { test as it, expect } from './pageTest'; +import type { Route } from 'playwright-core'; it('should work', async ({ page, server }) => { await page.route('**/*', route => route.continue()); @@ -142,6 +143,31 @@ it('should not throw when continuing after page is closed', async ({ page, serve expect(error).toBeInstanceOf(Error); }); +it('should throw if request was cancelled by the page', async ({ page, server, browserName }) => { + let interceptCallback; + const interceptPromise = new Promise(f => interceptCallback = f); + await page.route('**/data.json', route => interceptCallback(route), { noWaitForFinish: true }); + await page.goto(server.EMPTY_PAGE); + page.evaluate(url => { + globalThis.controller = new AbortController(); + return fetch(url, { signal: globalThis.controller.signal }); + }, server.PREFIX + '/data.json').catch(() => {}); + const route = await interceptPromise; + const failurePromise = page.waitForEvent('requestfailed'); + await page.evaluate(() => globalThis.controller.abort()); + const cancelledRequest = await failurePromise; + console.log('cancelledRequest', cancelledRequest.failure()); + let error; + if (browserName === 'chromium') + error = 'net::ERR_ABORTED'; + else if (browserName === 'webkit') + error = 'cancelled'; + else if (browserName === 'firefox') + error = 'NS_BINDING_ABORTED'; + expect(cancelledRequest.failure().errorText).toBe(error); + await expect(route.continue()).rejects.toThrow(new RegExp(error)); +}); + it('should override method along with url', async ({ page, server }) => { const request = server.waitForRequest('/empty.html'); await page.route('**/foo', route => { diff --git a/tests/page/page-request-fulfill.spec.ts b/tests/page/page-request-fulfill.spec.ts index 376b03d69cbdb..f5e8342c7da63 100644 --- a/tests/page/page-request-fulfill.spec.ts +++ b/tests/page/page-request-fulfill.spec.ts @@ -18,6 +18,7 @@ import { test as base, expect } from './pageTest'; import fs from 'fs'; import type * as har from '../../packages/trace/src/har'; +import type { Route } from 'playwright-core'; const it = base.extend<{ // We access test servers at 10.0.2.2 from inside the browser on Android, @@ -77,6 +78,51 @@ it('should work with status code 422', async ({ page, server }) => { expect(await page.evaluate(() => document.body.textContent)).toBe('Yo, page!'); }); +it('should throw exception if status code is not supported', async ({ page, server, browserName }) => { + let fulfillPromiseCallback; + const fulfillPromise = new Promise(f => fulfillPromiseCallback = f); + await page.route('**/data.json', route => { + fulfillPromiseCallback(route.fulfill({ + status: 430, + body: 'Yo, page!' + }).catch(e => e)); + }); + await page.goto(server.EMPTY_PAGE); + page.evaluate(url => fetch(url), server.PREFIX + '/data.json').catch(() => {}); + const error = await fulfillPromise; + if (browserName === 'chromium') { + expect(error).toBeTruthy(); + expect(error.message).toContain(' Invalid http status code or phrase'); + } else { + expect(error).toBe(undefined); + } +}); + +it('should throw if request was cancelled by the page', async ({ page, server, browserName }) => { + let interceptCallback; + const interceptPromise = new Promise(f => interceptCallback = f); + await page.route('**/data.json', route => interceptCallback(route), { noWaitForFinish: true }); + await page.goto(server.EMPTY_PAGE); + page.evaluate(url => { + globalThis.controller = new AbortController(); + return fetch(url, { signal: globalThis.controller.signal }); + }, server.PREFIX + '/data.json').catch(() => {}); + const route = await interceptPromise; + const failurePromise = page.waitForEvent('requestfailed'); + await page.evaluate(() => globalThis.controller.abort()); + const cancelledRequest = await failurePromise; + console.log('cancelledRequest', cancelledRequest.failure()); + let error; + if (browserName === 'chromium') + error = 'net::ERR_ABORTED'; + else if (browserName === 'webkit') + error = 'cancelled'; + else if (browserName === 'firefox') + error = 'NS_BINDING_ABORTED'; + expect(cancelledRequest.failure().errorText).toBe(error); + await expect(route.fulfill({ status: 200 })).rejects.toThrow(new RegExp(error)); +}); + it('should allow mocking binary responses', async ({ page, server, browserName, headless, asset, isAndroid, mode }) => { it.skip(browserName === 'firefox' && !headless, 'Firefox headed produces a different image.'); it.skip(isAndroid); diff --git a/tests/page/page-route.spec.ts b/tests/page/page-route.spec.ts index d1757ce1bebc8..a6b97ef1bc8a6 100644 --- a/tests/page/page-route.spec.ts +++ b/tests/page/page-route.spec.ts @@ -375,6 +375,31 @@ it('should be abortable with custom error codes', async ({ page, server, browser expect(failedRequest.failure().errorText).toBe('net::ERR_INTERNET_DISCONNECTED'); }); +it('should throw if request was cancelled by the page', async ({ page, server, browserName }) => { + let interceptCallback; + const interceptPromise = new Promise(f => interceptCallback = f); + await page.route('**/data.json', route => interceptCallback(route), { noWaitForFinish: true }); + await page.goto(server.EMPTY_PAGE); + page.evaluate(url => { + globalThis.controller = new AbortController(); + return fetch(url, { signal: globalThis.controller.signal }); + }, server.PREFIX + '/data.json').catch(() => {}); + const route = await interceptPromise; + // const failurePromise = page.waitForEvent('requestfailed'); + // await page.evaluate(() => globalThis.controller.abort()); + // const cancelledRequest = await failurePromise; + // console.log('cancelledRequest', cancelledRequest.failure()); + let error; + if (browserName === 'chromium') + error = 'net::ERR_ABORTED'; + else if (browserName === 'webkit') + error = 'cancelled'; + else if (browserName === 'firefox') + error = 'NS_BINDING_ABORTED'; + // expect(cancelledRequest.failure().errorText).toBe(error); + await expect(route.abort()).rejects.toThrow(new RegExp(error)); +}); + it('should send referer', async ({ page, server }) => { await page.setExtraHTTPHeaders({ referer: 'http://google.com/' From 0fb4ed5e45fa0fc9f0aa178fc676a865e5188f79 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Thu, 7 Dec 2023 15:11:45 -0800 Subject: [PATCH 2/3] Silently catch request cancelled errors --- .../playwright-core/src/server/network.ts | 18 ++++++---------- tests/page/page-request-continue.spec.ts | 17 +++++---------- tests/page/page-request-fulfill.spec.ts | 17 ++++++--------- tests/page/page-route.spec.ts | 21 +++++++------------ 4 files changed, 24 insertions(+), 49 deletions(-) diff --git a/packages/playwright-core/src/server/network.ts b/packages/playwright-core/src/server/network.ts index e4ffb5e7134e0..8f2f5c475d02f 100644 --- a/packages/playwright-core/src/server/network.ts +++ b/packages/playwright-core/src/server/network.ts @@ -261,8 +261,8 @@ export class Route extends SdkObject { 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 error and throw it here. - this._waitForFailure() + // we'll receive loading failed event and will ignore route handling error. + this._request.response() ]); this._endHandling(); @@ -300,18 +300,12 @@ export class Route extends SdkObject { isBase64, }), // If the request is already cancelled by the page before we handle the route, - // we'll receive loading failed error and throw it here. - this._waitForFailure() + // we'll receive loading failed event and will ignore route handling error. + this._request.response() ]); this._endHandling(); } - private async _waitForFailure() { - const response = await this._request.response(); - if (!response) - throw new Error(this._request._failureText || 'Request was cancelled'); - } - // See https://github.com/microsoft/playwright/issues/12929 private _maybeAddCorsHeaders(headers: NameValue[]) { const origin = this._request.headerValue('origin'); @@ -344,8 +338,8 @@ export class Route extends SdkObject { 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 error and throw it here. - this._waitForFailure() + // we'll receive loading failed event and will ignore route handling error. + this._request.response() ]); this._endHandling(); diff --git a/tests/page/page-request-continue.spec.ts b/tests/page/page-request-continue.spec.ts index 0eef6de86ccf6..57e7ce3926cc9 100644 --- a/tests/page/page-request-continue.spec.ts +++ b/tests/page/page-request-continue.spec.ts @@ -143,10 +143,11 @@ it('should not throw when continuing after page is closed', async ({ page, serve expect(error).toBeInstanceOf(Error); }); -it('should throw if request was cancelled by the page', async ({ page, server, browserName }) => { +it('should not throw if request was cancelled by the page', async ({ page, server, browserName }) => { + it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/28490' }); let interceptCallback; const interceptPromise = new Promise(f => interceptCallback = f); - await page.route('**/data.json', route => interceptCallback(route), { noWaitForFinish: true }); + await page.route('**/data.json', route => interceptCallback(route)); await page.goto(server.EMPTY_PAGE); page.evaluate(url => { globalThis.controller = new AbortController(); @@ -156,16 +157,8 @@ it('should throw if request was cancelled by the page', async ({ page, server, b const failurePromise = page.waitForEvent('requestfailed'); await page.evaluate(() => globalThis.controller.abort()); const cancelledRequest = await failurePromise; - console.log('cancelledRequest', cancelledRequest.failure()); - let error; - if (browserName === 'chromium') - error = 'net::ERR_ABORTED'; - else if (browserName === 'webkit') - error = 'cancelled'; - else if (browserName === 'firefox') - error = 'NS_BINDING_ABORTED'; - expect(cancelledRequest.failure().errorText).toBe(error); - await expect(route.continue()).rejects.toThrow(new RegExp(error)); + expect(cancelledRequest.failure().errorText).toMatch(/cancelled|aborted/i); + await route.continue(); // Should not throw. }); it('should override method along with url', async ({ page, server }) => { diff --git a/tests/page/page-request-fulfill.spec.ts b/tests/page/page-request-fulfill.spec.ts index f5e8342c7da63..f4401dab109e2 100644 --- a/tests/page/page-request-fulfill.spec.ts +++ b/tests/page/page-request-fulfill.spec.ts @@ -79,6 +79,7 @@ it('should work with status code 422', async ({ page, server }) => { }); it('should throw exception if status code is not supported', async ({ page, server, browserName }) => { + it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/28490' }); let fulfillPromiseCallback; const fulfillPromise = new Promise(f => fulfillPromiseCallback = f); await page.route('**/data.json', route => { @@ -98,7 +99,8 @@ it('should throw exception if status code is not supported', async ({ page, serv } }); -it('should throw if request was cancelled by the page', async ({ page, server, browserName }) => { +it('should not throw if request was cancelled by the page', async ({ page, server }) => { + it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/28490' }); let interceptCallback; const interceptPromise = new Promise(f => interceptCallback = f); await page.route('**/data.json', route => interceptCallback(route), { noWaitForFinish: true }); @@ -111,16 +113,9 @@ it('should throw if request was cancelled by the page', async ({ page, server, b const failurePromise = page.waitForEvent('requestfailed'); await page.evaluate(() => globalThis.controller.abort()); const cancelledRequest = await failurePromise; - console.log('cancelledRequest', cancelledRequest.failure()); - let error; - if (browserName === 'chromium') - error = 'net::ERR_ABORTED'; - else if (browserName === 'webkit') - error = 'cancelled'; - else if (browserName === 'firefox') - error = 'NS_BINDING_ABORTED'; - expect(cancelledRequest.failure().errorText).toBe(error); - await expect(route.fulfill({ status: 200 })).rejects.toThrow(new RegExp(error)); + expect(cancelledRequest.failure()).toBeTruthy(); + expect(cancelledRequest.failure().errorText).toMatch(/cancelled|aborted/i); + await route.fulfill({ status: 200 }); // Should not throw. }); it('should allow mocking binary responses', async ({ page, server, browserName, headless, asset, isAndroid, mode }) => { diff --git a/tests/page/page-route.spec.ts b/tests/page/page-route.spec.ts index a6b97ef1bc8a6..578d763914173 100644 --- a/tests/page/page-route.spec.ts +++ b/tests/page/page-route.spec.ts @@ -375,7 +375,8 @@ it('should be abortable with custom error codes', async ({ page, server, browser expect(failedRequest.failure().errorText).toBe('net::ERR_INTERNET_DISCONNECTED'); }); -it('should throw if request was cancelled by the page', async ({ page, server, browserName }) => { +it('should not throw if request was cancelled by the page', async ({ page, server }) => { + it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/28490' }); let interceptCallback; const interceptPromise = new Promise(f => interceptCallback = f); await page.route('**/data.json', route => interceptCallback(route), { noWaitForFinish: true }); @@ -385,19 +386,11 @@ it('should throw if request was cancelled by the page', async ({ page, server, b return fetch(url, { signal: globalThis.controller.signal }); }, server.PREFIX + '/data.json').catch(() => {}); const route = await interceptPromise; - // const failurePromise = page.waitForEvent('requestfailed'); - // await page.evaluate(() => globalThis.controller.abort()); - // const cancelledRequest = await failurePromise; - // console.log('cancelledRequest', cancelledRequest.failure()); - let error; - if (browserName === 'chromium') - error = 'net::ERR_ABORTED'; - else if (browserName === 'webkit') - error = 'cancelled'; - else if (browserName === 'firefox') - error = 'NS_BINDING_ABORTED'; - // expect(cancelledRequest.failure().errorText).toBe(error); - await expect(route.abort()).rejects.toThrow(new RegExp(error)); + const failurePromise = page.waitForEvent('requestfailed'); + await page.evaluate(() => globalThis.controller.abort()); + const cancelledRequest = await failurePromise; + expect(cancelledRequest.failure().errorText).toMatch(/cancelled|aborted/i); + await route.abort(); // Should not throw. }); it('should send referer', async ({ page, server }) => { From b54763c39f5eb82e57c9b9f8e3af12e8b057b9f8 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Thu, 7 Dec 2023 16:24:02 -0800 Subject: [PATCH 3/3] Address comment --- packages/playwright-core/src/server/network.ts | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/playwright-core/src/server/network.ts b/packages/playwright-core/src/server/network.ts index 8f2f5c475d02f..dc2fa2ea3c01d 100644 --- a/packages/playwright-core/src/server/network.ts +++ b/packages/playwright-core/src/server/network.ts @@ -134,6 +134,18 @@ 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(); @@ -262,7 +274,7 @@ export class Route extends SdkObject { 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.response() + this._request._waitForRequestFailure() ]); this._endHandling(); @@ -301,7 +313,7 @@ export class Route extends SdkObject { }), // 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.response() + this._request._waitForRequestFailure() ]); this._endHandling(); } @@ -339,7 +351,7 @@ export class Route extends SdkObject { 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.response() + this._request._waitForRequestFailure() ]); this._endHandling();