Merge 1aa681b in staging-32

Co-authored-by: Benoît Zugmeyer <benoit.zugmeyer@datadoghq.com>

packages/rum/src/boot/recorderApi.spec.ts

@@ -1,11 +1,12 @@
-import { isIE, noop } from '@datadog/browser-core'
+import { display, isIE } from '@datadog/browser-core'
 import type { RecorderApi, ViewContexts, LifeCycle, RumConfiguration } from '@datadog/browser-rum-core'
 import { LifeCycleEventType } from '@datadog/browser-rum-core'
 import { deleteEventBridgeStub, initEventBridgeStub, createNewEvent } from '@datadog/browser-core/test'
 import type { RumSessionManagerMock, TestSetupBuilder } from '../../../rum-core/test'
 import { createRumSessionManagerMock, setup } from '../../../rum-core/test'
-import type { DeflateWorker, startDeflateWorker } from '../domain/deflate'
+import type { CreateDeflateWorker } from '../domain/deflate'
 import { MockWorker } from '../../test'
+import { resetDeflateWorkerState } from '../domain/deflate'
 import type { StartRecording } from './recorderApi'
 import { makeRecorderApi } from './recorderApi'
 
@@ -14,38 +15,26 @@ describe('makeRecorderApi', () => {
   let recorderApi: RecorderApi
   let startRecordingSpy: jasmine.Spy<StartRecording>
   let stopRecordingSpy: jasmine.Spy<() => void>
-  let startDeflateWorkerSpy: jasmine.Spy<typeof startDeflateWorker>
-  const FAKE_WORKER = new MockWorker()
-
-  function startDeflateWorkerWith(worker?: DeflateWorker) {
-    if (!startDeflateWorkerSpy) {
-      startDeflateWorkerSpy = jasmine.createSpy<typeof startDeflateWorker>('startDeflateWorker')
-    }
-    startDeflateWorkerSpy.and.callFake((_, callback) => callback(worker))
-  }
-
-  function callLastRegisteredInitialisationCallback() {
-    startDeflateWorkerSpy.calls.mostRecent().args[1](FAKE_WORKER)
-  }
-
-  function stopDeflateWorker() {
-    startDeflateWorkerSpy.and.callFake(noop)
-  }
+  let mockWorker: MockWorker
+  let createDeflateWorkerSpy: jasmine.Spy<CreateDeflateWorker>
 
   let rumInit: () => void
 
   beforeEach(() => {
     if (isIE()) {
       pending('IE not supported')
     }
+    mockWorker = new MockWorker()
+    createDeflateWorkerSpy = jasmine.createSpy('createDeflateWorkerSpy').and.callFake(() => mockWorker)
+    spyOn(display, 'error')
+
     setupBuilder = setup().beforeBuild(({ lifeCycle, sessionManager }) => {
       stopRecordingSpy = jasmine.createSpy('stopRecording')
       startRecordingSpy = jasmine.createSpy('startRecording').and.callFake(() => ({
         stop: stopRecordingSpy,
       }))
 
-      startDeflateWorkerWith(FAKE_WORKER)
-      recorderApi = makeRecorderApi(startRecordingSpy, startDeflateWorkerSpy)
+      recorderApi = makeRecorderApi(startRecordingSpy, createDeflateWorkerSpy)
       rumInit = () => {
         recorderApi.onRumStart(lifeCycle, {} as RumConfiguration, sessionManager, {} as ViewContexts)
       }
@@ -54,6 +43,7 @@ describe('makeRecorderApi', () => {
 
   afterEach(() => {
     setupBuilder.cleanup()
+    resetDeflateWorkerState()
   })
 
   describe('boot', () => {
@@ -116,25 +106,22 @@ describe('makeRecorderApi', () => {
       expect(startRecordingSpy).not.toHaveBeenCalled()
     })
 
-    it('do not start recording if worker fails to be instantiated', () => {
+    it('do not start recording if worker creation fails', () => {
       setupBuilder.build()
       rumInit()
-      startDeflateWorkerWith(undefined)
+      createDeflateWorkerSpy.and.throwError('Crash')
       recorderApi.start()
       expect(startRecordingSpy).not.toHaveBeenCalled()
     })
 
-    it('does not start recording multiple times if restarted before worker is initialized', () => {
+    it('stops recording if worker initialization fails', () => {
       setupBuilder.build()
       rumInit()
-      stopDeflateWorker()
       recorderApi.start()
-      recorderApi.stop()
 
-      callLastRegisteredInitialisationCallback()
-      recorderApi.start()
-      callLastRegisteredInitialisationCallback()
-      expect(startRecordingSpy).toHaveBeenCalledTimes(1)
+      mockWorker.dispatchErrorEvent()
+
+      expect(stopRecordingSpy).toHaveBeenCalled()
     })
 
     describe('if event bridge present', () => {
@@ -395,24 +382,54 @@ describe('makeRecorderApi', () => {
   })
 
   describe('isRecording', () => {
-    it('is true only if recording', () => {
+    it('is false when recording has not been started', () => {
       setupBuilder.build()
       rumInit()
+
       expect(recorderApi.isRecording()).toBeFalse()
+    })
+
+    it('is false when the worker is not yet initialized', () => {
+      setupBuilder.build()
+      rumInit()
+
       recorderApi.start()
-      expect(recorderApi.isRecording()).toBeTrue()
-      recorderApi.stop()
       expect(recorderApi.isRecording()).toBeFalse()
     })
 
+    it('is false when the worker failed to initialize', () => {
+      setupBuilder.build()
+      rumInit()
+
+      recorderApi.start()
+      mockWorker.dispatchErrorEvent()
+
+      expect(recorderApi.isRecording()).toBeFalse()
+    })
+
+    it('is true when recording is started and the worker is initialized', () => {
+      setupBuilder.build()
+      rumInit()
+
+      recorderApi.start()
+      mockWorker.processAllMessages()
+
+      expect(recorderApi.isRecording()).toBeTrue()
+    })
+
     it('is false before the DOM is loaded', () => {
       setupBuilder.build()
       const { triggerOnDomLoaded } = mockDocumentReadyState()
       rumInit()
-      expect(recorderApi.isRecording()).toBeFalse()
+
       recorderApi.start()
+      mockWorker.processAllMessages()
+
       expect(recorderApi.isRecording()).toBeFalse()
+
       triggerOnDomLoaded()
+      mockWorker.processAllMessages()
+
       expect(recorderApi.isRecording()).toBeTrue()
     })
   })

packages/rum/src/boot/recorderApi.ts

@@ -10,7 +10,14 @@ import type {
 import { LifeCycleEventType } from '@datadog/browser-rum-core'
 import { getReplayStats } from '../domain/replayStats'
 import { getSessionReplayLink } from '../domain/getSessionReplayLink'
-import { DeflateEncoderStreamId, createDeflateEncoder, startDeflateWorker } from '../domain/deflate'
+import type { CreateDeflateWorker } from '../domain/deflate'
+import {
+  DeflateEncoderStreamId,
+  createDeflateEncoder,
+  startDeflateWorker,
+  DeflateWorkerStatus,
+  getDeflateWorkerStatus,
+} from '../domain/deflate'
 
 import { getSerializedNodeId } from '../domain/record'
 import type { startRecording } from './startRecording'
@@ -46,7 +53,7 @@ type RecorderState =
 
 export function makeRecorderApi(
   startRecordingImpl: StartRecording,
-  startDeflateWorkerImpl = startDeflateWorker
+  createDeflateWorkerImpl?: CreateDeflateWorker
 ): RecorderApi {
   const recorderStartObservable = new Observable<RelativeTime>()
 
@@ -76,7 +83,6 @@ export function makeRecorderApi(
   return {
     start: () => startStrategy(),
     stop: () => stopStrategy(),
-    getReplayStats,
     getSessionReplayLink: (configuration, sessionManager, viewContexts) =>
       getSessionReplayLink(configuration, sessionManager, viewContexts, state.status !== RecorderStatus.Stopped),
     recorderStartObservable,
@@ -118,31 +124,33 @@ export function makeRecorderApi(
             return
           }
 
-          startDeflateWorkerImpl(configuration, (worker) => {
-            if (state.status !== RecorderStatus.Starting) {
-              return
-            }
-
-            if (!worker) {
-              state = {
-                status: RecorderStatus.Stopped,
-              }
-              return
-            }
+          const worker = startDeflateWorker(
+            configuration,
+            () => {
+              stopStrategy()
+            },
+            createDeflateWorkerImpl
+          )
 
-            const { stop: stopRecording } = startRecordingImpl(
-              lifeCycle,
-              configuration,
-              sessionManager,
-              viewContexts,
-              createDeflateEncoder(configuration, worker, DeflateEncoderStreamId.REPLAY)
-            )
-            recorderStartObservable.notify(relativeNow())
+          if (!worker) {
             state = {
-              status: RecorderStatus.Started,
-              stopRecording,
+              status: RecorderStatus.Stopped,
             }
-          })
+            return
+          }
+
+          const { stop: stopRecording } = startRecordingImpl(
+            lifeCycle,
+            configuration,
+            sessionManager,
+            viewContexts,
+            createDeflateEncoder(configuration, worker, DeflateEncoderStreamId.REPLAY)
+          )
+          recorderStartObservable.notify(relativeNow())
+          state = {
+            status: RecorderStatus.Started,
+            stopRecording,
+          }
         })
       }
 
@@ -165,6 +173,32 @@ export function makeRecorderApi(
       }
     },
 
-    isRecording: () => state.status === RecorderStatus.Started,
+    isRecording: () =>
+      // The worker is started optimistically, meaning we could have started to record but its
+      // initialization fails a bit later. This could happen when:
+      // * the worker URL (blob or plain URL) is blocked by CSP in Firefox only (Chromium and Safari
+      // throw an exception when instantiating the worker, and IE doesn't care about CSP)
+      // * the browser fails to load the worker in case the workerUrl is used
+      // * an unexpected error occurs in the Worker before initialization, ex:
+      //   * a runtime exception collected by monitor()
+      //   * a syntax error notified by the browser via an error event
+      // * the worker is unresponsive for some reason and timeouts
+      //
+      // It is not expected to happen often. Nonetheless, the "replayable" status on RUM events is
+      // an important part of the Datadog App:
+      // * If we have a false positive (we set has_replay: true even if no replay data is present),
+      // we might display broken links to the Session Replay player.
+      // * If we have a false negative (we don't set has_replay: true even if replay data is
+      // available), it is less noticeable because no link will be displayed.
+      //
+      // Thus, it is better to have false negative, so let's make sure the worker is correctly
+      // initialized before advertizing that we are recording.
+      //
+      // In the future, when the compression worker will also be used for RUM data, this will be
+      // less important since no RUM event will be sent when the worker fails to initialize.
+      getDeflateWorkerStatus() === DeflateWorkerStatus.Initialized && state.status === RecorderStatus.Started,
+
+    getReplayStats: (viewId) =>
+      getDeflateWorkerStatus() === DeflateWorkerStatus.Initialized ? getReplayStats(viewId) : undefined,
   }
 }

packages/rum/src/boot/startRecording.spec.ts

@@ -28,7 +28,7 @@ describe('startRecording', () => {
   let clock: Clock | undefined
   let configuration: RumConfiguration
 
-  beforeEach((done) => {
+  beforeEach(() => {
     if (isIE()) {
       pending('IE not supported')
     }
@@ -42,37 +42,36 @@ describe('startRecording', () => {
     textField = document.createElement('input')
     sandbox.appendChild(textField)
 
-    startDeflateWorker(configuration, (worker) => {
-      setupBuilder = setup()
-        .withViewContexts({
-          findView() {
-            return { id: viewId, startClocks: {} as ClocksState }
-          },
-        })
-        .withSessionManager(sessionManager)
-        .withConfiguration({
-          defaultPrivacyLevel: DefaultPrivacyLevel.ALLOW,
-        })
-        .beforeBuild(({ lifeCycle, configuration, viewContexts, sessionManager }) => {
-          requestSendSpy = jasmine.createSpy()
-          const httpRequest = {
-            send: requestSendSpy,
-            sendOnExit: requestSendSpy,
-          }
-
-          const recording = startRecording(
-            lifeCycle,
-            configuration,
-            sessionManager,
-            viewContexts,
-            createDeflateEncoder(configuration, worker!, DeflateEncoderStreamId.REPLAY),
-            httpRequest
-          )
-          stopRecording = recording ? recording.stop : noop
-          return { stop: stopRecording }
-        })
-      done()
-    })
+    const worker = startDeflateWorker(configuration, noop)!
+
+    setupBuilder = setup()
+      .withViewContexts({
+        findView() {
+          return { id: viewId, startClocks: {} as ClocksState }
+        },
+      })
+      .withSessionManager(sessionManager)
+      .withConfiguration({
+        defaultPrivacyLevel: DefaultPrivacyLevel.ALLOW,
+      })
+      .beforeBuild(({ lifeCycle, configuration, viewContexts, sessionManager }) => {
+        requestSendSpy = jasmine.createSpy()
+        const httpRequest = {
+          send: requestSendSpy,
+          sendOnExit: requestSendSpy,
+        }
+
+        const recording = startRecording(
+          lifeCycle,
+          configuration,
+          sessionManager,
+          viewContexts,
+          createDeflateEncoder(configuration, worker, DeflateEncoderStreamId.REPLAY),
+          httpRequest
+        )
+        stopRecording = recording ? recording.stop : noop
+        return { stop: stopRecording }
+      })
   })
 
   afterEach(() => {