Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: throw if fuliflled with unsupported status code #28539

Merged
merged 3 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)],
Expand Down
33 changes: 25 additions & 8 deletions packages/playwright-core/src/server/network.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 event and will ignore route handling error.
this._request.response()
]);

this._endHandling();
}

Expand Down Expand Up @@ -286,12 +292,17 @@ 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 event and will ignore route handling error.
this._request.response()
yury-s marked this conversation as resolved.
Show resolved Hide resolved
]);
this._endHandling();
}

Expand Down Expand Up @@ -324,7 +335,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 event and will ignore route handling error.
this._request.response()
]);

this._endHandling();
}

Expand Down
19 changes: 19 additions & 0 deletions tests/page/page-request-continue.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -142,6 +143,24 @@ it('should not throw when continuing after page is closed', async ({ page, serve
expect(error).toBeInstanceOf(Error);
});

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<Route>(f => interceptCallback = f);
await page.route('**/data.json', route => interceptCallback(route));
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;
expect(cancelledRequest.failure().errorText).toMatch(/cancelled|aborted/i);
await route.continue(); // Should not throw.
});

it('should override method along with url', async ({ page, server }) => {
const request = server.waitForRequest('/empty.html');
await page.route('**/foo', route => {
Expand Down
41 changes: 41 additions & 0 deletions tests/page/page-request-fulfill.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -77,6 +78,46 @@ 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 }) => {
it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/28490' });
let fulfillPromiseCallback;
const fulfillPromise = new Promise<Error|undefined>(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 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<Route>(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;
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 }) => {
it.skip(browserName === 'firefox' && !headless, 'Firefox headed produces a different image.');
it.skip(isAndroid);
Expand Down
18 changes: 18 additions & 0 deletions tests/page/page-route.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,24 @@ it('should be abortable with custom error codes', async ({ page, server, browser
expect(failedRequest.failure().errorText).toBe('net::ERR_INTERNET_DISCONNECTED');
});

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<Route>(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;
expect(cancelledRequest.failure().errorText).toMatch(/cancelled|aborted/i);
await route.abort(); // Should not throw.
});

it('should send referer', async ({ page, server }) => {
await page.setExtraHTTPHeaders({
referer: 'http://google.com/'
Expand Down