From 31e26a2208147d2381db71f04a12113c92d683ca Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 2 Mar 2020 18:32:56 -0800 Subject: [PATCH] fix(api): fire BrowserContext.Page event in WebKit and Firefox (#1186) --- src/chromium/crBrowser.ts | 2 +- src/events.ts | 2 +- src/firefox/ffBrowser.ts | 19 +++--- src/server/chromium.ts | 2 +- src/webkit/wkBrowser.ts | 20 ++++-- src/webkit/wkPageProxy.ts | 16 ++--- test/browsercontext.spec.js | 119 ++++++++++++++++++++++++++++++++- test/chromium/chromium.spec.js | 100 +-------------------------- 8 files changed, 153 insertions(+), 127 deletions(-) diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 4fe7334d7c..5a7ddf5b97 100644 --- a/src/chromium/crBrowser.ts +++ b/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': { diff --git a/src/events.ts b/src/events.ts index c9686e5ad5..ee5b63d3a9 100644 --- a/src/events.ts +++ b/src/events.ts @@ -22,7 +22,7 @@ export const Events = { BrowserContext: { Close: 'close', - PageEvent: 'page', + Page: 'page', }, BrowserServer: { diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index e3265e02fc..95fc7ef6db 100644 --- a/src/firefox/ffBrowser.ts +++ b/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)) { - const popupPage = await target.page(); - openerPage.emit(Events.Page.Popup, popupPage); - } + if (openerPage.listenerCount(Events.Page.Popup)) + openerPage.emit(Events.Page.Popup, page); } } @@ -189,14 +192,14 @@ class Target { _pagePromise?: Promise; _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; } diff --git a/src/server/chromium.ts b/src/server/chromium.ts index 46693e518b..fc01e53d59 100644 --- a/src/server/chromium.ts +++ b/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; diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index d115e4c924..f43aed611e 100644 --- a/src/webkit/wkBrowser.ts +++ b/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) { diff --git a/src/webkit/wkPageProxy.ts b/src/webkit/wkPageProxy.ts index 0956f2584a..69f6407ecb 100644 --- a/src/webkit/wkPageProxy.ts +++ b/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; diff --git a/test/browsercontext.spec.js b/test/browsercontext.spec.js index 35da41a040..da4c134f46 100644 --- a/test/browsercontext.spec.js +++ b/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(); + }); + }); }; diff --git a/test/chromium/chromium.spec.js b/test/chromium/chromium.spec.js index d1ad0d6094..cbf968c06a 100644 --- a/test/chromium/chromium.spec.js +++ b/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() { - 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); - }); + describe('ChromiumBrowserContext', function() { 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() {