fix: issue with service worker attachments (#28147)

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

cli/CHANGELOG.md

@@ -1,4 +1,12 @@
 <!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
+## 13.3.4
+
+_Released 11/7/2023 (PENDING)_
+
+**Bugfixes:**
+
+- Fixed a regression in [`13.3.2`](https://docs.cypress.io/guides/references/changelog/13.3.2) where loading a service worker and immediately reloading the page can cause a crash. Fixes [#28141](https://github.com/cypress-io/cypress/issues/28141).
+
 ## 13.3.3
 
 _Released 10/24/2023_

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

@@ -198,12 +198,17 @@ export class BrowserCriClient {
         // 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)
-          }
+          try {
+            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)
+            if (event.waitingForDebugger) {
+              await browserClient.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 browser', error)
           }
         })
 

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

@@ -285,13 +285,18 @@ export const create = async ({
 
       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)
-          }
+          try {
+            // 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)
+            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)
           }
         })
 

packages/server/test/unit/browsers/browser-cri-client_spec.ts

@@ -187,6 +187,31 @@ describe('lib/browsers/cri-client', function () {
         expect(protocolManager.connectToBrowser).to.be.calledWith(client)
       })
 
+      it('creates a page client when the passed in url is found and notifies the protocol manager and fully managed tabs and attaching to target throws', async function () {
+        const mockPageClient = {}
+        const protocolManager: any = {
+          connectToBrowser: sinon.stub().resolves(),
+        }
+
+        send.withArgs('Target.getTargets').resolves({ targetInfos: [{ targetId: '1', url: 'http://foo.com' }, { targetId: '2', url: 'http://bar.com' }] })
+        send.withArgs('Target.setDiscoverTargets', { discover: true })
+        on.withArgs('Target.targetDestroyed', sinon.match.func)
+
+        send.withArgs('Network.enable').throws(new Error('ProtocolError: Inspected target navigated or closed'))
+
+        criClientCreateStub.withArgs({ target: '1', onAsynchronousError: onError, host: HOST, port: PORT, protocolManager, fullyManageTabs: true, browserClient: { on, send, close } }).resolves(mockPageClient)
+
+        const browserClient = await getClient({ protocolManager, fullyManageTabs: true })
+
+        const client = await browserClient.attachToTargetUrl('http://foo.com')
+
+        expect(client).to.be.equal(mockPageClient)
+        expect(protocolManager.connectToBrowser).to.be.calledWith(client)
+
+        // This would throw if the error was not caught
+        await on.withArgs('Target.attachedToTarget').args[0][1]({ targetInfo: { type: 'worker' } })
+      })
+
       it('retries when the passed in url is not found', async function () {
         sinon.stub(protocol, '_getDelayMsForRetry')
         .onFirstCall().returns(100)

packages/server/test/unit/browsers/cri-client_spec.ts

@@ -88,6 +88,14 @@ describe('lib/browsers/cri-client', function () {
         await expect(client.send(command, { depth: -1 })).to.be.rejectedWith(`${command} will not run as the target browser or tab CRI connection has crashed`)
       })
 
+      it('does not reject if attachToTarget work throws', async function () {
+        criStub.send.withArgs('Network.enable').throws(new Error('ProtocolError: Inspected target navigated or closed'))
+        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' } })
+      })
+
       context('retries', () => {
         ([
           'WebSocket is not open',