[SR] Fix Session Replay crashes (#3628)

getsentry · Aug 12, 2024 · f6e97b1 · f6e97b1
1 parent 32eed6a
commit f6e97b1
Show file tree

Hide file tree

Showing 9 changed files with 86 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,11 @@
   - Align next segment timestamp with the end of the buffered segment when converting from buffer mode to session mode
   - Persist `buffer` replay type for the entire replay when converting from buffer mode to session mode
   - Properly store screen names for `buffer` mode
+- Session Replay: fix various crashes and issues ([#3628](https://github.com/getsentry/sentry-java/pull/3628))
+  - Fix video not being encoded on Pixel devices
+  - Fix SIGABRT native crashes on Xiaomi devices when encoding a video
+  - Fix `RejectedExecutionException` when redacting a screenshot
+  - Fix `FileNotFoundException` when persisting segment values
 
 ### Chores
 

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt
@@ -26,6 +26,7 @@ import io.sentry.SentryReplayOptions
 import io.sentry.android.replay.util.MainLooperHandler
 import io.sentry.android.replay.util.getVisibleRects
 import io.sentry.android.replay.util.gracefullyShutdown
+import io.sentry.android.replay.util.submitSafely
 import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode
 import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.ImageViewHierarchyNode
 import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.TextViewHierarchyNode
@@ -122,7 +123,7 @@ internal class ScreenshotRecorder(
                         val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options)
                         root.traverse(viewHierarchy)
 
-                        recorder.submit {
+                        recorder.submitSafely(options, "screenshot_recorder.redact") {
                             val canvas = Canvas(bitmap)
                             canvas.setMatrix(prescaledMatrix)
                             viewHierarchy.traverse { node ->

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt
@@ -100,10 +100,10 @@ internal abstract class BaseCaptureStrategy(
     ) {
         cache = replayCacheProvider?.invoke(replayId, recorderConfig) ?: ReplayCache(options, replayId, recorderConfig)
 
+        this.currentReplayId = replayId
+        this.currentSegment = segmentId
         this.replayType = replayType ?: (if (this is SessionCaptureStrategy) SESSION else BUFFER)
         this.recorderConfig = recorderConfig
-        this.currentSegment = segmentId
-        this.currentReplayId = replayId
 
         segmentTimestamp = DateUtils.getCurrentDateTime()
         replayStartTimestamp.set(dateProvider.currentTimeMillis)

diff --git a/...y-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt b/...y-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt
@@ -124,7 +124,6 @@ internal class SessionCaptureStrategy(
     }
 
     override fun onConfigurationChanged(recorderConfig: ScreenshotRecorderConfig) {
-        val currentSegmentTimestamp = segmentTimestamp ?: return
         createCurrentSegment("onConfigurationChanged") { segment ->
             if (segment is ReplaySegment.Created) {
                 segment.capture(hub)

diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt
@@ -14,6 +14,8 @@ import android.os.Build.VERSION
 import android.os.Build.VERSION_CODES
 import android.text.Layout
 import android.view.View
+import android.widget.TextView
+import java.lang.NullPointerException
 
 /**
  * Adapted copy of AccessibilityNodeInfo from https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/view/View.java;l=10718
@@ -26,7 +28,7 @@ internal fun View.isVisibleToUser(): Pair<Boolean, Rect?> {
         }
         // An invisible predecessor or one with alpha zero means
         // that this view is not visible to the user.
-        var current: Any = this
+        var current: Any? = this
         while (current is View) {
             val view = current
             val transitionAlpha = if (VERSION.SDK_INT >= VERSION_CODES.Q) view.transitionAlpha else 1f
@@ -53,7 +55,10 @@ internal fun Drawable?.isRedactable(): Boolean {
     // TODO: otherwise maybe check for the bitmap size and don't redact those that take a lot of height (e.g. a background of a whatsapp chat)
     return when (this) {
         is InsetDrawable, is ColorDrawable, is VectorDrawable, is GradientDrawable -> false
-        is BitmapDrawable -> !bitmap.isRecycled && bitmap.height > 10 && bitmap.width > 10
+        is BitmapDrawable -> {
+            val bmp = bitmap ?: return false
+            return !bmp.isRecycled && bmp.height > 10 && bmp.width > 10
+        }
         else -> true
     }
 }
@@ -84,3 +89,15 @@ internal fun Layout?.getVisibleRects(globalRect: Rect, paddingLeft: Int, padding
     }
     return rects
 }
+
+/**
+ * [TextView.getVerticalOffset] which is used by [TextView.getTotalPaddingTop] may throw an NPE on
+ * some devices (Redmi), so we try-catch it specifically for an NPE and then fallback to
+ * [TextView.getExtendedPaddingTop]
+ */
+internal val TextView.totalPaddingTopSafe: Int
+    get() = try {
+        totalPaddingTop
+    } catch (e: NullPointerException) {
+        extendedPaddingTop
+    }
diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/video/SimpleVideoEncoder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/video/SimpleVideoEncoder.kt
@@ -33,7 +33,9 @@ import android.annotation.TargetApi
 import android.graphics.Bitmap
 import android.media.MediaCodec
 import android.media.MediaCodecInfo
+import android.media.MediaCodecList
 import android.media.MediaFormat
+import android.os.Build
 import android.view.Surface
 import io.sentry.SentryLevel.DEBUG
 import io.sentry.SentryOptions
@@ -50,8 +52,22 @@ internal class SimpleVideoEncoder(
     val onClose: (() -> Unit)? = null
 ) {
 
+    private val hasExynosCodec: Boolean by lazy(NONE) {
+        // MediaCodecList ctor will initialize an internal in-memory static cache of codecs, so this
+        // call is only expensive the first time
+        MediaCodecList(MediaCodecList.REGULAR_CODECS)
+            .codecInfos
+            .any { it.name.contains("c2.exynos") }
+    }
+
     internal val mediaCodec: MediaCodec = run {
-        val codec = MediaCodec.createEncoderByType(muxerConfig.mimeType)
+        // c2.exynos.h264.encoder seems to have problems encoding the video (Pixel and Samsung devices)
+        // so we use the default encoder instead
+        val codec = if (hasExynosCodec) {
+            MediaCodec.createByCodecName("c2.android.avc.encoder")
+        } else {
+            MediaCodec.createEncoderByType(muxerConfig.mimeType)
+        }
 
         codec
     }
@@ -139,10 +155,13 @@ internal class SimpleVideoEncoder(
     }
 
     fun encode(image: Bitmap) {
-        // NOTE do not use `lockCanvas` like what is done in bitmap2video
-        // This is because https://developer.android.com/reference/android/media/MediaCodec#createInputSurface()
-        // says that, "Surface.lockCanvas(android.graphics.Rect) may fail or produce unexpected results."
-        val canvas = surface?.lockHardwareCanvas()
+        // it seems that Xiaomi devices have problems with hardware canvas, so we have to use
+        // lockCanvas instead https://stackoverflow.com/a/73520742
+        val canvas = if (Build.MANUFACTURER.contains("xiaomi", ignoreCase = true)) {
+            surface?.lockCanvas(null)
+        } else {
+            surface?.lockHardwareCanvas()
+        }
         canvas?.drawBitmap(image, 0f, 0f, null)
         surface?.unlockCanvasAndPost(canvas)
         drainCodec(false)

diff --git a/...-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ViewHierarchyNode.kt b/...-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ViewHierarchyNode.kt
@@ -9,6 +9,7 @@ import android.widget.TextView
 import io.sentry.SentryOptions
 import io.sentry.android.replay.util.isRedactable
 import io.sentry.android.replay.util.isVisibleToUser
+import io.sentry.android.replay.util.totalPaddingTopSafe
 
 @TargetApi(26)
 sealed class ViewHierarchyNode(
@@ -245,7 +246,7 @@ sealed class ViewHierarchyNode(
                         layout = view.layout,
                         dominantColor = view.currentTextColor.toOpaque(),
                         paddingLeft = view.totalPaddingLeft,
-                        paddingTop = view.totalPaddingTop,
+                        paddingTop = view.totalPaddingTopSafe,
                         x = view.x,
                         y = view.y,
                         width = view.width,

diff --git a/...ndroid-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt b/...ndroid-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt
@@ -64,7 +64,7 @@ class BufferCaptureStrategyTest {
                 (it.arguments[0] as ScopeCallback).run(scope)
             }.whenever(it).configureScope(any())
         }
-        var persistedSegment = mutableMapOf<String, String?>()
+        var persistedSegment = LinkedHashMap<String, String?>()
         val replayCache = mock<ReplayCache> {
             on { frames }.thenReturn(mutableListOf(ReplayFrame(File("1720693523997.jpg"), 1720693523997)))
             on { persistSegmentValues(any(), anyOrNull()) }.then {
@@ -293,4 +293,19 @@ class BufferCaptureStrategyTest {
 
         verify(fixture.hub).captureReplay(any(), any())
     }
+
+    @Test
+    fun `replayId should be set and serialized first`() {
+        val strategy = fixture.getSut()
+        val replayId = SentryId()
+
+        strategy.start(fixture.recorderConfig, 0, replayId)
+
+        assertEquals(
+            replayId.toString(),
+            fixture.persistedSegment.values.first(),
+            "The replayId must be set first, so when we clean up stale replays" +
+                "the current replay cache folder is not being deleted."
+        )
+    }
 }
diff --git a/...droid-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt b/...droid-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt
@@ -70,7 +70,7 @@ class SessionCaptureStrategyTest {
                 (it.arguments[0] as ScopeCallback).run(scope)
             }.whenever(it).configureScope(any())
         }
-        var persistedSegment = mutableMapOf<String, String?>()
+        var persistedSegment = LinkedHashMap<String, String?>()
         val replayCache = mock<ReplayCache> {
             on { frames }.thenReturn(mutableListOf(ReplayFrame(File("1720693523997.jpg"), 1720693523997)))
             on { persistSegmentValues(any(), anyOrNull()) }.then {
@@ -352,4 +352,19 @@ class SessionCaptureStrategyTest {
             }
         )
     }
+
+    @Test
+    fun `replayId should be set and serialized first`() {
+        val strategy = fixture.getSut()
+        val replayId = SentryId()
+
+        strategy.start(fixture.recorderConfig, 0, replayId)
+
+        assertEquals(
+            replayId.toString(),
+            fixture.persistedSegment.values.first(),
+            "The replayId must be set first, so when we clean up stale replays" +
+                "the current replay cache folder is not being deleted."
+        )
+    }
 }