fix(chromium): ignore lifecycle events for the initial empty page (#1486)

Author: yury-s

src/chromium/crBrowser.ts

@@ -40,7 +40,6 @@ export class CRBrowser extends platform.EventEmitter implements Browser {
   _targets = new Map<string, CRTarget>();
   readonly _firstPagePromise: Promise<void>;
   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 };

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<any>;
     const promises: Promise<any>[] = [
       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);
   }
 

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 {

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);