fix(api): fire BrowserContext.Page event in WebKit and Firefox (#1186)

src/chromium/crBrowser.ts

@@ -97,7 +97,7 @@ export class CRBrowser extends platform.EventEmitter implements Browser {
         case 'page': {
           const page = await target.page();
           const event = new PageEvent(page!);
-          context.emit(CommonEvents.BrowserContext.PageEvent, event);
+          context.emit(CommonEvents.BrowserContext.Page, event);
           break;
         }
         case 'background_page': {

src/events.ts

@@ -22,7 +22,7 @@ export const Events = {
 
   BrowserContext: {
     Close: 'close',
-    PageEvent: 'page',
+    Page: 'page',
   },
 
   BrowserServer: {

src/firefox/ffBrowser.ts

@@ -21,7 +21,7 @@ import { Events } from '../events';
 import { assert, helper, RegisteredListener, debugError } from '../helper';
 import * as network from '../network';
 import * as types from '../types';
-import { Page } from '../page';
+import { Page, PageEvent } from '../page';
 import { ConnectionEvents, FFConnection, FFSessionEvents, FFSession } from './ffConnection';
 import { FFPage } from './ffPage';
 import * as platform from '../platform';
@@ -162,13 +162,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 opener = target.opener();
     if (opener && opener._pagePromise) {
       const openerPage = await opener._pagePromise;
-      if (openerPage.listenerCount(Events.Page.Popup)) {
+      if (openerPage.listenerCount(Events.Page.Popup))
-        const popupPage = await target.page();
+        openerPage.emit(Events.Page.Popup, page);
-        openerPage.emit(Events.Page.Popup, popupPage);
-      }
     }
   }
 
@@ -189,14 +192,14 @@ class Target {
   _pagePromise?: Promise<Page>;
   _ffPage: FFPage | null = null;
   private readonly _browser: FFBrowser;
-  private readonly _context: BrowserContext;
+  private readonly _context: FFBrowserContext;
   private readonly _connection: FFConnection;
   private readonly _targetId: string;
   private readonly _type: 'page' | 'browser';
   _url: string;
   private readonly _openerId: string | undefined;
 
-  constructor(connection: any, browser: FFBrowser, context: BrowserContext, targetId: string, type: 'page' | 'browser', url: string, openerId: string | undefined) {
+  constructor(connection: any, browser: FFBrowser, context: FFBrowserContext, targetId: string, type: 'page' | 'browser', url: string, openerId: string | undefined) {
     this._browser = browser;
     this._context = context;
     this._connection = connection;
@@ -223,7 +226,7 @@ class Target {
     return this._url;
   }
 
-  context(): BrowserContext {
+  context(): FFBrowserContext {
     return this._context;
   }
 

src/server/chromium.ts

@@ -70,7 +70,7 @@ export class Chromium implements BrowserType {
     const { timeout = 30000 } = options || {};
     const { browserServer, transport } = await this._launchServer(options, 'persistent', userDataDir);
     const browser = await CRBrowser.connect(transport!);
-    const firstPage = new Promise(r => browser._defaultContext.once(Events.BrowserContext.PageEvent, r));
+    const firstPage = new Promise(r => browser._defaultContext.once(Events.BrowserContext.Page, r));
     await helper.waitWithTimeout(firstPage, 'first page', timeout);
     // Hack: for typical launch scenario, ensure that close waits for actual process termination.
     const browserContext = browser._defaultContext;

src/webkit/wkBrowser.ts

@@ -17,9 +17,9 @@
 
 import { Browser, createPageInNewContext } from '../browser';
 import { BrowserContext, BrowserContextOptions, validateBrowserContextOptions, assertBrowserContextIsNotOwned, verifyGeolocation } from '../browserContext';
-import { assert, helper, RegisteredListener } from '../helper';
+import { assert, helper, RegisteredListener, debugError } from '../helper';
 import * as network from '../network';
-import { Page } from '../page';
+import { Page, PageEvent } from '../page';
 import { ConnectionTransport, SlowMoTransport } from '../transport';
 import * as types from '../types';
 import { Events } from '../events';
@@ -102,13 +102,13 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
   _onPageProxyCreated(event: Protocol.Browser.pageProxyCreatedPayload) {
     const { pageProxyInfo } = event;
     const pageProxyId = pageProxyInfo.pageProxyId;
-    let context = null;
+    let context: WKBrowserContext | null = null;
     if (pageProxyInfo.browserContextId) {
       // FIXME: we don't know about the default context id, so assume that all targets from
       // unknown contexts are created in the 'default' context which can in practice be represented
       // by multiple actual contexts in WebKit. Solving this properly will require adding context
       // lifecycle events.
-      context = this._contexts.get(pageProxyInfo.browserContextId);
+      context = this._contexts.get(pageProxyInfo.browserContextId) || null;
     }
     if (!context && !this._attachToDefaultContext)
       return;
@@ -125,6 +125,18 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
       this._firstPageProxyCallback();
       this._firstPageProxyCallback = undefined;
     }
+
+    pageProxy.page().then(async page => {
+      if (!page)
+        return;
+      context!.emit(Events.BrowserContext.Page, new PageEvent(page));
+      if (!opener)
+        return;
+      const openerPage = await opener.page();
+      if (!openerPage || page.isClosed())
+        return;
+      openerPage.emit(Events.Page.Popup, page);
+    }).catch(debugError); // Just not emit the event in case of initialization failure.
   }
 
   _onPageProxyDestroyed(event: Protocol.Browser.pageProxyDestroyedPayload) {

src/webkit/wkPageProxy.ts

@@ -14,13 +14,12 @@
  * limitations under the License.
  */
 
+import { assert, debugError, helper, RegisteredListener } from '../helper';
 import { Page } from '../page';
 import { Protocol } from './protocol';
+import { WKBrowserContext } from './wkBrowser';
 import { WKSession } from './wkConnection';
 import { WKPage } from './wkPage';
-import { RegisteredListener, helper, assert, debugError } from '../helper';
-import { Events } from '../events';
-import { WKBrowserContext } from './wkBrowser';
 
 const isPovisionalSymbol = Symbol('isPovisional');
 
@@ -122,19 +121,12 @@ export class WKPageProxy {
         if (!this._pageProxySession.isDisposed())
           error = e;
       }
+      if (targetInfo.isPaused)
+        this._resumeTarget(targetInfo.targetId);
       if (error)
         this._pagePromiseReject(error);
       else
         this._pagePromiseFulfill(page);
-      if (targetInfo.isPaused)
-        this._resumeTarget(targetInfo.targetId);
-      if (page && this._opener) {
-        this._opener.page().then(openerPage => {
-          if (!openerPage || page!.isClosed())
-            return;
-          openerPage.emit(Events.Page.Popup, page);
-        });
-      }
     } else {
       assert(targetInfo.isProvisional);
       (session as any)[isPovisionalSymbol] = true;

test/browsercontext.spec.js

@@ -20,7 +20,7 @@ const utils = require('./utils');
 /**
  * @type {BrowserTestSuite}
  */
-module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, WEBKIT}) {
+module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FFOX, WEBKIT}) {
   const {describe, xdescribe, fdescribe} = testRunner;
   const {it, fit, xit, dit} = testRunner;
   const {beforeAll, beforeEach, afterAll, afterEach} = testRunner;
@@ -292,4 +292,121 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, WE
       await context.close();
     });
   });
+
+  describe('BrowserContext.pages()', function() {
+    it('should return all of the pages', async({browser, server}) => {
+      const context = await browser.newContext();
+      const page = await context.newPage();
+      const second = await context.newPage();
+      const allPages = await context.pages();
+      expect(allPages.length).toBe(2);
+      expect(allPages).toContain(page);
+      expect(allPages).toContain(second);
+      await context.close();
+    });
+  });
+
+  describe('Events.BrowserContext.Page', function() {
+    it('should report when a new page is created and closed', async({browser, server}) => {
+      const context = await browser.newContext();
+      const page = await context.newPage();
+      const [otherPage] = await Promise.all([
+        new Promise(r => context.once('page', async event => r(await event.page()))),
+        page.evaluate(url => window.open(url), server.CROSS_PROCESS_PREFIX + '/empty.html').catch(e => console.log('eee = ' + e)),
+      ]);
+      await otherPage.waitForLoadState();
+      expect(otherPage.url()).toContain(server.CROSS_PROCESS_PREFIX);
+      expect(await otherPage.evaluate(() => ['Hello', 'world'].join(' '))).toBe('Hello world');
+      expect(await otherPage.$('body')).toBeTruthy();
+
+      let allPages = await context.pages();
+      expect(allPages).toContain(page);
+      expect(allPages).toContain(otherPage);
+
+      let closeEventReceived;
+      otherPage.once('close', () => closeEventReceived = true);
+      await otherPage.close();
+      expect(closeEventReceived).toBeTruthy();
+
+      allPages = await context.pages();
+      expect(allPages).toContain(page);
+      expect(allPages).not.toContain(otherPage);
+      await context.close();
+    });
+    it('should not report uninitialized pages', async({browser, server}) => {
+      const context = await browser.newContext();
+      const pagePromise = new Promise(fulfill => context.once('page', async event => fulfill(await event.page())));
+      context.newPage();
+      const newPage = await pagePromise;
+      expect(newPage.url()).toBe('about:blank');
+
+      const popupPromise = new Promise(fulfill => context.once('page', async event => fulfill(await event.page())));
+      const evaluatePromise = newPage.evaluate(() => window.open('about:blank'));
+      const popup = await popupPromise;
+      await popup.waitForLoadState();
+      expect(popup.url()).toBe('about:blank');
+      await evaluatePromise;
+      await context.close();
+    });
+    it('should not crash while redirecting if original request was missed', async({browser, server}) => {
+      const context = await browser.newContext();
+      const page = await context.newPage();
+      let serverResponse = null;
+      server.setRoute('/one-style.css', (req, res) => serverResponse = res);
+      // Open a new page. Use window.open to connect to the page later.
+      const [newPage] = await Promise.all([
+        new Promise(fulfill => context.once('page', async event => fulfill(await event.page()))),
+        page.evaluate(url => window.open(url), server.PREFIX + '/one-style.html'),
+        server.waitForRequest('/one-style.css')
+      ]);
+      // Issue a redirect.
+      serverResponse.writeHead(302, { location: '/injectedstyle.css' });
+      serverResponse.end();
+      // Wait for the new page to load.
+      await newPage.waitForLoadState();
+      // Connect to the opened page.
+      expect(newPage.url()).toBe(server.PREFIX + '/one-style.html');
+      // Cleanup.
+      await context.close();
+    });
+    it.fail(WEBKIT)('should have an opener', async({browser, server}) => {
+      const context = await browser.newContext();
+      const page = await context.newPage();
+      await page.goto(server.EMPTY_PAGE);
+      const [popup] = await Promise.all([
+        new Promise(fulfill => context.once('page', async event => fulfill(await event.page()))),
+        page.goto(server.PREFIX + '/popup/window-open.html')
+      ]);
+      await popup.waitForLoadState();
+      expect(popup.url()).toBe(server.PREFIX + '/popup/popup.html');
+      expect(await popup.opener()).toBe(page);
+      expect(await page.opener()).toBe(null);
+      await context.close();
+    });
+    it('should close all belonging targets once closing context', async function({browser}) {
+      const context = await browser.newContext();
+      await context.newPage();
+      expect((await context.pages()).length).toBe(1);
+
+      await context.close();
+      expect((await context.pages()).length).toBe(0);
+    });
+    it('should fire page lifecycle events', async function({browser, server}) {
+      const context = await browser.newContext();
+      const events = [];
+      context.on('page', async event => {
+        const page = await event.page();
+        events.push('CREATED: ' + page.url());
+        page.on('close', () => events.push('DESTROYED: ' + page.url()));
+      });
+      const page = await context.newPage();
+      await page.goto(server.EMPTY_PAGE);
+      await page.close();
+      expect(events).toEqual([
+        'CREATED: about:blank',
+        `DESTROYED: ${server.EMPTY_PAGE}`
+      ]);
+      await context.close();
+    });
+  });
 };

test/chromium/chromium.spec.js

@@ -24,37 +24,7 @@ module.exports.describe = function({testRunner, expect, playwright, FFOX, CHROMI
   const {it, fit, xit, dit} = testRunner;
   const {beforeAll, beforeEach, afterAll, afterEach} = testRunner;
 
-  describe('BrowserContext', function() {
+  describe('ChromiumBrowserContext', function() {
-    it('pages() should return all of the pages', async({page, server, context}) => {
-      const second = await page.context().newPage();
-      const allPages = await context.pages();
-      expect(allPages.length).toBe(2);
-      expect(allPages).toContain(page);
-      expect(allPages).toContain(second);
-      await second.close();
-    });
-    it('should report when a new page is created and closed', async({browser, page, server, context}) => {
-      const [otherPage] = await Promise.all([
-        new Promise(r => context.once('page', async event => r(await event.page()))),
-        page.evaluate(url => window.open(url), server.CROSS_PROCESS_PREFIX + '/empty.html'),
-      ]);
-      expect(otherPage.url()).toContain(server.CROSS_PROCESS_PREFIX);
-      expect(await otherPage.evaluate(() => ['Hello', 'world'].join(' '))).toBe('Hello world');
-      expect(await otherPage.$('body')).toBeTruthy();
-
-      let allPages = await context.pages();
-      expect(allPages).toContain(page);
-      expect(allPages).toContain(otherPage);
-
-      let closeEventReceived;
-      otherPage.once('close', () => closeEventReceived = true);
-      await otherPage.close();
-      expect(closeEventReceived).toBeTruthy();
-
-      allPages = await context.pages();
-      expect(allPages).toContain(page);
-      expect(allPages).not.toContain(otherPage);
-    });
     it('should create a worker from a service worker', async({browser, page, server, context}) => {
       const [worker] = await Promise.all([
         new Promise(fulfill => context.once('serviceworker', fulfill)),
@@ -71,74 +41,6 @@ module.exports.describe = function({testRunner, expect, playwright, FFOX, CHROMI
       });
       expect(serviceWorkerCreated).not.toBeTruthy();
     });
-    it('should not report uninitialized pages', async({browser, context}) => {
-      const pagePromise = new Promise(fulfill => context.once('page', async event => fulfill(await event.page())));
-      context.newPage();
-      const newPage = await pagePromise;
-      expect(newPage.url()).toBe('about:blank');
-
-      const popupPromise = new Promise(fulfill => context.once('page', async event => fulfill(await event.page())));
-      const evaluatePromise = newPage.evaluate(() => window.open('about:blank'));
-      const popup = await popupPromise;
-      expect(popup.url()).toBe('about:blank');
-      await evaluatePromise;
-      await newPage.close();
-    });
-    it('should not crash while redirecting if original request was missed', async({browser, page, server, context}) => {
-      let serverResponse = null;
-      server.setRoute('/one-style.css', (req, res) => serverResponse = res);
-      // Open a new page. Use window.open to connect to the page later.
-      const [newPage] = await Promise.all([
-        new Promise(fulfill => context.once('page', async event => fulfill(await event.page()))),
-        page.evaluate(url => window.open(url), server.PREFIX + '/one-style.html'),
-        server.waitForRequest('/one-style.css')
-      ]);
-      // Connect to the opened page.
-      expect(newPage.url()).toBe(server.PREFIX + '/one-style.html');
-      // Issue a redirect.
-      serverResponse.writeHead(302, { location: '/injectedstyle.css' });
-      serverResponse.end();
-      // Wait for the new page to load.
-      await waitEvent(newPage, 'load');
-      // Cleanup.
-      await newPage.close();
-    });
-    it('should have an opener', async({browser, page, server, context}) => {
-      await page.goto(server.EMPTY_PAGE);
-      const [popup] = await Promise.all([
-        new Promise(fulfill => context.once('page', async event => fulfill(await event.page()))),
-        page.goto(server.PREFIX + '/popup/window-open.html')
-      ]);
-      await popup.waitForLoadState();
-      expect(popup.url()).toBe(server.PREFIX + '/popup/popup.html');
-      expect(await popup.opener()).toBe(page);
-      expect(await page.opener()).toBe(null);
-    });
-    it('should close all belonging targets once closing context', async function({browser}) {
-      const context = await browser.newContext();
-      await context.newPage();
-      expect((await context.pages()).length).toBe(1);
-
-      await context.close();
-      expect((await context.pages()).length).toBe(0);
-    });
-    it('should fire page lifecycle events', async function({browser, server}) {
-      const context = await browser.newContext();
-      const events = [];
-      context.on('page', async event => {
-        const page = await event.page();
-        events.push('CREATED: ' + page.url());
-        page.on('close', () => events.push('DESTROYED: ' + page.url()))
-      });
-      const page = await context.newPage();
-      await page.goto(server.EMPTY_PAGE);
-      await page.close();
-      expect(events).toEqual([
-        'CREATED: about:blank',
-        `DESTROYED: ${server.EMPTY_PAGE}`
-      ]);
-      await context.close();
-    });
   });
 
   describe('Chromium-Specific Page Tests', function() {