From 8fe29feb21825dec631536691de3c6549934fbb6 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 8 Jul 2020 21:36:03 -0700 Subject: [PATCH] feat(rpc): support more chromium-specific apis (#2883) This includes page CDPSession, backgroundPages() and serviceWorkers(). This has also revealed an issue with closing order between the context and the service worker. --- src/browser.ts | 1 + src/browserContext.ts | 12 +++++-- src/chromium/crBrowser.ts | 10 ++++++ src/rpc/channels.ts | 11 ++++-- src/rpc/client/browser.ts | 3 +- src/rpc/client/browserContext.ts | 42 +++++++++++++++++++++- src/rpc/client/worker.ts | 9 +++-- src/rpc/server/browserContextDispatcher.ts | 25 +++++++++++-- src/rpc/server/browserDispatcher.ts | 3 +- src/server/browserType.ts | 3 +- src/server/electron.ts | 2 +- test/chromium/chromium.spec.js | 15 +++++++- test/chromium/launcher.spec.js | 6 ++-- test/chromium/oopif.spec.js | 2 +- test/chromium/session.spec.js | 6 ++-- 15 files changed, 124 insertions(+), 26 deletions(-) diff --git a/src/browser.ts b/src/browser.ts index ecab07e731..36bbc0d665 100644 --- a/src/browser.ts +++ b/src/browser.ts @@ -26,6 +26,7 @@ import { ProxySettings } from './types'; import { LoggerSink } from './loggerSink'; export type BrowserOptions = { + name: string, loggers: Loggers, downloadsPath?: string, headful?: boolean, diff --git a/src/browserContext.ts b/src/browserContext.ts index 0a41322b9b..36ea0c987e 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -62,7 +62,7 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser readonly _options: BrowserContextOptions; _routes: { url: types.URLMatch, handler: network.RouteHandler }[] = []; private _isPersistentContext: boolean; - private _startedClosing = false; + private _closedStatus: 'open' | 'closing' | 'closed' = 'open'; readonly _closePromise: Promise; private _closePromiseFulfill: ((error: Error) => void) | undefined; readonly _permissions = new Map(); @@ -109,6 +109,12 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser } private _didCloseInternal() { + if (this._closedStatus === 'closed') { + // We can come here twice if we close browser context and browser + // at the same time. + return; + } + this._closedStatus = 'closed'; this._downloads.clear(); this._closePromiseFulfill!(new Error('Context closed')); this.emit(Events.BrowserContext.Close); @@ -235,8 +241,8 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser await this._browserBase.close(); return; } - if (!this._startedClosing) { - this._startedClosing = true; + if (this._closedStatus === 'open') { + this._closedStatus = 'closing'; await this._doClose(); await Promise.all([...this._downloads].map(d => d.delete())); this._didCloseInternal(); diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 9ca27a00ee..32b431e535 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -425,6 +425,16 @@ export class CRBrowserContext extends BrowserContextBase { assert(this._browserContextId); await this._browser._session.send('Target.disposeBrowserContext', { browserContextId: this._browserContextId }); this._browser._contexts.delete(this._browserContextId); + for (const [targetId, serviceWorker] of this._browser._serviceWorkers) { + if (serviceWorker._browserContext !== this) + continue; + // When closing a browser context, service workers are shutdown + // asynchronously and we get detached from them later. + // To avoid the wrong order of notifications, we manually fire + // "close" event here and forget about the serivce worker. + serviceWorker.emit(CommonEvents.Worker.Close); + this._browser._serviceWorkers.delete(targetId); + } } backgroundPages(): Page[] { diff --git a/src/rpc/channels.ts b/src/rpc/channels.ts index d565d4aa95..96fbb82c4c 100644 --- a/src/rpc/channels.ts +++ b/src/rpc/channels.ts @@ -63,8 +63,7 @@ export interface BrowserChannel extends Channel { close(): Promise; newContext(params: types.BrowserContextOptions): Promise; - // Chromium-specific. - newBrowserCDPSession(): Promise; + crNewBrowserCDPSession(): Promise; } export type BrowserInitializer = {}; @@ -92,9 +91,15 @@ export interface BrowserContextChannel extends Channel { setNetworkInterceptionEnabled(params: { enabled: boolean }): Promise; setOffline(params: { offline: boolean }): Promise; waitForEvent(params: { event: string }): Promise; + + on(event: 'crBackgroundPage', callback: (params: PageChannel) => void): this; + on(event: 'crServiceWorker', callback: (params: WorkerChannel) => void): this; + crNewCDPSession(params: { page: PageChannel }): Promise; } export type BrowserContextInitializer = { - pages: PageChannel[] + pages: PageChannel[], + crBackgroundPages: PageChannel[], + crServiceWorkers: WorkerChannel[], }; diff --git a/src/rpc/client/browser.ts b/src/rpc/client/browser.ts index 99e1667079..f6211105d0 100644 --- a/src/rpc/client/browser.ts +++ b/src/rpc/client/browser.ts @@ -79,8 +79,7 @@ export class Browser extends ChannelOwner { await this._channel.close(); } - // Chromium-specific. async newBrowserCDPSession(): Promise { - return CDPSession.from(await this._channel.newBrowserCDPSession()); + return CDPSession.from(await this._channel.crNewBrowserCDPSession()); } } diff --git a/src/rpc/client/browserContext.ts b/src/rpc/client/browserContext.ts index e17af9e096..ff45633fc0 100644 --- a/src/rpc/client/browserContext.ts +++ b/src/rpc/client/browserContext.ts @@ -26,9 +26,14 @@ import { Browser } from './browser'; import { ConnectionScope } from './connection'; import { Events } from '../../events'; import { TimeoutSettings } from '../../timeoutSettings'; +import { CDPSession } from './cdpSession'; +import { Events as ChromiumEvents } from '../../chromium/events'; +import { Worker } from './worker'; export class BrowserContext extends ChannelOwner { _pages = new Set(); + _crBackgroundPages = new Set(); + _crServiceWorkers = new Set(); private _routes: { url: types.URLMatch, handler: network.RouteHandler }[] = []; _browser: Browser | undefined; readonly _bindings = new Map(); @@ -46,7 +51,7 @@ export class BrowserContext extends ChannelOwner { + initializer.pages.forEach(p => { const page = Page.from(p); this._pages.add(page); page._setBrowserContext(this); @@ -55,6 +60,29 @@ export class BrowserContext extends ChannelOwner this._onClose()); this._channel.on('page', page => this._onPage(Page.from(page))); this._channel.on('route', ({ route, request }) => this._onRoute(network.Route.from(route), network.Request.from(request))); + + initializer.crBackgroundPages.forEach(p => { + const page = Page.from(p); + this._crBackgroundPages.add(page); + page._setBrowserContext(this); + }); + this._channel.on('crBackgroundPage', pageChannel => { + const page = Page.from(pageChannel); + page._setBrowserContext(this); + this._crBackgroundPages.add(page); + this.emit(ChromiumEvents.CRBrowserContext.BackgroundPage, page); + }); + initializer.crServiceWorkers.forEach(w => { + const worker = Worker.from(w); + worker._context = this; + this._crServiceWorkers.add(worker); + }); + this._channel.on('crServiceWorker', serviceWorkerChannel => { + const worker = Worker.from(serviceWorkerChannel); + worker._context = this; + this._crServiceWorkers.add(worker); + this.emit(ChromiumEvents.CRBrowserContext.ServiceWorker, worker); + }); } private _onPage(page: Page): void { @@ -199,4 +227,16 @@ export class BrowserContext extends ChannelOwner { await this._channel.close(); } + + async newCDPSession(page: Page): Promise { + return CDPSession.from(await this._channel.crNewCDPSession({ page: page._channel })); + } + + backgroundPages(): Page[] { + return [...this._crBackgroundPages]; + } + + serviceWorkers(): Worker[] { + return [...this._crServiceWorkers]; + } } diff --git a/src/rpc/client/worker.ts b/src/rpc/client/worker.ts index b959cdd939..ce0e055ca8 100644 --- a/src/rpc/client/worker.ts +++ b/src/rpc/client/worker.ts @@ -21,9 +21,11 @@ import { ConnectionScope } from './connection'; import { ChannelOwner } from './channelOwner'; import { Func1, JSHandle, parseResult, serializeArgument, SmartHandle } from './jsHandle'; import { Page } from './page'; +import { BrowserContext } from './browserContext'; export class Worker extends ChannelOwner { - _page: Page | undefined; + _page: Page | undefined; // Set for web workers. + _context: BrowserContext | undefined; // Set for service workers. static from(worker: WorkerChannel): Worker { return (worker as any)._object; @@ -32,7 +34,10 @@ export class Worker extends ChannelOwner { constructor(scope: ConnectionScope, guid: string, initializer: WorkerInitializer) { super(scope, guid, initializer); this._channel.on('close', () => { - this._page!._workers.delete(this); + if (this._page) + this._page._workers.delete(this); + if (this._context) + this._context._crServiceWorkers.delete(this); this.emit(Events.Worker.Close, this); }); } diff --git a/src/rpc/server/browserContextDispatcher.ts b/src/rpc/server/browserContextDispatcher.ts index dc99c16a77..b825bdb58a 100644 --- a/src/rpc/server/browserContextDispatcher.ts +++ b/src/rpc/server/browserContextDispatcher.ts @@ -18,17 +18,31 @@ import * as types from '../../types'; import { BrowserContextBase, BrowserContext } from '../../browserContext'; import { Events } from '../../events'; import { Dispatcher, DispatcherScope, lookupNullableDispatcher, lookupDispatcher } from './dispatcher'; -import { PageDispatcher, BindingCallDispatcher } from './pageDispatcher'; -import { PageChannel, BrowserContextChannel, BrowserContextInitializer } from '../channels'; +import { PageDispatcher, BindingCallDispatcher, WorkerDispatcher } from './pageDispatcher'; +import { PageChannel, BrowserContextChannel, BrowserContextInitializer, CDPSessionChannel } from '../channels'; import { RouteDispatcher, RequestDispatcher } from './networkDispatchers'; import { Page } from '../../page'; +import { CRBrowserContext } from '../../chromium/crBrowser'; +import { CDPSessionDispatcher } from './cdpSessionDispatcher'; +import { Events as ChromiumEvents } from '../../chromium/events'; export class BrowserContextDispatcher extends Dispatcher implements BrowserContextChannel { private _context: BrowserContextBase; constructor(scope: DispatcherScope, context: BrowserContextBase) { + let crBackgroundPages: PageDispatcher[] = []; + let crServiceWorkers: WorkerDispatcher[] = []; + if (context._browserBase._options.name === 'chromium') { + crBackgroundPages = (context as CRBrowserContext).backgroundPages().map(p => new PageDispatcher(scope, p)); + context.on(ChromiumEvents.CRBrowserContext.BackgroundPage, page => this._dispatchEvent('crBackgroundPage', new PageDispatcher(this._scope, page))); + crServiceWorkers = (context as CRBrowserContext).serviceWorkers().map(w => new WorkerDispatcher(scope, w)); + context.on(ChromiumEvents.CRBrowserContext.ServiceWorker, serviceWorker => this._dispatchEvent('crServiceWorker', new WorkerDispatcher(this._scope, serviceWorker))); + } + super(scope, context, 'context', { - pages: context.pages().map(p => new PageDispatcher(scope, p)) + pages: context.pages().map(p => new PageDispatcher(scope, p)), + crBackgroundPages, + crServiceWorkers, }, true); this._context = context; context.on(Events.BrowserContext.Page, page => this._dispatchEvent('page', new PageDispatcher(this._scope, page))); @@ -118,4 +132,9 @@ export class BrowserContextDispatcher extends Dispatcher { await this._context.close(); } + + async crNewCDPSession(params: { page: PageDispatcher }): Promise { + const crBrowserContext = this._object as CRBrowserContext; + return new CDPSessionDispatcher(this._scope, await crBrowserContext.newCDPSession(params.page._object)); + } } diff --git a/src/rpc/server/browserDispatcher.ts b/src/rpc/server/browserDispatcher.ts index c19b203dff..b8f1177eff 100644 --- a/src/rpc/server/browserDispatcher.ts +++ b/src/rpc/server/browserDispatcher.ts @@ -41,8 +41,7 @@ export class BrowserDispatcher extends Dispatcher i await this._object.close(); } - // Chromium-specific. - async newBrowserCDPSession(): Promise { + async crNewBrowserCDPSession(): Promise { const crBrowser = this._object as CRBrowser; return new CDPSessionDispatcher(this._scope, await crBrowser.newBrowserCDPSession()); } diff --git a/src/server/browserType.ts b/src/server/browserType.ts index 85d1cc4f74..1c927fde55 100644 --- a/src/server/browserType.ts +++ b/src/server/browserType.ts @@ -106,6 +106,7 @@ export abstract class BrowserTypeBase implements BrowserType { if ((options as any).__testHookBeforeCreateBrowser) await (options as any).__testHookBeforeCreateBrowser(); const browserOptions: BrowserOptions = { + name: this._name, slowMo: options.slowMo, persistent, headful: !options.headless, @@ -142,7 +143,7 @@ export abstract class BrowserTypeBase implements BrowserType { progress.cleanupWhenAborted(() => transport.closeAndWait()); if ((options as any).__testHookBeforeCreateBrowser) await (options as any).__testHookBeforeCreateBrowser(); - const browser = await this._connectToTransport(transport, { slowMo: options.slowMo, loggers }); + const browser = await this._connectToTransport(transport, { name: this._name, slowMo: options.slowMo, loggers }); return browser; }, loggers.browser, TimeoutSettings.timeout(options), 'browserType.connect'); } diff --git a/src/server/electron.ts b/src/server/electron.ts index 3adedfe309..c325cbed5d 100644 --- a/src/server/electron.ts +++ b/src/server/electron.ts @@ -202,7 +202,7 @@ export class Electron { const chromeMatch = await waitForLine(progress, launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/); const chromeTransport = await WebSocketTransport.connect(progress, chromeMatch[1]); const browserServer = new BrowserServer(launchedProcess, gracefullyClose, kill); - const browser = await CRBrowser.connect(chromeTransport, { headful: true, loggers, persistent: { viewport: null }, ownedServer: browserServer }); + const browser = await CRBrowser.connect(chromeTransport, { name: 'electron', headful: true, loggers, persistent: { viewport: null }, ownedServer: browserServer }); app = new ElectronApplication(loggers, browser, nodeConnection); await app._init(); return app; diff --git a/test/chromium/chromium.spec.js b/test/chromium/chromium.spec.js index 5b9fa5c5a7..f36d4d530a 100644 --- a/test/chromium/chromium.spec.js +++ b/test/chromium/chromium.spec.js @@ -16,7 +16,7 @@ const {FFOX, CHROMIUM, WEBKIT, CHANNEL} = require('../utils').testOptions(browserType); -describe.skip(CHANNEL)('ChromiumBrowserContext', function() { +describe('ChromiumBrowserContext', function() { it('should create a worker from a service worker', async({browser, page, server, context}) => { const [worker] = await Promise.all([ context.waitForEvent('serviceworker'), @@ -50,6 +50,19 @@ describe.skip(CHANNEL)('ChromiumBrowserContext', function() { }); expect(serviceWorkerCreated).not.toBeTruthy(); }); + it('should close service worker together with the context', async({browser, server}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + const [worker] = await Promise.all([ + context.waitForEvent('serviceworker'), + page.goto(server.PREFIX + '/serviceworkers/empty/sw.html') + ]); + const messages = []; + context.on('close', () => messages.push('context')); + worker.on('close', () => messages.push('worker')); + await context.close(); + expect(messages.join('|')).toBe('worker|context'); + }); }); describe('Chromium-Specific Page Tests', function() { diff --git a/test/chromium/launcher.spec.js b/test/chromium/launcher.spec.js index 6dbe45bbd2..a3e0664705 100644 --- a/test/chromium/launcher.spec.js +++ b/test/chromium/launcher.spec.js @@ -19,7 +19,7 @@ const utils = require('../utils'); const {makeUserDataDir, removeUserDataDir} = utils; const {FFOX, CHROMIUM, WEBKIT, WIN, CHANNEL} = utils.testOptions(browserType); -describe.skip(CHANNEL)('launcher', function() { +describe('launcher', function() { it('should throw with remote-debugging-pipe argument', async({browserType, defaultBrowserOptions}) => { const options = Object.assign({}, defaultBrowserOptions); options.args = ['--remote-debugging-pipe'].concat(options.args || []); @@ -49,7 +49,7 @@ describe.skip(CHANNEL)('launcher', function() { }); }); -describe.skip(CHANNEL)('extensions', () => { +describe('extensions', () => { it('should return background pages', async({browserType, defaultBrowserOptions}) => { const userDataDir = await makeUserDataDir(); const extensionPath = path.join(__dirname, '..', 'assets', 'simple-extension'); @@ -73,7 +73,7 @@ describe.skip(CHANNEL)('extensions', () => { }); }); -describe.skip(CHANNEL)('BrowserContext', function() { +describe('BrowserContext', function() { it('should not create pages automatically', async ({browserType, defaultBrowserOptions}) => { const browser = await browserType.launch(defaultBrowserOptions); const browserSession = await browser.newBrowserCDPSession(); diff --git a/test/chromium/oopif.spec.js b/test/chromium/oopif.spec.js index 496cdf04fe..730ecff1f5 100644 --- a/test/chromium/oopif.spec.js +++ b/test/chromium/oopif.spec.js @@ -16,7 +16,7 @@ const {FFOX, CHROMIUM, WEBKIT, CHANNEL} = require('../utils').testOptions(browserType); -describe.skip(CHANNEL)('OOPIF', function() { +describe('OOPIF', function() { beforeAll(async function(state) { state.browser = await state.browserType.launch(Object.assign({}, state.defaultBrowserOptions, { args: (state.defaultBrowserOptions.args || []).concat(['--site-per-process']), diff --git a/test/chromium/session.spec.js b/test/chromium/session.spec.js index c347f05c50..bda12a04ea 100644 --- a/test/chromium/session.spec.js +++ b/test/chromium/session.spec.js @@ -16,7 +16,7 @@ const {FFOX, CHROMIUM, WEBKIT, CHANNEL} = require('../utils').testOptions(browserType); -describe.skip(CHANNEL)('ChromiumBrowserContext.createSession', function() { +describe('ChromiumBrowserContext.createSession', function() { it('should work', async function({page, browser, server}) { const client = await page.context().newCDPSession(page); @@ -35,7 +35,7 @@ describe.skip(CHANNEL)('ChromiumBrowserContext.createSession', function() { await page.goto(server.EMPTY_PAGE); expect(events.length).toBe(1); }); - it('should enable and disable domains independently', async function({page, browser, server}) { + it.skip(CHANNEL)('should enable and disable domains independently', async function({page, browser, server}) { const client = await page.context().newCDPSession(page); await client.send('Runtime.enable'); await client.send('Debugger.enable'); @@ -64,7 +64,7 @@ describe.skip(CHANNEL)('ChromiumBrowserContext.createSession', function() { } catch (e) { error = e; } - expect(error.message).toContain('Session closed.'); + expect(error.message).toContain(CHANNEL ? 'Target browser or context has been closed' : 'Session closed.'); }); it('should throw nice errors', async function({page, browser}) { const client = await page.context().newCDPSession(page);