Skip to content

Commit 7a72adc

Browse files
authored
cherry-pick(#29180): fix: interception id not found error in route.co… (#29222)
…ntinue We stopped catching all exceptions in #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<Route>(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 #29123
1 parent 8f0163f commit 7a72adc

File tree

2 files changed

+37
-49
lines changed

2 files changed

+37
-49
lines changed

packages/playwright-core/src/server/chromium/crNetworkManager.ts

+29-12
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import type * as types from '../types';
2828
import type { CRPage } from './crPage';
2929
import { assert, headersObjectToArray } from '../../utils';
3030
import type { CRServiceWorker } from './crServiceWorker';
31+
import { isProtocolError } from '../protocolError';
3132

3233
type SessionInfo = {
3334
session: CRSession;
@@ -571,34 +572,50 @@ class RouteImpl implements network.RouteDelegate {
571572
method: overrides.method,
572573
postData: overrides.postData ? overrides.postData.toString('base64') : undefined
573574
};
574-
await this._session.send('Fetch.continueRequest', this._alreadyContinuedParams);
575+
await catchDisallowedErrors(async () => {
576+
await this._session.send('Fetch.continueRequest', this._alreadyContinuedParams);
577+
});
575578
}
576579

577580
async fulfill(response: types.NormalizedFulfillResponse) {
578581
const body = response.isBase64 ? response.body : Buffer.from(response.body).toString('base64');
579582

580583
const responseHeaders = splitSetCookieHeader(response.headers);
581-
await this._session.send('Fetch.fulfillRequest', {
582-
requestId: this._interceptionId!,
583-
responseCode: response.status,
584-
responsePhrase: network.STATUS_TEXTS[String(response.status)],
585-
responseHeaders,
586-
body,
584+
await catchDisallowedErrors(async () => {
585+
await this._session.send('Fetch.fulfillRequest', {
586+
requestId: this._interceptionId!,
587+
responseCode: response.status,
588+
responsePhrase: network.STATUS_TEXTS[String(response.status)],
589+
responseHeaders,
590+
body,
591+
});
587592
});
588593
}
589594

590595
async abort(errorCode: string = 'failed') {
591596
const errorReason = errorReasons[errorCode];
592597
assert(errorReason, 'Unknown error code: ' + errorCode);
593-
// In certain cases, protocol will return error if the request was already canceled
594-
// or the page was closed. We should tolerate these errors.
595-
await this._session._sendMayFail('Fetch.failRequest', {
596-
requestId: this._interceptionId!,
597-
errorReason
598+
await catchDisallowedErrors(async () => {
599+
await this._session.send('Fetch.failRequest', {
600+
requestId: this._interceptionId!,
601+
errorReason
602+
});
598603
});
599604
}
600605
}
601606

607+
// In certain cases, protocol will return error if the request was already canceled
608+
// or the page was closed. We should tolerate these errors but propagate other.
609+
async function catchDisallowedErrors(callback: () => Promise<void>) {
610+
try {
611+
return await callback();
612+
} catch (e) {
613+
if (isProtocolError(e) && e.message.includes('Invalid http status code or phrase'))
614+
throw e;
615+
}
616+
}
617+
618+
602619
function splitSetCookieHeader(headers: types.HeadersArray): types.HeadersArray {
603620
const index = headers.findIndex(({ name }) => name.toLowerCase() === 'set-cookie');
604621
if (index === -1)

packages/playwright-core/src/server/network.ts

+8-37
Original file line numberDiff line numberDiff line change
@@ -134,18 +134,6 @@ export class Request extends SdkObject {
134134
this._waitForResponsePromise.resolve(null);
135135
}
136136

137-
async _waitForRequestFailure() {
138-
const response = await this._waitForResponsePromise;
139-
// If response is null it was a failure an we are done.
140-
if (!response)
141-
return;
142-
await response._finishedPromise;
143-
if (this.failure())
144-
return;
145-
// If request finished without errors, we stall.
146-
await new Promise(() => {});
147-
}
148-
149137
_setOverrides(overrides: types.NormalizedContinueOverrides) {
150138
this._overrides = overrides;
151139
this._updateHeadersMap();
@@ -270,13 +258,7 @@ export class Route extends SdkObject {
270258
async abort(errorCode: string = 'failed') {
271259
this._startHandling();
272260
this._request._context.emit(BrowserContext.Events.RequestAborted, this._request);
273-
await Promise.race([
274-
this._delegate.abort(errorCode),
275-
// If the request is already cancelled by the page before we handle the route,
276-
// we'll receive loading failed event and will ignore route handling error.
277-
this._request._waitForRequestFailure()
278-
]);
279-
261+
await this._delegate.abort(errorCode);
280262
this._endHandling();
281263
}
282264

@@ -304,17 +286,12 @@ export class Route extends SdkObject {
304286
const headers = [...(overrides.headers || [])];
305287
this._maybeAddCorsHeaders(headers);
306288
this._request._context.emit(BrowserContext.Events.RequestFulfilled, this._request);
307-
await Promise.race([
308-
this._delegate.fulfill({
309-
status: overrides.status || 200,
310-
headers,
311-
body,
312-
isBase64,
313-
}),
314-
// If the request is already cancelled by the page before we handle the route,
315-
// we'll receive loading failed event and will ignore route handling error.
316-
this._request._waitForRequestFailure()
317-
]);
289+
await this._delegate.fulfill({
290+
status: overrides.status || 200,
291+
headers,
292+
body: body!,
293+
isBase64,
294+
});
318295
this._endHandling();
319296
}
320297

@@ -347,13 +324,7 @@ export class Route extends SdkObject {
347324
this._request._setOverrides(overrides);
348325
if (!overrides.isFallback)
349326
this._request._context.emit(BrowserContext.Events.RequestContinued, this._request);
350-
await Promise.race([
351-
this._delegate.continue(this._request, overrides),
352-
// If the request is already cancelled by the page before we handle the route,
353-
// we'll receive loading failed event and will ignore route handling error.
354-
this._request._waitForRequestFailure()
355-
]);
356-
327+
await this._delegate.continue(this._request, overrides);
357328
this._endHandling();
358329
}
359330

0 commit comments

Comments
 (0)