fix: issue with proxy correlations and web/shared workers (#28105)

Author: ryanthemanuel

cli/CHANGELOG.md

@@ -5,6 +5,7 @@ _Released 10/24/2023 (PENDING)_
 
 **Bugfixes:**
 
+- Fixed a performance regression in `13.3.1` with proxy correlation timeouts and requests issued from web and shared workers. Fixes [#28104](https://github.com/cypress-io/cypress/issues/28104).
 - Fixed a performance problem with proxy correlation when requests get aborted and then get miscorrelated with follow up requests. Fixed in [#28094](https://github.com/cypress-io/cypress/pull/28094).
 - Fixed a regression in [10.0.0](#10.0.0), where search would not find a spec if the file name contains "-" or "\_", but search prompt contains " " instead (e.g. search file "spec-file.cy.ts" with prompt "spec file"). Fixes [#25303](https://github.com/cypress-io/cypress/issues/25303).
 

packages/server/lib/browsers/browser-cri-client.ts

@@ -3,7 +3,7 @@ import CRI from 'chrome-remote-interface'
 import Debug from 'debug'
 import { _connectAsync, _getDelayMsForRetry } from './protocol'
 import * as errors from '../errors'
-import { create, CriClient } from './cri-client'
+import { create, CriClient, DEFAULT_NETWORK_ENABLE_OPTIONS } from './cri-client'
 import type { ProtocolManagerShape } from '@packages/types'
 
 const debug = Debug('cypress:server:browsers:browser-cri-client')
@@ -13,6 +13,27 @@ interface Version {
   minor: number
 }
 
+type BrowserCriClientOptions = {
+  browserClient: CriClient
+  versionInfo: CRI.VersionResult
+  host: string
+  port: number
+  browserName: string
+  onAsynchronousError: Function
+  protocolManager?: ProtocolManagerShape
+  fullyManageTabs?: boolean
+}
+
+type BrowserCriClientCreateOptions = {
+  hosts: string[]
+  port: number
+  browserName: string
+  onAsynchronousError: Function
+  onReconnect?: (client: CriClient) => void
+  protocolManager?: ProtocolManagerShape
+  fullyManageTabs?: boolean
+}
+
 const isVersionGte = (a: Version, b: Version) => {
   return a.major > b.major || (a.major === b.major && a.minor >= b.minor)
 }
@@ -114,6 +135,14 @@ const retryWithIncreasingDelay = async <T>(retryable: () => Promise<T>, browserN
 }
 
 export class BrowserCriClient {
+  private browserClient: CriClient
+  private versionInfo: CRI.VersionResult
+  private host: string
+  private port: number
+  private browserName: string
+  private onAsynchronousError: Function
+  private protocolManager?: ProtocolManagerShape
+  private fullyManageTabs?: boolean
   currentlyAttachedTarget: CriClient | undefined
   // whenever we instantiate the instance we're already connected bc
   // we receive an underlying CRI connection
@@ -125,7 +154,16 @@ export class BrowserCriClient {
   gracefulShutdown?: Boolean
   onClose: Function | null = null
 
-  private constructor (private browserClient: CriClient, private versionInfo, public host: string, public port: number, private browserName: string, private onAsynchronousError: Function, private protocolManager?: ProtocolManagerShape) { }
+  private constructor ({ browserClient, versionInfo, host, port, browserName, onAsynchronousError, protocolManager, fullyManageTabs }: BrowserCriClientOptions) {
+    this.browserClient = browserClient
+    this.versionInfo = versionInfo
+    this.host = host
+    this.port = port
+    this.browserName = browserName
+    this.onAsynchronousError = onAsynchronousError
+    this.protocolManager = protocolManager
+    this.fullyManageTabs = fullyManageTabs
+  }
 
   /**
    * Factory method for the browser cri client. Connects to the browser and then returns a chrome remote interface wrapper around the
@@ -140,7 +178,7 @@ export class BrowserCriClient {
    * @param fullyManageTabs whether or not to fully manage tabs. This is useful for firefox where some work is done with marionette and some with CDP. We don't want to handle disconnections in this class in those scenarios
    * @returns a wrapper around the chrome remote interface that is connected to the browser target
    */
-  static async create (hosts: string[], port: number, browserName: string, onAsynchronousError: Function, onReconnect?: (client: CriClient) => void, protocolManager?: ProtocolManagerShape, { fullyManageTabs }: { fullyManageTabs?: boolean } = {}): Promise<BrowserCriClient> {
+  static async create ({ hosts, port, browserName, onAsynchronousError, onReconnect, protocolManager, fullyManageTabs }: BrowserCriClientCreateOptions): Promise<BrowserCriClient> {
     const host = await ensureLiveBrowser(hosts, port, browserName)
 
     return retryWithIncreasingDelay(async () => {
@@ -151,11 +189,26 @@ export class BrowserCriClient {
         onAsynchronousError,
         onReconnect,
         protocolManager,
+        fullyManageTabs,
       })
 
-      const browserCriClient = new BrowserCriClient(browserClient, versionInfo, host!, port, browserName, onAsynchronousError, protocolManager)
+      const browserCriClient = new BrowserCriClient({ browserClient, versionInfo, host, port, browserName, onAsynchronousError, protocolManager, fullyManageTabs })
 
       if (fullyManageTabs) {
+        // 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.
+        browserClient.on('Target.attachedToTarget', async (event) => {
+          if (event.targetInfo.type !== 'page') {
+            await browserClient.send('Network.enable', protocolManager?.networkEnableOptions ?? DEFAULT_NETWORK_ENABLE_OPTIONS, event.sessionId)
+          }
+
+          if (event.waitingForDebugger) {
+            await browserClient.send('Runtime.runIfWaitingForDebugger', undefined, event.sessionId)
+          }
+        })
+
+        // Ideally we could use filter rather than checking the type above, but that was added relatively recently
+        await browserClient.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true })
         await browserClient.send('Target.setDiscoverTargets', { discover: true })
         browserClient.on('Target.targetDestroyed', (event) => {
           debug('Target.targetDestroyed %o', {
@@ -270,7 +323,8 @@ export class BrowserCriClient {
         throw new Error(`Could not find url target in browser ${url}. Targets were ${JSON.stringify(targets)}`)
       }
 
-      this.currentlyAttachedTarget = await create({ target: target.targetId, onAsynchronousError: this.onAsynchronousError, host: this.host, port: this.port, protocolManager: this.protocolManager })
+      this.currentlyAttachedTarget = await create({ target: target.targetId, onAsynchronousError: this.onAsynchronousError, host: this.host, port: this.port, protocolManager: this.protocolManager, fullyManageTabs: this.fullyManageTabs, browserClient: this.browserClient })
+
       await this.protocolManager?.connectToBrowser(this.currentlyAttachedTarget)
 
       return this.currentlyAttachedTarget
@@ -323,6 +377,7 @@ export class BrowserCriClient {
         host: this.host,
         port: this.port,
         protocolManager: this.protocolManager,
+        fullyManageTabs: this.fullyManageTabs,
       })
     } else {
       this.currentlyAttachedTarget = undefined

packages/server/lib/browsers/cdp_automation.ts

@@ -13,7 +13,7 @@ import type { ResourceType, BrowserPreRequest, BrowserResponseReceived } from '@
 import type { CDPClient, ProtocolManagerShape, WriteVideoFrame } from '@packages/types'
 import type { Automation } from '../automation'
 import { cookieMatches, CyCookie, CyCookieFilter } from '../automation/util'
-import type { CriClient } from './cri-client'
+import { DEFAULT_NETWORK_ENABLE_OPTIONS, CriClient } from './cri-client'
 
 export type CdpCommand = keyof ProtocolMapping.Commands
 
@@ -140,7 +140,7 @@ export const normalizeResourceType = (resourceType: string | undefined): Resourc
   return ffToStandardResourceTypeMap[resourceType] || 'other'
 }
 
-export type SendDebuggerCommand = <T extends CdpCommand>(message: T, data?: ProtocolMapping.Commands[T]['paramsType'][0]) => Promise<ProtocolMapping.Commands[T]['returnType']>
+export type SendDebuggerCommand = <T extends CdpCommand>(message: T, data?: ProtocolMapping.Commands[T]['paramsType'][0], sessionId?: string) => Promise<ProtocolMapping.Commands[T]['returnType']>
 
 export type OnFn = <T extends CdpEvent>(eventName: T, cb: (data: ProtocolMapping.Events[T][0]) => void) => void
 
@@ -198,17 +198,7 @@ export class CdpAutomation implements CDPClient {
   static async create (sendDebuggerCommandFn: SendDebuggerCommand, onFn: OnFn, offFn: OffFn, sendCloseCommandFn: SendCloseCommand, automation: Automation, protocolManager?: ProtocolManagerShape): Promise<CdpAutomation> {
     const cdpAutomation = new CdpAutomation(sendDebuggerCommandFn, onFn, offFn, sendCloseCommandFn, automation)
 
-    const networkEnabledOptions = protocolManager?.protocolEnabled ? {
-      maxTotalBufferSize: 0,
-      maxResourceBufferSize: 0,
-      maxPostDataSize: 64 * 1024,
-    } : {
-      maxTotalBufferSize: 0,
-      maxResourceBufferSize: 0,
-      maxPostDataSize: 0,
-    }
-
-    await sendDebuggerCommandFn('Network.enable', networkEnabledOptions)
+    await sendDebuggerCommandFn('Network.enable', protocolManager?.networkEnableOptions ?? DEFAULT_NETWORK_ENABLE_OPTIONS)
 
     return cdpAutomation
   }

packages/server/lib/browsers/chrome.ts

@@ -473,7 +473,7 @@ export = {
     debug('connecting to existing chrome instance with url and debugging port', { url: options.url, port })
     if (!options.onError) throw new Error('Missing onError in connectToExisting')
 
-    const browserCriClient = await BrowserCriClient.create(['127.0.0.1'], port, browser.displayName, options.onError, onReconnect, undefined, { fullyManageTabs: false })
+    const browserCriClient = await BrowserCriClient.create({ hosts: ['127.0.0.1'], port, browserName: browser.displayName, onAsynchronousError: options.onError, onReconnect, fullyManageTabs: false })
 
     if (!options.url) throw new Error('Missing url in connectToExisting')
 
@@ -488,7 +488,11 @@ export = {
     const browserCriClient = this._getBrowserCriClient()
 
     // Handle chrome tab crashes.
-    pageCriClient.on('Inspector.targetCrashed', async () => {
+    pageCriClient.on('Target.targetCrashed', async (event) => {
+      if (event.targetId !== browserCriClient?.currentlyAttachedTarget?.targetId) {
+        return
+      }
+
       const err = errors.get('RENDERER_CRASHED', browser.displayName)
 
       await memory.endProfiling()
@@ -597,7 +601,7 @@ export = {
     // navigate to the actual url
     if (!options.onError) throw new Error('Missing onError in chrome#open')
 
-    browserCriClient = await BrowserCriClient.create(['127.0.0.1'], port, browser.displayName, options.onError, onReconnect, options.protocolManager, { fullyManageTabs: true })
+    browserCriClient = await BrowserCriClient.create({ hosts: ['127.0.0.1'], port, browserName: browser.displayName, onAsynchronousError: options.onError, onReconnect, protocolManager: options.protocolManager, fullyManageTabs: true })
 
     la(browserCriClient, 'expected Chrome remote interface reference', browserCriClient)
 

packages/server/lib/browsers/cri-client.ts

@@ -28,11 +28,13 @@ type EnqueuedCommand = {
   command: CdpCommand
   params?: object
   p: DeferredPromise
+  sessionId?: string
 }
 
 type EnableCommand = {
   command: CdpCommand
   params?: object
+  sessionId?: string
 }
 
 type Subscription = {
@@ -45,6 +47,12 @@ interface CDPClient extends CDP.Client {
   _ws: WebSocket
 }
 
+export const DEFAULT_NETWORK_ENABLE_OPTIONS = {
+  maxTotalBufferSize: 0,
+  maxResourceBufferSize: 0,
+  maxPostDataSize: 0,
+}
+
 export interface CriClient {
   /**
    * The target id attached to by this client
@@ -138,6 +146,8 @@ type CreateParams = {
   port?: number
   onReconnect?: (client: CriClient) => void
   protocolManager?: ProtocolManagerShape
+  fullyManageTabs?: boolean
+  browserClient?: CriClient
 }
 
 export const create = async ({
@@ -147,6 +157,8 @@ export const create = async ({
   port,
   onReconnect,
   protocolManager,
+  fullyManageTabs,
+  browserClient,
 }: CreateParams): Promise<CriClient> => {
   const subscriptions: Subscription[] = []
   const enableCommands: EnableCommand[] = []
@@ -183,12 +195,12 @@ export const create = async ({
 
     // '*.enable' commands need to be resent on reconnect or any events in
     // that namespace will no longer be received
-    await Promise.all(enableCommands.map(({ command, params }) => {
-      return cri.send(command, params)
+    await Promise.all(enableCommands.map(({ command, params, sessionId }) => {
+      return cri.send(command, params, sessionId)
     }))
 
     enqueuedCommands.forEach((cmd) => {
-      cri.send(cmd.command, cmd.params).then(cmd.p.resolve as any, cmd.p.reject as any)
+      cri.send(cmd.command, cmd.params, cmd.sessionId).then(cmd.p.resolve as any, cmd.p.reject as any)
     })
 
     enqueuedCommands = []
@@ -258,35 +270,35 @@ export const create = async ({
       cri.on('disconnect', retryReconnect)
     }
 
-    cri.on('Inspector.targetCrashed', async () => {
-      debug('crash detected')
-      crashed = true
-    })
-
-    // We only want to try and add service worker traffic if we have a host set. This indicates that this is the child cri client.
+    // We only want to try and add child target traffic if we have a host set. This indicates that this is the child cri client.
+    // 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 (host) {
-      cri.on('Target.targetCreated', async (event) => {
-        if (event.targetInfo.type === 'service_worker') {
-          const networkEnabledOptions = protocolManager?.protocolEnabled ? {
-            maxTotalBufferSize: 0,
-            maxResourceBufferSize: 0,
-            maxPostDataSize: 64 * 1024,
-          } : {
-            maxTotalBufferSize: 0,
-            maxResourceBufferSize: 0,
-            maxPostDataSize: 0,
-          }
-
-          const { sessionId } = await cri.send('Target.attachToTarget', {
-            targetId: event.targetInfo.targetId,
-            flatten: true,
-          })
-
-          await cri.send('Network.enable', networkEnabledOptions, sessionId)
+      cri.on('Target.targetCrashed', async (event) => {
+        if (event.targetId !== target) {
+          return
         }
+
+        debug('crash detected')
+        crashed = true
       })
 
-      await cri.send('Target.setDiscoverTargets', { discover: true })
+      if (fullyManageTabs) {
+        cri.on('Target.attachedToTarget', async (event) => {
+          // Service workers get attached at the page and browser level. We only want to handle them at the browser level
+          if (event.targetInfo.type !== 'service_worker' && event.targetInfo.type !== 'page') {
+            await cri.send('Network.enable', protocolManager?.networkEnableOptions ?? DEFAULT_NETWORK_ENABLE_OPTIONS, event.sessionId)
+          }
+
+          if (event.waitingForDebugger) {
+            await cri.send('Runtime.runIfWaitingForDebugger', undefined, event.sessionId)
+          }
+        })
+
+        // Ideally we could use filter rather than checking the type above, but that was added relatively recently
+        await cri.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true })
+        await cri.send('Target.setDiscoverTargets', { discover: true })
+      }
     }
   }
 
@@ -295,7 +307,7 @@ export const create = async ({
   client = {
     targetId: target,
 
-    async send (command: CdpCommand, params?: object) {
+    async send (command: CdpCommand, params?: object, sessionId?: string) {
       if (crashed) {
         return Promise.reject(new Error(`${command} will not run as the target browser or tab CRI connection has crashed`))
       }
@@ -313,6 +325,10 @@ export const create = async ({
             obj.params = params
           }
 
+          if (sessionId) {
+            obj.sessionId = sessionId
+          }
+
           enqueuedCommands.push(obj)
         })
       }
@@ -328,12 +344,16 @@ export const create = async ({
           obj.params = params
         }
 
+        if (sessionId) {
+          obj.sessionId = sessionId
+        }
+
         enableCommands.push(obj)
       }
 
       if (connected) {
         try {
-          return await cri.send(command, params)
+          return await cri.send(command, params, sessionId)
         } catch (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
@@ -343,7 +363,7 @@ export const create = async ({
             throw err
           }
 
-          debug('encountered closed websocket on send %o', { command, params, err })
+          debug('encountered closed websocket on send %o', { command, params, sessionId, err })
 
           const p = enqueue() as Promise<any>
 
@@ -367,15 +387,25 @@ export const create = async ({
       subscriptions.push({ eventName, cb })
       debug('registering CDP on event %o', { eventName })
 
-      return cri.on(eventName, cb)
+      cri.on(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.')) {
+        browserClient?.on(eventName, cb)
+      }
     },
 
     off (eventName, cb) {
       subscriptions.splice(subscriptions.findIndex((sub) => {
         return sub.eventName === eventName && sub.cb === cb
       }), 1)
 
-      return cri.off(eventName, cb)
+      cri.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.')) {
+        browserClient?.off(eventName, cb)
+      }
     },
 
     get ws () {