From 561cb23e8d097f82c2f957d61ce4a0584121985f Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 2 Apr 2021 11:15:07 -0700 Subject: [PATCH] fix: dispatch popup event on the client end (#6044) --- src/client/browserContext.ts | 2 ++ src/client/page.ts | 9 +++--- src/dispatchers/pageDispatcher.ts | 15 +++++---- src/protocol/channels.ts | 11 +------ src/protocol/protocol.yml | 9 +----- src/protocol/validator.ts | 1 - src/server/chromium/crPage.ts | 21 +++++-------- src/server/firefox/ffPage.ts | 32 +++++++++----------- src/server/page.ts | 29 ++++++++++-------- src/server/supplements/recorderSupplement.ts | 9 ++---- src/server/webkit/wkPage.ts | 14 +-------- 11 files changed, 59 insertions(+), 93 deletions(-) diff --git a/src/client/browserContext.ts b/src/client/browserContext.ts index 23ba426f27..b62792f230 100644 --- a/src/client/browserContext.ts +++ b/src/client/browserContext.ts @@ -88,6 +88,8 @@ export class BrowserContext extends ChannelOwner this._onBinding(BindingCall.from(binding))); this._channel.on('close', () => this._onClose()); @@ -130,7 +132,6 @@ export class Page extends ChannelOwner this._onFrameDetached(Frame.from(frame))); this._channel.on('load', () => this.emit(Events.Page.Load, this)); this._channel.on('pageError', ({ error }) => this.emit(Events.Page.PageError, parseError(error))); - this._channel.on('popup', ({ page }) => this.emit(Events.Page.Popup, Page.from(page))); this._channel.on('request', ({ request }) => this.emit(Events.Page.Request, Request.from(request))); this._channel.on('requestFailed', ({ request, failureText, responseEndTiming }) => this._onRequestFailed(Request.from(request), responseEndTiming, failureText)); this._channel.on('requestFinished', ({ request, responseEndTiming }) => this._onRequestFinished(Request.from(request), responseEndTiming)); @@ -220,9 +221,9 @@ export class Page extends ChannelOwner { - return this._wrapApiCall('page.opener', async (channel: channels.PageChannel) => { - return Page.fromNullable((await channel.opener()).page); - }); + if (!this._opener || this._opener.isClosed()) + return null; + return this._opener; } mainFrame(): Frame { diff --git a/src/dispatchers/pageDispatcher.ts b/src/dispatchers/pageDispatcher.ts index f8893f2886..7c4c4c51af 100644 --- a/src/dispatchers/pageDispatcher.ts +++ b/src/dispatchers/pageDispatcher.ts @@ -38,13 +38,21 @@ import { Download } from '../server/download'; export class PageDispatcher extends Dispatcher implements channels.PageChannel { private _page: Page; + private static fromNullable(scope: DispatcherScope, page: Page | undefined): PageDispatcher | undefined { + if (!page) + return undefined; + const result = existingDispatcher(page); + return result || new PageDispatcher(scope, page); + } + constructor(scope: DispatcherScope, page: Page) { // TODO: theoretically, there could be more than one frame already. // If we split pageCreated and pageReady, there should be no main frame during pageCreated. super(scope, page, 'Page', { mainFrame: FrameDispatcher.from(scope, page.mainFrame()), viewportSize: page.viewportSize() || undefined, - isClosed: page.isClosed() + isClosed: page.isClosed(), + opener: PageDispatcher.fromNullable(scope, page.opener()) }, true); this._page = page; page.on(Page.Events.Close, () => { @@ -66,7 +74,6 @@ export class PageDispatcher extends Dispatcher i page.on(Page.Events.FrameDetached, frame => this._onFrameDetached(frame)); page.on(Page.Events.Load, () => this._dispatchEvent('load')); page.on(Page.Events.PageError, error => this._dispatchEvent('pageError', { error: serializeError(error) })); - page.on(Page.Events.Popup, page => this._dispatchEvent('popup', { page: lookupDispatcher(page) })); page.on(Page.Events.Request, request => this._dispatchEvent('request', { request: RequestDispatcher.from(this._scope, request) })); page.on(Page.Events.RequestFailed, (request: Request) => this._dispatchEvent('requestFailed', { request: RequestDispatcher.from(this._scope, request), @@ -93,10 +100,6 @@ export class PageDispatcher extends Dispatcher i this._page.setDefaultTimeout(params.timeout); } - async opener(params: channels.PageOpenerParams, metadata: CallMetadata): Promise { - return { page: lookupNullableDispatcher(await this._page.opener()) }; - } - async exposeBinding(params: channels.PageExposeBindingParams, metadata: CallMetadata): Promise { await this._page.exposeBinding(params.name, !!params.needsHandle, (source, ...args) => { const binding = new BindingCallDispatcher(this._scope, params.name, !!params.needsHandle, source, args); diff --git a/src/protocol/channels.ts b/src/protocol/channels.ts index 58592fa0e3..acdffef168 100644 --- a/src/protocol/channels.ts +++ b/src/protocol/channels.ts @@ -797,6 +797,7 @@ export type PageInitializer = { height: number, }, isClosed: boolean, + opener?: PageChannel, }; export interface PageChannel extends Channel { on(event: 'bindingCall', callback: (params: PageBindingCallEvent) => void): this; @@ -811,7 +812,6 @@ export interface PageChannel extends Channel { on(event: 'frameDetached', callback: (params: PageFrameDetachedEvent) => void): this; on(event: 'load', callback: (params: PageLoadEvent) => void): this; on(event: 'pageError', callback: (params: PagePageErrorEvent) => void): this; - on(event: 'popup', callback: (params: PagePopupEvent) => void): this; on(event: 'request', callback: (params: PageRequestEvent) => void): this; on(event: 'requestFailed', callback: (params: PageRequestFailedEvent) => void): this; on(event: 'requestFinished', callback: (params: PageRequestFinishedEvent) => void): this; @@ -829,7 +829,6 @@ export interface PageChannel extends Channel { exposeBinding(params: PageExposeBindingParams, metadata?: Metadata): Promise; goBack(params: PageGoBackParams, metadata?: Metadata): Promise; goForward(params: PageGoForwardParams, metadata?: Metadata): Promise; - opener(params?: PageOpenerParams, metadata?: Metadata): Promise; reload(params: PageReloadParams, metadata?: Metadata): Promise; screenshot(params: PageScreenshotParams, metadata?: Metadata): Promise; setExtraHTTPHeaders(params: PageSetExtraHTTPHeadersParams, metadata?: Metadata): Promise; @@ -884,9 +883,6 @@ export type PageLoadEvent = {}; export type PagePageErrorEvent = { error: SerializedError, }; -export type PagePopupEvent = { - page: PageChannel, -}; export type PageRequestEvent = { request: RequestChannel, }; @@ -989,11 +985,6 @@ export type PageGoForwardOptions = { export type PageGoForwardResult = { response?: ResponseChannel, }; -export type PageOpenerParams = {}; -export type PageOpenerOptions = {}; -export type PageOpenerResult = { - page?: PageChannel, -}; export type PageReloadParams = { timeout?: number, waitUntil?: 'load' | 'domcontentloaded' | 'networkidle', diff --git a/src/protocol/protocol.yml b/src/protocol/protocol.yml index bfef85c51f..79da2fb11f 100644 --- a/src/protocol/protocol.yml +++ b/src/protocol/protocol.yml @@ -644,6 +644,7 @@ Page: width: number height: number isClosed: boolean + opener: Page? commands: @@ -714,10 +715,6 @@ Page: returns: response: Response? - opener: - returns: - page: Page? - reload: parameters: timeout: number? @@ -958,10 +955,6 @@ Page: parameters: error: SerializedError - popup: - parameters: - page: Page - request: parameters: request: Request diff --git a/src/protocol/validator.ts b/src/protocol/validator.ts index 76171b9496..e1247f1c82 100644 --- a/src/protocol/validator.ts +++ b/src/protocol/validator.ts @@ -415,7 +415,6 @@ export function createScheme(tChannel: (name: string) => Validator): Scheme { timeout: tOptional(tNumber), waitUntil: tOptional(tEnum(['load', 'domcontentloaded', 'networkidle'])), }); - scheme.PageOpenerParams = tOptional(tObject({})); scheme.PageReloadParams = tObject({ timeout: tOptional(tNumber), waitUntil: tOptional(tEnum(['load', 'domcontentloaded', 'networkidle'])), diff --git a/src/server/chromium/crPage.ts b/src/server/chromium/crPage.ts index 1ef1a94f3e..e9354c9c0b 100644 --- a/src/server/chromium/crPage.ts +++ b/src/server/chromium/crPage.ts @@ -93,7 +93,13 @@ export class CRPage implements PageDelegate { } // Note: it is important to call |reportAsNew| before resolving pageOrError promise, // so that anyone who awaits pageOrError got a ready and reported page. - this._pagePromise = this._mainFrameSession._initialize(hasUIWindow).then(() => { + this._pagePromise = this._mainFrameSession._initialize(hasUIWindow).then(async r => { + await this._page.initOpener(this._opener); + return r; + }).catch(async e => { + await this._page.initOpener(this._opener); + throw e; + }).then(() => { this._initializedPage = this._page; this._reportAsNew(); return this._page; @@ -146,10 +152,6 @@ export class CRPage implements PageDelegate { return this._pagePromise; } - openerDelegate(): PageDelegate | null { - return this._opener; - } - didClose() { for (const session of this._sessions.values()) session.dispose(); @@ -203,15 +205,6 @@ export class CRPage implements PageDelegate { await this._forAllFrameSessions(frame => frame._setFileChooserIntercepted(enabled)); } - async opener(): Promise { - if (!this._opener) - return null; - const openerPage = await this._opener.pageOrError(); - if (openerPage instanceof Page && !openerPage.isClosed()) - return openerPage; - return null; - } - async reload(): Promise { await this._mainFrameSession._client.send('Page.reload'); } diff --git a/src/server/firefox/ffPage.ts b/src/server/firefox/ffPage.ts index d0f54e672b..6941588c26 100644 --- a/src/server/firefox/ffPage.ts +++ b/src/server/firefox/ffPage.ts @@ -46,6 +46,7 @@ export class FFPage implements PageDelegate { private _pagePromise: Promise; private _pageCallback: (pageOrError: Page | Error) => void = () => {}; _initializedPage: Page | null = null; + private _initializationFailed = false; readonly _opener: FFPage | null; private readonly _contextIdToContext: Map; private _eventListeners: RegisteredListener[]; @@ -91,8 +92,12 @@ export class FFPage implements PageDelegate { helper.addEventListener(this._session, 'Page.webSocketFrameSent', this._onWebSocketFrameSent.bind(this)), ]; this._pagePromise = new Promise(f => this._pageCallback = f); - session.once(FFSessionEvents.Disconnected, () => this._page._didDisconnect()); - this._session.once('Page.ready', () => { + session.once(FFSessionEvents.Disconnected, () => { + this._markAsError(new Error('Page closed')); + this._page._didDisconnect(); + }); + this._session.once('Page.ready', async () => { + await this._page.initOpener(this._opener); // Note: it is important to call |reportAsNew| before resolving pageOrError promise, // so that anyone who awaits pageOrError got a ready and reported page. this._initializedPage = this._page; @@ -104,8 +109,14 @@ export class FFPage implements PageDelegate { this._session.send('Page.addScriptToEvaluateOnNewDocument', { script: '', worldName: UTILITY_WORLD_NAME }).catch(e => this._markAsError(e)); } - _markAsError(error: Error) { + async _markAsError(error: Error) { + // Same error may be report twice: channer disconnected and session.send fails. + if (this._initializationFailed) + return; + this._initializationFailed = true; + if (!this._initializedPage) { + await this._page.initOpener(this._opener); this._page.reportAsNew(error); this._pageCallback(error); } @@ -115,10 +126,6 @@ export class FFPage implements PageDelegate { return this._pagePromise; } - openerDelegate(): PageDelegate | null { - return this._opener; - } - _onWebSocketCreated(event: Protocol.Page.webSocketCreatedPayload) { this._page._frameManager.onWebSocketCreated(webSocketId(event.frameId, event.wsid), event.requestURL); this._page._frameManager.onWebSocketRequest(webSocketId(event.frameId, event.wsid)); @@ -310,8 +317,6 @@ export class FFPage implements PageDelegate { } didClose() { - if (!this._initializedPage) - this._markAsError(new Error('Page has been closed')); this._session.dispose(); helper.removeEventListeners(this._eventListeners); this._networkManager.dispose(); @@ -358,15 +363,6 @@ export class FFPage implements PageDelegate { await this._session.send('Page.setInterceptFileChooserDialog', { enabled }).catch(e => {}); // target can be closed. } - async opener(): Promise { - if (!this._opener) - return null; - const result = await this._opener.pageOrError(); - if (result instanceof Page && !result.isClosed()) - return result; - return null; - } - async reload(): Promise { await this._session.send('Page.reload', { frameId: this._page.mainFrame()._id }); } diff --git a/src/server/page.ts b/src/server/page.ts index a8893acc44..74d1b23b69 100644 --- a/src/server/page.ts +++ b/src/server/page.ts @@ -39,8 +39,6 @@ export interface PageDelegate { readonly rawKeyboard: input.RawKeyboard; readonly rawTouchscreen: input.RawTouchscreen; - opener(): Promise; - reload(): Promise; goBack(): Promise; goForward(): Promise; @@ -48,7 +46,6 @@ export interface PageDelegate { evaluateOnNewDocument(source: string): Promise; closePage(runBeforeUnload: boolean): Promise; pageOrError(): Promise; - openerDelegate(): PageDelegate | null; navigateFrame(frame: frames.Frame, url: string, referrer: string | undefined): Promise; @@ -114,7 +111,6 @@ export class Page extends SdkObject { FrameDetached: 'framedetached', InternalFrameNavigatedToNewDocument: 'internalframenavigatedtonewdocument', Load: 'load', - Popup: 'popup', Video: 'video', WebSocket: 'websocket', Worker: 'worker', @@ -150,6 +146,7 @@ export class Page extends SdkObject { readonly uniqueId: string; _pageIsError: Error | undefined; _video: Artifact | null = null; + _opener: Page | undefined; constructor(delegate: PageDelegate, browserContext: BrowserContext) { super(browserContext); @@ -182,6 +179,14 @@ export class Page extends SdkObject { this.selectors = browserContext.selectors(); } + async initOpener(opener: PageDelegate | null) { + if (!opener) + return; + const openerPage = await opener.pageOrError(); + if (openerPage instanceof Page && !openerPage.isClosed()) + this._opener = openerPage; + } + reportAsNew(error?: Error) { if (error) { // Initialization error could have happened because of @@ -191,13 +196,11 @@ export class Page extends SdkObject { this._setIsError(error); } this._browserContext.emit(BrowserContext.Events.Page, this); - const openerDelegate = this._delegate.openerDelegate(); - if (openerDelegate) { - openerDelegate.pageOrError().then(openerPage => { - if (openerPage instanceof Page && !openerPage.isClosed()) - openerPage.emit(Page.Events.Popup, this); - }); - } + // I may happen that page iniatialization finishes after Close event has already been sent, + // in that case we fire another Close event to ensure that each reported Page will have + // corresponding Close event after it is reported on the context. + if (this.isClosed()) + this.emit(Page.Events.Close); } async _doSlowMo() { @@ -248,8 +251,8 @@ export class Page extends SdkObject { return this._browserContext; } - async opener(): Promise { - return await this._delegate.opener(); + opener(): Page | undefined { + return this._opener; } mainFrame(): frames.Frame { diff --git a/src/server/supplements/recorderSupplement.ts b/src/server/supplements/recorderSupplement.ts index ec3c3e4fe5..5745ee1cad 100644 --- a/src/server/supplements/recorderSupplement.ts +++ b/src/server/supplements/recorderSupplement.ts @@ -304,17 +304,14 @@ export class RecorderSupplement { }); frame.on(Frame.Events.Navigation, () => this._onFrameNavigated(frame, page)); page.on(Page.Events.Download, () => this._onDownload(page)); - page.on(Page.Events.Popup, popup => this._onPopup(page, popup)); page.on(Page.Events.Dialog, () => this._onDialog(page)); const suffix = this._pageAliases.size ? String(++this._lastPopupOrdinal) : ''; const pageAlias = 'page' + suffix; this._pageAliases.set(page, pageAlias); - const isPopup = !!await page.opener(); - // Could happen due to the await above. - if (page.isClosed()) - return; - if (!isPopup) { + if (page.opener()) { + this._onPopup(page.opener()!, page); + } else { this._generator.addAction({ pageAlias, ...describeFrame(page.mainFrame()), diff --git a/src/server/webkit/wkPage.ts b/src/server/webkit/wkPage.ts index 5afb17e540..487f704bf9 100644 --- a/src/server/webkit/wkPage.ts +++ b/src/server/webkit/wkPage.ts @@ -273,10 +273,6 @@ export class WKPage implements PageDelegate { return this._pagePromise; } - openerDelegate(): PageDelegate | null { - return this._opener; - } - private async _onTargetCreated(event: Protocol.Target.targetCreatedPayload) { const { targetInfo } = event; const session = new WKSession(this._pageProxySession.connection, targetInfo.targetId, `The ${targetInfo.type} has been closed.`, (message: any) => { @@ -316,6 +312,7 @@ export class WKPage implements PageDelegate { // Avoid rejection on disconnect. this._firstNonInitialNavigationCommittedPromise.catch(() => {}); } + await this._page.initOpener(this._opener); // Note: it is important to call |reportAsNew| before resolving pageOrError promise, // so that anyone who awaits pageOrError got a ready and reported page. this._initializedPage = pageOrError instanceof Page ? pageOrError : null; @@ -664,15 +661,6 @@ export class WKPage implements PageDelegate { await this._session.send('Page.setInterceptFileChooserDialog', { enabled }).catch(e => {}); // target can be closed. } - async opener(): Promise { - if (!this._opener) - return null; - const openerPage = await this._opener.pageOrError(); - if (openerPage instanceof Page && !openerPage.isClosed()) - return openerPage; - return null; - } - async reload(): Promise { await this._session.send('Page.reload'); }