From 8211287a23cde0866ec5413ddaa8d9487ae27623 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 6 Mar 2020 15:11:03 -0800 Subject: [PATCH] fix(session): use isolated root session for client page sessions (#1271) --- src/chromium/crBrowser.ts | 46 +++++++++++++++++++++-------------- src/chromium/crConnection.ts | 26 ++++++++++++-------- test/chromium/session.spec.js | 18 ++++++++++++++ 3 files changed, 62 insertions(+), 28 deletions(-) diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 2e55d21b0f..aca7c7d966 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -32,8 +32,9 @@ import { Events } from './events'; import { Protocol } from './protocol'; export class CRBrowser extends platform.EventEmitter implements Browser { - _connection: CRConnection; - _client: CRSession; + readonly _connection: CRConnection; + _session: CRSession; + private _clientRootSessionPromise: Promise | null = null; readonly _defaultContext: CRBrowserContext; readonly _contexts = new Map(); _targets = new Map(); @@ -70,7 +71,7 @@ export class CRBrowser extends platform.EventEmitter implements Browser { constructor(connection: CRConnection) { super(); this._connection = connection; - this._client = this._connection.rootSession; + this._session = this._connection.rootSession; this._defaultContext = new CRBrowserContext(this, null, validateBrowserContextOptions({})); this._connection.on(ConnectionEvents.Disconnected, () => { @@ -78,15 +79,15 @@ export class CRBrowser extends platform.EventEmitter implements Browser { context._browserClosed(); this.emit(CommonEvents.Browser.Disconnected); }); - this._client.on('Target.targetCreated', this._targetCreated.bind(this)); - this._client.on('Target.targetDestroyed', this._targetDestroyed.bind(this)); - this._client.on('Target.targetInfoChanged', this._targetInfoChanged.bind(this)); - this._client.on('Target.attachedToTarget', this._onAttachedToTarget.bind(this)); + this._session.on('Target.targetCreated', this._targetCreated.bind(this)); + this._session.on('Target.targetDestroyed', this._targetDestroyed.bind(this)); + this._session.on('Target.targetInfoChanged', this._targetInfoChanged.bind(this)); + this._session.on('Target.attachedToTarget', this._onAttachedToTarget.bind(this)); } async newContext(options: BrowserContextOptions = {}): Promise { options = validateBrowserContextOptions(options); - const { browserContextId } = await this._client.send('Target.createBrowserContext', { disposeOnDetach: true }); + const { browserContextId } = await this._session.send('Target.createBrowserContext', { disposeOnDetach: true }); const context = new CRBrowserContext(this, browserContextId, options); await context._initialize(); this._contexts.set(browserContextId, context); @@ -159,7 +160,7 @@ export class CRBrowser extends platform.EventEmitter implements Browser { } async _closePage(page: Page) { - await this._client.send('Target.closeTarget', { targetId: CRTarget.fromPage(page)._targetId }); + await this._session.send('Target.closeTarget', { targetId: CRTarget.fromPage(page)._targetId }); } _allTargets(): CRTarget[] { @@ -179,7 +180,7 @@ export class CRBrowser extends platform.EventEmitter implements Browser { async startTracing(page?: Page, options: { path?: string; screenshots?: boolean; categories?: string[]; } = {}) { assert(!this._tracingRecording, 'Cannot start recording trace while already recording trace.'); - this._tracingClient = page ? (page._delegate as CRPage)._client : this._client; + this._tracingClient = page ? (page._delegate as CRPage)._client : this._session; const defaultCategories = [ '-*', 'devtools.timeline', 'v8.execute', 'disabled-by-default-devtools.timeline', @@ -220,6 +221,12 @@ export class CRBrowser extends platform.EventEmitter implements Browser { return !this._connection._closed; } + async _clientRootSession(): Promise { + if (!this._clientRootSessionPromise) + this._clientRootSessionPromise = this._connection.createBrowserSession(); + return this._clientRootSessionPromise; + } + _setDebugFunction(debugFunction: (message: string) => void) { this._connection._debugProtocol = debugFunction; } @@ -265,7 +272,7 @@ export class CRBrowserContext extends BrowserContextBase { async newPage(): Promise { assertBrowserContextIsNotOwned(this); - const { targetId } = await this._browser._client.send('Target.createTarget', { url: 'about:blank', browserContextId: this._browserContextId || undefined }); + const { targetId } = await this._browser._session.send('Target.createTarget', { url: 'about:blank', browserContextId: this._browserContextId || undefined }); const target = this._browser._targets.get(targetId)!; const result = await target.pageOrError(); if (result instanceof Page) { @@ -277,7 +284,7 @@ export class CRBrowserContext extends BrowserContextBase { } async cookies(urls?: string | string[]): Promise { - const { cookies } = await this._browser._client.send('Storage.getCookies', { browserContextId: this._browserContextId || undefined }); + const { cookies } = await this._browser._session.send('Storage.getCookies', { browserContextId: this._browserContextId || undefined }); return network.filterCookies(cookies.map(c => { const copy: any = { sameSite: 'None', ...c }; delete copy.size; @@ -287,11 +294,11 @@ export class CRBrowserContext extends BrowserContextBase { } async setCookies(cookies: network.SetNetworkCookieParam[]) { - await this._browser._client.send('Storage.setCookies', { cookies: network.rewriteCookies(cookies), browserContextId: this._browserContextId || undefined }); + await this._browser._session.send('Storage.setCookies', { cookies: network.rewriteCookies(cookies), browserContextId: this._browserContextId || undefined }); } async clearCookies() { - await this._browser._client.send('Storage.clearCookies', { browserContextId: this._browserContextId || undefined }); + await this._browser._session.send('Storage.clearCookies', { browserContextId: this._browserContextId || undefined }); } async setPermissions(origin: string, permissions: string[]): Promise { @@ -319,11 +326,11 @@ export class CRBrowserContext extends BrowserContextBase { throw new Error('Unknown permission: ' + permission); return protocolPermission; }); - await this._browser._client.send('Browser.grantPermissions', { origin, browserContextId: this._browserContextId || undefined, permissions: filtered }); + await this._browser._session.send('Browser.grantPermissions', { origin, browserContextId: this._browserContextId || undefined, permissions: filtered }); } async clearPermissions() { - await this._browser._client.send('Browser.resetPermissions', { browserContextId: this._browserContextId || undefined }); + await this._browser._session.send('Browser.resetPermissions', { browserContextId: this._browserContextId || undefined }); } async setGeolocation(geolocation: types.Geolocation | null): Promise { @@ -376,7 +383,7 @@ export class CRBrowserContext extends BrowserContextBase { if (this._closed) return; assert(this._browserContextId, 'Non-incognito profiles cannot be closed!'); - await this._browser._client.send('Target.disposeBrowserContext', { browserContextId: this._browserContextId }); + await this._browser._session.send('Target.disposeBrowserContext', { browserContextId: this._browserContextId }); this._browser._contexts.delete(this._browserContextId); this._didCloseInternal(); } @@ -388,6 +395,9 @@ export class CRBrowserContext extends BrowserContextBase { } async createSession(page: Page): Promise { - return CRTarget.fromPage(page).sessionFactory(); + const targetId = CRTarget.fromPage(page)._targetId; + const rootSession = await this._browser._clientRootSession(); + const { sessionId } = await rootSession.send('Target.attachToTarget', { targetId, flatten: true }); + return this._browser._connection.session(sessionId)!; } } diff --git a/src/chromium/crConnection.ts b/src/chromium/crConnection.ts index aaae4fb770..4100e0f256 100644 --- a/src/chromium/crConnection.ts +++ b/src/chromium/crConnection.ts @@ -15,9 +15,9 @@ * limitations under the License. */ +import { assert } from '../helper'; import * as platform from '../platform'; import { ConnectionTransport } from '../transport'; -import { assert } from '../helper'; import { Protocol } from './protocol'; export const ConnectionEvents = { @@ -30,8 +30,8 @@ export const kBrowserCloseMessageId = -9999; export class CRConnection extends platform.EventEmitter { private _lastId = 0; - private _transport: ConnectionTransport; - private _sessions = new Map(); + private readonly _transport: ConnectionTransport; + private readonly _sessions = new Map(); readonly rootSession: CRSession; _closed = false; _debugProtocol: (message: string) => void; @@ -41,7 +41,7 @@ export class CRConnection extends platform.EventEmitter { this._transport = transport; this._transport.onmessage = this._onMessage.bind(this); this._transport.onclose = this._onClose.bind(this); - this.rootSession = new CRSession(this, 'browser', ''); + this.rootSession = new CRSession(this, '', 'browser', ''); this._sessions.set('', this.rootSession); this._debugProtocol = platform.debug('pw:protocol'); } @@ -72,7 +72,8 @@ export class CRConnection extends platform.EventEmitter { return; if (object.method === 'Target.attachedToTarget') { const sessionId = object.params.sessionId; - const session = new CRSession(this, object.params.targetInfo.type, sessionId); + const rootSessionId = object.sessionId || ''; + const session = new CRSession(this, rootSessionId, object.params.targetInfo.type, sessionId); this._sessions.set(sessionId, session); } else if (object.method === 'Target.detachedFromTarget') { const session = this._sessions.get(object.params.sessionId); @@ -118,18 +119,20 @@ export const CRSessionEvents = { export class CRSession extends platform.EventEmitter { _connection: CRConnection | null; - private _callbacks = new Map void, reject: (e: Error) => void, error: Error, method: string}>(); - private _targetType: string; - private _sessionId: string; + private readonly _callbacks = new Map void, reject: (e: Error) => void, error: Error, method: string}>(); + private readonly _targetType: string; + private readonly _sessionId: string; + private readonly _rootSessionId: string; on: (event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this; addListener: (event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this; off: (event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this; removeListener: (event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this; once: (event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this; - constructor(connection: CRConnection, targetType: string, sessionId: string) { + constructor(connection: CRConnection, rootSessionId: string, targetType: string, sessionId: string) { super(); this._connection = connection; + this._rootSessionId = rootSessionId; this._targetType = targetType; this._sessionId = sessionId; @@ -169,7 +172,10 @@ export class CRSession extends platform.EventEmitter { async detach() { if (!this._connection) throw new Error(`Session already detached. Most likely the ${this._targetType} has been closed.`); - await this._connection.rootSession.send('Target.detachFromTarget', { sessionId: this._sessionId }); + const rootSession = this._connection.session(this._rootSessionId); + if (!rootSession) + throw new Error('Root session has been closed'); + await rootSession.send('Target.detachFromTarget', { sessionId: this._sessionId }); } _onClosed() { diff --git a/test/chromium/session.spec.js b/test/chromium/session.spec.js index d51224bb8c..3069680d6d 100644 --- a/test/chromium/session.spec.js +++ b/test/chromium/session.spec.js @@ -82,6 +82,24 @@ module.exports.describe = function({testRunner, expect, FFOX, CHROMIUM, WEBKIT}) await client.send('ThisCommand.DoesNotExist'); } }); + it('should not break page.close()', async function({browser, server}) { + const context = await browser.newContext(); + const page = await context.newPage(); + const session = await page.context().createSession(page); + await session.detach(); + await page.close(); + await context.close(); + }); + it('should detach when page closes', async function({browser, server}) { + const context = await browser.newContext(); + const page = await context.newPage(); + const session = await context.createSession(page); + await page.close(); + let error; + await session.detach().catch(e => error = e); + expect(error).toBeTruthy('Calling detach on a closed page\'s session should throw'); + await context.close(); + }); }); describe('ChromiumBrowser.createBrowserSession', function() { it('should work', async function({page, browser, server}) {