From 650d73445c4999e8167d6ae6c38607e81969de49 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 13 May 2020 17:20:33 -0700 Subject: [PATCH] fix(actions): do not wait for the created popups (#2219) Since we are recommending Promise.all pattern anyway, this special logic just adds to the possibility of timeout if something goes wrong. For example, Firefox sometimes send Page.willOpenNewWindowAsynchronously later than the new target arrives and input action just hangs. --- src/chromium/crBrowser.ts | 4 ---- src/firefox/ffBrowser.ts | 4 ---- src/firefox/ffPage.ts | 7 +------ src/frames.ts | 21 +-------------------- src/webkit/wkBrowser.ts | 4 ---- src/webkit/wkPage.ts | 15 +-------------- test/autowaiting.spec.js | 36 +++++++----------------------------- 7 files changed, 10 insertions(+), 81 deletions(-) diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index b461a5d35fb71..01cac268ddccd 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -152,10 +152,6 @@ export class CRBrowser extends BrowserBase { const opener = targetInfo.openerId ? this._crPages.get(targetInfo.openerId) || null : null; const crPage = new CRPage(session, targetInfo.targetId, context, opener, this._isHeadful); this._crPages.set(targetInfo.targetId, crPage); - if (opener && opener._initializedPage) { - for (const signalBarrier of opener._initializedPage._frameManager._signalBarriers) - signalBarrier.addPopup(crPage.pageOrError()); - } crPage.pageOrError().then(() => { context!.emit(CommonEvents.BrowserContext.Page, crPage._page); if (opener) { diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index 146d08a40ec20..565551e3c5676 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -99,10 +99,6 @@ export class FFBrowser extends BrowserBase { const ffPage = new FFPage(session, context, opener); this._ffPages.set(targetId, ffPage); - if (opener && opener._initializedPage) { - for (const signalBarrier of opener._initializedPage._frameManager._signalBarriers) - signalBarrier.addPopup(ffPage.pageOrError()); - } ffPage.pageOrError().then(async () => { const page = ffPage._page; context.emit(Events.BrowserContext.Page, page); diff --git a/src/firefox/ffPage.ts b/src/firefox/ffPage.ts index ff67ec9481f14..a5891c95adf36 100644 --- a/src/firefox/ffPage.ts +++ b/src/firefox/ffPage.ts @@ -62,6 +62,7 @@ export class FFPage implements PageDelegate { this._page = new Page(this, browserContext); this._networkManager = new FFNetworkManager(session, this._page); this._page.on(Events.Page.FrameDetached, frame => this._removeContextsForFrame(frame)); + // TODO: remove Page.willOpenNewWindowAsynchronously from the protocol. this._eventListeners = [ helper.addEventListener(this._session, 'Page.eventFired', this._onEventFired.bind(this)), helper.addEventListener(this._session, 'Page.frameAttached', this._onFrameAttached.bind(this)), @@ -73,7 +74,6 @@ export class FFPage implements PageDelegate { helper.addEventListener(this._session, 'Runtime.executionContextCreated', this._onExecutionContextCreated.bind(this)), helper.addEventListener(this._session, 'Runtime.executionContextDestroyed', this._onExecutionContextDestroyed.bind(this)), helper.addEventListener(this._session, 'Page.linkClicked', event => this._onLinkClicked(event.phase)), - helper.addEventListener(this._session, 'Page.willOpenNewWindowAsynchronously', this._onWillOpenNewWindowAsynchronously.bind(this)), helper.addEventListener(this._session, 'Page.uncaughtError', this._onUncaughtError.bind(this)), helper.addEventListener(this._session, 'Runtime.console', this._onConsole.bind(this)), helper.addEventListener(this._session, 'Page.dialogOpened', this._onDialogOpened.bind(this)), @@ -136,11 +136,6 @@ export class FFPage implements PageDelegate { this._page._frameManager.frameDidPotentiallyRequestNavigation(); } - _onWillOpenNewWindowAsynchronously() { - for (const barrier of this._page._frameManager._signalBarriers) - barrier.expectPopup(); - } - _onNavigationStarted(params: Protocol.Page.navigationStartedPayload) { this._page._frameManager.frameRequestedNavigation(params.frameId, params.navigationId); } diff --git a/src/frames.ts b/src/frames.ts index ddd9789579c75..0c7d6b8fb17e9 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -954,10 +954,8 @@ function selectorToString(selector: string, state: 'attached' | 'detached' | 'vi } export class SignalBarrier { - private _frameIds = new Map(); private _options: types.NavigatingActionWaitOptions; private _protectCount = 0; - private _expectedPopups = 0; private _promise: Promise; private _promiseCallback = () => {}; private _deadline: number; @@ -981,23 +979,6 @@ export class SignalBarrier { this.release(); } - async expectPopup() { - ++this._expectedPopups; - } - - async unexpectPopup() { - --this._expectedPopups; - this._maybeResolve(); - } - - async addPopup(pageOrError: Promise) { - if (this._expectedPopups) - --this._expectedPopups; - this.retain(); - await pageOrError; - this.release(); - } - retain() { ++this._protectCount; } @@ -1008,7 +989,7 @@ export class SignalBarrier { } private async _maybeResolve() { - if (!this._protectCount && !this._expectedPopups && !this._frameIds.size) + if (!this._protectCount) this._promiseCallback(); } } diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 4543be21c9f2a..b712c287f9e9e 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -142,10 +142,6 @@ export class WKBrowser extends BrowserBase { const wkPage = new WKPage(context, pageProxySession, opener || null); this._wkPages.set(pageProxyId, wkPage); - if (opener && opener._initializedPage) { - for (const signalBarrier of opener._initializedPage._frameManager._signalBarriers) - signalBarrier.addPopup(wkPage.pageOrError()); - } wkPage.pageOrError().then(async () => { const page = wkPage._page; context!.emit(Events.BrowserContext.Page, page); diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index caa17250c656b..10db5b191573a 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -309,6 +309,7 @@ export class WKPage implements PageDelegate { } private _addSessionListeners() { + // TODO: remove Page.willRequestOpenWindow and Page.didRequestOpenWindow from the protocol. this._sessionListeners = [ helper.addEventListener(this._session, 'Page.frameNavigated', event => this._onFrameNavigated(event.frame, false)), helper.addEventListener(this._session, 'Page.navigatedWithinDocument', event => this._onFrameNavigatedWithinDocument(event.frameId, event.url)), @@ -318,8 +319,6 @@ export class WKPage implements PageDelegate { helper.addEventListener(this._session, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)), helper.addEventListener(this._session, 'Page.loadEventFired', event => this._onLifecycleEvent(event.frameId, 'load')), helper.addEventListener(this._session, 'Page.domContentEventFired', event => this._onLifecycleEvent(event.frameId, 'domcontentloaded')), - helper.addEventListener(this._session, 'Page.willRequestOpenWindow', event => this._onWillRequestOpenWindow()), - helper.addEventListener(this._session, 'Page.didRequestOpenWindow', event => this._onDidRequestOpenWindow(event)), helper.addEventListener(this._session, 'Runtime.executionContextCreated', event => this._onExecutionContextCreated(event.context)), helper.addEventListener(this._session, 'Console.messageAdded', event => this._onConsoleMessage(event)), helper.addEventListener(this._session, 'Console.messageRepeatCountUpdated', event => this._onConsoleRepeatCountUpdated(event)), @@ -363,18 +362,6 @@ export class WKPage implements PageDelegate { this._page._frameManager.frameLifecycleEvent(frameId, event); } - private _onWillRequestOpenWindow() { - for (const barrier of this._page._frameManager._signalBarriers) - barrier.expectPopup(); - } - - private _onDidRequestOpenWindow(event: Protocol.Page.didRequestOpenWindowPayload) { - if (!event.opened) { - for (const barrier of this._page._frameManager._signalBarriers) - barrier.unexpectPopup(); - } - } - private _handleFrameTree(frameTree: Protocol.Page.FrameResourceTree) { this._onFrameAttached(frameTree.frame.id, frameTree.frame.parentId || null); this._onFrameNavigated(frameTree.frame, true); diff --git a/test/autowaiting.spec.js b/test/autowaiting.spec.js index 6bc0ba6182f1a..6f0e36bde8ab8 100644 --- a/test/autowaiting.spec.js +++ b/test/autowaiting.spec.js @@ -34,26 +34,6 @@ describe('Auto waiting', () => { ]); expect(messages.join('|')).toBe('route|navigated|click'); }); - it('should await popup when clicking anchor', async function({page, server}) { - await page.goto(server.EMPTY_PAGE); - await page.setContent('link'); - const messages = []; - await Promise.all([ - page.waitForEvent('popup').then(() => messages.push('popup')), - page.click('a').then(() => messages.push('click')), - ]); - expect(messages.join('|')).toBe('popup|click'); - }); - it('should await popup when clicking anchor with noopener', async function({page, server}) { - await page.goto(server.EMPTY_PAGE); - await page.setContent('link'); - const messages = []; - await Promise.all([ - page.waitForEvent('popup').then(() => messages.push('popup')), - page.click('a').then(() => messages.push('click')), - ]); - expect(messages.join('|')).toBe('popup|click'); - }); it('should await cross-process navigation when clicking anchor', async({page, server}) => { const messages = []; server.setRoute('/empty.html', async (req, res) => { @@ -150,15 +130,6 @@ describe('Auto waiting', () => { ]); expect(messages.join('|')).toBe('route|navigated|evaluate'); }); - it('should await new popup when evaluating', async function({page, server}) { - await page.goto(server.EMPTY_PAGE); - const messages = []; - await Promise.all([ - page.waitForEvent('popup').then(() => messages.push('popup')), - page.evaluate(() => window._popup = window.open(window.location.href)).then(() => messages.push('evaluate')), - ]); - expect(messages.join('|')).toBe('popup|evaluate'); - }); it('should await navigating specified target', async({page, server}) => { const messages = []; server.setRoute('/empty.html', async (req, res) => { @@ -254,5 +225,12 @@ describe('Auto waiting should not hang when', () => { popup.close(); }); }); + it('opening a popup', async function({page, server}) { + await page.goto(server.EMPTY_PAGE); + await Promise.all([ + page.waitForEvent('popup'), + page.evaluate(() => window._popup = window.open(window.location.href)), + ]); + }); });