[SR] Fix crashes and add replay lifecycle management (#4135)

* Wrap viewTreeObserver into try-catch * Fix FileNotFoundException when storing segment values * Use software canvas for Motorola devices * Manage ReplayIntegration lifecycle properly * Make ReplayState internal * Changelog * Fix * Remove import * Always resume the replay in lifecycle watcher
getsentry · Feb 5, 2025 · 5202700 · 5202700
1 parent 1f149c3
commit 5202700
Show file tree

Hide file tree

Showing 13 changed files with 517 additions and 87 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,10 @@
 - Do not log if `OtelContextScopesStorage` cannot be found ([#4127](https://github.com/getsentry/sentry-java/pull/4127))
   - Previously `java.lang.ClassNotFoundException: io.sentry.opentelemetry.OtelContextScopesStorage` was shown in the log if the class could not be found.
   - This is just a lookup the SDK performs to configure itself. The SDK also works without OpenTelemetry.
+- 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
 - Mention javadoc and sources for published artifacts in Gradle `.module` metadata ([#3936](https://github.com/getsentry/sentry-java/pull/3936))
 
 ### Dependencies

diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java
@@ -12,7 +12,6 @@
 import io.sentry.util.AutoClosableReentrantLock;
 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;
@@ -21,7 +20,6 @@
 final class LifecycleWatcher implements DefaultLifecycleObserver {
 
   private final AtomicLong lastUpdatedSession = new AtomicLong(0L);
-  private final AtomicBoolean isFreshSession = new AtomicBoolean(false);
 
   private final long sessionIntervalMillis;
 
@@ -82,7 +80,6 @@ private void startSession() {
             final @Nullable Session currentSession = scope.getSession();
             if (currentSession != null && currentSession.getStarted() != null) {
               lastUpdatedSession.set(currentSession.getStarted().getTime());
-              isFreshSession.set(true);
             }
           }
         });
@@ -94,11 +91,8 @@ private void startSession() {
         scopes.startSession();
       }
       scopes.getOptions().getReplayController().start();
-    } else if (!isFreshSession.get()) {
-      // only resume if it's not a fresh session, which has been started in SentryAndroid.init
-      scopes.getOptions().getReplayController().resume();
     }
-    isFreshSession.set(false);
+    scopes.getOptions().getReplayController().resume();
     this.lastUpdatedSession.set(currentTimeMillis);
   }
 

diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt b/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()

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt
@@ -55,7 +55,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
         }
@@ -275,6 +275,9 @@ public class ReplayCache(
             if (isClosed.get()) {
                 return
             }
+            if (ongoingSegmentFile?.exists() != true) {
+                ongoingSegmentFile?.createNewFile()
+            }
             if (ongoingSegment.isEmpty()) {
                 ongoingSegmentFile?.useLines { lines ->
                     lines.associateTo(ongoingSegment) {

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt b/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
@@ -40,6 +45,7 @@ import io.sentry.protocol.SentryId
 import io.sentry.transport.ICurrentDateProvider
 import io.sentry.transport.RateLimiter
 import io.sentry.transport.RateLimiter.IRateLimitObserver
+import io.sentry.util.AutoClosableReentrantLock
 import io.sentry.util.FileUtils
 import io.sentry.util.HintUtils
 import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion
@@ -100,15 +106,16 @@ 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 lifecycleLock = AutoClosableReentrantLock()
+    private val lifecycle = ReplayLifecycle()
 
     override fun register(scopes: IScopes, options: SentryOptions) {
         this.options = options
@@ -151,51 +158,68 @@ public class ReplayIntegration(
         finalizePreviousReplay()
     }
 
-    override fun isRecording(): Boolean = isRecording.get()
+    override fun isRecording(): Boolean = lifecycle.currentState >= STARTED && lifecycle.currentState < STOPPED
 
     override fun start() {
-        // TODO: add lifecycle state instead and manage it in start/pause/resume/stop
-        if (!isEnabled.get()) {
-            return
-        }
+        lifecycleLock.acquire().use {
+            if (!isEnabled.get()) {
+                return
+            }
 
-        if (isRecording.getAndSet(true)) {
-            options.logger.log(
-                DEBUG,
-                "Session replay is already being recorded, not starting a new one"
-            )
-            return
-        }
+            if (!lifecycle.isAllowed(STARTED)) {
+                options.logger.log(
+                    DEBUG,
+                    "Session replay is already being recorded, not starting a new one"
+                )
+                return
+            }
 
-        val isFullSession = random.sample(options.sessionReplay.sessionSampleRate)
-        if (!isFullSession && !options.sessionReplay.isSessionReplayForErrorsEnabled) {
-            options.logger.log(INFO, "Session replay is not started, full session was not sampled and onErrorSampleRate is not specified")
-            return
-        }
+            val isFullSession = random.sample(options.sessionReplay.sessionSampleRate)
+            if (!isFullSession && !options.sessionReplay.isSessionReplayForErrorsEnabled) {
+                options.logger.log(INFO, "Session replay is not started, full session was not sampled and onErrorSampleRate is not specified")
+                return
+            }
 
-        val recorderConfig = recorderConfigProvider?.invoke(false) ?: ScreenshotRecorderConfig.from(context, options.sessionReplay)
-        captureStrategy = replayCaptureStrategyProvider?.invoke(isFullSession) ?: if (isFullSession) {
-            SessionCaptureStrategy(options, scopes, dateProvider, replayExecutor, replayCacheProvider)
-        } else {
-            BufferCaptureStrategy(options, scopes, dateProvider, random, replayExecutor, replayCacheProvider)
-        }
+            val recorderConfig = recorderConfigProvider?.invoke(false) ?: ScreenshotRecorderConfig.from(context, options.sessionReplay)
+            captureStrategy = replayCaptureStrategyProvider?.invoke(isFullSession) ?: if (isFullSession) {
+                SessionCaptureStrategy(options, scopes, dateProvider, replayExecutor, replayCacheProvider)
+            } else {
+                BufferCaptureStrategy(options, scopes, dateProvider, random, replayExecutor, replayCacheProvider)
+            }
 
-        captureStrategy?.start(recorderConfig)
-        recorder?.start(recorderConfig)
-        registerRootViewListeners()
+            captureStrategy?.start(recorderConfig)
+            recorder?.start(recorderConfig)
+            registerRootViewListeners()
+            lifecycle.currentState = STARTED
+        }
     }
 
     override fun resume() {
-        if (!isEnabled.get() || !isRecording.get()) {
-            return
-        }
+        isManualPause.set(false)
+        resumeInternal()
+    }
 
-        captureStrategy?.resume()
-        recorder?.resume()
+    private fun resumeInternal() {
+        lifecycleLock.acquire().use {
+            if (!isEnabled.get() || !lifecycle.isAllowed(RESUMED)) {
+                return
+            }
+
+            if (isManualPause.get() || options.connectionStatusProvider.connectionStatus == DISCONNECTED ||
+                scopes?.rateLimiter?.isActiveForCategory(All) == true ||
+                scopes?.rateLimiter?.isActiveForCategory(Replay) == true
+            ) {
+                return
+            }
+
+            captureStrategy?.resume()
+            recorder?.resume()
+            lifecycle.currentState = RESUMED
+        }
     }
 
     override fun captureReplay(isTerminating: Boolean?) {
-        if (!isEnabled.get() || !isRecording.get()) {
+        if (!isEnabled.get() || !isRecording()) {
             return
         }
 
@@ -220,25 +244,35 @@ public class ReplayIntegration(
     override fun getBreadcrumbConverter(): ReplayBreadcrumbConverter = replayBreadcrumbConverter
 
     override fun pause() {
-        if (!isEnabled.get() || !isRecording.get()) {
-            return
-        }
+        isManualPause.set(true)
+        pauseInternal()
+    }
 
-        recorder?.pause()
-        captureStrategy?.pause()
+    private fun pauseInternal() {
+        lifecycleLock.acquire().use {
+            if (!isEnabled.get() || !lifecycle.isAllowed(PAUSED)) {
+                return
+            }
+
+            recorder?.pause()
+            captureStrategy?.pause()
+            lifecycle.currentState = PAUSED
+        }
     }
 
     override fun stop() {
-        if (!isEnabled.get() || !isRecording.get()) {
-            return
-        }
+        lifecycleLock.acquire().use {
+            if (!isEnabled.get() || !lifecycle.isAllowed(STOPPED)) {
+                return
+            }
 
-        unregisterRootViewListeners()
-        recorder?.stop()
-        gestureRecorder?.stop()
-        captureStrategy?.stop()
-        isRecording.set(false)
-        captureStrategy = null
+            unregisterRootViewListeners()
+            recorder?.stop()
+            gestureRecorder?.stop()
+            captureStrategy?.stop()
+            captureStrategy = null
+            lifecycle.currentState = STOPPED
+        }
     }
 
     override fun onScreenshotRecorded(bitmap: Bitmap) {
@@ -258,27 +292,30 @@ public class ReplayIntegration(
     }
 
     override fun close() {
-        if (!isEnabled.get()) {
-            return
-        }
+        lifecycleLock.acquire().use {
+            if (!isEnabled.get() || !lifecycle.isAllowed(CLOSED)) {
+                return
+            }
 
-        options.connectionStatusProvider.removeConnectionStatusObserver(this)
-        scopes?.rateLimiter?.removeRateLimitObserver(this)
-        if (options.sessionReplay.isTrackOrientationChange) {
-            try {
-                context.unregisterComponentCallbacks(this)
-            } catch (ignored: Throwable) {
+            options.connectionStatusProvider.removeConnectionStatusObserver(this)
+            scopes?.rateLimiter?.removeRateLimitObserver(this)
+            if (options.sessionReplay.isTrackOrientationChange) {
+                try {
+                    context.unregisterComponentCallbacks(this)
+                } catch (ignored: Throwable) {
+                }
             }
+            stop()
+            recorder?.close()
+            recorder = null
+            rootViewsSpy.close()
+            replayExecutor.gracefullyShutdown(options)
+            lifecycle.currentState = CLOSED
         }
-        stop()
-        recorder?.close()
-        recorder = null
-        rootViewsSpy.close()
-        replayExecutor.gracefullyShutdown(options)
     }
 
     override fun onConfigurationChanged(newConfig: Configuration) {
-        if (!isEnabled.get() || !isRecording.get()) {
+        if (!isEnabled.get() || !isRecording()) {
             return
         }
 
@@ -289,6 +326,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 +339,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 +353,18 @@ public class ReplayIntegration(
         }
 
         if (rateLimiter.isActiveForCategory(All) || rateLimiter.isActiveForCategory(Replay)) {
-            pause()
+            pauseInternal()
         } else {
-            resume()
+            resumeInternal()
         }
     }
 
     override fun onLowMemory(): Unit = Unit
 
     override fun onTouchEvent(event: MotionEvent) {
+        if (!isEnabled.get() || !lifecycle.isTouchRecordingAllowed()) {
+            return
+        }
         captureStrategy?.onTouchEvent(event)
     }
 
@@ -336,7 +380,7 @@ public class ReplayIntegration(
                     scopes?.rateLimiter?.isActiveForCategory(Replay) == true
                 )
         ) {
-            pause()
+            pauseInternal()
         }
     }