chore: close dialogs upon last disconnect (#36125)

packages/playwright-core/src/server/bidi/bidiPage.ts

@@ -16,7 +16,6 @@
 
 import { assert } from '../../utils';
 import { eventsHelper } from '../utils/eventsHelper';
-import { BrowserContext } from '../browserContext';
 import * as dialog from '../dialog';
 import * as dom from '../dom';
 import { Page } from '../page';
@@ -220,7 +219,7 @@ export class BidiPage implements PageDelegate {
   }
 
   private _onUserPromptOpened(event: bidi.BrowsingContext.UserPromptOpenedParameters) {
-    this._page.emitOnContext(BrowserContext.Events.Dialog, new dialog.Dialog(
+    this._page.browserContext.dialogManager.dialogDidOpen(new dialog.Dialog(
         this._page,
         event.type as dialog.DialogType,
         event.message,

packages/playwright-core/src/server/browserContext.ts

@@ -22,6 +22,7 @@ import { createGuid } from './utils/crypto';
 import { debugMode } from './utils/debug';
 import { Clock } from './clock';
 import { Debugger } from './debugger';
+import { DialogManager } from './dialog';
 import { BrowserContextAPIRequestContext } from './fetch';
 import { mkdirIfNeeded } from './utils/fileUtils';
 import { HarRecorder } from './har/harRecorder';
@@ -51,7 +52,6 @@ export abstract class BrowserContext extends SdkObject {
   static Events = {
     Console: 'console',
     Close: 'close',
-    Dialog: 'dialog',
     Page: 'page',
     // Can't use just 'error' due to node.js special treatment of error events.
     // @see https://nodejs.org/api/events.html#events_error_events
@@ -95,6 +95,7 @@ export abstract class BrowserContext extends SdkObject {
   readonly clock: Clock;
   _clientCertificatesProxy: ClientCertificatesProxy | undefined;
   private _playwrightBindingExposed = false;
+  readonly dialogManager: DialogManager;
 
   constructor(browser: Browser, options: types.BrowserContextOptions, browserContextId: string | undefined) {
     super(browser, 'browser-context');
@@ -109,6 +110,7 @@ export abstract class BrowserContext extends SdkObject {
     this.fetchRequest = new BrowserContextAPIRequestContext(this);
     this.tracing = new Tracing(this, browser.options.tracesDir);
     this.clock = new Clock(this);
+    this.dialogManager = new DialogManager(this.instrumentation);
   }
 
   isPersistentContext(): boolean {
@@ -210,12 +212,8 @@ export abstract class BrowserContext extends SdkObject {
       page = undefined;
     }
 
-    // Unless dialogs are dismissed, setting extra http headers below does not respond.
-    page?.frameManager.setCloseAllOpeningDialogs(true);
-    await page?.frameManager.closeOpenDialogs();
     // Navigate to about:blank first to ensure no page scripts are running after this point.
     await page?.mainFrame().goto(metadata, 'about:blank', { timeout: 0 });
-    page?.frameManager.setCloseAllOpeningDialogs(false);
 
     await this._resetStorage();
     await this.clock.resetForReuse();

packages/playwright-core/src/server/chromium/crBrowser.ts

@@ -30,7 +30,6 @@ import { CRPage } from './crPage';
 import { saveProtocolStream } from './crProtocolHelper';
 import { CRServiceWorker } from './crServiceWorker';
 
-import type { Dialog } from '../dialog';
 import type { InitScript, Worker } from '../page';
 import type { ConnectionTransport } from '../transport';
 import type * as types from '../types';
@@ -498,12 +497,7 @@ export class CRBrowserContext extends BrowserContext {
     // 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._crPages()) {
-      const dialogs = [...crPage._page.frameManager._openedDialogs].filter(dialog => dialog.type() === 'beforeunload');
-      openedBeforeUnloadDialogs.push(...dialogs);
-    }
-    await Promise.all(openedBeforeUnloadDialogs.map(dialog => dialog.dismiss()));
+    await this.dialogManager.closeBeforeUnloadDialogs();
 
     if (!this._browserContextId) {
       await this.stopVideoRecording();

packages/playwright-core/src/server/chromium/crPage.ts

@@ -817,7 +817,7 @@ class FrameSession {
   _onDialog(event: Protocol.Page.javascriptDialogOpeningPayload) {
     if (!this._page.frameManager.frame(this._targetId))
       return; // Our frame/subtree may be gone already.
-    this._page.emitOnContext(BrowserContext.Events.Dialog, new dialog.Dialog(
+    this._page.browserContext.dialogManager.dialogDidOpen(new dialog.Dialog(
         this._page,
         event.type,
         event.message,

packages/playwright-core/src/server/dialog.ts

@@ -18,6 +18,7 @@
 import { assert } from '../utils';
 import { SdkObject } from './instrumentation';
 
+import type { Instrumentation } from './instrumentation';
 import type { Page } from './page';
 
 type OnHandle = (accept: boolean, promptText?: string) => Promise<void>;
@@ -39,8 +40,6 @@ export class Dialog extends SdkObject {
     this._message = message;
     this._onHandle = onHandle;
     this._defaultValue = defaultValue || '';
-    this._page.frameManager.dialogDidOpen(this);
-    this.instrumentation.onDialog(this);
   }
 
   page() {
@@ -62,14 +61,14 @@ export class Dialog extends SdkObject {
   async accept(promptText?: string) {
     assert(!this._handled, 'Cannot accept dialog which is already handled!');
     this._handled = true;
-    this._page.frameManager.dialogWillClose(this);
+    this._page.browserContext.dialogManager.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);
+    this._page.browserContext.dialogManager.dialogWillClose(this);
     await this._onHandle(false);
   }
 
@@ -80,3 +79,56 @@ export class Dialog extends SdkObject {
       await this.dismiss();
   }
 }
+
+export class DialogManager {
+  private _instrumentation: Instrumentation;
+  private _dialogHandlers = new Set<(dialog: Dialog) => boolean>();
+  private _openedDialogs = new Set<Dialog>();
+
+  constructor(instrumentation: Instrumentation) {
+    this._instrumentation = instrumentation;
+  }
+
+  dialogDidOpen(dialog: Dialog) {
+    // Any ongoing evaluations will be stalled until the dialog is closed.
+    for (const frame of dialog.page().frameManager.frames())
+      frame._invalidateNonStallingEvaluations('JavaScript dialog interrupted evaluation');
+    this._openedDialogs.add(dialog);
+    this._instrumentation.onDialog(dialog);
+
+    let hasHandlers = false;
+    for (const handler of this._dialogHandlers) {
+      if (handler(dialog))
+        hasHandlers = true;
+    }
+    if (!hasHandlers)
+      dialog.close().then(() => {});
+  }
+
+  dialogWillClose(dialog: Dialog) {
+    this._openedDialogs.delete(dialog);
+  }
+
+  addDialogHandler(handler: (dialog: Dialog) => boolean) {
+    this._dialogHandlers.add(handler);
+  }
+
+  removeDialogHandler(handler: (dialog: Dialog) => boolean) {
+    this._dialogHandlers.delete(handler);
+    if (!this._dialogHandlers.size) {
+      for (const dialog of this._openedDialogs)
+        dialog.close().catch(() => {});
+    }
+  }
+
+  hasOpenDialogsForPage(page: Page) {
+    return [...this._openedDialogs].some(dialog => dialog.page() === page);
+  }
+
+  async closeBeforeUnloadDialogs() {
+    await Promise.all([...this._openedDialogs].map(async dialog => {
+      if (dialog.type() === 'beforeunload')
+        await dialog.dismiss();
+    }));
+  }
+}

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

@@ -53,6 +53,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channel
   _webSocketInterceptionPatterns: channels.BrowserContextSetWebSocketInterceptionPatternsParams['patterns'] = [];
   private _bindings: PageBinding[] = [];
   private _initScritps: InitScript[] = [];
+  private _dialogHandler: (dialog: Dialog) => boolean;
 
   static from(parentScope: DispatcherScope, context: BrowserContext): BrowserContextDispatcher {
     const result = parentScope.connection.existingDispatcher<BrowserContextDispatcher>(context);
@@ -115,12 +116,13 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channel
         });
       }
     });
-    this.addObjectListener(BrowserContext.Events.Dialog, (dialog: Dialog) => {
-      if (this._shouldDispatchEvent(dialog.page(), 'dialog'))
-        this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this, dialog) });
-      else
-        dialog.close().catch(() => {});
-    });
+    this._dialogHandler = dialog => {
+      if (!this._shouldDispatchEvent(dialog.page(), 'dialog'))
+        return false;
+      this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this, dialog) });
+      return true;
+    };
+    context.dialogManager.addDialogHandler(this._dialogHandler);
 
     if (context._browser.options.name === 'chromium') {
       for (const page of (context as CRBrowserContext).backgroundPages())
@@ -378,6 +380,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channel
     // Avoid protocol calls for the closed context.
     if (this._context.isClosingOrClosed())
       return;
+    this._context.dialogManager.removeDialogHandler(this._dialogHandler);
     this._context.setRequestInterceptor(undefined).catch(() => {});
     this._context.removeExposedBindings(this._bindings).catch(() => {});
     this._bindings = [];

packages/playwright-core/src/server/firefox/ffPage.ts

@@ -237,7 +237,7 @@ export class FFPage implements PageDelegate {
   }
 
   _onDialogOpened(params: Protocol.Page.dialogOpenedPayload) {
-    this._page.emitOnContext(BrowserContext.Events.Dialog, new dialog.Dialog(
+    this._page.browserContext.dialogManager.dialogDidOpen(new dialog.Dialog(
         this._page,
         params.type,
         params.message,

packages/playwright-core/src/server/frames.ts

@@ -36,7 +36,6 @@ import { ManualPromise } from '../utils/isomorphic/manualPromise';
 import { compressCallLog } from './callLog';
 
 import type { ConsoleMessage } from './console';
-import type { Dialog } from './dialog';
 import type { ElementStateWithoutStable, FrameExpectParams, InjectedScript } from '@injected/injectedScript';
 import type { CallMetadata } from './instrumentation';
 import type { Progress } from './progress';
@@ -101,8 +100,6 @@ export class FrameManager {
   readonly _consoleMessageTags = new Map<string, ConsoleTagHandler>();
   readonly _signalBarriers = new Set<SignalBarrier>();
   private _webSockets = new Map<string, network.WebSocket>();
-  _openedDialogs: Set<Dialog> = new Set();
-  private _closeAllOpeningDialogs = false;
 
   constructor(page: Page) {
     this._page = page;
@@ -343,29 +340,6 @@ export class FrameManager {
     this._page.emitOnContext(BrowserContext.Events.RequestFailed, request);
   }
 
-  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');
-    if (this._closeAllOpeningDialogs)
-      dialog.close().then(() => {});
-    else
-      this._openedDialogs.add(dialog);
-  }
-
-  dialogWillClose(dialog: Dialog) {
-    this._openedDialogs.delete(dialog);
-  }
-
-  async closeOpenDialogs() {
-    await Promise.all([...this._openedDialogs].map(dialog => dialog.close())).catch(() => {});
-    this._openedDialogs.clear();
-  }
-
-  setCloseAllOpeningDialogs(closeDialogs: boolean) {
-    this._closeAllOpeningDialogs = closeDialogs;
-  }
-
   removeChildFramesRecursively(frame: Frame) {
     for (const child of frame.childFrames())
       this._removeFramesRecursively(child);
@@ -563,7 +537,7 @@ export class Frame extends SdkObject {
   async raceAgainstEvaluationStallingEvents<T>(cb: () => Promise<T>): Promise<T> {
     if (this._pendingDocument)
       throw new Error('Frame is currently attempting a navigation');
-    if (this._page.frameManager._openedDialogs.size)
+    if (this._page.browserContext.dialogManager.hasOpenDialogsForPage(this._page))
       throw new Error('Open JavaScript dialog prevents evaluation');
 
     const promise = new ManualPromise<T>();

packages/playwright-core/src/server/recorder/contextRecorder.ts

@@ -29,7 +29,6 @@ import { generateCode } from '../codegen/language';
 
 import type { RegisteredListener } from '../../utils';
 import type { Language, LanguageGenerator, LanguageGeneratorOptions } from '../codegen/types';
-import type { Dialog } from '../dialog';
 import type * as channels from '@protocol/channels';
 import type * as actions from '@recorder/actions';
 import type { Source } from '@recorder/recorderTypes';
@@ -135,7 +134,11 @@ export class ContextRecorder extends EventEmitter {
     this._context.on(BrowserContext.Events.Page, (page: Page) => this._onPage(page));
     for (const page of this._context.pages())
       this._onPage(page);
-    this._context.on(BrowserContext.Events.Dialog, (dialog: Dialog) => this._onDialog(dialog.page()));
+    this._context.dialogManager.addDialogHandler(dialog => {
+      this._onDialog(dialog.page());
+      // Not handling the dialog, let it automatically close.
+      return false;
+    });
 
     // Input actions that potentially lead to navigation are intercepted on the page and are
     // performed by the Playwright.

packages/playwright-core/src/server/webkit/wkPage.ts

@@ -593,7 +593,7 @@ export class WKPage implements PageDelegate {
   }
 
   _onDialog(event: Protocol.Dialog.javascriptDialogOpeningPayload) {
-    this._page.emitOnContext(BrowserContext.Events.Dialog, new dialog.Dialog(
+    this._page.browserContext.dialogManager.dialogDidOpen(new dialog.Dialog(
         this._page,
         event.type as dialog.DialogType,
         event.message,