fix(replay): Start replay in afterAllSetup instead of next tick (#12709)

This should hopefully fix some race conditions.

Instead of initializing replay in the next tick, do it in the current
tick but in `afterAllSetup`, to ensure this runs e.g. after the
replay-canvas integration.

Fixes #12707

Author: mydea

.github/workflows/build.yml

@@ -761,6 +761,7 @@ jobs:
           - loader_debug
           - loader_tracing
           - loader_replay
+          - loader_replay_buffer
           - loader_tracing_replay
 
     steps:

dev-packages/browser-integration-tests/loader-suites/loader/noOnLoad/replay/test.ts

@@ -3,8 +3,11 @@ import { expect } from '@playwright/test';
 import { sentryTest } from '../../../../utils/fixtures';
 import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
 
+const bundle = process.env.PW_BUNDLE || '';
+
 sentryTest('should capture a replay', async ({ getLocalTestUrl, page }) => {
-  if (shouldSkipReplayTest()) {
+  // When in buffer mode, there will not be a replay by default
+  if (shouldSkipReplayTest() || bundle === 'loader_replay_buffer') {
     sentryTest.skip();
   }
 

dev-packages/browser-integration-tests/loader-suites/loader/noOnLoad/replayError/subject.js

@@ -0,0 +1 @@
+window.doSomethingWrong();

dev-packages/browser-integration-tests/loader-suites/loader/noOnLoad/replayError/test.ts

@@ -0,0 +1,35 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../../utils/fixtures';
+import { envelopeRequestParser, waitForErrorRequestOnUrl } from '../../../../utils/helpers';
+import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
+
+sentryTest('should capture a replay & attach an error', async ({ getLocalTestUrl, page }) => {
+  if (shouldSkipReplayTest()) {
+    sentryTest.skip();
+  }
+
+  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+    return route.fulfill({
+      status: 200,
+      contentType: 'application/json',
+      body: JSON.stringify({ id: 'test-id' }),
+    });
+  });
+
+  const req = waitForReplayRequest(page);
+
+  const url = await getLocalTestUrl({ testDir: __dirname });
+  const reqError = await waitForErrorRequestOnUrl(page, url);
+
+  const errorEventData = envelopeRequestParser(reqError);
+  expect(errorEventData.exception?.values?.length).toBe(1);
+  expect(errorEventData.exception?.values?.[0]?.value).toContain('window.doSomethingWrong is not a function');
+
+  const eventData = getReplayEvent(await req);
+
+  expect(eventData).toBeDefined();
+  expect(eventData.segment_id).toBe(0);
+
+  expect(errorEventData.tags?.replayId).toEqual(eventData.replay_id);
+});

dev-packages/browser-integration-tests/loader-suites/loader/onLoad/customReplay/init.js

@@ -6,5 +6,7 @@ Sentry.onLoad(function () {
         useCompression: false,
       }),
     ],
+
+    replaysSessionSampleRate: 1,
   });
 });

dev-packages/browser-integration-tests/loader-suites/loader/onLoad/replay/init.js

@@ -1,3 +1,5 @@
 Sentry.onLoad(function () {
-  Sentry.init({});
+  Sentry.init({
+    replaysSessionSampleRate: 1,
+  });
 });

dev-packages/browser-integration-tests/package.json

@@ -32,6 +32,7 @@
     "test:loader:eager": "PW_BUNDLE=loader_eager yarn test:loader",
     "test:loader:tracing": "PW_BUNDLE=loader_tracing yarn test:loader",
     "test:loader:replay": "PW_BUNDLE=loader_replay yarn test:loader",
+    "test:loader:replay_buffer": "PW_BUNDLE=loader_replay_buffer yarn test:loader",
     "test:loader:full": "PW_BUNDLE=loader_tracing_replay yarn test:loader",
     "test:loader:debug": "PW_BUNDLE=loader_debug yarn test:loader",
     "test:ci": "yarn test:all --reporter='line'",

dev-packages/browser-integration-tests/suites/replay/bufferMode/init.js renamed to dev-packages/browser-integration-tests/suites/replay/bufferModeManual/init.js

@@ -0,0 +1 @@
+window.doSomethingWrong();

dev-packages/browser-integration-tests/suites/replay/bufferMode/subject.js renamed to dev-packages/browser-integration-tests/suites/replay/bufferModeManual/subject.js

@@ -0,0 +1,38 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../../utils/fixtures';
+import { envelopeRequestParser, waitForErrorRequestOnUrl } from '../../../../utils/helpers';
+import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
+
+sentryTest(
+  '[error-mode] should capture error that happens immediately after init',
+  async ({ getLocalTestUrl, page }) => {
+    if (shouldSkipReplayTest()) {
+      sentryTest.skip();
+    }
+
+    await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+      return route.fulfill({
+        status: 200,
+        contentType: 'application/json',
+        body: JSON.stringify({ id: 'test-id' }),
+      });
+    });
+
+    const req = waitForReplayRequest(page);
+
+    const url = await getLocalTestUrl({ testDir: __dirname });
+    const reqError = await waitForErrorRequestOnUrl(page, url);
+
+    const errorEventData = envelopeRequestParser(reqError);
+    expect(errorEventData.exception?.values?.length).toBe(1);
+    expect(errorEventData.exception?.values?.[0]?.value).toContain('window.doSomethingWrong is not a function');
+
+    const eventData = getReplayEvent(await req);
+
+    expect(eventData).toBeDefined();
+    expect(eventData.segment_id).toBe(0);
+
+    expect(errorEventData.tags?.replayId).toEqual(eventData.replay_id);
+  },
+);

dev-packages/browser-integration-tests/suites/replay/bufferMode/template.html renamed to dev-packages/browser-integration-tests/suites/replay/bufferModeManual/template.html

@@ -55,6 +55,7 @@ const BUNDLE_PATHS: Record<string, Record<string, string>> = {
     loader_debug: 'build/bundles/bundle.debug.min.js',
     loader_tracing: 'build/bundles/bundle.tracing.min.js',
     loader_replay: 'build/bundles/bundle.replay.min.js',
+    loader_replay_buffer: 'build/bundles/bundle.replay.min.js',
     loader_tracing_replay: 'build/bundles/bundle.tracing.replay.debug.min.js',
   },
   integrations: {
@@ -96,6 +97,10 @@ export const LOADER_CONFIGS: Record<string, { options: Record<string, unknown>;
     options: { replaysSessionSampleRate: 1, replaysOnErrorSampleRate: 1 },
     lazy: false,
   },
+  loader_replay_buffer: {
+    options: { replaysSessionSampleRate: 0, replaysOnErrorSampleRate: 1 },
+    lazy: false,
+  },
   loader_tracing_replay: {
     options: { tracesSampleRate: 1, replaysSessionSampleRate: 1, replaysOnErrorSampleRate: 1, debug: true },
     lazy: false,

dev-packages/browser-integration-tests/suites/replay/bufferMode/test.ts renamed to dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts

@@ -1,5 +1,5 @@
-import { getClient, parseSampleRate } from '@sentry/core';
-import type { BrowserClientReplayOptions, Integration, IntegrationFn } from '@sentry/types';
+import { parseSampleRate } from '@sentry/core';
+import type { BrowserClientReplayOptions, Client, Integration, IntegrationFn } from '@sentry/types';
 import { consoleSandbox, dropUndefinedKeys, isBrowser } from '@sentry/utils';
 
 import {
@@ -215,22 +215,13 @@ export class Replay implements Integration {
   /**
    * Setup and initialize replay container
    */
-  public setupOnce(): void {
-    if (!isBrowser()) {
+  public afterAllSetup(client: Client): void {
+    if (!isBrowser() || this._replay) {
       return;
     }
 
-    this._setup();
-
-    // Once upon a time, we tried to create a transaction in `setupOnce` and it would
-    // potentially create a transaction before some native SDK integrations have run
-    // and applied their own global event processor. An example is:
-    // https://github.com/getsentry/sentry-javascript/blob/b47ceafbdac7f8b99093ce6023726ad4687edc48/packages/browser/src/integrations/useragent.ts
-    //
-    // So we call `this._initialize()` in next event loop as a workaround to wait for other
-    // global event processors to finish. This is no longer needed, but keeping it
-    // here to avoid any future issues.
-    setTimeout(() => this._initialize());
+    this._setup(client);
+    this._initialize(client);
   }
 
   /**
@@ -301,24 +292,19 @@ export class Replay implements Integration {
   /**
    * Initializes replay.
    */
-  protected _initialize(): void {
+  protected _initialize(client: Client): void {
     if (!this._replay) {
       return;
     }
 
-    // We have to run this in _initialize, because this runs in setTimeout
-    // So when this runs all integrations have been added
-    // Before this, we cannot access integrations on the client,
-    // so we need to mutate the options here
-    this._maybeLoadFromReplayCanvasIntegration();
-
+    this._maybeLoadFromReplayCanvasIntegration(client);
     this._replay.initializeSampling();
   }
 
   /** Setup the integration. */
-  private _setup(): void {
+  private _setup(client: Client): void {
     // Client is not available in constructor, so we need to wait until setupOnce
-    const finalOptions = loadReplayOptionsFromClient(this._initialOptions);
+    const finalOptions = loadReplayOptionsFromClient(this._initialOptions, client);
 
     this._replay = new ReplayContainer({
       options: finalOptions,
@@ -327,12 +313,11 @@ export class Replay implements Integration {
   }
 
   /** Get canvas options from ReplayCanvas integration, if it is also added. */
-  private _maybeLoadFromReplayCanvasIntegration(): void {
+  private _maybeLoadFromReplayCanvasIntegration(client: Client): void {
     // To save bundle size, we skip checking for stuff here
     // and instead just try-catch everything - as generally this should all be defined
     /* eslint-disable @typescript-eslint/no-non-null-assertion */
     try {
-      const client = getClient()!;
       const canvasIntegration = client.getIntegrationByName('ReplayCanvas') as Integration & {
         getOptions(): ReplayCanvasIntegrationOptions;
       };
@@ -349,24 +334,15 @@ export class Replay implements Integration {
 }
 
 /** Parse Replay-related options from SDK options */
-function loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions): ReplayPluginOptions {
-  const client = getClient();
-  const opt = client && (client.getOptions() as BrowserClientReplayOptions);
+function loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions, client: Client): ReplayPluginOptions {
+  const opt = client.getOptions() as BrowserClientReplayOptions;
 
   const finalOptions: ReplayPluginOptions = {
     sessionSampleRate: 0,
     errorSampleRate: 0,
     ...dropUndefinedKeys(initialOptions),
   };
 
-  if (!opt) {
-    consoleSandbox(() => {
-      // eslint-disable-next-line no-console
-      console.warn('SDK client is not available.');
-    });
-    return finalOptions;
-  }
-
   const replaysSessionSampleRate = parseSampleRate(opt.replaysSessionSampleRate);
   const replaysOnErrorSampleRate = parseSampleRate(opt.replaysOnErrorSampleRate);
 

dev-packages/browser-integration-tests/suites/replay/errors/immediateError/subject.js

@@ -1,3 +1,4 @@
+import { getClient } from '@sentry/core';
 import type { Event } from '@sentry/types';
 
 import { REPLAY_EVENT_NAME, SESSION_IDLE_EXPIRE_DURATION } from '../../../src/constants';
@@ -133,8 +134,8 @@ describe('Integration | coreHandlers | handleGlobalEvent', () => {
 
   it('tags errors and transactions with replay id for session samples', async () => {
     const { replay, integration } = await resetSdkMock({});
-    // @ts-expect-error protected but ok to use for testing
-    integration._initialize();
+    integration['_initialize'](getClient()!);
+
     const transaction = Transaction();
     const error = Error();
     expect(handleGlobalEventListener(replay)(transaction, {})).toEqual(

dev-packages/browser-integration-tests/suites/replay/errors/immediateError/test.ts

@@ -866,7 +866,7 @@ describe('Integration | errorSampleRate', () => {
       },
       autoStart: false,
     });
-    integration['_initialize']();
+    integration['_initialize'](getClient()!);
 
     expect(replay.recordingMode).toBe('session');
     const sessionId = replay.getSessionId();
@@ -899,7 +899,7 @@ describe('Integration | errorSampleRate', () => {
       },
       autoStart: false,
     });
-    integration['_initialize']();
+    integration['_initialize'](getClient()!);
 
     vi.runAllTimers();
 
@@ -943,7 +943,7 @@ describe('Integration | errorSampleRate', () => {
       },
       autoStart: false,
     });
-    integration['_initialize']();
+    integration['_initialize'](getClient()!);
     const optionsEvent = createOptionsEvent(replay);
 
     const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP });

dev-packages/browser-integration-tests/utils/generatePlugin.ts

@@ -1,3 +1,4 @@
+import { getClient } from '@sentry/core';
 import { resetSdkMock } from '../mocks/resetSdkMock';
 import { useFakeTimers } from '../utils/use-fake-timers';
 
@@ -57,8 +58,7 @@ describe('Integration | sampling', () => {
     // @ts-expect-error private API
     const spyAddListeners = vi.spyOn(replay, '_addListeners');
 
-    // @ts-expect-error protected
-    integration._initialize();
+    integration['_initialize'](getClient()!);
 
     vi.runAllTimers();
 

packages/replay-internal/src/integration.ts

@@ -61,12 +61,8 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true }
       _initialized = value;
     }
 
-    public setupOnce(): void {
-      // do nothing
-    }
-
-    public initialize(): void {
-      return super._initialize();
+    public afterAllSetup(): void {
+      // do nothing, we need to manually initialize this
     }
   }
 
@@ -76,7 +72,7 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true }
     ...replayOptions,
   });
 
-  init({
+  const client = init({
     ...getDefaultClientOptions(),
     dsn: 'https://dsn@ingest.f00.f00/1',
     autoSessionTracking: false,
@@ -86,14 +82,14 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true }
     replaysOnErrorSampleRate: 0.0,
     ...sentryOptions,
     integrations: [replayIntegration],
-  });
+  })!;
 
-  // Instead of `setupOnce`, which is tricky to test, we call this manually here
-  replayIntegration['_setup']();
+  // Instead of `afterAllSetup`, which is tricky to test, we call this manually here
+  replayIntegration['_setup'](client);
 
   if (autoStart) {
     // Only exists in our mock
-    replayIntegration.initialize();
+    replayIntegration['_initialize'](client);
   }
 
   const replay = replayIntegration['_replay']!;

packages/replay-internal/test/integration/coreHandlers/handleGlobalEvent.test.ts

@@ -1,6 +1,7 @@
 import { BaseClient, createTransport, initAndBind } from '@sentry/core';
 import type {
   BrowserClientReplayOptions,
+  Client,
   ClientOptions,
   Event,
   ParameterizedString,
@@ -33,8 +34,8 @@ export class TestClient extends BaseClient<TestClientOptions> {
   }
 }
 
-export function init(options: TestClientOptions): void {
-  initAndBind(TestClient, options);
+export function init(options: TestClientOptions): Client {
+  return initAndBind(TestClient, options);
 }
 
 export function getDefaultClientOptions(options: Partial<TestClientOptions> = {}): ClientOptions {