Skip to content

Commit

Permalink
fix: interception id not found error in route.continue (#29180)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
yury-s authored Jan 26, 2024
1 parent 12a9a30 commit 82981a8
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 49 deletions.
41 changes: 29 additions & 12 deletions packages/playwright-core/src/server/chromium/crNetworkManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<void>) {
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)
Expand Down
45 changes: 8 additions & 37 deletions packages/playwright-core/src/server/network.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
}

Expand Down

0 comments on commit 82981a8

Please sign in to comment.