Cherry-pick: Session Replay: Fix various crashes and issues (#4135) (#4145)

* Cherry-pick session replay fixes

* Fix test

Author: romtsn

CHANGELOG.md

@@ -4,8 +4,12 @@
 
 ### Fixes
 
+- Session Replay: Fix various crashes and issues ([#4135](https://github.com/getsentry/sentry-java/pull/4135))
+    - Fix `FileNotFoundException` when trying to read/write `.ongoing_segment` file
+    - Fix `IllegalStateException` when registering `onDrawListener`
+    - Fix SIGABRT native crashes on Motorola devices when encoding a video
 - (Jetpack Compose) Modifier.sentryTag now uses Modifier.Node ([#4029](https://github.com/getsentry/sentry-java/pull/4029))
-  - This allows Composables that use this modifier to be skippable
+    - This allows Composables that use this modifier to be skippable
 
 ## 7.21.0
 

sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java

@@ -10,7 +10,6 @@
 import io.sentry.transport.ICurrentDateProvider;
 import java.util.Timer;
 import java.util.TimerTask;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicLong;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -19,7 +18,6 @@
 final class LifecycleWatcher implements DefaultLifecycleObserver {
 
   private final AtomicLong lastUpdatedSession = new AtomicLong(0L);
-  private final AtomicBoolean isFreshSession = new AtomicBoolean(false);
 
   private final long sessionIntervalMillis;
 
@@ -80,7 +78,6 @@ private void startSession() {
             final @Nullable Session currentSession = scope.getSession();
             if (currentSession != null && currentSession.getStarted() != null) {
               lastUpdatedSession.set(currentSession.getStarted().getTime());
-              isFreshSession.set(true);
             }
           }
         });
@@ -92,11 +89,8 @@ private void startSession() {
         hub.startSession();
       }
       hub.getOptions().getReplayController().start();
-    } else if (!isFreshSession.get()) {
-      // only resume if it's not a fresh session, which has been started in SentryAndroid.init
-      hub.getOptions().getReplayController().resume();
     }
-    isFreshSession.set(false);
+    hub.getOptions().getReplayController().resume();
     this.lastUpdatedSession.set(currentTimeMillis);
   }
 

sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt

@@ -254,7 +254,7 @@ class LifecycleWatcherTest {
     }
 
     @Test
-    fun `if the hub has already a fresh session running, doesn't resume replay`() {
+    fun `if the hub has already a fresh session running, resumes replay to invalidate isManualPause flag`() {
         val watcher = fixture.getSUT(
             enableAppLifecycleBreadcrumbs = false,
             session = Session(
@@ -276,7 +276,7 @@ class LifecycleWatcherTest {
         )
 
         watcher.onStart(fixture.ownerMock)
-        verify(fixture.replayController, never()).resume()
+        verify(fixture.replayController).resume()
     }
 
     @Test
@@ -293,7 +293,7 @@ class LifecycleWatcherTest {
         verify(fixture.replayController).pause()
 
         watcher.onStart(fixture.ownerMock)
-        verify(fixture.replayController).resume()
+        verify(fixture.replayController, times(2)).resume()
 
         watcher.onStop(fixture.ownerMock)
         verify(fixture.replayController, timeout(10000)).stop()

sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt

@@ -53,7 +53,7 @@ public class ReplayCache(
     internal val frames = mutableListOf<ReplayFrame>()
 
     private val ongoingSegment = LinkedHashMap<String, String>()
-    private val ongoingSegmentFile: File? by lazy {
+    internal val ongoingSegmentFile: File? by lazy {
         if (replayCacheDir == null) {
             return@lazy null
         }
@@ -273,6 +273,9 @@ public class ReplayCache(
         if (isClosed.get()) {
             return
         }
+        if (ongoingSegmentFile?.exists() != true) {
+            ongoingSegmentFile?.createNewFile()
+        }
         if (ongoingSegment.isEmpty()) {
             ongoingSegmentFile?.useLines { lines ->
                 lines.associateTo(ongoingSegment) {

sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt

@@ -21,6 +21,11 @@ import io.sentry.SentryIntegrationPackageStorage
 import io.sentry.SentryLevel.DEBUG
 import io.sentry.SentryLevel.INFO
 import io.sentry.SentryOptions
+import io.sentry.android.replay.ReplayState.CLOSED
+import io.sentry.android.replay.ReplayState.PAUSED
+import io.sentry.android.replay.ReplayState.RESUMED
+import io.sentry.android.replay.ReplayState.STARTED
+import io.sentry.android.replay.ReplayState.STOPPED
 import io.sentry.android.replay.capture.BufferCaptureStrategy
 import io.sentry.android.replay.capture.CaptureStrategy
 import io.sentry.android.replay.capture.CaptureStrategy.ReplaySegment
@@ -100,15 +105,15 @@ public class ReplayIntegration(
         Executors.newSingleThreadScheduledExecutor(ReplayExecutorServiceThreadFactory())
     }
 
-    // TODO: probably not everything has to be thread-safe here
     internal val isEnabled = AtomicBoolean(false)
-    private val isRecording = AtomicBoolean(false)
+    internal val isManualPause = AtomicBoolean(false)
     private var captureStrategy: CaptureStrategy? = null
     public val replayCacheDir: File? get() = captureStrategy?.replayCacheDir
     private var replayBreadcrumbConverter: ReplayBreadcrumbConverter = NoOpReplayBreadcrumbConverter.getInstance()
     private var replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null
     private var mainLooperHandler: MainLooperHandler = MainLooperHandler()
     private var gestureRecorderProvider: (() -> GestureRecorder)? = null
+    private val lifecycle = ReplayLifecycle()
 
     override fun register(hub: IHub, options: SentryOptions) {
         this.options = options
@@ -151,15 +156,15 @@ public class ReplayIntegration(
         finalizePreviousReplay()
     }
 
-    override fun isRecording() = isRecording.get()
+    override fun isRecording() = lifecycle.currentState >= STARTED && lifecycle.currentState < STOPPED
 
+    @Synchronized
     override fun start() {
-        // TODO: add lifecycle state instead and manage it in start/pause/resume/stop
         if (!isEnabled.get()) {
             return
         }
 
-        if (isRecording.getAndSet(true)) {
+        if (!lifecycle.isAllowed(STARTED)) {
             options.logger.log(
                 DEBUG,
                 "Session replay is already being recorded, not starting a new one"
@@ -183,19 +188,35 @@ public class ReplayIntegration(
         captureStrategy?.start(recorderConfig)
         recorder?.start(recorderConfig)
         registerRootViewListeners()
+        lifecycle.currentState = STARTED
     }
 
     override fun resume() {
-        if (!isEnabled.get() || !isRecording.get()) {
+        isManualPause.set(false)
+        resumeInternal()
+    }
+
+    @Synchronized
+    private fun resumeInternal() {
+        if (!isEnabled.get() || !lifecycle.isAllowed(RESUMED)) {
+            return
+        }
+
+        if (isManualPause.get() || options.connectionStatusProvider.connectionStatus == DISCONNECTED ||
+            hub?.rateLimiter?.isActiveForCategory(All) == true ||
+            hub?.rateLimiter?.isActiveForCategory(Replay) == true
+        ) {
             return
         }
 
         captureStrategy?.resume()
         recorder?.resume()
+        lifecycle.currentState = RESUMED
     }
 
+    @Synchronized
     override fun captureReplay(isTerminating: Boolean?) {
-        if (!isEnabled.get() || !isRecording.get()) {
+        if (!isEnabled.get() || !isRecording()) {
             return
         }
 
@@ -220,25 +241,33 @@ public class ReplayIntegration(
     override fun getBreadcrumbConverter(): ReplayBreadcrumbConverter = replayBreadcrumbConverter
 
     override fun pause() {
-        if (!isEnabled.get() || !isRecording.get()) {
+        isManualPause.set(true)
+        pauseInternal()
+    }
+
+    @Synchronized
+    private fun pauseInternal() {
+        if (!isEnabled.get() || !lifecycle.isAllowed(PAUSED)) {
             return
         }
 
         recorder?.pause()
         captureStrategy?.pause()
+        lifecycle.currentState = PAUSED
     }
 
+    @Synchronized
     override fun stop() {
-        if (!isEnabled.get() || !isRecording.get()) {
+        if (!isEnabled.get() || !lifecycle.isAllowed(STOPPED)) {
             return
         }
 
         unregisterRootViewListeners()
         recorder?.stop()
         gestureRecorder?.stop()
         captureStrategy?.stop()
-        isRecording.set(false)
         captureStrategy = null
+        lifecycle.currentState = STOPPED
     }
 
     override fun onScreenshotRecorded(bitmap: Bitmap) {
@@ -257,8 +286,9 @@ public class ReplayIntegration(
         }
     }
 
+    @Synchronized
     override fun close() {
-        if (!isEnabled.get()) {
+        if (!isEnabled.get() || !lifecycle.isAllowed(CLOSED)) {
             return
         }
 
@@ -275,10 +305,11 @@ public class ReplayIntegration(
         recorder = null
         rootViewsSpy.close()
         replayExecutor.gracefullyShutdown(options)
+        lifecycle.currentState = CLOSED
     }
 
     override fun onConfigurationChanged(newConfig: Configuration) {
-        if (!isEnabled.get() || !isRecording.get()) {
+        if (!isEnabled.get() || !isRecording()) {
             return
         }
 
@@ -289,6 +320,10 @@ public class ReplayIntegration(
         captureStrategy?.onConfigurationChanged(recorderConfig)
 
         recorder?.start(recorderConfig)
+        // we have to restart recorder with a new config and pause immediately if the replay is paused
+        if (lifecycle.currentState == PAUSED) {
+            recorder?.pause()
+        }
     }
 
     override fun onConnectionStatusChanged(status: ConnectionStatus) {
@@ -298,10 +333,10 @@ public class ReplayIntegration(
         }
 
         if (status == DISCONNECTED) {
-            pause()
+            pauseInternal()
         } else {
             // being positive for other states, even if it's NO_PERMISSION
-            resume()
+            resumeInternal()
         }
     }
 
@@ -312,15 +347,18 @@ public class ReplayIntegration(
         }
 
         if (rateLimiter.isActiveForCategory(All) || rateLimiter.isActiveForCategory(Replay)) {
-            pause()
+            pauseInternal()
         } else {
-            resume()
+            resumeInternal()
         }
     }
 
     override fun onLowMemory() = Unit
 
     override fun onTouchEvent(event: MotionEvent) {
+        if (!isEnabled.get() || !lifecycle.isTouchRecordingAllowed()) {
+            return
+        }
         captureStrategy?.onTouchEvent(event)
     }
 
@@ -336,7 +374,7 @@ public class ReplayIntegration(
                     hub?.rateLimiter?.isActiveForCategory(Replay) == true
                 )
         ) {
-            pause()
+            pauseInternal()
         }
     }
 

sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt

@@ -0,0 +1,58 @@
+package io.sentry.android.replay
+
+internal enum class ReplayState {
+    /**
+     * Initial state of a Replay session. This is the state when ReplayIntegration is constructed
+     * but has not been started yet.
+     */
+    INITIAL,
+
+    /**
+     * Started state for a Replay session. This state is reached after the start() method is called
+     * and the recording is initialized successfully.
+     */
+    STARTED,
+
+    /**
+     * Resumed state for a Replay session. This state is reached after resume() is called on an
+     * already started recording.
+     */
+    RESUMED,
+
+    /**
+     * Paused state for a Replay session. This state is reached after pause() is called on a
+     * resumed recording.
+     */
+    PAUSED,
+
+    /**
+     * Stopped state for a Replay session. This state is reached after stop() is called.
+     * The recording can be started again from this state.
+     */
+    STOPPED,
+
+    /**
+     * Closed state for a Replay session. This is the terminal state reached after close() is called.
+     * No further state transitions are possible after this.
+     */
+    CLOSED;
+}
+
+/**
+ * Class to manage state transitions for ReplayIntegration
+ */
+internal class ReplayLifecycle {
+    @field:Volatile
+    internal var currentState = ReplayState.INITIAL
+
+    fun isAllowed(newState: ReplayState): Boolean = when (currentState) {
+        ReplayState.INITIAL -> newState == ReplayState.STARTED || newState == ReplayState.CLOSED
+        ReplayState.STARTED -> newState == ReplayState.PAUSED || newState == ReplayState.STOPPED || newState == ReplayState.CLOSED
+        ReplayState.RESUMED -> newState == ReplayState.PAUSED || newState == ReplayState.STOPPED || newState == ReplayState.CLOSED
+        ReplayState.PAUSED -> newState == ReplayState.RESUMED || newState == ReplayState.STOPPED || newState == ReplayState.CLOSED
+        ReplayState.STOPPED -> newState == ReplayState.STARTED || newState == ReplayState.CLOSED
+        ReplayState.CLOSED -> false
+    }
+
+    fun isTouchRecordingAllowed(): Boolean = currentState == ReplayState.STARTED || currentState == ReplayState.RESUMED
+}

sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt

@@ -5,6 +5,7 @@ import android.view.MotionEvent
 import io.sentry.Breadcrumb
 import io.sentry.DateUtils
 import io.sentry.IHub
+import io.sentry.SentryLevel.ERROR
 import io.sentry.SentryOptions
 import io.sentry.SentryReplayEvent.ReplayType
 import io.sentry.SentryReplayEvent.ReplayType.BUFFER
@@ -183,7 +184,11 @@ internal abstract class BaseCaptureStrategy(
                         task()
                     }
                 } else {
-                    task()
+                    try {
+                        task()
+                    } catch (e: Throwable) {
+                        options.logger.log(ERROR, "Failed to execute task $TAG.runInBackground", e)
+                    }
                 }
             }