Skip to content

Commit

Permalink
chore: remove Page._didDisconnect (microsoft#27317)
Browse files Browse the repository at this point in the history 
Instead of having `didClose` based on page creation/destruction and
`didDisconnect` based on session lifetime, we make session lifetime
being managed by the `CRPage`/`FFPage`/`WKPage` instead.
  • Loading branch information
@dgozman
dgozman authored Sep 27, 2023
1 parent 7da1dfd commit c814374
Show file tree
Hide file tree
Showing 18 changed files with 122 additions and 125 deletions.
2 changes: 1 addition & 1 deletion packages/playwright-core/src/server/browserContext.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ export abstract class BrowserContext extends SdkObject {
const [, ...otherPages] = this.pages();
for (const p of otherPages)
await p.close(metadata);
if (page && page._crashedScope.isClosed()) {
if (page && page.hasCrashed()) {
await page.close(metadata);
page = undefined;
}
Expand Down
19 changes: 16 additions & 3 deletions packages/playwright-core/src/server/chromium/crBrowser.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class CRBrowser extends Browser {
super(parent, options);
this._connection = connection;
this._session = this._connection.rootSession;
this._connection.on(ConnectionEvents.Disconnected, () => this._didClose());
this._connection.on(ConnectionEvents.Disconnected, () => this._didDisconnect());
this._session.on('Target.attachedToTarget', this._onAttachedToTarget.bind(this));
this._session.on('Target.detachedFromTarget', this._onDetachedFromTarget.bind(this));
this._session.on('Browser.downloadWillBegin', this._onDownloadWillBegin.bind(this));
Expand Down Expand Up @@ -149,7 +149,7 @@ export class CRBrowser extends Browser {
_onAttachedToTarget({ targetInfo, sessionId, waitingForDebugger }: Protocol.Target.attachedToTargetPayload) {
if (targetInfo.type === 'browser')
return;
const session = this._connection.session(sessionId)!;
const session = this._session.createChildSession(sessionId);
assert(targetInfo.browserContextId, 'targetInfo: ' + JSON.stringify(targetInfo, null, 2));
let context = this._contexts.get(targetInfo.browserContextId) || null;
if (!context) {
Expand Down Expand Up @@ -234,6 +234,19 @@ export class CRBrowser extends Browser {
}
}

private _didDisconnect() {
for (const crPage of this._crPages.values())
crPage.didClose();
this._crPages.clear();
for (const backgroundPage of this._backgroundPages.values())
backgroundPage.didClose();
this._backgroundPages.clear();
for (const serviceWorker of this._serviceWorkers.values())
serviceWorker.didClose();
this._serviceWorkers.clear();
this._didClose();
}

private _findOwningPage(frameId: string) {
for (const crPage of this._crPages.values()) {
const frame = crPage._page._frameManager.frame(frameId);
Expand Down Expand Up @@ -593,6 +606,6 @@ export class CRBrowserContext extends BrowserContext {

const rootSession = await this._browser._clientRootSession();
const { sessionId } = await rootSession.send('Target.attachToTarget', { targetId, flatten: true });
return this._browser._connection.session(sessionId)!;
return rootSession.createChildSession(sessionId);
}
}
94 changes: 39 additions & 55 deletions packages/playwright-core/src/server/chromium/crConnection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,10 @@ export const kBrowserCloseMessageId = -9999;
export class CRConnection extends EventEmitter {
private _lastId = 0;
private readonly _transport: ConnectionTransport;
private readonly _sessions = new Map<string, CRSession>();
readonly _sessions = new Map<string, CRSession>();
private readonly _protocolLogger: ProtocolLogger;
private readonly _browserLogsCollector: RecentLogsCollector;
_browserDisconnectedLogs: string | undefined;
readonly rootSession: CRSession;
_closed = false;

Expand All @@ -49,21 +50,13 @@ export class CRConnection extends EventEmitter {
this._transport = transport;
this._protocolLogger = protocolLogger;
this._browserLogsCollector = browserLogsCollector;
this.rootSession = new CRSession(this, '', 'browser', '');
this.rootSession = new CRSession(this, null, '');
this._sessions.set('', this.rootSession);
this._transport.onmessage = this._onMessage.bind(this);
// onclose should be set last, since it can be immediately called.
this._transport.onclose = this._onClose.bind(this);
}

static fromSession(session: CRSession): CRConnection {
return session._connection!;
}

session(sessionId: string): CRSession | null {
return this._sessions.get(sessionId) || null;
}

_rawSend(sessionId: string, method: string, params: any): number {
const id = ++this._lastId;
const message: ProtocolRequest = { id, method, params };
Expand All @@ -78,18 +71,6 @@ export class CRConnection extends EventEmitter {
this._protocolLogger('receive', message);
if (message.id === kBrowserCloseMessageId)
return;
if (message.method === 'Target.attachedToTarget') {
const sessionId = message.params.sessionId;
const rootSessionId = message.sessionId || '';
const session = new CRSession(this, rootSessionId, message.params.targetInfo.type, sessionId);
this._sessions.set(sessionId, session);
} else if (message.method === 'Target.detachedFromTarget') {
const session = this._sessions.get(message.params.sessionId);
if (session) {
session._onClosed(undefined);
this._sessions.delete(message.params.sessionId);
}
}
const session = this._sessions.get(message.sessionId || '');
if (session)
session._onMessage(message);
Expand All @@ -99,10 +80,8 @@ export class CRConnection extends EventEmitter {
this._closed = true;
this._transport.onmessage = undefined;
this._transport.onclose = undefined;
const browserDisconnectedLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs());
for (const session of this._sessions.values())
session._onClosed(browserDisconnectedLogs);
this._sessions.clear();
this._browserDisconnectedLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs());
this.rootSession.dispose();
Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected));
}

Expand All @@ -111,14 +90,9 @@ export class CRConnection extends EventEmitter {
this._transport.close();
}

async createSession(targetInfo: Protocol.Target.TargetInfo): Promise<CRSession> {
const { sessionId } = await this.rootSession.send('Target.attachToTarget', { targetId: targetInfo.targetId, flatten: true });
return this._sessions.get(sessionId)!;
}

async createBrowserSession(): Promise<CRSession> {
const { sessionId } = await this.rootSession.send('Target.attachToBrowserTarget');
return this._sessions.get(sessionId)!;
return this.rootSession.createChildSession(sessionId);
}
}

Expand All @@ -127,28 +101,26 @@ export const CRSessionEvents = {
};

export class CRSession extends EventEmitter {
_connection: CRConnection | null;
private readonly _connection: CRConnection;
_eventListener?: (method: string, params?: Object) => void;
private readonly _callbacks = new Map<number, {resolve: (o: any) => void, reject: (e: ProtocolError) => void, error: ProtocolError, method: string}>();
private readonly _targetType: string;
private readonly _sessionId: string;
private readonly _rootSessionId: string;
private readonly _parentSession: CRSession | null;
private _crashed: boolean = false;
private _browserDisconnectedLogs: string | undefined;
private _closed = false;
override 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;
override 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;
override 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;
override 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;
override 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;
readonly guid: string;

constructor(connection: CRConnection, rootSessionId: string, targetType: string, sessionId: string) {
constructor(connection: CRConnection, parentSession: CRSession | null, sessionId: string) {
super();
this.guid = `cdp-session@${sessionId}`;
this.setMaxListeners(0);
this._connection = connection;
this._rootSessionId = rootSessionId;
this._targetType = targetType;
this._parentSession = parentSession;
this._sessionId = sessionId;

this.on = super.on;
Expand All @@ -162,16 +134,28 @@ export class CRSession extends EventEmitter {
this._crashed = true;
}

createChildSession(sessionId: string): CRSession {
const session = new CRSession(this._connection, this, sessionId);
this._connection._sessions.set(sessionId, session);
return session;
}

private _closedErrorMessage() {
if (this._crashed)
return 'Target crashed';
if (this._connection._browserDisconnectedLogs !== undefined)
return `Browser closed.` + this._connection._browserDisconnectedLogs;
if (this._closed)
return `Target closed`;
}

async send<T extends keyof Protocol.CommandParameters>(
method: T,
params?: Protocol.CommandParameters[T]
): Promise<Protocol.CommandReturnValues[T]> {
if (this._crashed)
throw new ProtocolError(true, 'Target crashed');
if (this._browserDisconnectedLogs !== undefined)
throw new ProtocolError(true, `Browser closed.` + this._browserDisconnectedLogs);
if (!this._connection)
throw new ProtocolError(true, `Target closed`);
const closedErrorMessage = this._closedErrorMessage();
if (closedErrorMessage)
throw new ProtocolError(true, closedErrorMessage);
const id = this._connection._rawSend(this._sessionId, method, params);
return new Promise((resolve, reject) => {
this._callbacks.set(id, { resolve, reject, error: new ProtocolError(false), method });
Expand Down Expand Up @@ -203,23 +187,23 @@ export class CRSession extends EventEmitter {
}

async detach() {
if (!this._connection)
throw new Error(`Session already detached. Most likely the ${this._targetType} has been closed.`);
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 });
if (this._closed)
throw new Error(`Session already detached. Most likely the page has been closed.`);
if (!this._parentSession)
throw new Error('Root session cannot be closed');
await this._parentSession.send('Target.detachFromTarget', { sessionId: this._sessionId });
this.dispose();
}

_onClosed(browserDisconnectedLogs: string | undefined) {
this._browserDisconnectedLogs = browserDisconnectedLogs;
const errorMessage = browserDisconnectedLogs !== undefined ? 'Browser closed.' + browserDisconnectedLogs : 'Target closed';
dispose() {
this._closed = true;
this._connection._sessions.delete(this._sessionId);
const errorMessage = this._closedErrorMessage()!;
for (const callback of this._callbacks.values()) {
callback.error.sessionClosed = true;
callback.reject(rewriteErrorMessage(callback.error, errorMessage));
}
this._callbacks.clear();
this._connection = null;
Promise.resolve().then(() => this.emit(CRSessionEvents.Disconnected));
}
}
Expand Down
17 changes: 10 additions & 7 deletions packages/playwright-core/src/server/chromium/crPage.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import type * as channels from '@protocol/channels';
import { getAccessibilityTree } from './crAccessibility';
import { CRBrowserContext } from './crBrowser';
import type { CRSession } from './crConnection';
import { CRConnection, CRSessionEvents } from './crConnection';
import { CRCoverage } from './crCoverage';
import { DragManager } from './crDragDrop';
import { CRExecutionContext } from './crExecutionContext';
Expand Down Expand Up @@ -93,7 +92,6 @@ export class CRPage implements PageDelegate {
this._page = new Page(this, browserContext);
this._mainFrameSession = new FrameSession(this, client, targetId, null);
this._sessions.set(targetId, this._mainFrameSession);
client.once(CRSessionEvents.Disconnected, () => this._page._didDisconnect());
if (opener && !browserContext._options.noDefaultViewport) {
const features = opener._nextWindowOpenPopupFeatures.shift() || [];
const viewportSize = helper.getViewportSizeFromWindowFeatures(features);
Expand Down Expand Up @@ -407,6 +405,7 @@ class FrameSession {
private _evaluateOnNewDocumentIdentifiers: string[] = [];
private _exposedBindingNames: string[] = [];
private _metricsOverride: Protocol.Emulation.setDeviceMetricsOverrideParameters | undefined;
private _workerSessions = new Map<string, CRSession>();

constructor(crPage: CRPage, client: CRSession, targetId: string, parentSession: FrameSession | null) {
this._client = client;
Expand All @@ -421,9 +420,6 @@ class FrameSession {
this._firstNonInitialNavigationCommittedFulfill = f;
this._firstNonInitialNavigationCommittedReject = r;
});
client.once(CRSessionEvents.Disconnected, () => {
this._firstNonInitialNavigationCommittedReject(new Error('Page closed'));
});
}

_isMainFrame(): boolean {
Expand Down Expand Up @@ -578,13 +574,15 @@ class FrameSession {
}

dispose() {
this._firstNonInitialNavigationCommittedReject(new Error('Page closed'));
for (const childSession of this._childSessions)
childSession.dispose();
if (this._parentSession)
this._parentSession._childSessions.delete(this);
eventsHelper.removeEventListeners(this._eventListeners);
this._networkManager.dispose();
this._crPage._sessions.delete(this._targetId);
this._client.dispose();
}

async _navigate(frame: frames.Frame, url: string, referrer: string | undefined): Promise<frames.GotoResult> {
Expand Down Expand Up @@ -719,7 +717,7 @@ class FrameSession {
}

_onAttachedToTarget(event: Protocol.Target.attachedToTargetPayload) {
const session = CRConnection.fromSession(this._client).session(event.sessionId)!;
const session = this._client.createChildSession(event.sessionId);

if (event.targetInfo.type === 'iframe') {
// Frame id equals target id.
Expand All @@ -745,6 +743,7 @@ class FrameSession {
const url = event.targetInfo.url;
const worker = new Worker(this._page, url);
this._page._addWorker(event.sessionId, worker);
this._workerSessions.set(event.sessionId, session);
session.once('Runtime.executionContextCreated', async event => {
worker._createExecutionContext(new CRExecutionContext(session, event.context));
});
Expand All @@ -763,7 +762,11 @@ class FrameSession {

_onDetachedFromTarget(event: Protocol.Target.detachedFromTargetPayload) {
// This might be a worker...
this._page._removeWorker(event.sessionId);
const workerSession = this._workerSessions.get(event.sessionId);
if (workerSession) {
workerSession.dispose();
this._page._removeWorker(event.sessionId);
}

// ... or an oopif.
const childFrameSession = this._crPage._sessions.get(event.targetId!);
Expand Down
5 changes: 5 additions & 0 deletions packages/playwright-core/src/server/chromium/crServiceWorker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ export class CRServiceWorker extends Worker {
});
}

override didClose() {
this._session.dispose();
super.didClose();
}

async updateOffline(initial: boolean): Promise<void> {
if (!this._isNetworkInspectionEnabled())
return;
Expand Down
3 changes: 3 additions & 0 deletions packages/playwright-core/src/server/firefox/ffBrowser.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ export class FFBrowser extends Browser {
for (const video of this._idToVideo.values())
video.artifact.reportFinished(kBrowserClosedError);
this._idToVideo.clear();
for (const ffPage of this._ffPages.values())
ffPage.didClose();
this._ffPages.clear();
this._didClose();
}
}
Expand Down
8 changes: 0 additions & 8 deletions packages/playwright-core/src/server/firefox/ffConnection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,6 @@ export class FFConnection extends EventEmitter {
this._transport.onmessage = undefined;
this._transport.onclose = undefined;
const formattedBrowserLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs());
for (const session of this._sessions.values())
session.dispose();
this._sessions.clear();
for (const callback of this._callbacks.values()) {
const error = rewriteErrorMessage(callback.error, `Protocol error (${callback.method}): Browser closed.` + formattedBrowserLogs);
error.sessionClosed = true;
Expand All @@ -150,10 +147,6 @@ export class FFConnection extends EventEmitter {
}
}

export const FFSessionEvents = {
Disconnected: Symbol('Disconnected')
};

export class FFSession extends EventEmitter {
_connection: FFConnection;
_disposed = false;
Expand Down Expand Up @@ -228,7 +221,6 @@ export class FFSession extends EventEmitter {
this._callbacks.clear();
this._disposed = true;
this._connection._sessions.delete(this._sessionId);
Promise.resolve().then(() => this.emit(FFSessionEvents.Disconnected));
}
}

Expand Down
7 changes: 2 additions & 5 deletions packages/playwright-core/src/server/firefox/ffPage.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { Page, Worker } from '../page';
import type * as types from '../types';
import { getAccessibilityTree } from './ffAccessibility';
import type { FFBrowserContext } from './ffBrowser';
import { FFSession, FFSessionEvents } from './ffConnection';
import { FFSession } from './ffConnection';
import { FFExecutionContext } from './ffExecutionContext';
import { RawKeyboardImpl, RawMouseImpl, RawTouchscreenImpl } from './ffInput';
import { FFNetworkManager } from './ffNetworkManager';
Expand Down Expand Up @@ -100,10 +100,6 @@ export class FFPage implements PageDelegate {
eventsHelper.addEventListener(this._session, 'Page.screencastFrame', this._onScreencastFrame.bind(this)),

];
session.once(FFSessionEvents.Disconnected, () => {
this._markAsError(new Error('Page closed'));
this._page._didDisconnect();
});
this._session.once('Page.ready', async () => {
await this._page.initOpener(this._opener);
if (this._initializationFailed)
Expand Down Expand Up @@ -346,6 +342,7 @@ export class FFPage implements PageDelegate {
}

didClose() {
this._markAsError(new Error('Page closed'));
this._session.dispose();
eventsHelper.removeEventListeners(this._eventListeners);
this._networkManager.dispose();
Expand Down
Loading

0 comments on commit c814374

Please sign in to comment.