feat(rpc): replace implicit scopes with explicit dispose (#3173)

This adds one more protocol message __dispose__
to dispose a scope and all child objects.

Now, client side does not need a notion of scope anymore -
it just disposes the whole object subtree upon __dispose__.
Server, on the other hand, marks some objects as scopes
and disposes them manually, also asserting that all parents
are proper scopes.

src/rpc/channels.ts

@@ -1575,7 +1575,6 @@ export type StreamReadResult = {
 export type CDPSessionInitializer = {};
 export interface CDPSessionChannel extends Channel {
   on(event: 'event', callback: (params: CDPSessionEventEvent) => void): this;
-  on(event: 'disconnected', callback: (params: CDPSessionDisconnectedEvent) => void): this;
   send(params: CDPSessionSendParams): Promise<CDPSessionSendResult>;
   detach(params?: CDPSessionDetachParams): Promise<CDPSessionDetachResult>;
 }
@@ -1583,7 +1582,6 @@ export type CDPSessionEventEvent = {
   method: string,
   params?: SerializedValue,
 };
-export type CDPSessionDisconnectedEvent = {};
 export type CDPSessionSendParams = {
   method: string,
   params?: SerializedValue,

src/rpc/client/browser.ts

@@ -40,13 +40,12 @@ export class Browser extends ChannelOwner<BrowserChannel, BrowserInitializer> {
   }
 
   constructor(parent: ChannelOwner, type: string, guid: string, initializer: BrowserInitializer) {
-    super(parent, type, guid, initializer, true);
+    super(parent, type, guid, initializer);
     this._browserType = parent as BrowserType;
     this._channel.on('close', () => {
       this._isConnected = false;
       this.emit(Events.Browser.Disconnected);
       this._isClosedOrClosing = true;
-      this._dispose();
     });
     this._closedPromise = new Promise(f => this.once(Events.Browser.Disconnected, f));
   }

src/rpc/client/browserContext.ts

@@ -48,7 +48,7 @@ export class BrowserContext extends ChannelOwner<BrowserContextChannel, BrowserC
   }
 
   constructor(parent: ChannelOwner, type: string, guid: string, initializer: BrowserContextInitializer, browserName: string) {
-    super(parent, type, guid, initializer, true);
+    super(parent, type, guid, initializer);
     if (parent instanceof Browser)
       this._browser = parent;
     this._browserName = browserName;
@@ -219,7 +219,6 @@ export class BrowserContext extends ChannelOwner<BrowserContextChannel, BrowserC
     if (this._browser)
       this._browser._contexts.delete(this);
     this.emit(Events.BrowserContext.Close);
-    this._dispose();
   }
 
   async close(): Promise<void> {

src/rpc/client/browserServer.ts

@@ -25,10 +25,9 @@ export class BrowserServer extends ChannelOwner<BrowserServerChannel, BrowserSer
   }
 
   constructor(parent: ChannelOwner, type: string, guid: string, initializer: BrowserServerInitializer) {
-    super(parent, type, guid, initializer, true);
+    super(parent, type, guid, initializer);
     this._channel.on('close', ({ exitCode, signal }) => {
       this.emit(Events.BrowserServer.Close, exitCode === undefined ? null : exitCode, signal === undefined ? null : signal);
-      this._dispose();
     });
   }
 

src/rpc/client/browserType.ts

@@ -34,7 +34,7 @@ export class BrowserType extends ChannelOwner<BrowserTypeChannel, BrowserTypeIni
   }
 
   constructor(parent: ChannelOwner, type: string, guid: string, initializer: BrowserTypeInitializer) {
-    super(parent, type, guid, initializer, true);
+    super(parent, type, guid, initializer);
   }
 
   executablePath(): string {

src/rpc/client/cdpSession.ts

@@ -31,13 +31,12 @@ export class CDPSession extends ChannelOwner<CDPSessionChannel, CDPSessionInitia
   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(parent: ChannelOwner, type: string, guid: string, initializer: CDPSessionInitializer) {
-    super(parent, type, guid, initializer, true);
+    super(parent, type, guid, initializer);
 
     this._channel.on('event', ({ method, params }) => {
       const cdpParams = params ? parseResult(params) : undefined;
       this.emit(method, cdpParams);
     });
-    this._channel.on('disconnected', () => this._dispose());
 
     this.on = super.on;
     this.addListener = super.addListener;

src/rpc/client/channelOwner.ts

@@ -17,16 +17,12 @@
 import { EventEmitter } from 'events';
 import type { Channel } from '../channels';
 import type { Connection } from './connection';
-import { assert } from '../../helper';
 import type { LoggerSink } from '../../loggerSink';
 import { DebugLoggerSink } from '../../logger';
 
 export abstract class ChannelOwner<T extends Channel = Channel, Initializer = {}> extends EventEmitter {
   private _connection: Connection;
-  private _isScope: boolean;
-  // Parent is always "isScope".
   private _parent: ChannelOwner | undefined;
-  // Only "isScope" channel owners have registered objects inside.
   private _objects = new Map<string, ChannelOwner>();
 
   readonly _type: string;
@@ -35,12 +31,11 @@ export abstract class ChannelOwner<T extends Channel = Channel, Initializer = {}
   readonly _initializer: Initializer;
   _logger: LoggerSink | undefined;
 
-  constructor(parent: ChannelOwner | Connection, type: string, guid: string, initializer: Initializer, isScope?: boolean) {
+  constructor(parent: ChannelOwner | Connection, type: string, guid: string, initializer: Initializer) {
     super();
     this._connection = parent instanceof ChannelOwner ? parent._connection : parent;
     this._type = type;
     this._guid = guid;
-    this._isScope = !!isScope;
     this._parent = parent instanceof ChannelOwner ? parent : undefined;
 
     this._connection._objects.set(guid, this);
@@ -74,27 +69,21 @@ export abstract class ChannelOwner<T extends Channel = Channel, Initializer = {}
   }
 
   _dispose() {
-    assert(this._isScope);
-
     // Clean up from parent and connection.
     if (this._parent)
       this._parent._objects.delete(this._guid);
     this._connection._objects.delete(this._guid);
 
     // Dispose all children.
-    for (const [guid, object] of [...this._objects]) {
+    for (const object of [...this._objects.values()])
-      if (object._isScope)
+      object._dispose();
-        object._dispose();
-      else
-        this._connection._objects.delete(guid);
-    }
     this._objects.clear();
   }
 
   _debugScopeState(): any {
     return {
       _guid: this._guid,
-      objects: this._isScope ? Array.from(this._objects.values()).map(o => o._debugScopeState()) : undefined,
+      objects: Array.from(this._objects.values()).map(o => o._debugScopeState()),
     };
   }
 

src/rpc/client/connection.ts

@@ -44,7 +44,7 @@ import { FirefoxBrowser } from './firefoxBrowser';
 
 class Root extends ChannelOwner<Channel, {}> {
   constructor(connection: Connection) {
-    super(connection, '', '', {}, true);
+    super(connection, '', '', {});
   }
 }
 
@@ -97,6 +97,10 @@ export class Connection {
       this._createRemoteObject(guid, params.type, params.guid, params.initializer);
       return;
     }
+    if (method === '__dispose__') {
+      this._objects.get(guid)!._dispose();
+      return;
+    }
     const object = this._objects.get(guid)!;
     object._channel.emit(method, this._replaceGuidsWithChannels(params));
   }

src/rpc/client/electron.ts

@@ -33,7 +33,7 @@ export class Electron extends ChannelOwner<ElectronChannel, ElectronInitializer>
   }
 
   constructor(parent: ChannelOwner, type: string, guid: string, initializer: ElectronInitializer) {
-    super(parent, type, guid, initializer, true);
+    super(parent, type, guid, initializer);
   }
 
   async launch(executablePath: string, options: ElectronLaunchOptionsBase & { logger?: LoggerSink } = {}): Promise<ElectronApplication> {
@@ -60,7 +60,7 @@ export class ElectronApplication extends ChannelOwner<ElectronApplicationChannel
   }
 
   constructor(parent: ChannelOwner, type: string, guid: string, initializer: ElectronApplicationInitializer) {
-    super(parent, type, guid, initializer, true);
+    super(parent, type, guid, initializer);
     this._channel.on('context', ({ context }) => this._context = BrowserContext.from(context));
     this._channel.on('window', ({ page, browserWindow }) => {
       const window = Page.from(page);
@@ -69,10 +69,7 @@ export class ElectronApplication extends ChannelOwner<ElectronApplicationChannel
       this.emit(ElectronEvents.ElectronApplication.Window, window);
       window.once(Events.Page.Close, () => this._windows.delete(window));
     });
-    this._channel.on('close', () => {
+    this._channel.on('close', () => this.emit(ElectronEvents.ElectronApplication.Close));
-      this.emit(ElectronEvents.ElectronApplication.Close);
-      this._dispose();
-    });
   }
 
   windows(): Page[] {

src/rpc/protocol.yml

@@ -1852,8 +1852,6 @@ CDPSession:
         method: string
         params: SerializedValue?
 
-    disconnected:
-
 
 
 Electron:

src/rpc/server/cdpSessionDispatcher.ts

@@ -26,10 +26,7 @@ export class CDPSessionDispatcher extends Dispatcher<CRSession, CDPSessionInitia
       const params = cdpParams ? serializeResult(cdpParams) : undefined;
       this._dispatchEvent('event', { method, params });
     };
-    crSession.on(CRSessionEvents.Disconnected, () => {
+    crSession.on(CRSessionEvents.Disconnected, () => this._dispose());
-      this._dispatchEvent('disconnected');
-      this._dispose();
-    });
   }
 
   async send(params: { method: string, params?: SerializedValue }): Promise<{ result: SerializedValue }> {

src/rpc/server/dispatcher.ts

@@ -43,6 +43,7 @@ export class Dispatcher<Type, Initializer> extends EventEmitter implements Chann
   private _parent: Dispatcher<any, any> | undefined;
   // Only "isScope" channel owners have registered dispatchers inside.
   private _dispatchers = new Map<string, Dispatcher<any, any>>();
+  private _disposed = false;
 
   readonly _guid: string;
   readonly _type: string;
@@ -70,7 +71,7 @@ export class Dispatcher<Type, Initializer> extends EventEmitter implements Chann
 
     (object as any)[dispatcherSymbol] = this;
     if (this._parent)
-      this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid });
+      this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid }, !!isScope);
   }
 
   _dispatchEvent(method: string, params: Dispatcher<any, any> | any = {}) {
@@ -78,7 +79,7 @@ export class Dispatcher<Type, Initializer> extends EventEmitter implements Chann
   }
 
   _dispose() {
-    assert(this._isScope);
+    assert(!this._disposed);
 
     // Clean up from parent and connection.
     if (this._parent)
@@ -86,19 +87,18 @@ export class Dispatcher<Type, Initializer> extends EventEmitter implements Chann
     this._connection._dispatchers.delete(this._guid);
 
     // Dispose all children.
-    for (const [guid, dispatcher] of [...this._dispatchers]) {
+    for (const dispatcher of [...this._dispatchers.values()])
-      if (dispatcher._isScope)
+      dispatcher._dispose();
-        dispatcher._dispose();
-      else
-        this._connection._dispatchers.delete(guid);
-    }
     this._dispatchers.clear();
+
+    if (this._isScope)
+      this._connection.sendMessageToClient(this._guid, '__dispose__', {});
   }
 
   _debugScopeState(): any {
     return {
       _guid: this._guid,
-      objects: this._isScope ? Array.from(this._dispatchers.values()).map(o => o._debugScopeState()) : undefined,
+      objects: Array.from(this._dispatchers.values()).map(o => o._debugScopeState()),
     };
   }
 }
@@ -117,8 +117,9 @@ export class DispatcherConnection {
   onmessage = (message: object) => {};
   private _validateParams: (type: string, method: string, params: any) => any;
 
-  async sendMessageToClient(guid: string, method: string, params: any): Promise<any> {
+  async sendMessageToClient(guid: string, method: string, params: any, disallowDispatchers?: boolean): Promise<any> {
-    this.onmessage({ guid, method, params: this._replaceDispatchersWithGuids(params) });
+    const allowDispatchers = !disallowDispatchers;
+    this.onmessage({ guid, method, params: this._replaceDispatchersWithGuids(params, allowDispatchers) });
   }
 
   constructor() {
@@ -165,23 +166,26 @@ export class DispatcherConnection {
     try {
       const validated = this._validateParams(dispatcher._type, method, params);
       const result = await (dispatcher as any)[method](validated);
-      this.onmessage({ id, result: this._replaceDispatchersWithGuids(result) });
+      this.onmessage({ id, result: this._replaceDispatchersWithGuids(result, true) });
     } catch (e) {
       this.onmessage({ id, error: serializeError(e) });
     }
   }
 
-  private _replaceDispatchersWithGuids(payload: any): any {
+  private _replaceDispatchersWithGuids(payload: any, allowDispatchers: boolean): any {
     if (!payload)
       return payload;
-    if (payload instanceof Dispatcher)
+    if (payload instanceof Dispatcher) {
+      if (!allowDispatchers)
+        throw new Error(`Channels are not allowed in the scope's initialzier`);
       return { guid: payload._guid };
+    }
     if (Array.isArray(payload))
-      return payload.map(p => this._replaceDispatchersWithGuids(p));
+      return payload.map(p => this._replaceDispatchersWithGuids(p, allowDispatchers));
     if (typeof payload === 'object') {
       const result: any = {};
       for (const key of Object.keys(payload))
-        result[key] = this._replaceDispatchersWithGuids(payload[key]);
+        result[key] = this._replaceDispatchersWithGuids(payload[key], allowDispatchers);
       return result;
     }
     return payload;

test/channels.jest.js

@@ -31,8 +31,8 @@ describe.skip(!CHANNEL)('Channels', function() {
         ] },
         { _guid: 'BrowserType', objects: [] },
         { _guid: 'BrowserType', objects: [] },
-        { _guid: 'Playwright' },
+        { _guid: 'Playwright', objects: [] },
-        { _guid: 'Selectors' },
+        { _guid: 'Selectors', objects: [] },
         { _guid: 'Electron', objects: [] },
       ]
     };
@@ -49,15 +49,15 @@ describe.skip(!CHANNEL)('Channels', function() {
         { _guid: 'BrowserType', objects: [
           { _guid: 'Browser', objects: [
             { _guid: 'BrowserContext', objects: [
-              { _guid: 'Frame' },
+              { _guid: 'Frame', objects: [] },
-              { _guid: 'Page' },
+              { _guid: 'Page', objects: [] },
-              { _guid: 'Request' },
+              { _guid: 'Request', objects: [] },
-              { _guid: 'Response' },
+              { _guid: 'Response', objects: [] },
             ]},
           ] },
         ] },
-        { _guid: 'Playwright' },
+        { _guid: 'Playwright', objects: [] },
-        { _guid: 'Selectors' },
+        { _guid: 'Selectors', objects: [] },
         { _guid: 'Electron', objects: [] },
       ]
     });
@@ -75,8 +75,8 @@ describe.skip(!CHANNEL)('Channels', function() {
         ] },
         { _guid: 'BrowserType', objects: [] },
         { _guid: 'BrowserType', objects: [] },
-        { _guid: 'Playwright' },
+        { _guid: 'Playwright', objects: [] },
-        { _guid: 'Selectors' },
+        { _guid: 'Selectors', objects: [] },
         { _guid: 'Electron', objects: [] },
       ]
     };
@@ -93,8 +93,8 @@ describe.skip(!CHANNEL)('Channels', function() {
         ] },
         { _guid: 'BrowserType', objects: [] },
         { _guid: 'BrowserType', objects: [] },
-        { _guid: 'Playwright' },
+        { _guid: 'Playwright', objects: [] },
-        { _guid: 'Selectors' },
+        { _guid: 'Selectors', objects: [] },
         { _guid: 'Electron', objects: [] },
       ]
     });
@@ -112,8 +112,8 @@ describe.skip(!CHANNEL)('Channels', function() {
         ] },
         { _guid: 'BrowserType', objects: [] },
         { _guid: 'BrowserType', objects: [] },
-        { _guid: 'Playwright' },
+        { _guid: 'Playwright', objects: [] },
-        { _guid: 'Selectors' },
+        { _guid: 'Selectors', objects: [] },
         { _guid: 'Electron', objects: [] },
       ]
     };
@@ -132,8 +132,8 @@ describe.skip(!CHANNEL)('Channels', function() {
         ] },
         { _guid: 'BrowserType', objects: [] },
         { _guid: 'BrowserType', objects: [] },
-        { _guid: 'Playwright' },
+        { _guid: 'Playwright', objects: [] },
-        { _guid: 'Selectors' },
+        { _guid: 'Selectors', objects: [] },
         { _guid: 'Electron', objects: [] },
       ]
     });
@@ -154,12 +154,6 @@ async function expectScopeState(object, golden) {
 function compareObjects(a, b) {
   if (a._guid !== b._guid)
     return a._guid.localeCompare(b._guid);
-  if (a.objects && !b.objects)
-    return -1;
-  if (!a.objects && b.objects)
-    return 1;
-  if (!a.objects && !b.objects)
-    return 0;
   return a.objects.length - b.objects.length;
 }