Skip to content

Commit 412c11d

Browse files
dgozmandgozman
authored
fix(reuse): make sure all dispose and close sequences are executed (#19572)
- When disposing recursively, only the root dispatcher received `_dispose()` call, while some dispatchers need `_onDispose()` to clean things up. - When reusing the context, pages should be notified with `_onClose()` so that all client-side waiting promises could reject. Fixes #19216.
1 parent 600d6bc commit 412c11d

7 files changed

Lines changed: 61 additions & 18 deletions

File tree

‎packages/playwright-core/src/client/browser.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ export class Browser extends ChannelOwner<channels.BrowserChannel> implements ap
6565
async _newContextForReuse(options: BrowserContextOptions = {}): Promise<BrowserContext> {
6666
for (const context of this._contexts) {
6767
await this._browserType._onWillCloseContext?.(context);
68+
for (const page of context.pages())
69+
page._onClose();
6870
context._onClose();
6971
}
7072
this._contexts.clear();

‎packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -270,8 +270,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channel
270270
this._subscriptions.delete(params.event);
271271
}
272272

273-
override _dispose() {
274-
super._dispose();
273+
override _onDispose() {
275274
// Avoid protocol calls for the closed context.
276275
if (!this._context.isClosingOrClosed())
277276
this._context.setRequestInterceptor(undefined).catch(() => {});

‎packages/playwright-core/src/server/dispatchers/debugControllerDispatcher.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,7 @@ export class DebugControllerDispatcher extends Dispatcher<DebugController, chann
7979
await this._object.closeAllBrowsers();
8080
}
8181

82-
override _dispose() {
83-
super._dispose();
82+
override _onDispose() {
8483
this._object.dispose();
8584
}
8685
}

‎packages/playwright-core/src/server/dispatchers/dispatcher.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,12 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
102102
this._connection.sendDispose(this);
103103
}
104104

105+
protected _onDispose() {
106+
}
107+
105108
private _disposeRecursively() {
106109
assert(!this._disposed, `${this._guid} is disposed more than once`);
110+
this._onDispose();
107111
this._disposed = true;
108112
eventsHelper.removeEventListeners(this._eventListeners);
109113

‎packages/playwright-core/src/server/dispatchers/pageDispatcher.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -299,9 +299,10 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel, Brows
299299
this._dispatchEvent('frameDetached', { frame: FrameDispatcher.from(this, frame) });
300300
}
301301

302-
override _dispose() {
303-
super._dispose();
304-
this._page.setClientRequestInterceptor(undefined).catch(() => {});
302+
override _onDispose() {
303+
// Avoid protocol calls for the closed page.
304+
if (!this._page.isClosedOrClosingOrCrashed())
305+
this._page.setClientRequestInterceptor(undefined).catch(() => {});
305306
}
306307
}
307308

‎packages/playwright-core/src/server/page.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,10 @@ export class Page extends SdkObject {
612612
return this._closedState === 'closed';
613613
}
614614

615+
isClosedOrClosingOrCrashed() {
616+
return this._closedState !== 'open' || this._crashedPromise.isDone();
617+
}
618+
615619
_addWorker(workerId: string, worker: Worker) {
616620
this._workers.set(workerId, worker);
617621
this.emit(Page.Events.Worker, worker);

‎tests/library/debug-controller.spec.ts‎

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@ import { createGuid } from '../../packages/playwright-core/lib/utils';
2020
import { Backend } from '../config/debugControllerBackend';
2121
import type { Browser, BrowserContext } from '@playwright/test';
2222

23+
type BrowserWithReuse = Browser & { _newContextForReuse: () => Promise<BrowserContext> };
2324
type Fixtures = {
2425
wsEndpoint: string;
2526
backend: Backend;
26-
connectedBrowser: Browser & { _newContextForReuse: () => Promise<BrowserContext> };
27+
connectedBrowserFactory: () => Promise<BrowserWithReuse>;
28+
connectedBrowser: BrowserWithReuse;
2729
};
2830

2931
const test = baseTest.extend<Fixtures>({
@@ -41,16 +43,24 @@ const test = baseTest.extend<Fixtures>({
4143
await use(backend);
4244
await backend.close();
4345
},
44-
connectedBrowser: async ({ wsEndpoint, browserType }, use) => {
45-
const oldValue = (browserType as any)._defaultConnectOptions;
46-
(browserType as any)._defaultConnectOptions = {
47-
wsEndpoint,
48-
headers: { 'x-playwright-reuse-context': '1', },
49-
};
50-
const browser = await browserType.launch();
51-
(browserType as any)._defaultConnectOptions = oldValue;
52-
await use(browser as any);
53-
await browser.close();
46+
connectedBrowserFactory: async ({ wsEndpoint, browserType }, use) => {
47+
const browsers: BrowserWithReuse [] = [];
48+
await use(async () => {
49+
const oldValue = (browserType as any)._defaultConnectOptions;
50+
(browserType as any)._defaultConnectOptions = {
51+
wsEndpoint,
52+
headers: { 'x-playwright-reuse-context': '1', },
53+
};
54+
const browser = await browserType.launch() as BrowserWithReuse;
55+
(browserType as any)._defaultConnectOptions = oldValue;
56+
browsers.push(browser);
57+
return browser;
58+
});
59+
for (const browser of browsers)
60+
await browser.close();
61+
},
62+
connectedBrowser: async ({ connectedBrowserFactory }, use) => {
63+
await use(await connectedBrowserFactory());
5464
},
5565
});
5666

@@ -231,3 +241,27 @@ test('should pause and resume', async ({ backend, connectedBrowser }) => {
231241
await backend.resume();
232242
await pausePromise;
233243
});
244+
245+
test('should reset routes before reuse', async ({ server, connectedBrowserFactory }) => {
246+
const browser1 = await connectedBrowserFactory();
247+
const context1 = await browser1._newContextForReuse();
248+
await context1.route(server.PREFIX + '/title.html', route => route.fulfill({ body: '<title>Hello</title>', contentType: 'text/html' }));
249+
const page1 = await context1.newPage();
250+
await page1.route(server.PREFIX + '/consolelog.html', route => route.fulfill({ body: '<title>World</title>', contentType: 'text/html' }));
251+
252+
await page1.goto(server.PREFIX + '/title.html');
253+
await expect(page1).toHaveTitle('Hello');
254+
await page1.goto(server.PREFIX + '/consolelog.html');
255+
await expect(page1).toHaveTitle('World');
256+
await browser1.close();
257+
258+
const browser2 = await connectedBrowserFactory();
259+
const context2 = await browser2._newContextForReuse();
260+
const page2 = await context2.newPage();
261+
262+
await page2.goto(server.PREFIX + '/title.html');
263+
await expect(page2).toHaveTitle('Woof-Woof');
264+
await page2.goto(server.PREFIX + '/consolelog.html');
265+
await expect(page2).toHaveTitle('console.log test');
266+
await browser2.close();
267+
});

0 commit comments

Comments
 (0)