Skip to content

Commit

Permalink
fix(chromium): continue requests paused for the second time (#27429)
Browse files Browse the repository at this point in the history
Sometimes Chromium restarts requests. This leads to multiple
`Fetch.requestPaused` for a single `Network.requestWillBeSent`.

Fixes #27294.
  • Loading branch information
dgozman authored Oct 4, 2023
1 parent 7dcba6f commit d426f2f
Show file tree
Hide file tree
Showing 8 changed files with 75 additions and 28 deletions.
54 changes: 33 additions & 21 deletions packages/playwright-core/src/server/chromium/crNetworkManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,21 @@ export class CRNetworkManager {
this._onRequest(sessionInfo, requestWillBeSentEvent, event);
this._requestIdToRequestWillBeSentEvent.delete(requestId);
} else {
const existingRequest = this._requestIdToRequest.get(requestId);
const alreadyContinuedParams = existingRequest?._route?._alreadyContinuedParams;
if (alreadyContinuedParams) {
// Sometimes Chromium network stack restarts the request internally.
// For example, when no-cors request hits a "less public address space", it should be resent with cors.
// There are some more examples here: https://source.chromium.org/chromium/chromium/src/+/main:services/network/url_loader.cc;l=1205-1234;drc=d5dd931e0ad3d9ffe74888ec62a3cc106efd7ea6
// There are probably even more cases deep inside the network stack.
//
// Anyway, in this case, continue the request in the same way as before, and it should go through.
this._session._sendMayFail('Fetch.continueRequest', {
...alreadyContinuedParams,
requestId: event.requestId,
});
return;
}
this._requestIdToRequestPausedEvent.set(requestId, event);
}
}
Expand Down Expand Up @@ -368,14 +383,20 @@ export class CRNetworkManager {
return response;
}

_deleteRequest(request: InterceptableRequest) {
this._requestIdToRequest.delete(request._requestId);
if (request._route)
request._route._alreadyContinuedParams = undefined;
if (request._interceptionId)
this._attemptedAuthentications.delete(request._interceptionId);
}

_handleRequestRedirect(request: InterceptableRequest, responsePayload: Protocol.Network.Response, timestamp: number, hasExtraInfo: boolean) {
const response = this._createResponse(request, responsePayload, hasExtraInfo);
response.setTransferSize(null);
response.setEncodedBodySize(null);
response._requestFinished((timestamp - request._timestamp) * 1000);
this._requestIdToRequest.delete(request._requestId);
if (request._interceptionId)
this._attemptedAuthentications.delete(request._interceptionId);
this._deleteRequest(request);
(this._page?._frameManager || this._serviceWorker)!.requestReceivedResponse(response);
(this._page?._frameManager || this._serviceWorker)!.reportRequestFinished(request.request, response);
}
Expand Down Expand Up @@ -423,9 +444,7 @@ export class CRNetworkManager {
response.responseHeadersSize().then(size => response.setEncodedBodySize(event.encodedDataLength - size));
response._requestFinished(helper.secondsToRoundishMillis(event.timestamp - request._timestamp));
}
this._requestIdToRequest.delete(request._requestId);
if (request._interceptionId)
this._attemptedAuthentications.delete(request._interceptionId);
this._deleteRequest(request);
(this._page?._frameManager || this._serviceWorker)!.reportRequestFinished(request.request, response);
}

Expand Down Expand Up @@ -458,9 +477,7 @@ export class CRNetworkManager {
response.setEncodedBodySize(null);
response._requestFinished(helper.secondsToRoundishMillis(event.timestamp - request._timestamp));
}
this._requestIdToRequest.delete(request._requestId);
if (request._interceptionId)
this._attemptedAuthentications.delete(request._interceptionId);
this._deleteRequest(request);
request.request._setFailureText(event.errorText);
(this._page?._frameManager || this._serviceWorker)!.requestFailed(request.request, !!event.canceled);
}
Expand Down Expand Up @@ -491,7 +508,7 @@ class InterceptableRequest {
readonly _documentId: string | undefined;
readonly _timestamp: number;
readonly _wallTime: number;
private _route: RouteImpl | null;
readonly _route: RouteImpl | null;
private _redirectedFrom: InterceptableRequest | null;
session: CRSession;

Expand Down Expand Up @@ -529,34 +546,29 @@ class InterceptableRequest {

this.request = new network.Request(context, frame, serviceWorker, redirectedFrom?.request || null, documentId, url, type, method, postDataBuffer, headersObjectToArray(headers));
}

_routeForRedirectChain(): RouteImpl | null {
let request: InterceptableRequest = this;
while (request._redirectedFrom)
request = request._redirectedFrom;
return request._route;
}
}

class RouteImpl implements network.RouteDelegate {
private readonly _session: CRSession;
private _interceptionId: string;
_alreadyContinuedParams: Protocol.Fetch.continueRequestParameters | undefined;

constructor(session: CRSession, interceptionId: string) {
this._session = session;
this._interceptionId = interceptionId;
}

async continue(request: network.Request, overrides: types.NormalizedContinueOverrides): Promise<void> {
// 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 = {
requestId: this._interceptionId!,
url: overrides.url,
headers: overrides.headers,
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);
}

async fulfill(response: types.NormalizedFulfillResponse) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,6 @@ export class WKInterceptableRequest {
resourceType, event.request.method, postDataBuffer, headersObjectToArray(event.request.headers));
}

_routeForRedirectChain(): WKRouteImpl | null {
let request: WKInterceptableRequest = this;
while (request._redirectedFrom)
request = request._redirectedFrom;
return request._route;
}

createResponse(responsePayload: Protocol.Network.Response): network.Response {
const getResponseBody = async () => {
const response = await this._session.send('Network.getResponseBody', { requestId: this._requestId });
Expand Down
3 changes: 3 additions & 0 deletions tests/components/ct-react-vite/playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ export default defineConfig({
use: {
trace: 'on-first-retry',
ctViteConfig: {
build: {
assetsInlineLimit: 0,
},
resolve: {
alias: {
'@': resolve(__dirname, './src'),
Expand Down
Binary file not shown.
9 changes: 9 additions & 0 deletions tests/components/ct-react-vite/src/assets/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,12 @@ code {
background-color: #1b1b1d;
}
}

@font-face {
font-family: 'pwtest-iconfont';
/* See tests/assets/webfont/README.md */
src: url('./iconfont.woff2') format('woff2');
font-weight: normal;
font-style: normal;
font-display: swap;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.title-with-font {
font-family: pwtest-iconfont, sans-serif;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import './TitleWithFont.css';

export default function TitleWithFont() {
return <div className='title-with-font'>+-</div>
}
22 changes: 22 additions & 0 deletions tests/components/ct-react-vite/tests/route.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { test, expect } from '@playwright/experimental-ct-react';
import TitleWithFont from '@/components/TitleWithFont';

test('should load font without routes', async ({ mount, page }) => {
const promise = page.waitForEvent('requestfinished', request => request.url().includes('iconfont'));
await mount(<TitleWithFont />);
const request = await promise;
const response = await request.response();
const body = await response!.body();
expect(body.length).toBe(2656);
});

test('should load font with routes', async ({ mount, page }) => {
test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/27294' });
await page.route('**/*.json', r => r.continue());
const promise = page.waitForEvent('requestfinished', request => request.url().includes('iconfont'));
await mount(<TitleWithFont />);
const request = await promise;
const response = await request.response();
const body = await response!.body();
expect(body.length).toBe(2656);
});

0 comments on commit d426f2f

Please sign in to comment.