api(popups): emit PageEvent immediately, and resolve page() once init…

…ialized (#1229)

This way we do not miss any popups, even immediately closed ones.

docs/api.md

@@ -304,7 +304,15 @@ Emitted when Browser context gets closed. This might happen because of one of th
 - <[PageEvent]>
 
 Emitted when a new Page is created in the BrowserContext. The event will also fire for popup
-pages.
+pages. See also [`Page.on('popup')`](#event-popup) to receive events about popups relevant to a specific page.
+
+```js
+const [event] = await Promise.all([
+  context.waitForEvent('page'),
+  page.click('a[target=_blank]'),
+]);
+const newPage = await event.page();
+```
 
 #### browserContext.addInitScript(script[, ...args])
 - `script` <[function]|[string]|[Object]> Script to be evaluated in all pages in the browser context.
@@ -726,22 +734,24 @@ Emitted when the JavaScript [`load`](https://developer.mozilla.org/en-US/docs/We
 Emitted when an uncaught exception happens within the page.
 
 #### event: 'popup'
-- <[Page]> Page corresponding to "popup" window
+- <[PageEvent]> Page event corresponding to "popup" window
 
-Emitted when the page opens a new tab or window.
+Emitted when the page opens a new tab or window. This event is emitted in addition to the [`browserContext.on('page')`](#event-page), but only for popups relevant to this page.
 
 ```js
-const [popup] = await Promise.all([
-  new Promise(resolve => page.once('popup', resolve)),
+const [event] = await Promise.all([
+  page.waitForEvent('popup'),
   page.click('a[target=_blank]'),
 ]);
+const popup = await event.page();
 ```
 
 ```js
-const [popup] = await Promise.all([
-  new Promise(resolve => page.once('popup', resolve)),
+const [event] = await Promise.all([
+  page.waitForEvent('popup'),
   page.evaluate(() => window.open('https://example.com')),
 ]);
+const popup = await event.page();
 ```
 
 #### event: 'request'
@@ -1753,8 +1763,7 @@ This method returns all of the dedicated [WebWorkers](https://developer.mozilla.
 
 ### class: PageEvent
 
-Event object passed to the listeners of ['page'](#event-page) on [`BrowserContext`](#class-browsercontext). Provides access
-to the newly created page.
+Event object passed to the listeners of [`browserContext.on('page')`](#event-page) and [`page.on('popup')`](#event-popup) events. Provides access to the newly created page.
 
 #### pageEvent.page()
 - returns: <[Promise]<[Page]>> Promise which resolves to the created page.

src/chromium/crBrowser.ts

@@ -71,7 +71,7 @@ export class CRBrowser extends platform.EventEmitter implements Browser {
   constructor(connection: CRConnection) {
     super();
     this._connection = connection;
-    this._client = connection.rootSession;
+    this._client = this._connection.rootSession;
 
     this._defaultContext = new CRBrowserContext(this, null, validateBrowserContextOptions({}));
     this._connection.on(ConnectionEvents.Disconnected, () => {
@@ -120,14 +120,18 @@ export class CRBrowser extends platform.EventEmitter implements Browser {
     try {
       switch (targetInfo.type) {
         case 'page': {
-          const page = await target.page();
-          const event = new PageEvent(page!);
+          const event = new PageEvent(target.pageOrError());
           context.emit(CommonEvents.BrowserContext.Page, event);
+          const opener = target.opener();
+          if (!opener)
+            break;
+          const openerPage = await opener.pageOrError();
+          if (openerPage instanceof Page && !openerPage.isClosed())
+            openerPage.emit(CommonEvents.Page.Popup, new PageEvent(target.pageOrError()));
           break;
         }
         case 'background_page': {
-          const page = await target.page();
-          const event = new PageEvent(page!);
+          const event = new PageEvent(target.pageOrError());
           context.emit(Events.CRBrowserContext.BackgroundPage, event);
           break;
         }
@@ -268,16 +272,21 @@ export class CRBrowserContext extends platform.EventEmitter implements BrowserCo
 
   async pages(): Promise<Page[]> {
     const targets = this._browser._allTargets().filter(target => target.context() === this && target.type() === 'page');
-    const pages = await Promise.all(targets.map(target => target.page()));
-    return pages.filter(page => !!page) as Page[];
+    const pages = await Promise.all(targets.map(target => target.pageOrError()));
+    return pages.filter(page => (page instanceof Page) && !page.isClosed()) as Page[];
   }
 
   async newPage(): Promise<Page> {
     assertBrowserContextIsNotOwned(this);
     const { targetId } = await this._browser._client.send('Target.createTarget', { url: 'about:blank', browserContextId: this._browserContextId || undefined });
     const target = this._browser._targets.get(targetId)!;
-    const page = await target.page();
-    return page!;
+    const result = await target.pageOrError();
+    if (result instanceof Page) {
+      if (result.isClosed())
+        throw new Error('Page has been closed.');
+      return result;
+    }
+    throw result;
   }
 
   async cookies(...urls: string[]): Promise<network.NetworkCookie[]> {
@@ -382,8 +391,8 @@ export class CRBrowserContext extends platform.EventEmitter implements BrowserCo
 
   async backgroundPages(): Promise<Page[]> {
     const targets = this._browser._allTargets().filter(target => target.context() === this && target.type() === 'background_page');
-    const pages = await Promise.all(targets.map(target => target.page()));
-    return pages.filter(page => !!page) as Page[];
+    const pages = await Promise.all(targets.map(target => target.pageOrError()));
+    return pages.filter(page => (page instanceof Page) && !page.isClosed()) as Page[];
   }
 
   async createSession(page: Page): Promise<CRSession> {

src/chromium/crPage.ts

@@ -388,7 +388,10 @@ export class CRPage implements PageDelegate {
     const openerTarget = CRTarget.fromPage(this._page).opener();
     if (!openerTarget)
       return null;
-    return await openerTarget.page();
+    const openerPage = await openerTarget.pageOrError();
+    if (openerPage instanceof Page && !openerPage.isClosed())
+      return openerPage;
+    return null;
   }
 
   async reload(): Promise<void> {

src/chromium/crTarget.ts

@@ -17,7 +17,6 @@
 
 import { CRBrowser, CRBrowserContext } from './crBrowser';
 import { CRSession, CRSessionEvents } from './crConnection';
-import { Events } from '../events';
 import { Page, Worker } from '../page';
 import { Protocol } from './protocol';
 import { debugError } from '../helper';
@@ -32,9 +31,8 @@ export class CRTarget {
   private readonly _browserContext: CRBrowserContext;
   readonly _targetId: string;
   readonly sessionFactory: () => Promise<CRSession>;
-  private _pagePromiseFulfill: ((page: Page) => void) | null = null;
-  private _pagePromiseReject: ((error: Error) => void) | null = null;
-  private _pagePromise: Promise<Page> | null = null;
+  private _pagePromiseCallback: ((pageOrError: Page | Error) => void) | null = null;
+  private _pagePromise: Promise<Page | Error> | null = null;
   _crPage: CRPage | null = null;
   private _workerPromise: Promise<Worker> | null = null;
 
@@ -56,44 +54,32 @@ export class CRTarget {
     this._browserContext = browserContext;
     this._targetId = targetInfo.targetId;
     this.sessionFactory = sessionFactory;
-    if (CRTarget.isPageType(targetInfo.type)) {
-      this._pagePromise = new Promise<Page>((fulfill, reject) => {
-        this._pagePromiseFulfill = fulfill;
-        this._pagePromiseReject = reject;
-      });
-    }
+    if (CRTarget.isPageType(targetInfo.type))
+      this._pagePromise = new Promise<Page | Error>(f => this._pagePromiseCallback = f);
   }
 
   _didClose() {
     if (this._crPage)
       this._crPage.didClose();
   }
 
-  async page(): Promise<Page | null> {
-    return this._pagePromise;
-  }
-
   async initializePageSession(session: CRSession) {
     this._crPage = new CRPage(session, this._browser, this._browserContext);
     const page = this._crPage.page();
     (page as any)[targetSymbol] = this;
     session.once(CRSessionEvents.Disconnected, () => page._didDisconnect());
     try {
       await this._crPage.initialize();
-      this._pagePromiseFulfill!(page);
-    } catch (error) {
-      this._pagePromiseReject!(error);
+      this._pagePromiseCallback!(page);
+    } catch (e) {
+      this._pagePromiseCallback!(e);
     }
+  }
 
-    if (this.type() !== 'page')
-      return;
-    const opener = this.opener();
-    if (!opener)
-      return;
-    const openerPage = await opener.page();
-    if (!openerPage)
-      return;
-    openerPage.emit(Events.Page.Popup, page);
+  async pageOrError(): Promise<Page | Error> {
+    if (this._targetInfo.type !== 'page' && this._targetInfo.type !== 'background_page')
+      throw new Error('Not a page.');
+    return this._pagePromise!;
   }
 
   async serviceWorker(): Promise<Worker | null> {

src/firefox/ffBrowser.ts

@@ -18,7 +18,7 @@
 import { Browser, createPageInNewContext } from '../browser';
 import { BrowserContext, BrowserContextOptions, validateBrowserContextOptions, assertBrowserContextIsNotOwned } from '../browserContext';
 import { Events } from '../events';
-import { assert, helper, RegisteredListener, debugError } from '../helper';
+import { assert, helper, RegisteredListener } from '../helper';
 import * as network from '../network';
 import * as types from '../types';
 import { Page, PageEvent, PageBinding } from '../page';
@@ -165,17 +165,16 @@ export class FFBrowser extends platform.EventEmitter implements Browser {
     const {targetId} = payload.targetInfo;
     const target = this._targets.get(targetId)!;
     target._initPagePromise(this._connection.getSession(payload.sessionId)!);
-    const page = await target.page();
-    if (!page)
-      return;
-    target.context().emit(Events.BrowserContext.Page, new PageEvent(page));
+
+    const pageEvent = new PageEvent(target.pageOrError());
+    target.context().emit(Events.BrowserContext.Page, pageEvent);
 
     const opener = target.opener();
-    if (opener && opener._pagePromise) {
-      const openerPage = await opener._pagePromise;
-      if (openerPage.listenerCount(Events.Page.Popup))
-        openerPage.emit(Events.Page.Popup, page);
-    }
+    if (!opener)
+      return;
+    const openerPage = await opener.pageOrError();
+    if (openerPage instanceof Page && !openerPage.isClosed())
+      openerPage.emit(Events.Page.Popup, pageEvent);
   }
 
   async close() {
@@ -192,7 +191,7 @@ export class FFBrowser extends platform.EventEmitter implements Browser {
 }
 
 class Target {
-  _pagePromise?: Promise<Page>;
+  _pagePromise?: Promise<Page | Error>;
   _ffPage: FFPage | null = null;
   private readonly _browser: FFBrowser;
   private readonly _context: FFBrowserContext;
@@ -233,7 +232,7 @@ class Target {
     return this._context;
   }
 
-  async page(): Promise<Page> {
+  async pageOrError(): Promise<Page | Error> {
     if (this._type !== 'page')
       throw new Error(`Cannot create page for "${this._type}" target`);
     if (!this._pagePromise)
@@ -247,12 +246,21 @@ class Target {
         const openerTarget = this.opener();
         if (!openerTarget)
           return null;
-        return await openerTarget.page();
+        const result = await openerTarget.pageOrError();
+        if (result instanceof Page && !result.isClosed())
+          return result;
+        return null;
       });
       const page = this._ffPage._page;
       session.once(FFSessionEvents.Disconnected, () => page._didDisconnect());
-      await this._ffPage._initialize().catch(debugError);
-      f(page);
+      let pageOrError: Page | Error;
+      try {
+        await this._ffPage._initialize();
+        pageOrError = page;
+      } catch (e) {
+        pageOrError = e;
+      }
+      f(pageOrError);
     });
   }
 
@@ -309,8 +317,8 @@ export class FFBrowserContext extends platform.EventEmitter implements BrowserCo
 
   async pages(): Promise<Page[]> {
     const targets = this._browser._allTargets().filter(target => target.context() === this && target.type() === 'page');
-    const pages = await Promise.all(targets.map(target => target.page()));
-    return pages.filter(page => !!page);
+    const pages = await Promise.all(targets.map(target => target.pageOrError()));
+    return pages.filter(page => page instanceof Page && !page.isClosed()) as Page[];
   }
 
   async newPage(): Promise<Page> {
@@ -319,7 +327,13 @@ export class FFBrowserContext extends platform.EventEmitter implements BrowserCo
       browserContextId: this._browserContextId || undefined
     });
     const target = this._browser._targets.get(targetId)!;
-    return target.page();
+    const result = await target.pageOrError();
+    if (result instanceof Page) {
+      if (result.isClosed())
+        throw new Error('Page has been closed.');
+      return result;
+    }
+    throw result;
   }
 
   async cookies(...urls: string[]): Promise<network.NetworkCookie[]> {

src/page.ts

@@ -89,14 +89,20 @@ export type FileChooser = {
 };
 
 export class PageEvent {
-  private readonly _page: Page;
+  private readonly _pageOrError: Promise<Page | Error>;
 
-  constructor(page: Page) {
-    this._page = page;
+  constructor(pageOrErrorPromise: Promise<Page | Error>) {
+    this._pageOrError = pageOrErrorPromise;
   }
 
   async page(/* options?: frames.NavigateOptions */): Promise<Page> {
-    return this._page;
+    const result = await this._pageOrError;
+    if (result instanceof Page) {
+      if (result.isClosed())
+        throw new Error('Page has been closed.');
+      return result;
+    }
+    throw result;
   }
 }
 

src/server/chromium.ts

@@ -70,10 +70,16 @@ export class Chromium implements BrowserType {
     const { timeout = 30000 } = options || {};
     const { browserServer, transport } = await this._launchServer(options, 'persistent', userDataDir);
     const browser = await CRBrowser.connect(transport!, true);
-    const firstPage = new Promise(r => browser._defaultContext.once(Events.BrowserContext.Page, r));
+    const browserContext = browser._defaultContext;
+
+    function targets() {
+      return browser._allTargets().filter(target => target.context() === browserContext && target.type() === 'page');
+    }
+    const firstTarget = targets().length ? Promise.resolve() : new Promise(f => browserContext.once('page', f));
+    const firstPage = firstTarget.then(() => targets()[0].pageOrError());
     await helper.waitWithTimeout(firstPage, 'first page', timeout);
+
     // Hack: for typical launch scenario, ensure that close waits for actual process termination.
-    const browserContext = browser._defaultContext;
     browserContext.close = () => browserServer.close();
     return browserContext;
   }