From 6a7a2971f2e40442d720f42412d299c504fcbd71 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Tue, 25 Jan 2022 14:52:18 -0700 Subject: [PATCH] fix(chromium): close all javascript dialogs when closing context (#11614) Fixes #11581 --- .../src/server/chromium/crBrowser.ts | 13 +++++++++++++ packages/playwright-core/src/server/dialog.ts | 6 +++--- packages/playwright-core/src/server/frames.ts | 13 +++++++------ tests/headful.spec.ts | 17 +++++++++++++++++ 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/packages/playwright-core/src/server/chromium/crBrowser.ts b/packages/playwright-core/src/server/chromium/crBrowser.ts index aca10d6102..e287e4276b 100644 --- a/packages/playwright-core/src/server/chromium/crBrowser.ts +++ b/packages/playwright-core/src/server/chromium/crBrowser.ts @@ -21,6 +21,7 @@ import { assert } from '../../utils/utils'; import * as network from '../network'; import { Page, PageBinding, PageDelegate, Worker } from '../page'; import { Frame } from '../frames'; +import { Dialog } from '../dialog'; import { ConnectionTransport } from '../transport'; import * as types from '../types'; import { ConnectionEvents, CRConnection, CRSession } from './crConnection'; @@ -471,6 +472,18 @@ 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; + const dialogs = [...crPage._page._frameManager._openedDialogs].filter(dialog => dialog.type() === 'beforeunload'); + openedBeforeUnloadDialogs.push(...dialogs); + } + await Promise.all(openedBeforeUnloadDialogs.map(dialog => dialog.dismiss())); 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/dialog.ts b/packages/playwright-core/src/server/dialog.ts index 93a0d7859d..d334c147b9 100644 --- a/packages/playwright-core/src/server/dialog.ts +++ b/packages/playwright-core/src/server/dialog.ts @@ -38,7 +38,7 @@ export class Dialog extends SdkObject { this._message = message; this._onHandle = onHandle; this._defaultValue = defaultValue || ''; - this._page._frameManager.dialogDidOpen(); + this._page._frameManager.dialogDidOpen(this); } type(): string { @@ -56,14 +56,14 @@ export class Dialog extends SdkObject { async accept(promptText: string | undefined) { assert(!this._handled, 'Cannot accept dialog which is already handled!'); this._handled = true; - this._page._frameManager.dialogWillClose(); + this._page._frameManager.dialogWillClose(this); await this._onHandle(true, promptText); } async dismiss() { assert(!this._handled, 'Cannot dismiss dialog which is already handled!'); this._handled = true; - this._page._frameManager.dialogWillClose(); + this._page._frameManager.dialogWillClose(this); await this._onHandle(false); } } diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index 54ef7fb139..3bbecd73df 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -22,6 +22,7 @@ import { helper } from './helper'; import { eventsHelper, RegisteredListener } from '../utils/eventsHelper'; import * as js from './javascript'; import * as network from './network'; +import { Dialog } from './dialog'; import { Page } from './page'; import * as types from './types'; import { BrowserContext } from './browserContext'; @@ -85,7 +86,7 @@ export class FrameManager { readonly _consoleMessageTags = new Map(); readonly _signalBarriers = new Set(); private _webSockets = new Map(); - _dialogCounter = 0; + _openedDialogs: Set = new Set(); constructor(page: Page) { this._page = page; @@ -307,15 +308,15 @@ export class FrameManager { this._page._browserContext.emit(BrowserContext.Events.RequestFailed, request); } - dialogDidOpen() { + dialogDidOpen(dialog: Dialog) { // Any ongoing evaluations will be stalled until the dialog is closed. for (const frame of this._frames.values()) frame._invalidateNonStallingEvaluations('JavaScript dialog interrupted evaluation'); - this._dialogCounter++; + this._openedDialogs.add(dialog); } - dialogWillClose() { - this._dialogCounter--; + dialogWillClose(dialog: Dialog) { + this._openedDialogs.delete(dialog); } removeChildFramesRecursively(frame: Frame) { @@ -508,7 +509,7 @@ export class Frame extends SdkObject { async nonStallingRawEvaluateInExistingMainContext(expression: string): Promise { if (this._pendingDocument) throw new Error('Frame is currently attempting a navigation'); - if (this._page._frameManager._dialogCounter) + if (this._page._frameManager._openedDialogs.size) throw new Error('Open JavaScript dialog prevents evaluation'); const context = this._existingMainContext(); if (!context) diff --git a/tests/headful.spec.ts b/tests/headful.spec.ts index 0faf0ce0a9..37a7a6c163 100644 --- a/tests/headful.spec.ts +++ b/tests/headful.spec.ts @@ -35,6 +35,23 @@ it('should close browser with beforeunload page', async ({ browserType, server, await browserContext.close(); }); +it('should close browsercontext with pending beforeunload dialog', async ({ server, browserType }) => { + const browser = await browserType.launch({ headless: false }); + const context = await browser.newContext(); + const page = await context.newPage(); + await page.goto(server.PREFIX + '/beforeunload.html'); + // We have to interact with a page so that 'beforeunload' handlers + // fire. + await page.click('body'); + await Promise.all([ + page.waitForEvent('dialog'), + page.close({ runBeforeUnload: true }), + ]); + await context.close(); + await browser.close(); +}); + + it('should not crash when creating second context', async ({ browserType }) => { const browser = await browserType.launch({ headless: false }); {