Skip to content

Commit

Permalink
fix(session): use isolated root session for client page sessions (#1271)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yury-s
yury-s committed Mar 6, 2020
1 parent 3fa000f commit 8211287
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 28 deletions.
46 changes: 28 additions & 18 deletions src/chromium/crBrowser.ts
View file
Expand Up @@ -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<CRSession> | null = null;
readonly _defaultContext: CRBrowserContext;
readonly _contexts = new Map<string, CRBrowserContext>();
_targets = new Map<string, CRTarget>();
Expand Down Expand Up @@ -70,23 +71,23 @@ 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, () => {
for (const context of this._contexts.values())
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<BrowserContext> {
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);
Expand Down Expand Up @@ -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[] {
Expand All @@ -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',
Expand Down Expand Up @@ -220,6 +221,12 @@ export class CRBrowser extends platform.EventEmitter implements Browser {
return !this._connection._closed;
}

async _clientRootSession(): Promise<CRSession> {
if (!this._clientRootSessionPromise)
this._clientRootSessionPromise = this._connection.createBrowserSession();
return this._clientRootSessionPromise;
}

_setDebugFunction(debugFunction: (message: string) => void) {
this._connection._debugProtocol = debugFunction;
}
Expand Down Expand Up @@ -265,7 +272,7 @@ export class CRBrowserContext extends BrowserContextBase {

async newPage(): Promise<Page> {
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) {
Expand All @@ -277,7 +284,7 @@ export class CRBrowserContext extends BrowserContextBase {
}

async cookies(urls?: string | string[]): Promise<network.NetworkCookie[]> {
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;
Expand All @@ -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<void> {
Expand Down Expand Up @@ -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<void> {
Expand Down Expand Up @@ -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();
}
Expand All @@ -388,6 +395,9 @@ export class CRBrowserContext extends BrowserContextBase {
}

async createSession(page: Page): Promise<CRSession> {
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)!;
}
}
26 changes: 16 additions & 10 deletions src/chromium/crConnection.ts
View file
Expand Up @@ -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 = {
Expand All @@ -30,8 +30,8 @@ export const kBrowserCloseMessageId = -9999;

export class CRConnection extends platform.EventEmitter {
private _lastId = 0;
private _transport: ConnectionTransport;
private _sessions = new Map<string, CRSession>();
private readonly _transport: ConnectionTransport;
private readonly _sessions = new Map<string, CRSession>();
readonly rootSession: CRSession;
_closed = false;
_debugProtocol: (message: string) => void;
Expand All @@ -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');
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -118,18 +119,20 @@ export const CRSessionEvents = {

export class CRSession extends platform.EventEmitter {
_connection: CRConnection | null;
private _callbacks = new Map<number, {resolve: (o: any) => void, reject: (e: Error) => void, error: Error, method: string}>();
private _targetType: string;
private _sessionId: string;
private readonly _callbacks = new Map<number, {resolve: (o: any) => void, reject: (e: Error) => void, error: Error, method: string}>();
private readonly _targetType: string;
private readonly _sessionId: string;
private readonly _rootSessionId: string;
on: <T extends keyof Protocol.Events | symbol>(event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this;
addListener: <T extends keyof Protocol.Events | symbol>(event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this;
off: <T extends keyof Protocol.Events | symbol>(event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this;
removeListener: <T extends keyof Protocol.Events | symbol>(event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this;
once: <T extends keyof Protocol.Events | symbol>(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;

Expand Down Expand Up @@ -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() {
Expand Down
18 changes: 18 additions & 0 deletions test/chromium/session.spec.js
View file
Expand Up @@ -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}) {
Expand Down

0 comments on commit 8211287

Please sign in to comment.