From 001405fed6ef976cb2f5662c1a9d5c43dbaba3f8 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 28 Nov 2024 14:34:23 +0100 Subject: [PATCH] fix: infinite loop when combining controls (#1569) --- cypress/e2e/session-recording.cy.ts | 1 + .../replay/sessionrecording.test.ts | 84 +++++++++++++++---- src/extensions/replay/sessionrecording.ts | 16 ++-- 3 files changed, 80 insertions(+), 21 deletions(-) diff --git a/cypress/e2e/session-recording.cy.ts b/cypress/e2e/session-recording.cy.ts index 56f1134b4..0875c32dd 100644 --- a/cypress/e2e/session-recording.cy.ts +++ b/cypress/e2e/session-recording.cy.ts @@ -523,6 +523,7 @@ describe('Session recording', () => { capturedSnapshot['properties']['$snapshot_data'][3], capturedSnapshot['properties']['$snapshot_data'][4], ]) + expectPageViewCustomEvent(customEvents[0]) expectPostHogConfigCustomEvent(customEvents[1]) expectSessionOptionsCustomEvent(customEvents[2]) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index a3bd307ec..d1ca09e00 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -184,8 +184,7 @@ describe('SessionRecording', () => { let sessionIdGeneratorMock: Mock let windowIdGeneratorMock: Mock let onFeatureFlagsCallback: ((flags: string[], variants: Record) => void) | null - let removeCaptureHookMock: Mock - let addCaptureHookMock: Mock + let removePageviewCaptureHookMock: Mock let simpleEventEmitter: SimpleEventEmitter const addRRwebToWindow = () => { @@ -208,6 +207,7 @@ describe('SessionRecording', () => { } beforeEach(() => { + removePageviewCaptureHookMock = jest.fn() sessionId = 'sessionId' + uuidv7() config = { @@ -241,10 +241,6 @@ describe('SessionRecording', () => { windowIdGeneratorMock ) - // add capture hook returns an unsubscribe function - removeCaptureHookMock = jest.fn() - addCaptureHookMock = jest.fn().mockImplementation(() => removeCaptureHookMock) - simpleEventEmitter = new SimpleEventEmitter() // TODO we really need to make this a real posthog instance :cry: posthog = { @@ -262,7 +258,6 @@ describe('SessionRecording', () => { }, sessionManager: sessionManager, requestRouter: new RequestRouter({ config } as any), - _addCaptureHook: addCaptureHookMock, consent: { isOptedOut(): boolean { return false @@ -270,9 +265,10 @@ describe('SessionRecording', () => { } as unknown as ConsentManager, register_for_session() {}, _internalEventEmitter: simpleEventEmitter, - on: (event, cb) => { - return simpleEventEmitter.on(event, cb) - }, + on: jest.fn().mockImplementation((event, cb) => { + const unsubscribe = simpleEventEmitter.on(event, cb) + return removePageviewCaptureHookMock.mockImplementation(unsubscribe) + }), } as Partial as PostHog loadScriptMock.mockImplementation((_ph, _path, callback) => { @@ -423,21 +419,21 @@ describe('SessionRecording', () => { sessionRecording.startIfEnabledOrStop() expect(sessionRecording['_removePageViewCaptureHook']).not.toBeUndefined() - expect(posthog._addCaptureHook).toHaveBeenCalledTimes(1) + expect(posthog.on).toHaveBeenCalledTimes(1) // calling a second time doesn't add another capture hook sessionRecording.startIfEnabledOrStop() - expect(posthog._addCaptureHook).toHaveBeenCalledTimes(1) + expect(posthog.on).toHaveBeenCalledTimes(1) }) it('removes the pageview capture hook on stop', () => { sessionRecording.startIfEnabledOrStop() expect(sessionRecording['_removePageViewCaptureHook']).not.toBeUndefined() - expect(removeCaptureHookMock).not.toHaveBeenCalled() + expect(removePageviewCaptureHookMock).not.toHaveBeenCalled() sessionRecording.stopRecording() - expect(removeCaptureHookMock).toHaveBeenCalledTimes(1) + expect(removePageviewCaptureHookMock).toHaveBeenCalledTimes(1) expect(sessionRecording['_removePageViewCaptureHook']).toBeUndefined() }) @@ -1774,6 +1770,66 @@ describe('SessionRecording', () => { expect(sessionRecording['_linkedFlagSeen']).toEqual(true) expect(sessionRecording['status']).toEqual('active') }) + + /** + * this is partly a regression test, with a running rrweb, + * if you don't pause while buffering + * the browser can be trapped in an infinite loop of pausing + * while trying to report it is paused 🙈 + */ + it('can be paused while waiting for flag', () => { + fakeNavigateTo('https://test.com/blocked') + + expect(sessionRecording['_linkedFlag']).toEqual(null) + expect(sessionRecording['_linkedFlagSeen']).toEqual(false) + expect(sessionRecording['status']).toEqual('buffering') + + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { + endpoint: '/s/', + linkedFlag: 'the-flag-key', + urlBlocklist: [ + { + matching: 'regex', + url: '/blocked', + }, + ], + }, + }) + ) + + expect(sessionRecording['_linkedFlag']).toEqual('the-flag-key') + expect(sessionRecording['_linkedFlagSeen']).toEqual(false) + expect(sessionRecording['status']).toEqual('buffering') + expect(sessionRecording['paused']).toBeUndefined() + + const snapshotEvent = { + event: 123, + type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, + data: { + source: 1, + }, + timestamp: new Date().getTime(), + } + _emit(snapshotEvent) + + expect(sessionRecording['_linkedFlag']).toEqual('the-flag-key') + expect(sessionRecording['_linkedFlagSeen']).toEqual(false) + expect(sessionRecording['status']).toEqual('paused') + + sessionRecording.overrideLinkedFlag() + + expect(sessionRecording['_linkedFlagSeen']).toEqual(true) + expect(sessionRecording['status']).toEqual('paused') + + fakeNavigateTo('https://test.com/allowed') + + expect(sessionRecording['status']).toEqual('paused') + + _emit(snapshotEvent) + expect(sessionRecording['status']).toEqual('active') + }) }) describe('buffering minimum duration', () => { diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index bfa2693a9..ce5f9d067 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -392,6 +392,10 @@ export class SessionRecording { return 'disabled' } + if (this._urlBlocked) { + return 'paused' + } + if (!isNullish(this._linkedFlag) && !this._linkedFlagSeen) { return 'buffering' } @@ -400,10 +404,6 @@ export class SessionRecording { return 'buffering' } - if (this._urlBlocked) { - return 'paused' - } - if (isBoolean(this.isSampled)) { return this.isSampled ? 'sampled' : 'disabled' } else { @@ -501,12 +501,14 @@ export class SessionRecording { if (isNullish(this._removePageViewCaptureHook)) { // :TRICKY: rrweb does not capture navigation within SPA-s, so hook into our $pageview events to get access to all events. // Dropping the initial event is fine (it's always captured by rrweb). - this._removePageViewCaptureHook = this.instance._addCaptureHook((eventName) => { + this._removePageViewCaptureHook = this.instance.on('eventCaptured', (event) => { // If anything could go wrong here it has the potential to block the main loop, // so we catch all errors. try { - if (eventName === '$pageview') { - const href = window ? this._maskUrl(window.location.href) : '' + if (event.event === '$pageview') { + const href = event?.properties.$current_url + ? this._maskUrl(event?.properties.$current_url) + : '' if (!href) { return }