From c56877032d9dd4b862e873d8ae211454c76b9c8d Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 1 Nov 2022 14:26:38 -0700 Subject: [PATCH] fix(persistent): close browser instead of pages (#18485) Previously, we closed pages one by one before closing the browser when shutting down the persistent context. This logic was introduced in https://github.com/microsoft/playwright/pull/4040 to properly finish video recordings in persistent context. Such a process makes it unnecessary brittle to close the persistent context. For example, Chromium headless is sometimes unable to close the last persistent page for unknown reasons. Instead, we can just stop video recordings manually and close the browser right away. Fixes #18229. --- .../src/server/browserContext.ts | 15 ++---------- .../src/server/chromium/crBrowser.ts | 24 +++++++++++-------- .../src/server/firefox/ffBrowser.ts | 16 ++++++++++--- .../src/server/webkit/wkBrowser.ts | 11 ++++++--- .../src/server/webkit/wkPage.ts | 2 +- 5 files changed, 38 insertions(+), 30 deletions(-) diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index 49fb561b75..7c556a9980 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -404,10 +404,6 @@ export abstract class BrowserContext extends SdkObject { if (this._customCloseHandler) { await this._customCloseHandler(); - } else if (this._isPersistentContext) { - // Close all the pages instead of the context, - // because we cannot close the default context. - await Promise.all(this.pages().map(page => page.close(metadata))); } else { // Close the context. await this.doClose(); @@ -420,15 +416,8 @@ export abstract class BrowserContext extends SdkObject { await Promise.all(promises); // Custom handler should trigger didCloseInternal itself. - if (this._customCloseHandler) - return; - - // Persistent context should also close the browser. - if (this._isPersistentContext) - await this._browser.close(); - - // Bookkeeping. - this._didCloseInternal(); + if (!this._customCloseHandler) + this._didCloseInternal(); } await this._closePromise; } diff --git a/packages/playwright-core/src/server/chromium/crBrowser.ts b/packages/playwright-core/src/server/chromium/crBrowser.ts index ee95110832..5f0a07a7f1 100644 --- a/packages/playwright-core/src/server/chromium/crBrowser.ts +++ b/packages/playwright-core/src/server/chromium/crBrowser.ts @@ -334,13 +334,12 @@ export class CRBrowserContext extends BrowserContext { await Promise.all(promises); } + private _crPages() { + return [...this._browser._crPages.values()].filter(crPage => crPage._browserContext === this); + } + pages(): Page[] { - const result: Page[] = []; - for (const crPage of this._browser._crPages.values()) { - if (crPage._browserContext === this && crPage._initializedPage) - result.push(crPage._initializedPage); - } - return result; + return this._crPages().map(crPage => crPage._initializedPage).filter(Boolean) as Page[]; } async newPageDelegate(): Promise { @@ -489,19 +488,24 @@ export class CRBrowserContext extends BrowserContext { } async doClose() { - assert(this._browserContextId); // Headful chrome cannot dispose browser context with opened 'beforeunload' // dialogs, so we should close all that are currently opened. // We also won't get new ones since `Target.disposeBrowserContext` does not trigger // beforeunload. const openedBeforeUnloadDialogs: Dialog[] = []; - for (const crPage of this._browser._crPages.values()) { - if (crPage._browserContext !== this) - continue; + for (const crPage of this._crPages()) { const dialogs = [...crPage._page._frameManager._openedDialogs].filter(dialog => dialog.type() === 'beforeunload'); openedBeforeUnloadDialogs.push(...dialogs); } await Promise.all(openedBeforeUnloadDialogs.map(dialog => dialog.dismiss())); + + if (!this._browserContextId) { + await Promise.all(this._crPages().map(crPage => crPage._mainFrameSession._stopVideoRecording())); + // Closing persistent context should close the browser. + await this._browser.close(); + return; + } + await this._browser._session.send('Target.disposeBrowserContext', { browserContextId: this._browserContextId }); this._browser._contexts.delete(this._browserContextId); for (const [targetId, serviceWorker] of this._browser._serviceWorkers) { diff --git a/packages/playwright-core/src/server/firefox/ffBrowser.ts b/packages/playwright-core/src/server/firefox/ffBrowser.ts index 5a65732d8d..7df8d98b0a 100644 --- a/packages/playwright-core/src/server/firefox/ffBrowser.ts +++ b/packages/playwright-core/src/server/firefox/ffBrowser.ts @@ -359,9 +359,19 @@ export class FFBrowserContext extends BrowserContext { onClosePersistent() {} async doClose() { - assert(this._browserContextId); - await this._browser._connection.send('Browser.removeBrowserContext', { browserContextId: this._browserContextId }); - this._browser._contexts.delete(this._browserContextId); + if (!this._browserContextId) { + if (this._options.recordVideo) { + await this._browser._connection.send('Browser.setVideoRecordingOptions', { + options: undefined, + browserContextId: this._browserContextId + }); + } + // Closing persistent context should close the browser. + await this._browser.close(); + } else { + await this._browser._connection.send('Browser.removeBrowserContext', { browserContextId: this._browserContextId }); + this._browser._contexts.delete(this._browserContextId); + } } async cancelDownload(uuid: string) { diff --git a/packages/playwright-core/src/server/webkit/wkBrowser.ts b/packages/playwright-core/src/server/webkit/wkBrowser.ts index ee7dae1150..179c76dd45 100644 --- a/packages/playwright-core/src/server/webkit/wkBrowser.ts +++ b/packages/playwright-core/src/server/webkit/wkBrowser.ts @@ -338,9 +338,14 @@ export class WKBrowserContext extends BrowserContext { onClosePersistent() {} async doClose() { - assert(this._browserContextId); - await this._browser._browserSession.send('Playwright.deleteContext', { browserContextId: this._browserContextId }); - this._browser._contexts.delete(this._browserContextId); + if (!this._browserContextId) { + await Promise.all(this._wkPages().map(wkPage => wkPage._stopVideo())); + // Closing persistent context should close the browser. + await this._browser.close(); + } else { + await this._browser._browserSession.send('Playwright.deleteContext', { browserContextId: this._browserContextId }); + this._browser._contexts.delete(this._browserContextId); + } } async cancelDownload(uuid: string) { diff --git a/packages/playwright-core/src/server/webkit/wkPage.ts b/packages/playwright-core/src/server/webkit/wkPage.ts index e367a5dc69..edbee1d87c 100644 --- a/packages/playwright-core/src/server/webkit/wkPage.ts +++ b/packages/playwright-core/src/server/webkit/wkPage.ts @@ -825,7 +825,7 @@ export class WKPage implements PageDelegate { this._browserContext._browser._videoStarted(this._browserContext, screencastId, options.outputFile, this.pageOrError()); } - private async _stopVideo(): Promise { + async _stopVideo(): Promise { if (!this._recordingVideoFile) return; await this._pageProxySession.sendMayFail('Screencast.stopVideo');