From 45a175d8fe7497ba747d7683df783802dc3fc34d Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 23 Mar 2020 13:50:04 -0700 Subject: [PATCH] fix(chromium): ignore lifecycle events for the initial empty page (#1486) --- src/chromium/crBrowser.ts | 23 +---------------------- src/chromium/crPage.ts | 21 ++++++++++++++++----- src/chromium/crTarget.ts | 8 +++----- test/emulation.spec.js | 4 ++-- 4 files changed, 22 insertions(+), 34 deletions(-) diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index f235aba915..02fcbc45e1 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -40,7 +40,6 @@ export class CRBrowser extends platform.EventEmitter implements Browser { _targets = new Map(); readonly _firstPagePromise: Promise; private _firstPageCallback = () => {}; - private _popupOpeners: string[] = []; private _tracingRecording = false; private _tracingPath: string | null = ''; @@ -105,15 +104,6 @@ export class CRBrowser extends platform.EventEmitter implements Browser { return createPageInNewContext(this, options); } - _onWindowOpen(openerId: string, payload: Protocol.Page.windowOpenPayload) { - // window.open($url) with noopener forces a new browser-initiated window, not - // a renderer-initiated one. When url is about:blank, we only get an - // initial navigation, and no real navigation to $url. - if (payload.windowFeatures.includes('noopener') && payload.url.startsWith('about:blank')) - return; - this._popupOpeners.push(openerId); - } - _onAttachedToTarget({targetInfo, sessionId, waitingForDebugger}: Protocol.Target.attachedToTargetPayload) { const session = this._connection.session(sessionId)!; if (!CRTarget.isPageType(targetInfo.type) && targetInfo.type !== 'service_worker') { @@ -159,18 +149,7 @@ export class CRBrowser extends platform.EventEmitter implements Browser { private _createTarget(targetInfo: Protocol.Target.TargetInfo, session: CRSession) { const {browserContextId} = targetInfo; const context = (browserContextId && this._contexts.has(browserContextId)) ? this._contexts.get(browserContextId)! : this._defaultContext; - let hasInitialAboutBlank = false; - if (CRTarget.isPageType(targetInfo.type) && targetInfo.openerId) { - const openerIndex = this._popupOpeners.indexOf(targetInfo.openerId); - if (openerIndex !== -1) { - this._popupOpeners.splice(openerIndex, 1); - // When this page is a result of window.open($url) call, we should have it's opener - // in the list of popup openers. In this case we know there is an initial - // about:blank navigation, followed by a navigation to $url. - hasInitialAboutBlank = true; - } - } - const target = new CRTarget(this, targetInfo, context, session, hasInitialAboutBlank); + const target = new CRTarget(this, targetInfo, context, session); assert(!this._targets.has(targetInfo.targetId), 'Target should not exist before targetCreated'); this._targets.set(targetInfo.targetId, target); return { context, target }; diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index e7c3483aea..270efec7d6 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -69,7 +69,8 @@ export class CRPage implements PageDelegate { this._firstNonInitialNavigationCommittedPromise = new Promise(f => this._firstNonInitialNavigationCommittedCallback = f); } - async initialize(hasInitialAboutBlank: boolean) { + async initialize() { + let lifecycleEventsEnabled: Promise; const promises: Promise[] = [ this._client.send('Page.enable'), this._client.send('Page.getFrameTree').then(({frameTree}) => { @@ -84,7 +85,6 @@ export class CRPage implements PageDelegate { helper.addEventListener(this._client, 'Page.frameRequestedNavigation', event => this._onFrameRequestedNavigation(event)), helper.addEventListener(this._client, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)), helper.addEventListener(this._client, 'Page.javascriptDialogOpening', event => this._onDialog(event)), - helper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event)), helper.addEventListener(this._client, 'Page.navigatedWithinDocument', event => this._onFrameNavigatedWithinDocument(event.frameId, event.url)), helper.addEventListener(this._client, 'Runtime.bindingCalled', event => this._onBindingCalled(event)), helper.addEventListener(this._client, 'Runtime.consoleAPICalled', event => this._onConsoleAPI(event)), @@ -105,9 +105,21 @@ export class CRPage implements PageDelegate { for (const binding of this._browserContext._pageBindings.values()) frame.evaluate(binding.source).catch(debugError); } + const isInitialEmptyPage = this._page.mainFrame().url() === ':'; + if (isInitialEmptyPage) { + // Ignore lifecycle events for the initial empty page. It is never the final page + // hence we are going to get more lifecycle updates after the actual navigation has + // started (even if the target url is about:blank). + lifecycleEventsEnabled.then(() => { + this._eventListeners.push(helper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event))); + }); + } else { + this._firstNonInitialNavigationCommittedCallback(); + this._eventListeners.push(helper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event))); + } }), this._client.send('Log.enable', {}), - this._client.send('Page.setLifecycleEventsEnabled', { enabled: true }), + lifecycleEventsEnabled = this._client.send('Page.setLifecycleEventsEnabled', { enabled: true }), this._client.send('Runtime.enable', {}), this._client.send('Page.addScriptToEvaluateOnNewDocument', { source: `//# sourceURL=${EVALUATION_SCRIPT_URL}`, @@ -145,8 +157,7 @@ export class CRPage implements PageDelegate { for (const source of this._browserContext._evaluateOnNewDocumentSources) promises.push(this.evaluateOnNewDocument(source)); promises.push(this._client.send('Runtime.runIfWaitingForDebugger')); - if (hasInitialAboutBlank) - promises.push(this._firstNonInitialNavigationCommittedPromise); + promises.push(this._firstNonInitialNavigationCommittedPromise); await Promise.all(promises); } diff --git a/src/chromium/crTarget.ts b/src/chromium/crTarget.ts index 19183b890c..4075902b05 100644 --- a/src/chromium/crTarget.ts +++ b/src/chromium/crTarget.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { assert, helper } from '../helper'; +import { assert } from '../helper'; import { Page, Worker } from '../page'; import { CRBrowser, CRBrowserContext } from './crBrowser'; import { CRSession, CRSessionEvents } from './crConnection'; @@ -48,19 +48,17 @@ export class CRTarget { browser: CRBrowser, targetInfo: Protocol.Target.TargetInfo, browserContext: CRBrowserContext, - session: CRSession, - hasInitialAboutBlank: boolean) { + session: CRSession) { this._targetInfo = targetInfo; this._browser = browser; this._browserContext = browserContext; this._targetId = targetInfo.targetId; if (CRTarget.isPageType(targetInfo.type)) { this._crPage = new CRPage(session, this._browser, this._browserContext); - helper.addEventListener(session, 'Page.windowOpen', event => browser._onWindowOpen(targetInfo.targetId, event)); const page = this._crPage.page(); (page as any)[targetSymbol] = this; session.once(CRSessionEvents.Disconnected, () => page._didDisconnect()); - this._pagePromise = this._crPage.initialize(hasInitialAboutBlank).then(() => this._initializedPage = page).catch(e => e); + this._pagePromise = this._crPage.initialize().then(() => this._initializedPage = page).catch(e => e); } else if (targetInfo.type === 'service_worker') { this._workerPromise = this._initializeServiceWorker(session); } else { diff --git a/test/emulation.spec.js b/test/emulation.spec.js index cac8194649..1acbe2a6ac 100644 --- a/test/emulation.spec.js +++ b/test/emulation.spec.js @@ -342,7 +342,7 @@ module.exports.describe = function({testRunner, expect, playwright, headless, FF await context.close(); } }); - it.fail(CHROMIUM || FFOX)('should format number in popups', async({browser, server}) => { + it.fail(FFOX)('should format number in popups', async({browser, server}) => { const context = await browser.newContext({ locale: 'fr-CH' }); const page = await context.newPage(); await page.goto(server.EMPTY_PAGE); @@ -356,7 +356,7 @@ module.exports.describe = function({testRunner, expect, playwright, headless, FF expect(result).toBe('1 000 000,5'); await context.close(); }); - it.fail(CHROMIUM)('should affect navigator.language in popups', async({browser, server}) => { + it('should affect navigator.language in popups', async({browser, server}) => { const context = await browser.newContext({ locale: 'fr-CH' }); const page = await context.newPage(); await page.goto(server.EMPTY_PAGE);