Merge branch 'main' into feat/feedback-screenshot-attachment

Author: markushi

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+### Fixes
+
+- Prevent duplicated breadcrumbs on tombstone-merged native crash events ([#5888](https://github.com/getsentry/sentry-java/pull/5888))
+- Prevent a class of Session Replay deadlocks by confining lifecycle state changes to Android's main thread ([#5965](https://github.com/getsentry/sentry-java/pull/5965))
+
 ### Features
 
 - Add screenshot attachment button to the Android user feedback widget ([#5828](https://github.com/getsentry/sentry-java/pull/5828))
@@ -10,6 +15,7 @@
 
 ### Performance
 
+- Defer starting Session Replay off the SDK initialization critical path ([#5965](https://github.com/getsentry/sentry-java/pull/5965))
 - Use manifest metadata resolved at build time to reduce Android SDK initialization overhead ([#5976](https://github.com/getsentry/sentry-java/pull/5976))
 
 ### Dependencies

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

@@ -349,16 +349,19 @@ private void setExtras(final @NotNull SentryBaseEvent event) {
 
   @SuppressWarnings("unchecked")
   private void setBreadcrumbs(final @NotNull SentryBaseEvent event) {
+    final List<Breadcrumb> eventBreadcrumbs = event.getBreadcrumbs();
+    if (eventBreadcrumbs != null && !eventBreadcrumbs.isEmpty()) {
+      // the event already carries its own breadcrumbs (e.g. a tombstone-merged native
+      // crash event), so appending the persisted ones here would duplicate entries. Skip the
+      // disk read altogether since the result would be discarded anyway.
+      return;
+    }
     final List<Breadcrumb> breadcrumbs =
         (List<Breadcrumb>) readFromDisk(options, BREADCRUMBS_FILENAME, List.class);
     if (breadcrumbs == null) {
       return;
     }
-    if (event.getBreadcrumbs() == null) {
-      event.setBreadcrumbs(breadcrumbs);
-    } else {
-      event.getBreadcrumbs().addAll(breadcrumbs);
-    }
+    event.setBreadcrumbs(breadcrumbs);
   }
 
   @SuppressWarnings("unchecked")

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

@@ -334,6 +334,21 @@ class ApplicationExitInfoEventProcessorTest {
     assertEquals("Google Chrome", processed.contexts.browser!!.name)
   }
 
+  @Test
+  fun `when backfillable event already has breadcrumbs, does not duplicate them with persisted ones`() {
+    // simulates a tombstone-merged native crash event, which already carries its own
+    // breadcrumb history captured at crash time, overlapping with what was persisted to disk
+    val hint = HintUtils.createWithTypeCheckHint(BackfillableHint())
+
+    val processed =
+      processEvent(hint, populateScopeCache = true) {
+        breadcrumbs = listOf(Breadcrumb.debug("own-crash-time-breadcrumb"))
+      }
+
+    assertEquals(1, processed.breadcrumbs!!.size)
+    assertEquals("own-crash-time-breadcrumb", processed.breadcrumbs!![0].message)
+  }
+
   @Test
   fun `when backfillable event is enrichable, does not backfill user ip`() {
     val hint = HintUtils.createWithTypeCheckHint(BackfillableHint())
@@ -670,10 +685,11 @@ class ApplicationExitInfoEventProcessorTest {
 
     assertEquals("MainActivity", processed.transaction)
     assertEquals(DEBUG, processed.level)
-    assertEquals(3, processed.breadcrumbs!!.size)
+    // breadcrumbs already set on the event are preserved as-is, not merged with the persisted
+    // ones, since the event already carries its own authoritative breadcrumb history
+    assertEquals(1, processed.breadcrumbs!!.size)
     assertEquals("debug", processed.breadcrumbs!![0].type)
-    assertEquals("debug", processed.breadcrumbs!![1].type)
-    assertEquals("navigation", processed.breadcrumbs!![2].type)
+    assertEquals("test", processed.breadcrumbs!![0].message)
 
     assertEquals("debug", processed.environment)
     assertEquals("io.sentry.samples@1.1.0+220", processed.release)

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

@@ -352,6 +352,7 @@ class SentryAndroidTest {
   @Config(sdk = [26])
   fun `init starts session replay if app is in foreground`() {
     initSentryWithForegroundImportance(true) { _ ->
+      Shadows.shadowOf(Looper.getMainLooper()).idle()
       assertTrue(Sentry.getCurrentHub().options.replayController.isRecording())
     }
   }
@@ -360,6 +361,7 @@ class SentryAndroidTest {
   @Config(sdk = [26])
   fun `init does not start session replay if the app is in background`() {
     initSentryWithForegroundImportance(false) { _ ->
+      Shadows.shadowOf(Looper.getMainLooper()).idle()
       assertFalse(Sentry.getCurrentHub().options.replayController.isRecording())
     }
   }

sentry-android-replay/api/sentry-android-replay.api

@@ -58,7 +58,7 @@ public final class io/sentry/android/replay/ReplayIntegration : io/sentry/IConne
 	public fun <init> (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;)V
 	public fun <init> (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function1;)V
 	public synthetic fun <init> (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function1;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
-	public fun captureReplay (Ljava/lang/Boolean;)V
+	public fun captureReplay (Ljava/lang/Boolean;)Lio/sentry/protocol/SentryId;
 	public fun close ()V
 	public fun disableDebugMaskingOverlay ()V
 	public fun enableDebugMaskingOverlay ()V

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

@@ -22,7 +22,6 @@ import java.io.File
 import java.io.StringReader
 import java.util.Date
 import java.util.LinkedList
-import java.util.concurrent.TimeUnit.MILLISECONDS
 import java.util.concurrent.atomic.AtomicBoolean
 
 /**
@@ -41,7 +40,6 @@ import java.util.concurrent.atomic.AtomicBoolean
 public class ReplayCache(private val options: SentryOptions, private val replayId: SentryId) :
   Closeable {
   private val isClosed = AtomicBoolean(false)
-  private val encoderLock = AutoClosableReentrantLock()
   private val lock = AutoClosableReentrantLock()
   private val framesLock = AutoClosableReentrantLock()
   private var encoder: SimpleVideoEncoder? = null
@@ -152,28 +150,26 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
     }
 
     encoder =
-      encoderLock.acquire().use {
-        SimpleVideoEncoder(
-            options,
-            MuxerConfig(
-              file = videoFile,
-              recordingHeight = height,
-              recordingWidth = width,
-              frameRate = frameRate,
-              bitRate = bitRate,
-            ),
-          )
-          .apply {
-            // the constructor already opened the MediaMuxer, so release it if start() fails,
-            // otherwise the encoder is never assigned and its resources leak (CloseGuard warning)
-            try {
-              start()
-            } catch (t: Throwable) {
-              release()
-              throw t
-            }
+      SimpleVideoEncoder(
+          options,
+          MuxerConfig(
+            file = videoFile,
+            recordingHeight = height,
+            recordingWidth = width,
+            frameRate = frameRate,
+            bitRate = bitRate,
+          ),
+        )
+        .apply {
+          // the constructor already opened the MediaMuxer, so release it if start() fails,
+          // otherwise the encoder is never assigned and its resources leak (CloseGuard warning)
+          try {
+            start()
+          } catch (t: Throwable) {
+            release()
+            throw t
           }
-      }
+        }
 
     val step = 1000 / frameRate.toLong()
     var frameCount = 0
@@ -209,20 +205,15 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
 
     if (frameCount == 0) {
       options.logger.log(DEBUG, "Generated a video with no frames, not capturing a replay segment")
-      encoderLock.acquire().use {
-        encoder?.release()
-        encoder = null
-      }
+      encoder?.release()
+      encoder = null
       deleteFile(videoFile)
       return null
     }
 
-    var videoDuration: Long
-    encoderLock.acquire().use {
-      encoder?.release()
-      videoDuration = encoder?.duration ?: 0
-      encoder = null
-    }
+    encoder?.release()
+    val videoDuration = encoder?.duration ?: 0
+    encoder = null
 
     rotate(until = (from + duration))
 
@@ -235,7 +226,7 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
     }
     return try {
       val bitmap = BitmapFactory.decodeFile(frame.screenshot.absolutePath)
-      encoderLock.acquire().use { encoder?.encode(bitmap) }
+      encoder?.encode(bitmap)
       bitmap.recycle()
       true
     } catch (e: Throwable) {
@@ -281,27 +272,10 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
   }
 
   override fun close() {
-    // close() is called inline from the lifecycle path (ReplayIntegration.stop/close), which holds
-    // its own lock, so blocking here can freeze the main thread. If the encoder is wedged in a
-    // native MediaCodec call we'd never get the lock, so we give up instead: the already-dead codec
-    // is not released (leaking a native handle), which beats an ANR.
     try {
-      val token = encoderLock.tryAcquire(ENCODER_RELEASE_TIMEOUT_MS, MILLISECONDS)
-      if (token == null) {
-        options.logger.log(
-          WARNING,
-          "Timed out waiting for the video encoder, skipping its release to not block the caller",
-        )
-      } else {
-        token.use {
-          encoder?.release()
-          encoder = null
-        }
-      }
-    } catch (e: InterruptedException) {
-      Thread.currentThread().interrupt()
+      encoder?.release()
+      encoder = null
     } finally {
-      // has to happen on all paths, callers rely on it to stop persisting segment values
       isClosed.set(true)
     }
   }
@@ -333,13 +307,6 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
   }
 
   internal companion object {
-    /**
-     * How long [close] waits for the video encoder to become available. Below Android's ~5s ANR
-     * budget, and above the encoder's own bail-out (see MAX_EOS_STALL_ITERATIONS), so an encoder
-     * that's merely slow is still awaited rather than abandoned.
-     */
-    private const val ENCODER_RELEASE_TIMEOUT_MS = 2000L
-
     internal const val ONGOING_SEGMENT = ".ongoing_segment"
 
     internal const val SEGMENT_KEY_HEIGHT = "config.height"