From 8b54d3e91994a8fea898200c44fc35096affa2d0 Mon Sep 17 00:00:00 2001 From: Cacie Prins Date: Fri, 16 Aug 2024 10:13:54 -0400 Subject: [PATCH] perf: cri client memory leak (#29988) * Refactor connection logic to cri-connection CDPConnection class instantiates a persistent event emitter, and creates a CDP Client instance via connect(). When this client emits a disconnect event, the CDPConnection will automatically attempt to reconnect, if the connection was not intentionally closed and the connection is created with the reconnect flag. It only listens to 'event' and 'disconnect' events itself, making it easy to unsubscribe from events in the event of a disconnection, helping ease mental load when considering potential memory leaks. Because it uses its own persistent event emitter, event listeners added to the CDPConnection do not need to be re-added when the client reconnects. The CriClient itself no longer handles reconnection logic: it only reacts to connection state changes in order to re-send failed commands and enablements when the CDPConnection restores the CDPClient. * changelog * clean up event listeners before reconnecting * changelog * changelog * changelog * unit tests * lift out of the remote-interface dir * complete lift from /remote-interface * further fix imports * import * fix disconnect behavior to make integration test pass * Update packages/server/test/integration/cdp_spec.ts comment * fix snapgen * improve debug * further debug improvements * do not attach crash handlers on browser level cri clients, add tests * adds bindingCalled listeners discretely, rather than relying on 'event', which does not trigger for them * back out of main `event` listener, use discrete listeners, add integration test for service worker bindingCalled * Revert "back out of main `event` listener, use discrete listeners, add integration test for service worker bindingCalled" This reverts commit 4855120d4278f7c57c756890c8d6d42633282dc6. * changelog * adds integration test for various ways to add cdp event listeners * note in changelog this is a cypress server fix * ensure enablement promises resolve after reconnect * Update cli/CHANGELOG.md Co-authored-by: Matt Schile --------- Co-authored-by: Matt Schile --- cli/CHANGELOG.md | 4 + .../server/lib/browsers/browser-cri-client.ts | 9 +- .../server/lib/browsers/cdp-connection.ts | 233 +++++++++ packages/server/lib/browsers/chrome.ts | 2 + packages/server/lib/browsers/cri-client.ts | 445 ++++++------------ packages/server/lib/browsers/cri-errors.ts | 54 +++ .../lib/browsers/debug-cdp-connection.ts | 81 ++++ packages/server/lib/modes/run.ts | 8 +- packages/server/test/integration/cdp_spec.ts | 53 ++- .../test/unit/browsers/cdp-connection_spec.ts | 351 ++++++++++++++ .../test/unit/browsers/cri-client_spec.ts | 146 +++++- 11 files changed, 1066 insertions(+), 320 deletions(-) create mode 100644 packages/server/lib/browsers/cdp-connection.ts create mode 100644 packages/server/lib/browsers/cri-errors.ts create mode 100644 packages/server/lib/browsers/debug-cdp-connection.ts create mode 100644 packages/server/test/unit/browsers/cdp-connection_spec.ts diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 793e20a3fdc9..2437da53ecd5 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -3,6 +3,10 @@ _Released 8/27/2024 (PENDING)_ +**Performance:** + +- Fixed a potential memory leak in the Cypress server when re-connecting to an unintentionally disconnected CDP connection. Fixes [#29744](https://github.com/cypress-io/cypress/issues/29744). Addressed in [#29988](https://github.com/cypress-io/cypress/pull/29988) + **Dependency Updates:** - Updated `detect-port` from `1.3.0` to `1.6.1`. Addressed in [#30038](https://github.com/cypress-io/cypress/pull/30038). diff --git a/packages/server/lib/browsers/browser-cri-client.ts b/packages/server/lib/browsers/browser-cri-client.ts index 55fc70739ef4..8aadb945d790 100644 --- a/packages/server/lib/browsers/browser-cri-client.ts +++ b/packages/server/lib/browsers/browser-cri-client.ts @@ -5,6 +5,7 @@ import Debug from 'debug' import type { Protocol } from 'devtools-protocol' import { _connectAsync, _getDelayMsForRetry } from './protocol' import * as errors from '../errors' +import type { CypressError } from '@packages/errors' import { CriClient, DEFAULT_NETWORK_ENABLE_OPTIONS } from './cri-client' import { serviceWorkerClientEventHandler, serviceWorkerClientEventHandlerName } from '@packages/proxy/lib/http/util/service-worker-manager' import type { ProtocolManagerShape } from '@packages/types' @@ -23,7 +24,7 @@ type BrowserCriClientOptions = { host: string port: number browserName: string - onAsynchronousError: Function + onAsynchronousError: (err: CypressError) => void protocolManager?: ProtocolManagerShape fullyManageTabs?: boolean onServiceWorkerClientEvent: ServiceWorkerEventHandler @@ -33,7 +34,7 @@ type BrowserCriClientCreateOptions = { browserName: string fullyManageTabs?: boolean hosts: string[] - onAsynchronousError: Function + onAsynchronousError: (err: CypressError) => void onReconnect?: (client: CriClient) => void port: number protocolManager?: ProtocolManagerShape @@ -181,7 +182,7 @@ export class BrowserCriClient { private host: string private port: number private browserName: string - private onAsynchronousError: Function + private onAsynchronousError: (err: CypressError) => void private protocolManager?: ProtocolManagerShape private fullyManageTabs?: boolean onServiceWorkerClientEvent: ServiceWorkerEventHandler @@ -479,7 +480,7 @@ export class BrowserCriClient { browserCriClient.onClose = resolve // or when the browser's CDP ws connection is closed - browserClient.ws.once('close', () => { + browserClient.ws?.once('close', () => { resolve(false) }) }) diff --git a/packages/server/lib/browsers/cdp-connection.ts b/packages/server/lib/browsers/cdp-connection.ts new file mode 100644 index 000000000000..c26a940a79ec --- /dev/null +++ b/packages/server/lib/browsers/cdp-connection.ts @@ -0,0 +1,233 @@ +import type ProtocolMapping from 'devtools-protocol/types/protocol-mapping' +import Debug from 'debug' +import EventEmitter from 'events' +import CDP from 'chrome-remote-interface' +import type { CypressError } from '@packages/errors' +import { debugCdpConnection, DebuggableCDPClient } from './debug-cdp-connection' +import type { CdpEvent, CdpCommand } from './cdp_automation' +import { CDPDisconnectedError, CDPTerminatedError, CDPAlreadyConnectedError } from './cri-errors' +import { asyncRetry } from '../util/async_retry' +import * as errors from '../errors' +import type WebSocket from 'ws' + +const verboseDebugNs = 'cypress-verbose:server:browsers:cdp-connection' + +export type CDPListener = (params: ProtocolMapping.Events[T][0], sessionId?: string) => void + +// CDPClient extends EventEmitter, but does not export that type information from its +// definitelytyped module +export type CdpClient = Exclude & CDP.Client + +/** + * There are three error messages we can encounter which should not be re-thrown, but + * should trigger a reconnection attempt if one is not in progress, and enqueue the + * command that errored. This regex is used in client.send to check for: + * - WebSocket connection closed + * - WebSocket not open + * - WebSocket already in CLOSING or CLOSED state + */ +const isWebsocketClosedErrorMessage = (message: string) => { + return /^WebSocket (?:connection closed|is (?:not open|already in CLOSING or CLOSED state))/.test(message) +} + +export type CDPConnectionOptions = { + automaticallyReconnect: boolean +} + +type CDPConnectionEventListeners = { + 'cdp-connection-reconnect-error': (err: CypressError) => void + 'cdp-connection-reconnect': () => void + 'cdp-connection-closed': () => void + 'cdp-connection-reconnect-attempt': (attemptNumber: number) => void +} +export type CDPConnectionEvent = keyof CDPConnectionEventListeners + +type CDPConnectionEventListener = CDPConnectionEventListeners[T] + +export class CDPConnection { + private _emitter: EventEmitter = new EventEmitter() + private _connection: CdpClient | undefined + private _autoReconnect: boolean + private _terminated: boolean = false + private _reconnection: Promise | undefined + private debug: Debug.Debugger + private verboseDebug: Debug.Debugger + + constructor (private readonly _options: CDP.Options, connectionOptions: CDPConnectionOptions) { + this._autoReconnect = connectionOptions.automaticallyReconnect + this.debug = Debug(`cypress:server:browsers:cdp-connection:${_options.target}`) + this.verboseDebug = Debug(`${verboseDebugNs}:${_options.target}`) + } + + get terminated () { + return this._terminated + } + + get ws () { + // this is reached into by browser-cri-client to detect close events - needs rethinking + return (this._connection as { _ws?: WebSocket})._ws + } + + on (event: T, callback: CDPListener) { + this.debug('attaching event listener to cdp connection', event) + + this._emitter.on(event, callback) + } + addConnectionEventListener (event: T, callback: CDPConnectionEventListener) { + this.debug('adding connection event listener for ', event) + this._emitter.on(event, callback) + } + off (event: T, callback: CDPListener) { + this._emitter.off(event, callback) + } + removeConnectionEventListener (event: T, callback: CDPConnectionEventListener) { + this._emitter.off(event, callback) + } + + async connect (): Promise { + if (this._terminated) { + throw new CDPTerminatedError(`Cannot connect to CDP. Client target ${this._options.target} has been terminated.`) + } + + if (this._connection) { + throw new CDPAlreadyConnectedError(`Cannot connect to CDP. Client target ${this._options.target} is already connected. Did you disconnect first?`) + } + + this._connection = await CDP(this._options) as CdpClient + + debugCdpConnection(this.verboseDebug.namespace, this._connection as DebuggableCDPClient) + + this._connection.on('event', this._broadcastEvent) + + if (this._autoReconnect) { + this._connection.on('disconnect', this._reconnect) + } + } + + async disconnect () { + this.debug('disconnect of target %s requested.', this._options.target, { terminated: this._terminated, connection: !!this._connection, reconnection: !!this._reconnection }) + if (this._terminated && !this._connection) { + return + } + + this._terminated = true + + if (this._connection) { + await this._gracefullyDisconnect() + this._emitter.emit('cdp-connection-closed') + } + } + + private _gracefullyDisconnect = async () => { + this._connection?.off('event', this._broadcastEvent) + this._connection?.off('disconnect', this._reconnect) + + await this._connection?.close() + this._connection = undefined + } + + async send ( + command: T, + data?: ProtocolMapping.Commands[T]['paramsType'][0], + sessionId?: string, + ): Promise { + if (this.terminated) { + throw new CDPDisconnectedError(`${command} will not run as the CRI connection to Target ${this._options.target} has been terminated.`) + } + + if (!this._connection) { + throw new CDPDisconnectedError(`${command} will not run as the CRI connection to Target ${this._options.target} has not been established.`) + } + + try { + return await this._connection.send(command, data, sessionId) + } catch (e) { + // Clients may wish to determine if the command should be enqueued + // should enqueue logic live in this class tho?? + if (isWebsocketClosedErrorMessage(e.message)) { + throw new CDPDisconnectedError(`${command} failed due to the websocket being disconnected.`, e) + } + + throw e + } + } + + private _reconnect = async () => { + this.debug('Reconnection requested') + if (this._terminated) { + return + } + + if (this._reconnection) { + return this._reconnection + } + + if (this._connection) { + try { + await this._gracefullyDisconnect() + } catch (e) { + this.debug('Error cleaning up existing CDP connection before creating a new connection: ', e) + } finally { + this._connection = undefined + } + } + + let attempt = 0 + + this._reconnection = asyncRetry(async () => { + attempt++ + + this.debug('Reconnection attempt %d for Target %s', attempt, this._options.target) + + if (this._terminated) { + this.debug('Not reconnecting, connection to %s has been terminated', this._options.target) + throw new CDPTerminatedError(`Cannot reconnect to CDP. Client target ${this._options.target} has been terminated.`) + } + + this._emitter.emit('cdp-connection-reconnect-attempt', attempt) + + await this.connect() + }, { + maxAttempts: 20, + retryDelay: () => { + return 100 + }, + shouldRetry (err) { + return !(err && CDPTerminatedError.isCDPTerminatedError(err)) + }, + })() + + try { + await this._reconnection + this._emitter.emit('cdp-connection-reconnect') + } catch (err) { + this.debug('error(s) on reconnecting: ', err) + const significantError: Error = err.errors ? (err as AggregateError).errors[err.errors.length - 1] : err + + const retryHaltedDueToClosed = CDPTerminatedError.isCDPTerminatedError(err) || + (err as AggregateError)?.errors?.find((predicate) => CDPTerminatedError.isCDPTerminatedError(predicate)) + + // if .disconnect() was called while trying to reconnect, there will be no active connection + // so the .disconnect() method will not emit the connection closed event. However, we do + // want to emit that once the reconnection attempts cease due to being closed. + if (retryHaltedDueToClosed) { + this._emitter.emit('cdp-connection-closed') + } else { + const cdpError = errors.get('CDP_COULD_NOT_RECONNECT', significantError) + + cdpError.isFatalApiErr = true + this._emitter.emit('cdp-connection-reconnect-error', cdpError) + } + } + + this._reconnection = undefined + } + + private _broadcastEvent = ({ method, params, sessionId }: { method: CdpEvent, params: Record, sessionId?: string }) => { + this.verboseDebug('rebroadcasting event', method, params, sessionId) + + this._emitter.emit('event', { method, params, sessionId }) + this._emitter.emit(method, params, sessionId) + this._emitter.emit(`${method}.${sessionId}`, params) + } +} diff --git a/packages/server/lib/browsers/chrome.ts b/packages/server/lib/browsers/chrome.ts index 33ae2d6643f9..d386dded7604 100644 --- a/packages/server/lib/browsers/chrome.ts +++ b/packages/server/lib/browsers/chrome.ts @@ -445,7 +445,9 @@ export = { const browserCriClient = this._getBrowserCriClient() // Handle chrome tab crashes. + debug('attaching crash handler to target ', pageCriClient.targetId) pageCriClient.on('Target.targetCrashed', async (event) => { + debug('target crashed!', event) if (event.targetId !== browserCriClient?.currentlyAttachedTarget?.targetId) { return } diff --git a/packages/server/lib/browsers/cri-client.ts b/packages/server/lib/browsers/cri-client.ts index 595fbeb09b08..36e123eb3f12 100644 --- a/packages/server/lib/browsers/cri-client.ts +++ b/packages/server/lib/browsers/cri-client.ts @@ -1,34 +1,14 @@ -import CDP from 'chrome-remote-interface' import debugModule from 'debug' -import _ from 'lodash' -import * as errors from '../errors' import { CDPCommandQueue } from './cdp-command-queue' -import { asyncRetry } from '../util/async_retry' +import { CDPConnection, CDPListener } from './cdp-connection' import type ProtocolMapping from 'devtools-protocol/types/protocol-mapping' -import type EventEmitter from 'events' import type WebSocket from 'ws' - +import type { CypressError } from '@packages/errors' import type { SendDebuggerCommand, OnFn, OffFn, CdpCommand, CdpEvent } from './cdp_automation' +import { CDPDisconnectedError } from './cri-errors' import type { ProtocolManagerShape } from '@packages/types' const debug = debugModule('cypress:server:browsers:cri-client') -const debugVerbose = debugModule('cypress-verbose:server:browsers:cri-client') -// debug using cypress-verbose:server:browsers:cri-client:send:* -const debugVerboseSend = debugModule(`${debugVerbose.namespace}:send:[-->]`) -// debug using cypress-verbose:server:browsers:cri-client:recv:* -const debugVerboseReceive = debugModule(`${debugVerbose.namespace}:recv:[<--]`) -// debug using cypress-verbose:server:browsers:cri-client:err:* -const debugVerboseLifecycle = debugModule(`${debugVerbose.namespace}:ws`) - -/** - * There are three error messages we can encounter which should not be re-thrown, but - * should trigger a reconnection attempt if one is not in progress, and enqueue the - * command that errored. This regex is used in client.send to check for: - * - WebSocket connection closed - * - WebSocket not open - * - WebSocket already in CLOSING or CLOSED state - */ -const WEBSOCKET_NOT_OPEN_RE = /^WebSocket (?:connection closed|is (?:not open|already in CLOSING or CLOSED state))/ type QueuedMessages = { enableCommands: EnableCommand[] @@ -56,20 +36,6 @@ type Subscription = { type CmdParams = ProtocolMapping.Commands[TCmd]['paramsType'][0] -interface CDPClient extends CDP.Client { - off: EventEmitter['off'] - _ws: WebSocket -} - -const ConnectionClosedKind: 'CONNECTION_CLOSED' = 'CONNECTION_CLOSED' - -class ConnectionClosedError extends Error { - public readonly kind = ConnectionClosedKind - static isConnectionClosedError (err: Error & { kind?: any }): err is ConnectionClosedError { - return err.kind === ConnectionClosedKind - } -} - export const DEFAULT_NETWORK_ENABLE_OPTIONS = { maxTotalBufferSize: 0, maxResourceBufferSize: 0, @@ -84,7 +50,7 @@ export interface ICriClient { /** * The underlying websocket connection */ - ws: CDPClient['_ws'] + ws?: WebSocket /** * Sends a command to the Chrome remote interface. * @example client.send('Page.navigate', { url }) @@ -118,70 +84,10 @@ export interface ICriClient { off: OffFn } -const maybeDebugCdpMessages = (cri: CDPClient) => { - if (debugVerboseReceive.enabled) { - cri._ws.prependListener('message', (data) => { - data = _ - .chain(JSON.parse(data)) - .tap((data) => { - ([ - 'params.data', // screencast frame data - 'result.data', // screenshot data - ]).forEach((truncatablePath) => { - const str = _.get(data, truncatablePath) - - if (!_.isString(str)) { - return - } - - _.set(data, truncatablePath, _.truncate(str, { - length: 100, - omission: `... [truncated string of total bytes: ${str.length}]`, - })) - }) - - return data - }) - .value() - - debugVerboseReceive('received CDP message %o', data) - }) - } - - if (debugVerboseSend.enabled) { - const send = cri._ws.send - - cri._ws.send = (data, callback) => { - debugVerboseSend('sending CDP command %o', JSON.parse(data)) - - try { - return send.call(cri._ws, data, callback) - } catch (e: any) { - debugVerboseSend('Error sending CDP command %o %O', JSON.parse(data), e) - throw e - } - } - } - - if (debugVerboseLifecycle.enabled) { - cri._ws.addEventListener('open', (event) => { - debugVerboseLifecycle(`[OPEN] %o`, event) - }) - - cri._ws.addEventListener('close', (event) => { - debugVerboseLifecycle(`[CLOSE] %o`, event) - }) - - cri._ws.addEventListener('error', (event) => { - debugVerboseLifecycle(`[ERROR] %o`, event) - }) - } -} - type DeferredPromise = { resolve: Function, reject: Function } type CreateParams = { target: string - onAsynchronousError: Function + onAsynchronousError: (err: CypressError) => void host?: string port?: number onReconnect?: (client: CriClient) => void @@ -193,6 +99,9 @@ type CreateParams = { } export class CriClient implements ICriClient { + // subscriptions are recorded, but this may no longer be necessary. cdp event listeners + // need only be added to the connection instance, not the (ephemeral) underlying + // CDP.Client instances private subscriptions: Subscription[] = [] private enableCommands: EnableCommand[] = [] private enqueuedCommands: EnqueuedCommand[] = [] @@ -201,23 +110,74 @@ export class CriClient implements ICriClient { private _closed = false private _connected = false - private crashed = false - private reconnection: Promise | undefined = undefined + private _isChildTarget = false - private cri: CDPClient | undefined + private _crashed = false + private cdpConnection: CDPConnection private constructor ( public targetId: string, - private onAsynchronousError: Function, + onAsynchronousError: (err: CypressError) => void, private host?: string, private port?: number, private onReconnect?: (client: CriClient) => void, private protocolManager?: ProtocolManagerShape, private fullyManageTabs?: boolean, private browserClient?: ICriClient, - private onReconnectAttempt?: (retryIndex: number) => void, - private onCriConnectionClosed?: () => void, - ) {} + onReconnectAttempt?: (retryIndex: number) => void, + onCriConnectionClosed?: () => void, + ) { + debug('creating cri client with', { + host, port, targetId, + }) + + // refactor opportunity: + // due to listeners passed in along with connection options, the fns that instantiate this + // class should instantiate and listen to the connection directly rather than having this + // constructor create them. The execution and/or definition of these callbacks is not this + // class' business. + this.cdpConnection = new CDPConnection({ + host: this.host, + port: this.port, + target: this.targetId, + local: true, + useHostName: true, + }, { + // Only automatically reconnect if: this is the root browser cri target (no host), or cy in cy + automaticallyReconnect: !this.host && !process.env.CYPRESS_INTERNAL_E2E_TESTING_SELF, + }) + + this.cdpConnection.addConnectionEventListener('cdp-connection-reconnect-error', onAsynchronousError) + this.cdpConnection.addConnectionEventListener('cdp-connection-reconnect', this._onCdpConnectionReconnect) + + if (onCriConnectionClosed) { + this.cdpConnection.addConnectionEventListener('cdp-connection-closed', onCriConnectionClosed) + } + + if (onReconnectAttempt) { + this.cdpConnection.addConnectionEventListener('cdp-connection-reconnect-attempt', onReconnectAttempt) + } + + this._isChildTarget = !!this.host + + if (this._isChildTarget) { + // If crash listeners are added at the browser level, tabs/page connections do not + // emit them. + this.cdpConnection.on('Target.targetCrashed', async (event) => { + debug('crash event detected', event) + if (event.targetId !== this.targetId) { + return + } + + debug('crash detected') + this._crashed = true + }) + + if (fullyManageTabs) { + this.cdpConnection.on('Target.attachedToTarget', this._onAttachedToTarget) + } + } + } static async create ({ target, @@ -238,10 +198,6 @@ export class CriClient implements ICriClient { return newClient } - get ws () { - return this.cri!._ws - } - // this property is accessed in a couple different places, but should be refactored to be // private - queues are internal to this class, and should not be exposed get queue () { @@ -257,6 +213,11 @@ export class CriClient implements ICriClient { } } + // this property is accessed by browser-cri-client, to event on websocket closed. + get ws () { + return this.cdpConnection.ws + } + get closed () { return this._closed } @@ -265,83 +226,24 @@ export class CriClient implements ICriClient { return this._connected } - public connect = async () => { - await this.cri?.close() + get crashed () { + return this._crashed + } + public connect = async () => { debug('connecting %o', { connected: this._connected, target: this.targetId }) - /** - * TODO: https://github.com/cypress-io/cypress/issues/29744 - * this (`cri` / `this.cri`) symbol is referenced via closure in event listeners added in this method; this - * may prevent old instances of `CDPClient` from being garbage collected. - */ - - const cri = this.cri = await CDP({ - host: this.host, - port: this.port, - target: this.targetId, - local: true, - useHostName: true, - }) as CDPClient + await this.cdpConnection.connect() this._connected = true - debug('connected %o', { connected: this._connected, target: this.targetId, ws: this.cri._ws }) - - maybeDebugCdpMessages(this.cri) - - // Having a host set indicates that this is the child cri target, a.k.a. - // the main Cypress tab (as opposed to the root browser cri target) - const isChildTarget = !!this.host - - // don't reconnect in these circumstances - if ( - // is a child target. we only need to reconnect the root browser target - !isChildTarget - // running cypress in cypress - there are a lot of disconnects that happen - // that we don't want to reconnect on - && !process.env.CYPRESS_INTERNAL_E2E_TESTING_SELF - ) { - this.cri.on('disconnect', this._reconnect) + if (this._isChildTarget) { + // Ideally we could use filter rather than checking the type above, but that was added relatively recently + await this.cdpConnection.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true }) + await this.cdpConnection.send('Target.setDiscoverTargets', { discover: true }) } - // We're only interested in child target traffic. Browser cri traffic is - // handled in browser-cri-client.ts. The basic approach here is we attach - // to targets and enable network traffic. We must attach in a paused state - // so that we can enable network traffic before the target starts running. - if (isChildTarget) { - this.cri.on('Target.targetCrashed', async (event) => { - if (event.targetId !== this.targetId) { - return - } - - debug('crash detected') - this.crashed = true - }) - - if (this.fullyManageTabs) { - cri.on('Target.attachedToTarget', async (event) => { - try { - // Service workers get attached at the page and browser level. We only want to handle them at the browser level - // We don't track child tabs/page network traffic. 'other' targets can't have network enabled - if (event.targetInfo.type !== 'service_worker' && event.targetInfo.type !== 'page' && event.targetInfo.type !== 'other') { - await cri.send('Network.enable', this.protocolManager?.networkEnableOptions ?? DEFAULT_NETWORK_ENABLE_OPTIONS, event.sessionId) - } - - if (event.waitingForDebugger) { - await cri.send('Runtime.runIfWaitingForDebugger', undefined, event.sessionId) - } - } catch (error) { - // it's possible that the target was closed before we could enable network and continue, in that case, just ignore - debug('error attaching to target cri', error) - } - }) - - // Ideally we could use filter rather than checking the type above, but that was added relatively recently - await this.cri.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true }) - await this.cri.send('Target.setDiscoverTargets', { discover: true }) - } - } + debug('connected %o', { connected: this._connected, target: this.targetId }) } public send = async ( @@ -349,7 +251,7 @@ export class CriClient implements ICriClient { params?: CmdParams, sessionId?: string, ): Promise => { - if (this.crashed) { + if (this._crashed) { return Promise.reject(new Error(`${command} will not run as the target browser or tab CRI connection has crashed`)) } @@ -372,17 +274,16 @@ export class CriClient implements ICriClient { this.enableCommands.push(obj) } - if (this._connected) { + if (this._connected && this.cdpConnection) { try { - return await this.cri!.send(command, params, sessionId) + return await this.cdpConnection.send(command, params, sessionId) } catch (err) { debug('Encountered error on send %o', { command, params, sessionId, err }) // This error occurs when the browser has been left open for a long // time and/or the user's computer has been put to sleep. The // socket disconnects and we need to recreate the socket and // connection - if (!WEBSOCKET_NOT_OPEN_RE.test(err.message)) { - debug('error classified as not WEBSOCKET_NOT_OPEN_RE, rethrowing') + if (!CDPDisconnectedError.isCDPDisconnectedError(err)) { throw err } @@ -390,10 +291,8 @@ export class CriClient implements ICriClient { const p = this._enqueueCommand(command, params, sessionId) - await this._reconnect() - // if enqueued commands were wiped out from the reconnect and the socket is already closed, reject the command as it will never be run - if (this.enqueuedCommands.length === 0 && this.closed) { + if (this.enqueuedCommands.length === 0 && this.cdpConnection.terminated) { debug('connection was closed was trying to reconnect') return Promise.reject(new Error(`${command} will not run as browser CRI connection was reset`)) @@ -406,12 +305,16 @@ export class CriClient implements ICriClient { return this._enqueueCommand(command, params, sessionId) } - public on = (eventName: T, cb: (data: ProtocolMapping.Events[T][0], sessionId?: string) => void) => { + public on = (eventName: T, cb: CDPListener) => { + if (eventName === 'Target.targetCrashed') { + debug('attaching crash listener') + } + + this.cdpConnection?.on(eventName, cb) + this.subscriptions.push({ eventName, cb }) debug('registering CDP on event %o', { eventName }) - this.cri!.on(eventName, cb) - if (eventName.startsWith('Network.')) { this.browserClient?.on(eventName, cb) } @@ -422,7 +325,7 @@ export class CriClient implements ICriClient { return sub.eventName === eventName && sub.cb === cb }), 1) - this.cri!.off(eventName, cb) + this.cdpConnection!.off(eventName, cb) // This ensures that we are notified about the browser's network events that have been registered (e.g. service workers) // Long term we should use flat mode entirely across all of chrome remote interface if (eventName.startsWith('Network.')) { @@ -432,8 +335,8 @@ export class CriClient implements ICriClient { public close = async () => { debug('closing') - if (this._closed) { - debug('not closing, cri client is already closed %o', { closed: this._closed, target: this.targetId }) + if (this._closed || this.cdpConnection?.terminated) { + debug('not closing, cri client is already closed %o', { closed: this._closed, target: this.targetId, connection: this.cdpConnection }) return } @@ -443,14 +346,35 @@ export class CriClient implements ICriClient { this._closed = true try { - await this.cri?.close() + await this.cdpConnection?.disconnect() + debug('closed cri client %o', { closed: this._closed, target: this.targetId }) } catch (e) { debug('error closing cri client targeting %s: %o', this.targetId, e) - } finally { - debug('closed cri client %o', { closed: this._closed, target: this.targetId }) - if (this.onCriConnectionClosed) { - this.onCriConnectionClosed() + } + } + + private _onAttachedToTarget = async (event: ProtocolMapping.Events['Target.attachedToTarget'][0]) => { + // We're only interested in child target traffic. Browser cri traffic is + // handled in browser-cri-client.ts. The basic approach here is we attach + // to targets and enable network traffic. We must attach in a paused state + // so that we can enable network traffic before the target starts running. + if (!this.fullyManageTabs || !this.host) { + return + } + + try { + // Service workers get attached at the page and browser level. We only want to handle them at the browser level + // We don't track child tabs/page network traffic. 'other' targets can't have network enabled + if (event.targetInfo.type !== 'service_worker' && event.targetInfo.type !== 'page' && event.targetInfo.type !== 'other') { + await this.cdpConnection.send('Network.enable', this.protocolManager?.networkEnableOptions ?? DEFAULT_NETWORK_ENABLE_OPTIONS, event.sessionId) } + + if (event.waitingForDebugger) { + await this.cdpConnection.send('Runtime.runIfWaitingForDebugger', undefined, event.sessionId) + } + } catch (error) { + // it's possible that the target was closed before we could enable network and continue, in that case, just ignore + debug('error attaching to target cri', error) } } @@ -462,107 +386,27 @@ export class CriClient implements ICriClient { return this._commandQueue.add(command, params, sessionId) } - private _isConnectionError (error: Error) { - return WEBSOCKET_NOT_OPEN_RE.test(error.message) - } - - private _reconnect = async () => { - debug('preparing to reconnect') - if (this.reconnection) { - debug('not reconnecting as there is an active reconnection attempt') - - return this.reconnection - } - - this._connected = false - - if (this._closed) { - debug('Target %s disconnected, not reconnecting because client is closed.', this.targetId) - this._commandQueue.clear() - - return - } - - let attempt = 1 - - try { - this.reconnection = asyncRetry(() => { - if (this._closed) { - throw new ConnectionClosedError('Reconnection halted due to a closed client.') - } - - this.onReconnectAttempt?.(attempt) - attempt++ - - return this.connect() - }, { - maxAttempts: 20, - retryDelay: () => 100, - shouldRetry: (err) => { - debug('error while reconnecting to Target %s: %o', this.targetId, err) - if (err && ConnectionClosedError.isConnectionClosedError(err)) { - return false - } - - debug('Retying reconnection attempt') - - return true - }, - })() - - await this.reconnection - this.reconnection = undefined - debug('reconnected') - } catch (err) { - debug('error(s) on reconnecting: ', err) - const significantError: Error = err.errors ? (err as AggregateError).errors[err.errors.length - 1] : err - - const retryHaltedDueToClosed = ConnectionClosedError.isConnectionClosedError(err) || - (err as AggregateError)?.errors?.find((predicate) => ConnectionClosedError.isConnectionClosedError(predicate)) - - if (!retryHaltedDueToClosed) { - const cdpError = errors.get('CDP_COULD_NOT_RECONNECT', significantError) - - cdpError.isFatalApiErr = true - this.reconnection = undefined - this._commandQueue.clear() - this.onAsynchronousError(cdpError) - } - - // do not re-throw; error handling is done via onAsynchronousError - return - } - + private _onCdpConnectionReconnect = async () => { + debug('cdp connection reconnected') try { await this._restoreState() await this._drainCommandQueue() await this.protocolManager?.cdpReconnect() - } catch (e) { - if (this._isConnectionError(e)) { - return this._reconnect() - } - - throw e - } - // previous timing of this had it happening before subscriptions/enablements were restored, - // and before any enqueued commands were sent. This made testing problematic. Changing the - // timing may have implications for browsers that wish to update frame tree - that process - // will now be kicked off after state restoration & pending commands, rather then before. - // This warrants extra scrutiny in tests. (convert to PR comment) - if (this.onReconnect) { - this.onReconnect(this) + try { + if (this.onReconnect) { + await this.onReconnect(this) + } + } catch (e) { + debug('uncaught error in CriClient reconnect callback: ', e) + } + } catch (e) { + debug('error re-establishing state on reconnection: ', e) } } private async _restoreState () { - debug('resubscribing to %d subscriptions', this.subscriptions.length) - - this.subscriptions.forEach((sub) => { - this.cri?.on(sub.eventName, sub.cb as any) - }) - // '*.enable' commands need to be resent on reconnect or any events in // that namespace will no longer be received debug('re-enabling %d enablements', this.enableCommands.length) @@ -572,14 +416,20 @@ export class CriClient implements ICriClient { const inFlightCommand = this._commandQueue.extract({ command, params, sessionId }) try { - const response = await this.cri?.send(command, params, sessionId) + const response = await this.cdpConnection.send(command, params, sessionId) inFlightCommand?.deferred.resolve(response) } catch (err) { debug('error re-enabling %s: ', command, err) - if (this._isConnectionError(err)) { - // Connection errors are thrown here so that a reconnection attempt - // can be made. + if (CDPDisconnectedError.isCDPDisconnectedError(err)) { + // this error is caught in _onCdpConnectionReconnect + // because this is a connection error, the enablement will be re-attempted + // when _onCdpConnectionReconnect is called again. We do need to ensure the + // original in-flight command, if present, is re-enqueued. + if (inFlightCommand) { + this._commandQueue.unshift(inFlightCommand) + } + throw err } else { // non-connection errors are appropriate for rejecting the original command promise @@ -600,18 +450,15 @@ export class CriClient implements ICriClient { try { debug('sending enqueued command %s', enqueued.command) - const response = await this.cri!.send(enqueued.command, enqueued.params, enqueued.sessionId) + const response = await this.cdpConnection.send(enqueued.command, enqueued.params, enqueued.sessionId) debug('sent command, received ', { response }) enqueued.deferred.resolve(response) debug('resolved enqueued promise') } catch (e) { debug('enqueued command %s failed:', enqueued.command, e) - if (this._isConnectionError(e)) { - // similar to restoring state, connection errors are re-thrown so that - // the connection can be restored. The command is queued for re-delivery - // upon reconnect. - debug('re-enqueuing command and re-throwing') + if (CDPDisconnectedError.isCDPDisconnectedError(e)) { + debug('command failed due to disconnection; enqueuing for resending once reconnected') this._commandQueue.unshift(enqueued) throw e } else { diff --git a/packages/server/lib/browsers/cri-errors.ts b/packages/server/lib/browsers/cri-errors.ts new file mode 100644 index 000000000000..2c05550f7321 --- /dev/null +++ b/packages/server/lib/browsers/cri-errors.ts @@ -0,0 +1,54 @@ +const Kinds = Object.freeze({ + CDP_CRASHED: 'cdp_crashed', + CDP_DISCONNECTED: 'cdp_disconnected', + CDP_TERMINATED: 'cdp_terminated', + CDP_ALREADY_CONNECTED: 'cdp_already_connected', +}) + +type CdpErrorKind = typeof Kinds[keyof typeof Kinds] + +type MaybeCdpError = Error & { kind?: CdpErrorKind } + +export class CDPCrashedError extends Error { + public readonly kind = Kinds.CDP_CRASHED + + public static isCDPCrashedError (error: MaybeCdpError): error is CDPCrashedError { + return error.kind === Kinds.CDP_CRASHED + } +} + +export class CDPDisconnectedError extends Error { + public readonly kind = Kinds.CDP_DISCONNECTED + + constructor (message: string, public readonly originalError?: Error) { + super(message) + } + + public static isCDPDisconnectedError (error: MaybeCdpError): error is CDPDisconnectedError { + return error.kind === Kinds.CDP_DISCONNECTED + } +} + +export class CDPTerminatedError extends Error { + public readonly kind = Kinds.CDP_TERMINATED + + constructor (message: string, public readonly originalError?: Error) { + super(message) + } + + public static isCDPTerminatedError (error: MaybeCdpError): error is CDPTerminatedError { + return error.kind === Kinds.CDP_TERMINATED + } +} + +export class CDPAlreadyConnectedError extends Error { + public readonly kind = Kinds.CDP_ALREADY_CONNECTED + + constructor (message: string, public readonly originalError?: Error) { + super(message) + } + + public static isCDPAlreadyConnectedError (error: MaybeCdpError): error is CDPAlreadyConnectedError { + return error.kind === Kinds.CDP_ALREADY_CONNECTED + } +} diff --git a/packages/server/lib/browsers/debug-cdp-connection.ts b/packages/server/lib/browsers/debug-cdp-connection.ts new file mode 100644 index 000000000000..0e0b5188807d --- /dev/null +++ b/packages/server/lib/browsers/debug-cdp-connection.ts @@ -0,0 +1,81 @@ +import Debug from 'debug' +import _ from 'lodash' +import type CDP from 'chrome-remote-interface' +import type EventEmitter from 'events' +import type WebSocket from 'ws' + +export interface DebuggableCDPClient extends CDP.Client { + off: EventEmitter['off'] + // ws is defined as optional here, because it is not a declared public property of + // CDP.Client - it may be removed in future minor/patch versions + _ws?: WebSocket +} + +export const debugCdpConnection = (namespace: string, cri: DebuggableCDPClient) => { + // debug using cypress-verbose:server:browsers:cri-client:send:* + const debugVerboseSend = Debug(`${namespace}:send:[-->]`) + // debug using cypress-verbose:server:browsers:cri-client:recv:* + const debugVerboseReceive = Debug(`${namespace}:recv:[<--]`) + // debug using cypress-verbose:server:browsers:cri-client:err:* + const debugVerboseLifecycle = Debug(`${namespace}:ws`) + + if (debugVerboseReceive.enabled) { + cri._ws?.prependListener('message', (data) => { + data = _ + .chain(JSON.parse(data)) + .tap((data) => { + ([ + 'params.data', // screencast frame data + 'result.data', // screenshot data + ]).forEach((truncatablePath) => { + const str = _.get(data, truncatablePath) + + if (!_.isString(str)) { + return + } + + _.set(data, truncatablePath, _.truncate(str, { + length: 100, + omission: `... [truncated string of total bytes: ${str.length}]`, + })) + }) + + return data + }) + .value() + + debugVerboseReceive('received CDP message %o', data) + }) + } + + if (debugVerboseSend.enabled) { + if (cri._ws) { + const send = cri._ws?.send + + cri._ws.send = (data, callback) => { + debugVerboseSend('sending CDP command %o', JSON.parse(data)) + + try { + return send.call(cri._ws, data, callback) + } catch (e: any) { + debugVerboseSend('Error sending CDP command %o %O', JSON.parse(data), e) + throw e + } + } + } + } + + if (debugVerboseLifecycle.enabled) { + cri._ws?.addEventListener('open', (event) => { + debugVerboseLifecycle(`[OPEN] %o`, event) + }) + + cri._ws?.addEventListener('close', (event) => { + debugVerboseLifecycle(`[CLOSE] %o`, event) + }) + + cri._ws?.addEventListener('error', (event) => { + debugVerboseLifecycle(`[ERROR] %o`, event) + }) + } +} diff --git a/packages/server/lib/modes/run.ts b/packages/server/lib/modes/run.ts index 66a626057e64..3dd24d2ae741 100644 --- a/packages/server/lib/modes/run.ts +++ b/packages/server/lib/modes/run.ts @@ -500,6 +500,7 @@ async function waitForBrowserToConnect (options: { project: Project, socketId: s telemetry.getSpan(`waitForBrowserToConnect:attempt:${browserLaunchAttempt}`)?.end() }) .catch(Bluebird.TimeoutError, async (err) => { + debug('Catch on waitForBrowserToConnect') telemetry.getSpan(`waitForBrowserToConnect:attempt:${browserLaunchAttempt}`)?.end() console.log('') @@ -936,7 +937,10 @@ async function runSpec (config, spec: SpecWithRelativeRoot, options: { project: project, browser, screenshots, - onError, + onError: (...args) => { + debug('onError from runSpec') + onError(...args) + }, videoRecording, socketId: options.socketId, webSecurity: options.webSecurity, @@ -996,7 +1000,7 @@ async function ready (options: ReadyOptions) { // TODO: refactor this so we don't need to extend options const onError = options.onError = (err) => { - debug('onError') + debug('onError', new Error().stack) earlyExitTerminator.exitEarly(err) } diff --git a/packages/server/test/integration/cdp_spec.ts b/packages/server/test/integration/cdp_spec.ts index 2bd2b5bd4422..8717800039e6 100644 --- a/packages/server/test/integration/cdp_spec.ts +++ b/packages/server/test/integration/cdp_spec.ts @@ -27,9 +27,10 @@ describe('CDP Clients', () => { let wsSrv: WebSocket.Server let criClient: CriClient + let wsClient: WebSocket let messages: object[] let onMessage: sinon.SinonStub - let messageResponse: ReturnType + let messageResponse: ReturnType | undefined let neverAck: boolean const startWsServer = async (onConnection?: OnWSConnection): Promise => { @@ -97,7 +98,7 @@ describe('CDP Clients', () => { const clientDisconnected = () => { return new Promise((resolve, reject) => { - criClient.ws.once('close', resolve) + criClient.ws?.once('close', resolve) }) } @@ -109,7 +110,9 @@ describe('CDP Clients', () => { nock.enableNetConnect() - wsSrv = await startWsServer() + wsSrv = await startWsServer((client) => { + wsClient = client + }) }) afterEach(async () => { @@ -118,6 +121,47 @@ describe('CDP Clients', () => { await closeWsServer() }) + it('properly handles various ways to add event listeners', async () => { + const sessionId = 'abc123' + const method = `Network.responseReceived` + const sessionDeferred = pDefer() + const sessionCb = sinon.stub().callsFake(sessionDeferred.resolve) + const eventDeferred = pDefer() + const eventCb = sinon.stub().callsFake(eventDeferred.resolve) + const globalDeferred = pDefer() + const globalCb = sinon.stub().callsFake(globalDeferred.resolve) + const params = { foo: 'bar' } + + criClient = await CriClient.create({ + target: `ws://127.0.0.1:${wsServerPort}`, + onAsynchronousError: (err) => { + sessionDeferred.reject(err) + eventDeferred.reject(err) + globalDeferred.reject(err) + }, + }) + + criClient.on(`${method}.${sessionId}` as CdpEvent, sessionCb) + criClient.on(method, eventCb) + criClient.on('event' as CdpEvent, globalCb) + + wsClient.send(JSON.stringify({ + method, + params, + sessionId, + })) + + await Promise.all([ + sessionDeferred.promise, + eventDeferred.promise, + globalDeferred.promise, + ]) + + expect(sessionCb).to.have.been.calledWith(params) + expect(eventCb).to.have.been.calledWith(params, sessionId) + expect(globalCb).to.have.been.calledWith({ method, params, sessionId }) + }) + context('reconnect after disconnect', () => { it('retries to connect', async () => { const stub = sinon.stub() @@ -198,7 +242,8 @@ describe('CDP Clients', () => { }) await haltedReconnection - + // Macrotask queue needs to flush for the event to trigger + await (new Promise((resolve) => setImmediate(resolve))) expect(onCriConnectionClosed).to.have.been.called }) diff --git a/packages/server/test/unit/browsers/cdp-connection_spec.ts b/packages/server/test/unit/browsers/cdp-connection_spec.ts new file mode 100644 index 000000000000..45c67c48a2e6 --- /dev/null +++ b/packages/server/test/unit/browsers/cdp-connection_spec.ts @@ -0,0 +1,351 @@ +import type CDP from 'chrome-remote-interface' +import type { CdpClient, CDPConnection, CDPConnectionOptions } from '../../../lib/browsers/cdp-connection' +import type { debugCdpConnection } from '../../../lib/browsers/debug-cdp-connection' +import type { CdpEvent, CdpCommand } from '../../../lib/browsers/cdp_automation' +import type ProtocolMapping from 'devtools-protocol/types/protocol-mapping' +import { CDPTerminatedError, CDPAlreadyConnectedError, CDPDisconnectedError } from '../../../lib/browsers/cri-errors' +import WebSocket from 'ws' +import pDefer, { DeferredPromise } from 'p-defer' +const { expect, proxyquire, sinon } = require('../../spec_helper') + +const DEBUGGER_URL = 'http://foo' + +type CDPConnectionCtor = new (_options: CDP.Options, connectionOptions: CDPConnectionOptions) => CDPConnection + +describe('CDPConnection', () => { + let stubbedCDPClient: sinon.SinonStubbedInstance & { _ws?: sinon.SinonStubbedInstance }> + let stubbedWebSocket: sinon.SinonStubbedInstance + let stubbedDebugger: sinon.SinonStub, ReturnType> + + let CDPConnection: CDPConnectionCtor + let CDPImport: sinon.SinonStub + + let cdpConnection: CDPConnection + + let onReconnectCb: sinon.SinonStub + let onReconnectAttemptCb: sinon.SinonStub + let onReconnectErrCb: sinon.SinonStub + let onConnectionClosedCb: sinon.SinonStub + + const createStubbedCdpClient = () => { + return { + send: sinon.stub(), + on: sinon.stub(), + off: sinon.stub(), + close: sinon.stub().resolves(), + _ws: stubbedWebSocket, + } + } + + beforeEach(() => { + stubbedWebSocket = sinon.createStubInstance(WebSocket) + + stubbedCDPClient = createStubbedCdpClient() + + stubbedDebugger = sinon.stub().callsFake(() => {}) + + CDPImport = sinon.stub() + + CDPConnection = proxyquire('../lib/browsers/cdp-connection', { + 'chrome-remote-interface': CDPImport, + './debug-cdp-connection': stubbedDebugger, + }).CDPConnection + + cdpConnection = new CDPConnection({ + target: DEBUGGER_URL, + local: true, + }, { automaticallyReconnect: false }) + + onReconnectCb = sinon.stub() + onReconnectAttemptCb = sinon.stub() + onReconnectErrCb = sinon.stub() + onConnectionClosedCb = sinon.stub() + + cdpConnection.addConnectionEventListener('cdp-connection-reconnect', onReconnectCb) + cdpConnection.addConnectionEventListener('cdp-connection-reconnect-attempt', onReconnectAttemptCb) + cdpConnection.addConnectionEventListener('cdp-connection-reconnect-error', onReconnectErrCb) + cdpConnection.addConnectionEventListener('cdp-connection-closed', onConnectionClosedCb) + }) + + describe('.connect()', () => { + describe('when CDP connects', () => { + beforeEach(() => { + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).resolves(stubbedCDPClient) + }) + + it('resolves', async () => { + await expect(cdpConnection.connect()).to.be.fulfilled + }) + + describe('when there is already an active connection', () => { + beforeEach(async () => { + await cdpConnection.connect() + }) + + it('rejects with a CDPAlreadyConnectedError', async () => { + await expect(cdpConnection.connect()).to.be.rejectedWith(CDPAlreadyConnectedError, DEBUGGER_URL) + }) + }) + }) + + describe('when CDP fails to connect', () => { + let someErr: Error + + beforeEach(() => { + someErr = new Error('some error') + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).rejects(someErr) + }) + + it('rejects', async () => { + await expect(cdpConnection.connect()).to.be.rejectedWith(someErr) + }) + }) + + describe('when the connection has been terminated', () => { + beforeEach(async () => { + await cdpConnection.disconnect() + }) + + it('rejects with a CdpTerminatedError', async () => { + await expect(cdpConnection.connect()).to.be.rejectedWith(CDPTerminatedError, DEBUGGER_URL) + }) + }) + + describe('when CDP disconnects', () => { + beforeEach(async () => { + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).resolves(stubbedCDPClient) + await cdpConnection.connect() + }) + + it('does not add a reconnect listener', async () => { + //@ts-expect-error + expect(stubbedCDPClient.on?.withArgs('disconnect')).not.to.have.been.called + }) + }) + }) + + describe('.disconnect()', () => { + describe('when the connection has not been terminated and there is an active connection', () => { + beforeEach(async () => { + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).resolves(stubbedCDPClient) + + await cdpConnection.connect() + }) + + it('removes any event listeners that have been added', async () => { + await cdpConnection.disconnect() + + const calls = stubbedCDPClient.on?.getCalls() + + if (calls?.length) { + for (const call of calls) { + const [event, listener] = call.args + + expect(stubbedCDPClient.off).to.have.been.calledWith(event, listener) + } + } + }) + + it('closes the CDP connection', async () => { + await cdpConnection.disconnect() + expect(stubbedCDPClient.close).to.have.been.called + }) + + it('marks this connection as terminated', async () => { + await cdpConnection.disconnect() + expect(cdpConnection.terminated).to.be.true + }) + + it('emits the cdp-connection-closed connection event', async () => { + await cdpConnection.disconnect() + await new Promise((resolve) => setImmediate(resolve)) + expect(onConnectionClosedCb).to.have.been.called + }) + + describe('when the connection has already been terminated', () => { + beforeEach(async () => { + await cdpConnection.disconnect() + stubbedCDPClient.close?.resetHistory() + onConnectionClosedCb.reset() + }) + + it('does not emit a lifecycle event, remove listeners, etc', async () => { + await expect(cdpConnection.disconnect()).to.be.fulfilled + expect(onConnectionClosedCb).not.to.have.been.called + expect(cdpConnection.terminated).to.be.true + expect(stubbedCDPClient.close).not.to.have.been.called + }) + }) + }) + + describe('when there is no active connection', () => { + it('does not throw, emit a lifecycle event, remove listeners, or call close on the cdp client', async () => { + await expect(cdpConnection.disconnect()).to.be.fulfilled + expect(onConnectionClosedCb).not.to.have.been.called + expect(cdpConnection.terminated).to.be.true + expect(stubbedCDPClient.close).not.to.have.been.called + }) + }) + }) + + describe('.send()', () => { + const method: CdpCommand = 'Runtime.runScript' + const params = { scriptId: 'efg' } + const sessionId = 'abc' + + describe('when the connection has been established', () => { + beforeEach(async () => { + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).resolves(stubbedCDPClient) + await cdpConnection.connect() + }) + + describe('when the CDP command resolves', () => { + const resolve: ProtocolMapping.Commands['Runtime.runScript']['returnType'] = { + result: { + type: 'undefined', + }, + } + + beforeEach(() => { + stubbedCDPClient.send?.withArgs(method, params, sessionId).resolves(resolve) + }) + + it('resolves with the same value', async () => { + await expect(cdpConnection.send(method, params, sessionId)).to.eventually.eq(resolve) + }) + }) + + describe('when the CDP command rejects with a general error', () => { + const err = new Error('some err') + + beforeEach(() => { + stubbedCDPClient.send?.rejects(err) + }) + + it('rejects with the same error', async () => { + await expect(cdpConnection.send(method, params, sessionId)).to.be.rejectedWith(err) + }) + }) + + describe('when the CDP command rejects with a websocket disconnection error message', () => { + ['WebSocket connection closed', 'WebSocket is not open', 'WebSocket is already in CLOSING or CLOSED state'].forEach((msg) => { + it(` it rejects "${msg}" with a CDPDisconnectedError`, async () => { + const err = new Error(msg) + + stubbedCDPClient.send?.rejects(err) + await expect(cdpConnection.send(method, params, sessionId)).to.be.rejectedWith(CDPDisconnectedError) + }) + }) + }) + }) + + describe('when the connection has yet to be established', () => { + it('rejects with a CDPDisconnectedError', async () => { + await expect(cdpConnection.send(method, params, sessionId)).to.be.rejectedWith(CDPDisconnectedError, 'has not been established') + }) + }) + + describe('when the connection has been terminated', () => { + it('rejects with a CDPDisconnectedError', async () => { + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).resolves(stubbedCDPClient) + await cdpConnection.connect() + await cdpConnection.disconnect() + await expect(cdpConnection.send(method, params, sessionId)).to.be.rejectedWith(CDPDisconnectedError, 'terminated') + }) + }) + }) + + describe('.on()', () => { + const event: CdpEvent = 'Browser.downloadProgress' + const params = { some: 'params' } + + it('calls the callback when cdp client broadcasts the event', async () => { + const cb = sinon.stub() + + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).resolves(stubbedCDPClient) + + await cdpConnection.connect() + cdpConnection.on(event, cb) + + //@ts-expect-error + stubbedCDPClient.on?.withArgs('event').args[0][1]({ + method: event, + params, + }) + + await (new Promise((resolve) => setImmediate(resolve))) + expect(cb).to.have.been.calledWith(params) + }) + }) + + describe('.off()', () => { + const event: CdpEvent = 'Browser.downloadProgress' + const params = { some: 'params' } + + it('no longer calls the callback when cdp client broadcasts the event', async () => { + const cb = sinon.stub() + + cdpConnection.on(event, cb) + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).resolves(stubbedCDPClient) + + await cdpConnection.connect() + cdpConnection.off(event, cb) + + //@ts-expect-error + stubbedCDPClient.on?.withArgs('event').args[0][1]({ + method: event, + params, + }) + + await (new Promise((resolve) => setImmediate(resolve))) + expect(cb).not.to.have.been.calledWith(params) + }) + }) + + describe('when created with auto reconnect behavior enabled and disconnected', () => { + let deferredReconnection: DeferredPromise + + beforeEach(async () => { + cdpConnection = new CDPConnection({ + target: DEBUGGER_URL, + local: true, + }, { automaticallyReconnect: true }) + + cdpConnection.addConnectionEventListener('cdp-connection-reconnect', onReconnectCb) + cdpConnection.addConnectionEventListener('cdp-connection-reconnect-attempt', onReconnectAttemptCb) + cdpConnection.addConnectionEventListener('cdp-connection-reconnect-error', onReconnectErrCb) + + deferredReconnection = pDefer() + onReconnectCb.callsFake(() => deferredReconnection.resolve()) + onReconnectErrCb.callsFake(() => deferredReconnection.reject()) + CDPImport.withArgs({ target: DEBUGGER_URL, local: true }).resolves(stubbedCDPClient) + + await cdpConnection.connect() + // @ts-expect-error + stubbedCDPClient.on?.withArgs('disconnect').args[0][1]() + CDPImport.reset() + }) + + it('reconnects when disconnected and reconnection succeeds on the third try', async () => { + CDPImport.onFirstCall().rejects() + CDPImport.onSecondCall().rejects() + + const newCDPClientStub = createStubbedCdpClient() + + CDPImport.onThirdCall().resolves(newCDPClientStub) + + await deferredReconnection.promise + + expect(onReconnectAttemptCb).to.have.callCount(3) + expect(onReconnectCb).to.be.called + expect(onReconnectErrCb).not.to.be.called + }) + + it('rejects when disconnected and reconnection fails after 20 attempts', async () => { + CDPImport.rejects() + await expect(deferredReconnection.promise).to.be.rejected + expect(onReconnectErrCb).to.be.called + expect(onReconnectAttemptCb).to.have.callCount(20) + }) + }) +}) diff --git a/packages/server/test/unit/browsers/cri-client_spec.ts b/packages/server/test/unit/browsers/cri-client_spec.ts index f57291a80606..608cfcf2e55b 100644 --- a/packages/server/test/unit/browsers/cri-client_spec.ts +++ b/packages/server/test/unit/browsers/cri-client_spec.ts @@ -1,7 +1,8 @@ +import type ProtocolMapping from 'devtools-protocol/types/protocol-mapping' import EventEmitter from 'events' import { ProtocolManagerShape } from '@packages/types' import type { CriClient } from '../../../lib/browsers/cri-client' - +import pDefer from 'p-defer' const { expect, proxyquire, sinon } = require('../../spec_helper') const DEBUGGER_URL = 'http://foo' @@ -11,24 +12,40 @@ const PORT = 50505 describe('lib/browsers/cri-client', function () { let send: sinon.SinonStub let on: sinon.SinonStub + let off: sinon.SinonStub + let criImport: sinon.SinonStub & { New: sinon.SinonStub } let criStub: { send: typeof send on: typeof on + off: typeof off close: sinon.SinonStub _notifier: EventEmitter } let onError: sinon.SinonStub - let getClient: (options?: { host?: string, fullyManageTabs?: boolean, protocolManager?: ProtocolManagerShape }) => ReturnType + let onReconnect: sinon.SinonStub + + let getClient: (options?: { host?: string, fullyManageTabs?: boolean, protocolManager?: ProtocolManagerShape }) => ReturnType + + const fireCDPEvent = (method: T, params: Partial, sessionId?: string) => { + criStub.on.withArgs('event').args[0][1]({ + method, + params, + sessionId, + }) + } beforeEach(function () { send = sinon.stub() onError = sinon.stub() + onReconnect = sinon.stub() on = sinon.stub() + off = sinon.stub() criStub = { on, + off, send, close: sinon.stub().resolves(), _notifier: new EventEmitter(), @@ -43,12 +60,16 @@ describe('lib/browsers/cri-client', function () { criImport.New = sinon.stub().withArgs({ host: HOST, port: PORT, url: 'about:blank' }).resolves({ webSocketDebuggerUrl: 'http://web/socket/url' }) - const { CriClient } = proxyquire('../lib/browsers/cri-client', { + const CDPConnectionRef = proxyquire('../lib/browsers/cdp-connection', { 'chrome-remote-interface': criImport, + }).CDPConnection + + const { CriClient } = proxyquire('../lib/browsers/cri-client', { + './cdp-connection': { CDPConnection: CDPConnectionRef }, }) getClient = ({ host, fullyManageTabs, protocolManager } = {}): Promise => { - return CriClient.create({ target: DEBUGGER_URL, host, onAsynchronousError: onError, fullyManageTabs, protocolManager }) + return CriClient.create({ target: DEBUGGER_URL, host, onAsynchronousError: onError, fullyManageTabs, protocolManager, onReconnect }) } }) @@ -59,6 +80,76 @@ describe('lib/browsers/cri-client', function () { expect(client.send).to.be.instanceOf(Function) }) + describe('when it has a host', () => { + it('adds a crash listener', async () => { + const client = await getClient({ host: HOST }) + + fireCDPEvent('Target.targetCrashed', { targetId: DEBUGGER_URL }) + expect(client.crashed).to.be.true + }) + }) + + describe('when it does not have a host', () => { + it('does not add a crash listener', async () => { + const client = await getClient() + + fireCDPEvent('Target.targetCrashed', { targetId: DEBUGGER_URL }) + expect(client.crashed).to.be.false + }) + }) + + describe('when it has a host and is fully managed and receives an attachedToTarget event', () => { + beforeEach(async () => { + await getClient({ host: HOST, fullyManageTabs: true }) + criStub.send.resolves() + }) + + describe('target type is service worker, page, or other', async () => { + it('does not enable network', async () => { + await Promise.all(['service_worker', 'page', 'other'].map((type) => { + return fireCDPEvent('Target.attachedToTarget', { + // do not need entire event payload for this test + // @ts-ignore + targetInfo: { + type, + }, + }) + })) + + expect(criStub.send).not.to.have.been.calledWith('Network.enable') + }) + }) + + describe('target type is something other than service worker, page, or other', () => { + it('enables network', async () => { + await fireCDPEvent('Target.attachedToTarget', { + // do not need entire event payload for this test + // @ts-ignore + targetInfo: { + type: 'somethin else', + }, + }) + + expect(criStub.send).to.have.been.calledWith('Network.enable') + }) + }) + + describe('target is waiting for debugger', () => { + it('sends Runtime.runIfWaitingForDebugger', async () => { + const sessionId = 'abc123' + + await fireCDPEvent('Target.attachedToTarget', { + waitingForDebugger: true, + sessionId, + // @ts-ignore + targetInfo: { type: 'service_worker' }, + }) + + expect(criStub.send).to.have.been.calledWith('Runtime.runIfWaitingForDebugger', undefined, sessionId) + }) + }) + }) + context('#send', function () { it('calls cri.send with command and data', async function () { send.resolves() @@ -82,7 +173,8 @@ describe('lib/browsers/cri-client', function () { const command = 'DOM.getDocument' const client = await getClient({ host: '127.0.0.1', fullyManageTabs: true }) - await criStub.on.withArgs('Target.targetCrashed').args[0][1]({ targetId: DEBUGGER_URL }) + fireCDPEvent('Target.targetCrashed', { targetId: DEBUGGER_URL }) + await expect(client.send(command, { depth: -1 })).to.be.rejectedWith(`${command} will not run as the target browser or tab CRI connection has crashed`) }) @@ -91,7 +183,7 @@ describe('lib/browsers/cri-client', function () { await getClient({ host: '127.0.0.1', fullyManageTabs: true }) // This would throw if the error was not caught - await criStub.on.withArgs('Target.attachedToTarget').args[0][1]({ targetInfo: { type: 'worker' } }) + await fireCDPEvent('Target.attachedToTarget', { targetInfo: { type: 'worker', targetId: DEBUGGER_URL, title: '', url: 'https://some_url', attached: true, canAccessOpener: true } }) }) context('retries', () => { @@ -109,8 +201,10 @@ describe('lib/browsers/cri-client', function () { const client = await getClient() - await client.send('DOM.getDocument', { depth: -1 }) + const p = client.send('DOM.getDocument', { depth: -1 }) + await criStub.on.withArgs('disconnect').args[0][1]() + await p expect(send).to.be.calledTwice }) @@ -123,7 +217,28 @@ describe('lib/browsers/cri-client', function () { const client = await getClient() - await client.send('DOM.getDocument', { depth: -1 }) + const getDocumentPromise = client.send('DOM.getDocument', { depth: -1 }) + + await criStub.on.withArgs('disconnect').args[0][1]() + await criStub.on.withArgs('disconnect').args[0][1]() + await getDocumentPromise + expect(send).to.have.callCount(3) + }) + + it(`with two '${msg}' message it retries enablements twice`, async () => { + const err = new Error(msg) + + send.onFirstCall().rejects(err) + send.onSecondCall().rejects(err) + send.onThirdCall().resolves() + + const client = await getClient() + + const enableNetworkPromise = client.send('Network.enable') + + await criStub.on.withArgs('disconnect').args[0][1]() + await criStub.on.withArgs('disconnect').args[0][1]() + await enableNetworkPromise expect(send).to.have.callCount(3) }) }) @@ -183,20 +298,29 @@ describe('lib/browsers/cri-client', function () { // @ts-ignore await criStub.on.withArgs('disconnect').args[0][1]() + const reconnection = pDefer() + + onReconnect.callsFake(() => reconnection.resolve()) + await reconnection.promise + expect(criStub.send).to.be.calledTwice expect(criStub.send).to.be.calledWith('Page.enable') expect(criStub.send).to.be.calledWith('Network.enable') expect(protocolManager.cdpReconnect).to.be.called + + await criStub.on.withArgs('disconnect').args[0][1]() }) it('errors if reconnecting fails', async () => { - criStub._notifier.on = sinon.stub() - criStub.close.throws(new Error('could not reconnect')) - await getClient() + + criImport.rejects() + // @ts-ignore await criStub.on.withArgs('disconnect').args[0][1]() + await (new Promise((resolve) => setImmediate(resolve))) + expect(onError).to.be.called const error = onError.lastCall.args[0]