From 42328478eac6057d739eb9643e74b243a359d4bb Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 5 May 2023 11:12:33 -0700 Subject: [PATCH] feat: make console/dialog events based on subscription (#22835) This way we do not send events from the server unless the client is interested. Fixes #22621. --- .../src/client/browserContext.ts | 6 +++ .../src/client/channelOwner.ts | 7 ++- packages/playwright-core/src/client/page.ts | 2 + .../playwright-core/src/protocol/validator.ts | 4 +- .../dispatchers/browserContextDispatcher.ts | 20 +++++++-- .../src/server/trace/recorder/tracing.ts | 43 ++++++++++++++----- packages/protocol/src/channels.ts | 4 +- packages/protocol/src/protocol.yml | 4 ++ tests/page/page-event-popup.spec.ts | 7 ++- 9 files changed, 76 insertions(+), 21 deletions(-) diff --git a/packages/playwright-core/src/client/browserContext.ts b/packages/playwright-core/src/client/browserContext.ts index 7ca870305c..67390a7fdb 100644 --- a/packages/playwright-core/src/client/browserContext.ts +++ b/packages/playwright-core/src/client/browserContext.ts @@ -107,6 +107,10 @@ export class BrowserContext extends ChannelOwner if (page) hasListeners = page.emit(Events.Page.Dialog, dialogObject) || hasListeners; if (!hasListeners) { + // Although we do similar handling on the server side, we still need this logic + // on the client side due to a possible race condition between two async calls: + // a) removing "dialog" listener subscription (client->server) + // b) actual "dialog" event (server->client) if (dialogObject.type() === 'beforeunload') dialog.accept({}).catch(() => {}); else @@ -120,6 +124,8 @@ export class BrowserContext extends ChannelOwner this._closedPromise = new Promise(f => this.once(Events.BrowserContext.Close, f)); this._setEventToSubscriptionMapping(new Map([ + [Events.BrowserContext.Console, 'console'], + [Events.BrowserContext.Dialog, 'dialog'], [Events.BrowserContext.Request, 'request'], [Events.BrowserContext.Response, 'response'], [Events.BrowserContext.RequestFinished, 'requestFinished'], diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index 0e3d72cef6..e1e70b5970 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -67,8 +67,11 @@ export abstract class ChannelOwner {}); + if (protocolEvent) { + this._wrapApiCall(async () => { + await (this._channel as any).updateSubscription({ event: protocolEvent, enabled }); + }, true).catch(() => {}); + } } override on(event: string | symbol, listener: Listener): this { diff --git a/packages/playwright-core/src/client/page.ts b/packages/playwright-core/src/client/page.ts index 3453bc33ec..fdf6330579 100644 --- a/packages/playwright-core/src/client/page.ts +++ b/packages/playwright-core/src/client/page.ts @@ -145,6 +145,8 @@ export class Page extends ChannelOwner implements api.Page this.once(Events.Page.Crash, () => this._closedOrCrashedRace.scopeClosed(new Error(kBrowserOrContextClosedError))); this._setEventToSubscriptionMapping(new Map([ + [Events.Page.Console, 'console'], + [Events.Page.Dialog, 'dialog'], [Events.Page.Request, 'request'], [Events.Page.Response, 'response'], [Events.Page.RequestFinished, 'requestFinished'], diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index f0a0cdb411..578ccd4a67 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -924,7 +924,7 @@ scheme.BrowserContextCreateTempFileResult = tObject({ writableStream: tChannel(['WritableStream']), }); scheme.BrowserContextUpdateSubscriptionParams = tObject({ - event: tEnum(['request', 'response', 'requestFinished', 'requestFailed']), + event: tEnum(['console', 'dialog', 'request', 'response', 'requestFinished', 'requestFailed']), enabled: tBoolean, }); scheme.BrowserContextUpdateSubscriptionResult = tOptional(tObject({})); @@ -1217,7 +1217,7 @@ scheme.PageStopCSSCoverageResult = tObject({ scheme.PageBringToFrontParams = tOptional(tObject({})); scheme.PageBringToFrontResult = tOptional(tObject({})); scheme.PageUpdateSubscriptionParams = tObject({ - event: tEnum(['fileChooser', 'request', 'response', 'requestFinished', 'requestFailed']), + event: tEnum(['console', 'dialog', 'fileChooser', 'request', 'response', 'requestFinished', 'requestFailed']), enabled: tBoolean, }); scheme.PageUpdateSubscriptionResult = tOptional(tObject({})); diff --git a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts index 22c2caf970..69e5ed355e 100644 --- a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts @@ -35,6 +35,9 @@ import { createGuid, urlMatches } from '../../utils'; import { WritableStreamDispatcher } from './writableStreamDispatcher'; import { ConsoleMessageDispatcher } from './consoleMessageDispatcher'; import { DialogDispatcher } from './dialogDispatcher'; +import type { Page } from '../page'; +import type { Dialog } from '../dialog'; +import type { ConsoleMessage } from '../console'; export class BrowserContextDispatcher extends Dispatcher implements channels.BrowserContextChannel { _type_EventTarget = true; @@ -81,8 +84,16 @@ export class BrowserContextDispatcher extends Dispatcher this._dispatchEvent('console', { message: new ConsoleMessageDispatcher(PageDispatcher.from(this, message.page()), message) })); - this.addObjectListener(BrowserContext.Events.Dialog, dialog => this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this, dialog) })); + this.addObjectListener(BrowserContext.Events.Console, (message: ConsoleMessage) => { + if (this._shouldDispatchEvent(message.page(), 'console')) + this._dispatchEvent('console', { message: new ConsoleMessageDispatcher(PageDispatcher.from(this, message.page()), message) }); + }); + 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(() => {}); + }); if (context._browser.options.name === 'chromium') { for (const page of (context as CRBrowserContext).backgroundPages()) @@ -141,9 +152,12 @@ export class BrowserContextDispatcher extends Dispatcher(page) : undefined; if (pageDispatcher?._subscriptions.has(event)) return true; diff --git a/packages/playwright-core/src/server/trace/recorder/tracing.ts b/packages/playwright-core/src/server/trace/recorder/tracing.ts index 38e2193e08..3eaa227297 100644 --- a/packages/playwright-core/src/server/trace/recorder/tracing.ts +++ b/packages/playwright-core/src/server/trace/recorder/tracing.ts @@ -38,12 +38,12 @@ import type { HarTracerDelegate } from '../../har/harTracer'; import { HarTracer } from '../../har/harTracer'; import type { FrameSnapshot } from '@trace/snapshot'; import type * as trace from '@trace/trace'; -import type { VERSION } from '@trace/trace'; import type { SnapshotterBlob, SnapshotterDelegate } from './snapshotter'; import { Snapshotter } from './snapshotter'; import { yazl } from '../../../zipBundle'; +import type { ConsoleMessage } from '../../console'; -const version: VERSION = 4; +const version: trace.VERSION = 4; export type TracerOptions = { name?: string; @@ -71,6 +71,7 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps private _snapshotter?: Snapshotter; private _harTracer: HarTracer; private _screencastListeners: RegisteredListener[] = []; + private _eventListeners: RegisteredListener[] = []; private _context: BrowserContext | APIRequestContext; private _state: RecordingState | undefined; private _isStopping = false; @@ -168,6 +169,9 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps }); this._context.instrumentation.addListener(this, this._context); + this._eventListeners.push( + eventsHelper.addEventListener(this._context, BrowserContext.Events.Console, this._onConsoleMessage.bind(this)), + ); if (state.options.screenshots) this._startScreencast(); if (state.options.snapshots) @@ -248,6 +252,7 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps const state = this._state!; this._context.instrumentation.removeListener(this); + eventsHelper.removeEventListeners(this._eventListeners); if (this._state?.options.screenshots) this._stopScreencast(); @@ -354,14 +359,8 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps onEvent(sdkObject: SdkObject, event: trace.EventTraceEvent) { if (!sdkObject.attribution.context) return; - if (event.method === '__create__' && event.class === 'ConsoleMessage') { - const object: trace.ObjectTraceEvent = { - type: 'object', - class: event.class, - guid: event.params.guid, - initializer: event.params.initializer, - }; - this._appendTraceEvent(object); + if (event.method === 'console' || (event.method === '__create__' && event.class === 'ConsoleMessage')) { + // Console messages are handled separately. return; } this._appendTraceEvent(event); @@ -390,6 +389,30 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps this._appendTraceEvent({ type: 'frame-snapshot', snapshot }); } + private _onConsoleMessage(message: ConsoleMessage) { + const object: trace.ObjectTraceEvent = { + type: 'object', + class: 'ConsoleMessage', + guid: message.guid, + initializer: { + type: message.type(), + text: message.text(), + location: message.location(), + }, + }; + this._appendTraceEvent(object); + + const event: trace.EventTraceEvent = { + type: 'event', + class: 'BrowserContext', + method: 'console', + params: { message: { guid: message.guid } }, + time: monotonicTime(), + pageId: message.page().guid, + }; + this._appendTraceEvent(event); + } + private _startScreencastInPage(page: Page) { page.setScreencastOptions(kScreencastOptions); const prefix = page.guid; diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index 03311c4f6f..bfd97e15b0 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -1684,7 +1684,7 @@ export type BrowserContextCreateTempFileResult = { writableStream: WritableStreamChannel, }; export type BrowserContextUpdateSubscriptionParams = { - event: 'request' | 'response' | 'requestFinished' | 'requestFailed', + event: 'console' | 'dialog' | 'request' | 'response' | 'requestFinished' | 'requestFailed', enabled: boolean, }; export type BrowserContextUpdateSubscriptionOptions = { @@ -2201,7 +2201,7 @@ export type PageBringToFrontParams = {}; export type PageBringToFrontOptions = {}; export type PageBringToFrontResult = void; export type PageUpdateSubscriptionParams = { - event: 'fileChooser' | 'request' | 'response' | 'requestFinished' | 'requestFailed', + event: 'console' | 'dialog' | 'fileChooser' | 'request' | 'response' | 'requestFinished' | 'requestFailed', enabled: boolean, }; export type PageUpdateSubscriptionOptions = { diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index fbb8f73e64..0a7fb5c6c1 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -1153,6 +1153,8 @@ BrowserContext: event: type: enum literals: + - console + - dialog - request - response - requestFinished @@ -1588,6 +1590,8 @@ Page: event: type: enum literals: + - console + - dialog - fileChooser - request - response diff --git a/tests/page/page-event-popup.spec.ts b/tests/page/page-event-popup.spec.ts index c04c7ab085..c8330f2254 100644 --- a/tests/page/page-event-popup.spec.ts +++ b/tests/page/page-event-popup.spec.ts @@ -63,9 +63,12 @@ it('should be able to capture alert', async ({ page }) => { const win = window.open(''); win.alert('hello'); }); - const popup = await page.waitForEvent('popup'); - const dialog = await popup.waitForEvent('dialog'); + const [popup, dialog] = await Promise.all([ + page.waitForEvent('popup'), + page.context().waitForEvent('dialog'), + ]); expect(dialog.message()).toBe('hello'); + expect(dialog.page()).toBe(popup); await dialog.dismiss(); await evaluatePromise; });