fix(PageEvent): properly wait for initial navigation in chromium and …

…webkit (#1412)

package.json

@@ -10,7 +10,7 @@
   "playwright": {
     "chromium_revision": "750417",
     "firefox_revision": "1043",
-    "webkit_revision": "1179"
+    "webkit_revision": "1180"
   },
   "scripts": {
     "ctest": "cross-env BROWSER=chromium node test/test.js",

src/chromium/crBrowser.ts

@@ -40,6 +40,7 @@ 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 = '';
@@ -104,6 +105,15 @@ 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)) {
@@ -148,7 +158,18 @@ export class CRBrowser extends platform.EventEmitter implements Browser {
   private _createTarget(targetInfo: Protocol.Target.TargetInfo, session: CRSession | null) {
     const {browserContextId} = targetInfo;
     const context = (browserContextId && this._contexts.has(browserContextId)) ? this._contexts.get(browserContextId)! : this._defaultContext;
-    const target = new CRTarget(this, targetInfo, context, session, () => this._connection.createSession(targetInfo));
+    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, () => this._connection.createSession(targetInfo), hasInitialAboutBlank);
     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

@@ -53,6 +53,8 @@ export class CRPage implements PageDelegate {
   private readonly _pdf: CRPDF;
   private readonly _coverage: CRCoverage;
   private readonly _browserContext: CRBrowserContext;
+  private _firstNonInitialNavigationCommittedPromise: Promise<void>;
+  private _firstNonInitialNavigationCommittedCallback = () => {};
 
   constructor(client: CRSession, browser: CRBrowser, browserContext: CRBrowserContext) {
     this._client = client;
@@ -64,9 +66,10 @@ export class CRPage implements PageDelegate {
     this._browserContext = browserContext;
     this._page = new Page(this, browserContext);
     this._networkManager = new CRNetworkManager(client, this._page);
+    this._firstNonInitialNavigationCommittedPromise = new Promise(f => this._firstNonInitialNavigationCommittedCallback = f);
   }
 
-  async initialize() {
+  async initialize(hasInitialAboutBlank: boolean) {
     const promises: Promise<any>[] = [
       this._client.send('Page.enable'),
       this._client.send('Page.getFrameTree').then(({frameTree}) => {
@@ -142,6 +145,8 @@ 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);
     await Promise.all(promises);
   }
 
@@ -189,6 +194,8 @@ export class CRPage implements PageDelegate {
 
   _onFrameNavigated(framePayload: Protocol.Page.Frame, initial: boolean) {
     this._page._frameManager.frameCommittedNewDocumentNavigation(framePayload.id, framePayload.url, framePayload.name || '', framePayload.loaderId, initial);
+    if (!initial)
+      this._firstNonInitialNavigationCommittedCallback();
   }
 
   _onFrameRequestedNavigation(payload: Protocol.Page.frameRequestedNavigationPayload) {

src/chromium/crTarget.ts

@@ -19,7 +19,7 @@ import { CRBrowser, CRBrowserContext } from './crBrowser';
 import { CRSession, CRSessionEvents } from './crConnection';
 import { Page, Worker } from '../page';
 import { Protocol } from './protocol';
-import { debugError, assert } from '../helper';
+import { debugError, assert, helper } from '../helper';
 import { CRPage } from './crPage';
 import { CRExecutionContext } from './crExecutionContext';
 
@@ -49,7 +49,8 @@ export class CRTarget {
     targetInfo: Protocol.Target.TargetInfo,
     browserContext: CRBrowserContext,
     session: CRSession | null,
-    sessionFactory: () => Promise<CRSession>) {
+    sessionFactory: () => Promise<CRSession>,
+    hasInitialAboutBlank: boolean) {
     this._targetInfo = targetInfo;
     this._browser = browser;
     this._browserContext = browserContext;
@@ -58,10 +59,11 @@ export class CRTarget {
     if (CRTarget.isPageType(targetInfo.type)) {
       assert(session, 'Page target must be created with existing session');
       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().then(() => this._initializedPage = page).catch(e => e);
+      this._pagePromise = this._crPage.initialize(hasInitialAboutBlank).then(() => this._initializedPage = page).catch(e => e);
     }
   }
 

src/webkit/wkBrowser.ts

@@ -38,6 +38,7 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
   readonly _contexts = new Map<string, WKBrowserContext>();
   readonly _wkPages = new Map<string, WKPage>();
   private readonly _eventListeners: RegisteredListener[];
+  private _popupOpeners: string[] = [];
 
   private _firstPageCallback: () => void = () => {};
   private readonly _firstPagePromise: Promise<void>;
@@ -59,6 +60,7 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
       helper.addEventListener(this._browserSession, 'Playwright.pageProxyCreated', this._onPageProxyCreated.bind(this)),
       helper.addEventListener(this._browserSession, 'Playwright.pageProxyDestroyed', this._onPageProxyDestroyed.bind(this)),
       helper.addEventListener(this._browserSession, 'Playwright.provisionalLoadFailed', event => this._onProvisionalLoadFailed(event)),
+      helper.addEventListener(this._browserSession, 'Playwright.windowOpen', this._onWindowOpen.bind(this)),
       helper.addEventListener(this._browserSession, kPageProxyMessageReceived, this._onPageProxyMessageReceived.bind(this)),
     ];
 
@@ -98,6 +100,10 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
     return this._firstPagePromise;
   }
 
+  _onWindowOpen(payload: Protocol.Playwright.windowOpenPayload) {
+    this._popupOpeners.push(payload.pageProxyId);
+  }
+
   _onPageProxyCreated(event: Protocol.Playwright.pageProxyCreatedPayload) {
     const { pageProxyInfo } = event;
     const pageProxyId = pageProxyInfo.pageProxyId;
@@ -117,7 +123,18 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
       this._connection.rawSend({ ...message, pageProxyId });
     });
     const opener = pageProxyInfo.openerId ? this._wkPages.get(pageProxyInfo.openerId) : undefined;
-    const wkPage = new WKPage(context, pageProxySession, opener || null);
+    let hasInitialAboutBlank = false;
+    if (pageProxyInfo.openerId) {
+      const openerIndex = this._popupOpeners.indexOf(pageProxyInfo.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 wkPage = new WKPage(context, pageProxySession, opener || null, hasInitialAboutBlank);
     this._wkPages.set(pageProxyId, wkPage);
 
     const pageEvent = new PageEvent(context, wkPage.pageOrError());

src/webkit/wkPage.ts

@@ -57,10 +57,14 @@ export class WKPage implements PageDelegate {
   private readonly _evaluateOnNewDocumentSources: string[] = [];
   readonly _browserContext: WKBrowserContext;
   private _initialized = false;
+  private _hasInitialAboutBlank: boolean;
+  private _firstNonInitialNavigationCommittedPromise: Promise<void>;
+  private _firstNonInitialNavigationCommittedCallback = () => {};
 
-  constructor(browserContext: WKBrowserContext, pageProxySession: WKSession, opener: WKPage | null) {
+  constructor(browserContext: WKBrowserContext, pageProxySession: WKSession, opener: WKPage | null, hasInitialAboutBlank: boolean) {
     this._pageProxySession = pageProxySession;
     this._opener = opener;
+    this._hasInitialAboutBlank = hasInitialAboutBlank;
     this.rawKeyboard = new RawKeyboardImpl(pageProxySession);
     this.rawMouse = new RawMouseImpl(pageProxySession);
     this._contextIdToContext = new Map();
@@ -76,6 +80,7 @@ export class WKPage implements PageDelegate {
       helper.addEventListener(this._pageProxySession, 'Target.didCommitProvisionalTarget', this._onDidCommitProvisionalTarget.bind(this)),
     ];
     this._pagePromise = new Promise(f => this._pagePromiseCallback = f);
+    this._firstNonInitialNavigationCommittedPromise = new Promise(f => this._firstNonInitialNavigationCommittedCallback = f);
   }
 
   _initializedPage(): Page | null {
@@ -251,6 +256,8 @@ export class WKPage implements PageDelegate {
       }
       if (targetInfo.isPaused)
         this._pageProxySession.send('Target.resume', { targetId: targetInfo.targetId }).catch(debugError);
+      if (this._hasInitialAboutBlank)
+        await this._firstNonInitialNavigationCommittedPromise;
       this._initialized = true;
       this._pagePromiseCallback(pageOrError);
     } else {
@@ -330,6 +337,8 @@ export class WKPage implements PageDelegate {
   private _handleFrameTree(frameTree: Protocol.Page.FrameResourceTree) {
     this._onFrameAttached(frameTree.frame.id, frameTree.frame.parentId || null);
     this._onFrameNavigated(frameTree.frame, true);
+    this._page._frameManager.frameLifecycleEvent(frameTree.frame.id, 'domcontentloaded');
+    this._page._frameManager.frameLifecycleEvent(frameTree.frame.id, 'load');
 
     if (!frameTree.childFrames)
       return;
@@ -348,6 +357,8 @@ export class WKPage implements PageDelegate {
     if (!framePayload.parentId)
       this._workers.clear();
     this._page._frameManager.frameCommittedNewDocumentNavigation(framePayload.id, framePayload.url, framePayload.name || '', framePayload.loaderId, initial);
+    if (!initial)
+      this._firstNonInitialNavigationCommittedCallback();
   }
 
   private _onFrameNavigatedWithinDocument(frameId: string, url: string) {

test/browsercontext.spec.js

@@ -480,7 +480,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF
   });
 
   describe('Events.BrowserContext.PageEvent', function() {
-    it.fail(true)('should have url with nowait', async({browser, server}) => {
+    it.fail(FFOX)('should have url with nowait', async({browser, server}) => {
       const context = await browser.newContext();
       const page = await context.newPage();
       const [otherPage] = await Promise.all([
@@ -490,7 +490,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF
       expect(otherPage.url()).toBe(server.EMPTY_PAGE);
       await context.close();
     });
-    it.fail(CHROMIUM)('should have url with domcontentloaded', async({browser, server}) => {
+    it('should have url with domcontentloaded', async({browser, server}) => {
       const context = await browser.newContext();
       const page = await context.newPage();
       const [otherPage] = await Promise.all([
@@ -500,6 +500,26 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF
       expect(otherPage.url()).toBe(server.EMPTY_PAGE);
       await context.close();
     });
+    it('should have about:blank url with domcontentloaded', async({browser, server}) => {
+      const context = await browser.newContext();
+      const page = await context.newPage();
+      const [otherPage] = await Promise.all([
+        context.waitForEvent('page').then(event => event.page({ waitUntil: 'domcontentloaded' })),
+        page.evaluate(url => window.open(url), 'about:blank')
+      ]);
+      expect(otherPage.url()).toBe('about:blank');
+      await context.close();
+    });
+    it.fail(FFOX)('should have about:blank for empty url with domcontentloaded', async({browser, server}) => {
+      const context = await browser.newContext();
+      const page = await context.newPage();
+      const [otherPage] = await Promise.all([
+        context.waitForEvent('page').then(event => event.page({ waitUntil: 'domcontentloaded' })),
+        page.evaluate(() => window.open())
+      ]);
+      expect(otherPage.url()).toBe('about:blank');
+      await context.close();
+    });
     it('should report when a new page is created and closed', async({browser, server}) => {
       const context = await browser.newContext();
       const page = await context.newPage();
@@ -527,7 +547,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF
     });
     it('should report initialized pages', async({browser, server}) => {
       const context = await browser.newContext();
-      const pagePromise = context.waitForEvent('page').then(e => e.page({ waitUntil: 'nowait' }));
+      const pagePromise = context.waitForEvent('page').then(e => e.page());
       context.newPage();
       const newPage = await pagePromise;
       expect(newPage.url()).toBe('about:blank');
@@ -576,7 +596,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF
       const context = await browser.newContext();
       const events = [];
       context.on('page', async event => {
-        const page = await event.page({ waitUntil: 'nowait' });
+        const page = await event.page();
         events.push('CREATED: ' + page.url());
         page.on('close', () => events.push('DESTROYED: ' + page.url()));
       });

test/page.spec.js

@@ -224,13 +224,14 @@ module.exports.describe = function({testRunner, expect, headless, playwright, FF
       });
     });
     // @see https://github.com/GoogleChrome/puppeteer/issues/3865
-    it('should not throw when there are console messages in detached iframes', async({page, server}) => {
+    it.fail(FFOX)('should not throw when there are console messages in detached iframes', async({page, server}) => {
       await page.goto(server.EMPTY_PAGE);
       const [popup] = await Promise.all([
         page.waitForEvent('popup').then(e => e.page()),
         page.evaluate(async() => {
           // 1. Create a popup that Playwright is not connected to.
-          const win = window.open(window.location.href);
+          const win = window.open('');
+          window._popup = win;
           if (window.document.readyState !== 'complete')
             await new Promise(f => window.addEventListener('load', f));
           // 2. In this popup, create an iframe that console.logs a message.