From e96593bb1e692f102a300c1c94fe0e950e8a130b Mon Sep 17 00:00:00 2001 From: Matt Schile Date: Mon, 9 Dec 2024 09:44:05 -0700 Subject: [PATCH] fix: hang when Network.enable is not implemented for a target (#30727) --- cli/CHANGELOG.md | 8 +++ .../server/lib/browsers/browser-cri-client.ts | 5 +- packages/server/lib/browsers/cri-client.ts | 16 +++--- .../test/unit/browsers/cri-client_spec.ts | 51 +++++++++++++++---- 4 files changed, 61 insertions(+), 19 deletions(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index b12efca21790..1d0971682710 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -1,4 +1,12 @@ +## 13.16.2 + +_Released 12/17/2024 (PENDING)_ + +**Bugfixes:** + +- Fixed an issue where targets may hang if `Network.enable` is not implemented for the target. Addresses [#29876](https://github.com/cypress-io/cypress/issues/29876). + ## 13.16.1 _Released 12/03/2024_ diff --git a/packages/server/lib/browsers/browser-cri-client.ts b/packages/server/lib/browsers/browser-cri-client.ts index c4f53cb8785d..9af893a31a9b 100644 --- a/packages/server/lib/browsers/browser-cri-client.ts +++ b/packages/server/lib/browsers/browser-cri-client.ts @@ -342,9 +342,8 @@ export class BrowserCriClient { try { await browserClient.send('Runtime.runIfWaitingForDebugger', undefined, 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 running Runtime.runIfWaitingForDebugger:', error) + // it's possible that the target was closed before we could tell it to run, in that case, just ignore + debug('error running Runtime.runIfWaitingForDebugger: %o', error) } } diff --git a/packages/server/lib/browsers/cri-client.ts b/packages/server/lib/browsers/cri-client.ts index 36e123eb3f12..47cd110e5e65 100644 --- a/packages/server/lib/browsers/cri-client.ts +++ b/packages/server/lib/browsers/cri-client.ts @@ -161,8 +161,7 @@ export class CriClient implements ICriClient { this._isChildTarget = !!this.host if (this._isChildTarget) { - // If crash listeners are added at the browser level, tabs/page connections do not - // emit them. + // 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) { @@ -368,13 +367,18 @@ export class CriClient implements ICriClient { 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) } + } catch (error) { + // it's possible that the target was closed before we could enable network, in that case, just ignore + debug('error attaching to target cri: %o', { error, event }) + } - if (event.waitingForDebugger) { + if (event.waitingForDebugger) { + try { await this.cdpConnection.send('Runtime.runIfWaitingForDebugger', undefined, event.sessionId) + } catch (error) { + // it's possible that the target was closed before we could tell it to run, in that case, just ignore + debug('error running Runtime.runIfWaitingForDebugger: %o', { error, event }) } - } 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) } } diff --git a/packages/server/test/unit/browsers/cri-client_spec.ts b/packages/server/test/unit/browsers/cri-client_spec.ts index 608cfcf2e55b..e6300e3de778 100644 --- a/packages/server/test/unit/browsers/cri-client_spec.ts +++ b/packages/server/test/unit/browsers/cri-client_spec.ts @@ -3,6 +3,7 @@ import EventEmitter from 'events' import { ProtocolManagerShape } from '@packages/types' import type { CriClient } from '../../../lib/browsers/cri-client' import pDefer from 'p-defer' +import type Protocol from 'devtools-protocol' const { expect, proxyquire, sinon } = require('../../spec_helper') const DEBUGGER_URL = 'http://foo' @@ -108,11 +109,9 @@ describe('lib/browsers/cri-client', function () { 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, - }, + } as Protocol.Target.TargetInfo, }) })) @@ -123,11 +122,9 @@ describe('lib/browsers/cri-client', function () { 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', - }, + type: 'iframe', + } as Protocol.Target.TargetInfo, }) expect(criStub.send).to.have.been.calledWith('Network.enable') @@ -135,18 +132,52 @@ describe('lib/browsers/cri-client', function () { }) describe('target is waiting for debugger', () => { + const sessionId = 'abc123' + it('sends Runtime.runIfWaitingForDebugger', async () => { - const sessionId = 'abc123' + await fireCDPEvent('Target.attachedToTarget', { + waitingForDebugger: true, + sessionId, + targetInfo: { type: 'service_worker' } as Protocol.Target.TargetInfo, + }) + + expect(criStub.send).to.have.been.calledWith('Runtime.runIfWaitingForDebugger', undefined, sessionId) + }) + + it('does not send Runtime.runIfWaitingForDebugger if not waiting for debugger', async () => { + await fireCDPEvent('Target.attachedToTarget', { + waitingForDebugger: false, + sessionId, + targetInfo: { type: 'service_worker' } as Protocol.Target.TargetInfo, + }) + + expect(criStub.send).not.to.have.been.calledWith('Runtime.runIfWaitingForDebugger') + }) + + it('sends Runtime.runIfWaitingForDebugger even if Network.enable throws', async () => { + criStub.send.withArgs('Network.enable').throws(new Error('ProtocolError: Inspected target closed')) await fireCDPEvent('Target.attachedToTarget', { waitingForDebugger: true, sessionId, - // @ts-ignore - targetInfo: { type: 'service_worker' }, + targetInfo: { type: 'iframe' } as Protocol.Target.TargetInfo, }) + expect(criStub.send).to.have.been.calledWith('Network.enable').and.to.have.thrown() expect(criStub.send).to.have.been.calledWith('Runtime.runIfWaitingForDebugger', undefined, sessionId) }) + + it('continues even if Runtime.runIfWaitingForDebugger throws', async () => { + criStub.send.withArgs('Runtime.runIfWaitingForDebugger').throws(new Error('ProtocolError: Inspected target closed')) + + await fireCDPEvent('Target.attachedToTarget', { + waitingForDebugger: true, + sessionId, + targetInfo: { type: 'service_worker' } as Protocol.Target.TargetInfo, + }) + + expect(criStub.send).to.have.been.calledWith('Runtime.runIfWaitingForDebugger', undefined, sessionId).and.to.have.thrown() + }) }) })