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.

Sign up for GitHub

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

Jump to bottom

fix(routes): catch route handlers upon respective page closure #23929

Closed
wants to merge 1 commit into from
Closed

fix(routes): catch route handlers upon respective page closure #23929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages/playwright-core/src/client/browserContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class BrowserContext extends ChannelOwner<channels.BrowserContextChannel>
readonly _serviceWorkers = new Set<Worker>();
readonly _isChromium: boolean;
private _harRecorders = new Map<string, { path: string, content: 'embed' | 'attach' | 'omit' | undefined }>();
private _closeWasCalled = false;
_closeWasCalled = false;

static from(context: channels.BrowserContextChannel): BrowserContext {
return (context as any)._object;
Expand Down
24 changes: 17 additions & 7 deletions packages/playwright-core/src/client/network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,10 @@ export class Request extends ChannelOwner<channels.RequestChannel> implements ap
_targetClosedRace(): ScopedRace {
return this.serviceWorker()?._closedRace || this.frame()._page?._closedOrCrashedRace || new ScopedRace();
}

_page(): Page | null {
return this._initializer.frame ? Frame.from(this._initializer.frame).page() : null;
}
}

export class Route extends ChannelOwner<channels.RouteChannel> implements api.Route {
Expand Down Expand Up @@ -658,13 +662,19 @@ export class RouteHandler {
public async handle(route: Route): Promise<boolean> {
++this.handledCount;
const handledPromise = route._startHandling();
// Extract handler into a variable to avoid [RouteHandler.handler] in the stack.
const handler = this.handler;
const [handled] = await Promise.all([
handledPromise,
handler(route, route.request()),
]);
return handled;
try {
// Extract handler into a variable to avoid [RouteHandler.handler] in the stack.
const handler = this.handler;
const [handled] = await Promise.all([
handledPromise,
handler(route, route.request()),
]);
return handled;
} catch (e) {
if (route.request()._page()?._shouldCatchAllRouteHandlers())
return true;
throw e;
}
}

public willExpire(): boolean {
Expand Down
6 changes: 6 additions & 0 deletions packages/playwright-core/src/client/page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export class Page extends ChannelOwner<channels.PageChannel> implements api.Page
private _frames = new Set<Frame>();
_workers = new Set<Worker>();
private _closed = false;
private _closeWasCalled = false;
readonly _closedOrCrashedRace = new ScopedRace();
private _viewportSize: Size | null;
private _routes: RouteHandler[] = [];
Expand Down Expand Up @@ -513,7 +514,12 @@ export class Page extends ChannelOwner<channels.PageChannel> implements api.Page
await this._channel.bringToFront();
}

_shouldCatchAllRouteHandlers() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_shouldCatchAllRouteHandlers() {
_shouldIgnoreAllRouteHandlerExceptions() {

nit: I find catch without context not that meaningful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call it _isClosed()

return this._closeWasCalled || this._browserContext._closeWasCalled;
}

async close(options: { runBeforeUnload?: boolean } = { runBeforeUnload: undefined }) {
this._closeWasCalled = true;
try {
if (this._ownedContext)
await this._ownedContext.close();
Expand Down
56 changes: 56 additions & 0 deletions tests/page/page-route.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,3 +940,59 @@ for (const method of ['fulfill', 'continue', 'fallback', 'abort'] as const) {
expect(e.message).toContain('Route is already handled!');
});
}

it('should catch route handler custom exception upon page closure', async ({ page, server }) => {
await page.route('**/empty.html', async (route, request) => {
await page.close();
throw new Error('my error');
});
await page.goto(server.EMPTY_PAGE).catch(e => {});
await new Promise(f => setTimeout(f, 2000));
});

it('should catch async-first route handler custom exception upon page closure', async ({ page, server }) => {
await page.route('**/empty.html', async (route, request) => {
await new Promise(f => setTimeout(f, 0));
await page.close();
throw new Error('my error');
});
await page.goto(server.EMPTY_PAGE).catch(e => {});
await new Promise(f => setTimeout(f, 2000));
});

it('should catch route handler fetch exception upon page closure', async ({ page, server }) => {
await page.route('**/empty.html', async (route, request) => {
const responsePromise = route.fetch();
await page.close();
await route.fulfill({ response: await responsePromise });
});
await page.goto(server.EMPTY_PAGE).catch(e => {});
await new Promise(f => setTimeout(f, 2000));
});

it('should catch route handler sync exception upon page closure', async ({ page, server }) => {
await page.route('**/empty.html', (route, request) => {
page.close().catch(() => {});
throw new Error('my error');
});
await page.goto(server.EMPTY_PAGE).catch(e => {});
await new Promise(f => setTimeout(f, 2000));
});

it('should catch context-route handler exception upon page closure', async ({ page, server }) => {
await page.context().route('**/empty.html', async (route, request) => {
await page.close();
throw new Error('my error');
});
await page.goto(server.EMPTY_PAGE).catch(e => {});
await new Promise(f => setTimeout(f, 2000));
});

it('should catch route handler exception upon context closure', async ({ page, server }) => {
await page.route('**/empty.html', async (route, request) => {
await page.context().close();
throw new Error('my error');
});
await page.goto(server.EMPTY_PAGE).catch(e => {});
await new Promise(f => setTimeout(f, 2000));
});
Loading