From a198b6d753d7710be55236ea31256c9518fb9cf5 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Fri, 15 Jul 2022 07:56:47 -0800 Subject: [PATCH] chore: reparent context objects into the object (#15689) --- .../src/client/browserContext.ts | 2 +- .../src/client/channelOwner.ts | 6 +++++ .../playwright-core/src/client/connection.ts | 24 +++++++++++++------ .../playwright-core/src/protocol/channels.ts | 2 +- .../playwright-core/src/protocol/protocol.yml | 2 +- .../playwright-core/src/protocol/validator.ts | 2 +- .../dispatchers/browserContextDispatcher.ts | 22 +++++++++++------ .../src/server/dispatchers/dispatcher.ts | 15 +++++++++++- .../server/dispatchers/networkDispatchers.ts | 17 ++++++------- .../src/server/dispatchers/pageDispatcher.ts | 22 +++++++++++------ .../server/dispatchers/tracingDispatcher.ts | 3 +-- packages/playwright-core/src/server/fetch.ts | 2 +- .../src/server/trace/recorder/tracing.ts | 7 +----- tests/library/channels.spec.ts | 16 +++++++------ 14 files changed, 92 insertions(+), 50 deletions(-) diff --git a/packages/playwright-core/src/client/browserContext.ts b/packages/playwright-core/src/client/browserContext.ts index 1de5976956..83bbc5db1f 100644 --- a/packages/playwright-core/src/client/browserContext.ts +++ b/packages/playwright-core/src/client/browserContext.ts @@ -74,7 +74,7 @@ export class BrowserContext extends ChannelOwner this._browser = parent; this._isChromium = this._browser?._name === 'chromium'; this.tracing = Tracing.from(initializer.tracing); - this.request = APIRequestContext.from(initializer.APIRequestContext); + this.request = APIRequestContext.from(initializer.requestContext); this._channel.on('bindingCall', ({ binding }) => this._onBinding(BindingCall.from(binding))); this._channel.on('close', () => this._onClose()); diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index 8a615afd4d..b37ea43017 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -57,6 +57,12 @@ export abstract class ChannelOwner) { + child._parent!._objects.delete(child._guid); + this._objects.set(child._guid, child); + child._parent = this; + } + _dispose() { // Clean up from parent and connection. if (this._parent) diff --git a/packages/playwright-core/src/client/connection.ts b/packages/playwright-core/src/client/connection.ts index dc73eb76a0..4090db4f05 100644 --- a/packages/playwright-core/src/client/connection.ts +++ b/packages/playwright-core/src/client/connection.ts @@ -143,16 +143,24 @@ export class Connection extends EventEmitter { this._createRemoteObject(guid, params.type, params.guid, params.initializer); return; } + + const object = this._objects.get(guid); + if (!object) + throw new Error(`Cannot find object to "${method}": ${guid}`); + + if (method === '__adopt__') { + const child = this._objects.get(params.guid); + if (!child) + throw new Error(`Unknown new child: ${params.guid}`); + object._adopt(child); + return; + } + if (method === '__dispose__') { - const object = this._objects.get(guid); - if (!object) - throw new Error(`Cannot find object to dispose: ${guid}`); object._dispose(); return; } - const object = this._objects.get(guid); - if (!object) - throw new Error(`Cannot find object to emit "${method}": ${guid}`); + const validator = findValidator(object._type, method, 'Event'); (object._channel as any).emit(method, validator(params, '', { tChannelImpl: this._tChannelImplFromWire.bind(this), binary: this.isRemote() ? 'fromBase64' : 'buffer' })); } @@ -166,8 +174,10 @@ export class Connection extends EventEmitter { } private _tChannelImplFromWire(names: '*' | string[], arg: any, path: string, context: ValidatorContext) { - if (arg && typeof arg === 'object' && typeof arg.guid === 'string' && this._objects.has(arg.guid)) { + if (arg && typeof arg === 'object' && typeof arg.guid === 'string') { const object = this._objects.get(arg.guid)!; + if (!object) + throw new Error(`Object with guid ${arg.guid} was not bound in the connection`); if (names !== '*' && !names.includes(object._type)) throw new ValidationError(`${path}: expected channel ${names.toString()}`); return object._channel; diff --git a/packages/playwright-core/src/protocol/channels.ts b/packages/playwright-core/src/protocol/channels.ts index 9c884dac5d..d782238a08 100644 --- a/packages/playwright-core/src/protocol/channels.ts +++ b/packages/playwright-core/src/protocol/channels.ts @@ -1207,7 +1207,7 @@ export interface EventTargetEvents { // ----------- BrowserContext ----------- export type BrowserContextInitializer = { isChromium: boolean, - APIRequestContext: APIRequestContextChannel, + requestContext: APIRequestContextChannel, tracing: TracingChannel, }; export interface BrowserContextEventTarget { diff --git a/packages/playwright-core/src/protocol/protocol.yml b/packages/playwright-core/src/protocol/protocol.yml index 6d4324a8e9..26fab326a9 100644 --- a/packages/playwright-core/src/protocol/protocol.yml +++ b/packages/playwright-core/src/protocol/protocol.yml @@ -835,7 +835,7 @@ BrowserContext: initializer: isChromium: boolean - APIRequestContext: APIRequestContext + requestContext: APIRequestContext tracing: Tracing commands: diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index 03edadd0ed..551b1b0dfe 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -644,7 +644,7 @@ scheme.ElectronApplicationWaitForEventInfoResult = tType('EventTargetWaitForEven scheme.AndroidDeviceWaitForEventInfoResult = tType('EventTargetWaitForEventInfoResult'); scheme.BrowserContextInitializer = tObject({ isChromium: tBoolean, - APIRequestContext: tChannel(['APIRequestContext']), + requestContext: tChannel(['APIRequestContext']), tracing: tChannel(['Tracing']), }); scheme.BrowserContextBindingCallEvent = tObject({ diff --git a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts index 20464d7cdf..d44546a046 100644 --- a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts @@ -39,12 +39,20 @@ export class BrowserContextDispatcher extends Dispatcher { // Note: Video must outlive Page and BrowserContext, so that client can saveAs it // after closing the context. We use |scope| for it. - const artifactDispatcher = new ArtifactDispatcher(scope, artifact); + const artifactDispatcher = new ArtifactDispatcher(parentScope, artifact); this._dispatchEvent('video', { artifact: artifactDispatcher }); }; this.addObjectListener(BrowserContext.Events.VideoStarted, onVideo); @@ -94,8 +102,8 @@ export class BrowserContextDispatcher extends Dispatcher this._dispatchEvent('requestFinished', { - request: RequestDispatcher.from(scope, request), - response: ResponseDispatcher.fromNullable(scope, response), + request: RequestDispatcher.from(this._scope, request), + response: ResponseDispatcher.fromNullable(this._scope, response), responseEndTiming: request._responseEndTiming, page: PageDispatcher.fromNullable(this._scope, request.frame()?._page.initializedOrUndefined()), })); diff --git a/packages/playwright-core/src/server/dispatchers/dispatcher.ts b/packages/playwright-core/src/server/dispatchers/dispatcher.ts index cec5390a10..fc1e37b534 100644 --- a/packages/playwright-core/src/server/dispatchers/dispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/dispatcher.ts @@ -88,6 +88,15 @@ export class Dispatcher extends Even this._eventListeners.push(eventsHelper.addEventListener(this._object as unknown as EventEmitter, eventName, handler)); } + adopt(child: Dispatcher) { + assert(this._isScope); + const oldParent = child._parent!; + oldParent._dispatchers.delete(child._guid); + this._dispatchers.set(child._guid, child); + child._parent = this; + this._connection.sendAdopt(this, child); + } + _dispatchEvent>(method: T, params?: channels.EventsTraits[T]) { if (this._disposed) { if (isUnderTest()) @@ -100,7 +109,7 @@ export class Dispatcher extends Even } _dispose() { - assert(!this._disposed); + assert(!this._disposed, `${this._guid} is disposed more than once`); this._disposed = true; eventsHelper.removeEventListeners(this._eventListeners); @@ -171,6 +180,10 @@ export class DispatcherConnection { this._sendMessageToClient(parent._guid, type, '__create__', { type, initializer, guid }, sdkObject); } + sendAdopt(parent: Dispatcher, dispatcher: Dispatcher) { + this._sendMessageToClient(parent._guid, dispatcher._type, '__adopt__', { guid: dispatcher._guid }); + } + sendDispose(dispatcher: Dispatcher) { this._sendMessageToClient(dispatcher._guid, dispatcher._type, '__dispose__', {}); } diff --git a/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts b/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts index 0dff9a50f6..0e145d20c0 100644 --- a/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts +++ b/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts @@ -15,7 +15,7 @@ */ import type * as channels from '../../protocol/channels'; -import { APIRequestContext } from '../fetch'; +import type { APIRequestContext } from '../fetch'; import type { CallMetadata } from '../instrumentation'; import type { Request, Response, Route } from '../network'; import { WebSocket } from '../network'; @@ -172,14 +172,15 @@ export class APIRequestContextDispatcher extends Dispatcher { - if (!this._disposed) - super._dispose(); - }); + + this.adopt(tracing); } async storageState(params?: channels.APIRequestContextStorageStateParams): Promise { diff --git a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts index 0cc9e46a91..6a398835d2 100644 --- a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts @@ -42,22 +42,29 @@ export class PageDispatcher extends Dispatcher imple _type_Page = true; private _page: Page; - static fromNullable(scope: DispatcherScope, page: Page | undefined): PageDispatcher | undefined { + static fromNullable(parentScope: DispatcherScope, page: Page | undefined): PageDispatcher | undefined { if (!page) return undefined; const result = existingDispatcher(page); - return result || new PageDispatcher(scope, page); + return result || new PageDispatcher(parentScope, page); } - constructor(scope: DispatcherScope, page: Page) { + constructor(parentScope: DispatcherScope, page: Page) { // TODO: theoretically, there could be more than one frame already. // If we split pageCreated and pageReady, there should be no main frame during pageCreated. - super(scope, page, 'Page', { - mainFrame: FrameDispatcher.from(scope, page.mainFrame()), + + // We will reparent it to the page below using adopt. + const mainFrame = FrameDispatcher.from(parentScope, page.mainFrame()); + + super(parentScope, page, 'Page', { + mainFrame, viewportSize: page.viewportSize() || undefined, isClosed: page.isClosed(), - opener: PageDispatcher.fromNullable(scope, page.opener()) + opener: PageDispatcher.fromNullable(parentScope, page.opener()) }, true); + + this.adopt(mainFrame); + this._page = page; this.addObjectListener(Page.Events.Close, () => { this._dispatchEvent('close'); @@ -67,7 +74,8 @@ export class PageDispatcher extends Dispatcher imple this.addObjectListener(Page.Events.Crash, () => this._dispatchEvent('crash')); this.addObjectListener(Page.Events.Dialog, dialog => this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this._scope, dialog) })); this.addObjectListener(Page.Events.Download, (download: Download) => { - this._dispatchEvent('download', { url: download.url, suggestedFilename: download.suggestedFilename(), artifact: new ArtifactDispatcher(scope, download.artifact) }); + // Artifact can outlive the page, so bind to the context scope. + this._dispatchEvent('download', { url: download.url, suggestedFilename: download.suggestedFilename(), artifact: new ArtifactDispatcher(parentScope, download.artifact) }); }); this.addObjectListener(Page.Events.FileChooser, (fileChooser: FileChooser) => this._dispatchEvent('fileChooser', { element: ElementHandleDispatcher.from(this._scope, fileChooser.element()), diff --git a/packages/playwright-core/src/server/dispatchers/tracingDispatcher.ts b/packages/playwright-core/src/server/dispatchers/tracingDispatcher.ts index 93830424af..8b5b99a039 100644 --- a/packages/playwright-core/src/server/dispatchers/tracingDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/tracingDispatcher.ts @@ -15,7 +15,7 @@ */ import type * as channels from '../../protocol/channels'; -import { Tracing } from '../trace/recorder/tracing'; +import type { Tracing } from '../trace/recorder/tracing'; import { ArtifactDispatcher } from './artifactDispatcher'; import type { DispatcherScope } from './dispatcher'; import { Dispatcher, existingDispatcher } from './dispatcher'; @@ -30,7 +30,6 @@ export class TracingDispatcher extends Dispatcher this._dispose()); } async tracingStart(params: channels.TracingTracingStartParams): Promise { diff --git a/packages/playwright-core/src/server/fetch.ts b/packages/playwright-core/src/server/fetch.ts index 2974f9e802..f731c37c96 100644 --- a/packages/playwright-core/src/server/fetch.ts +++ b/packages/playwright-core/src/server/fetch.ts @@ -89,7 +89,7 @@ export abstract class APIRequestContext extends SdkObject { } constructor(parent: SdkObject) { - super(parent, 'fetchRequest'); + super(parent, 'request-context'); APIRequestContext.allInstances.add(this); } diff --git a/packages/playwright-core/src/server/trace/recorder/tracing.ts b/packages/playwright-core/src/server/trace/recorder/tracing.ts index e39c726a6c..837b9ff524 100644 --- a/packages/playwright-core/src/server/trace/recorder/tracing.ts +++ b/packages/playwright-core/src/server/trace/recorder/tracing.ts @@ -67,10 +67,6 @@ type RecordingState = { const kScreencastOptions = { width: 800, height: 600, quality: 90 }; export class Tracing extends SdkObject implements InstrumentationListener, SnapshotterDelegate, HarTracerDelegate { - static Events = { - Dispose: 'dispose', - }; - private _writeChain = Promise.resolve(); private _snapshotter?: Snapshotter; private _harTracer: HarTracer; @@ -85,7 +81,7 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps private _contextCreatedEvent: trace.ContextCreatedTraceEvent; constructor(context: BrowserContext | APIRequestContext, tracesDir: string | undefined) { - super(context, 'Tracing'); + super(context, 'tracing'); this._context = context; this._precreatedTracesDir = tracesDir; this._harTracer = new HarTracer(context, null, this, { @@ -211,7 +207,6 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps async dispose() { this._snapshotter?.dispose(); - this.emit(Tracing.Events.Dispose); } async stopChunk(params: TracingTracingStopChunkParams): Promise<{ artifact: Artifact | null, sourceEntries: NameValue[] | undefined }> { diff --git a/tests/library/channels.spec.ts b/tests/library/channels.spec.ts index 15d9a5cc08..cba34e7442 100644 --- a/tests/library/channels.spec.ts +++ b/tests/library/channels.spec.ts @@ -69,13 +69,14 @@ it('should scope context handles', async ({ browserType, server, expectScopeStat { _guid: 'browser-type', objects: [ { _guid: 'browser', objects: [ { _guid: 'browser-context', objects: [ - { _guid: 'frame', objects: [] }, - { _guid: 'page', objects: [] }, + { _guid: 'page', objects: [ + { _guid: 'frame', objects: [] }, + ] }, { _guid: 'request', objects: [] }, + { _guid: 'request-context', objects: [] }, { _guid: 'response', objects: [] }, + { _guid: 'tracing', objects: [] } ] }, - { _guid: 'fetchRequest', objects: [] }, - { _guid: 'Tracing', objects: [] } ] }, ] }, { _guid: 'electron', objects: [] }, @@ -163,9 +164,10 @@ it('should scope browser handles', async ({ browserType, expectScopeState }) => { _guid: 'browser-type', objects: [ { _guid: 'browser', objects: [ - { _guid: 'browser-context', objects: [] }, - { _guid: 'fetchRequest', objects: [] }, - { _guid: 'Tracing', objects: [] } + { _guid: 'browser-context', objects: [ + { _guid: 'request-context', objects: [] }, + { _guid: 'tracing', objects: [] }, + ] }, ] }, ]