diff --git a/CHANGELOG.md b/CHANGELOG.md index c2e8119a42..2e43575525 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixes +- Fix potential ANR/deadlock in Session Replay when `checkCanRecord` runs on the replay executor thread ([#5837](https://github.com/getsentry/sentry-java/pull/5837)) - Prevent concurrent PixelCopy access during Session Replay masking and bitmap cleanup ([#5808](https://github.com/getsentry/sentry-java/pull/5808)) - Release `MediaMuxer` when the replay video encoder fails to start to avoid a resource leak ([#5607](https://github.com/getsentry/sentry-java/pull/5607)) 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 index d9cd15d891..98333260c7 100644 --- 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 @@ -4,6 +4,7 @@ import android.content.Context import android.graphics.Bitmap import android.graphics.BitmapFactory import android.os.Build +import android.os.Looper import android.view.MotionEvent import io.sentry.Breadcrumb import io.sentry.DataCategory.All @@ -130,7 +131,7 @@ public class ReplayIntegration( private var replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null private var mainLooperHandler: MainLooperHandler = MainLooperHandler() private var gestureRecorderProvider: (() -> GestureRecorder)? = null - private val lifecycleLock = AutoClosableReentrantLock() + internal val lifecycleLock = AutoClosableReentrantLock() private val lifecycle = ReplayLifecycle() override fun register(scopes: IScopes, options: SentryOptions) { @@ -352,7 +353,7 @@ public class ReplayIntegration( } addFrame(bitmap, frameTimeStamp, screen) } - checkCanRecord() + postOnMainThread { checkCanRecord() } } override fun onScreenshotRecorded(screenshot: File, frameTimestamp: Long) { @@ -375,7 +376,7 @@ public class ReplayIntegration( } addFrame(screenshot, frameTimestamp, screen) } - checkCanRecord() + postOnMainThread { checkCanRecord() } } override fun close() { @@ -390,21 +391,23 @@ public class ReplayIntegration( recorder?.close() recorder = null rootViewsSpy.close() - if (lazyReplayExecutor.isInitialized()) { - if (options.threadChecker.isMainThread) { - replayExecutor.gracefulShutdown() - } else { - replayExecutor.shutdown() - } + lifecycle.currentState = CLOSED + } + // shutdown outside lock — awaiting termination while holding lifecycleLock deadlocks + // if any executor task tries to acquire the same lock + if (lazyReplayExecutor.isInitialized()) { + if (options.threadChecker.isMainThread) { + replayExecutor.gracefulShutdown() + } else { + replayExecutor.shutdown() } - if (lazyPersistingExecutor.isInitialized()) { - if (options.threadChecker.isMainThread) { - persistingExecutor.gracefulShutdown() - } else { - persistingExecutor.shutdown() - } + } + if (lazyPersistingExecutor.isInitialized()) { + if (options.threadChecker.isMainThread) { + persistingExecutor.gracefulShutdown() + } else { + persistingExecutor.shutdown() } - lifecycle.currentState = CLOSED } } @@ -444,6 +447,17 @@ public class ReplayIntegration( captureStrategy?.onTouchEvent(event) } + // Runs [block] on the main thread. If already there, executes inline; otherwise posts via + // the main looper handler. Prevents deadlocks when lifecycle-lock-acquiring code (e.g. + // checkCanRecord -> pauseInternal) is called from the replay executor thread. + private inline fun postOnMainThread(crossinline block: () -> Unit) { + if (Looper.myLooper() == Looper.getMainLooper()) { + block() + } else { + mainLooperHandler.post { block() } + } + } + /** * Check if we're offline or rate-limited and pause for session mode to not overflow the envelope * cache. diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt index c26e6be9c4..b5e15b5534 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt @@ -23,13 +23,16 @@ import io.sentry.rrweb.RRWebMetaEvent import io.sentry.rrweb.RRWebVideoEvent import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider +import io.sentry.transport.RateLimiter import java.time.Duration +import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.BeforeTest import kotlin.test.assertEquals import kotlin.test.assertNotEquals +import kotlin.test.assertTrue import org.awaitility.core.ConditionTimeoutException import org.awaitility.kotlin.await import org.junit.Rule @@ -41,6 +44,7 @@ import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer +import org.mockito.kotlin.doReturn import org.mockito.kotlin.mock import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -61,11 +65,17 @@ class ReplaySmokeTest { internal class Fixture { val options = SentryOptions() val scope = Scope(options) + val rateLimiter = + mock { + on { isActiveForCategory(any()) }.thenReturn(false) + } val scopes = mock { doAnswer { (it.arguments[0] as ScopeCallback).run(scope) } .whenever(it) .configureScope(any()) + + on { rateLimiter }.doReturn(rateLimiter) } private class ImmediateHandler : @@ -91,7 +101,10 @@ class ReplaySmokeTest { mainLooperHandler = mock { whenever(mock.handler).thenReturn(ImmediateHandler()) - whenever(mock.post(any())).then { (it.arguments[0] as Runnable).run() } + whenever(mock.post(any())).then { + (it.arguments[0] as Runnable).run() + true + } whenever(mock.postDelayed(any(), anyLong())).then { // have to use another thread here otherwise it will block the test thread recordingThread.schedule( @@ -243,6 +256,45 @@ class ReplaySmokeTest { assertNotEquals(falseReplay.rootViewsSpy, replay.rootViewsSpy) assertEquals(0, falseReplay.rootViewsSpy.listeners.size) } + + @Test + fun `close does not deadlock when executor task is waiting on lifecycleLock`() { + fixture.options.sessionReplay.sessionSampleRate = 1.0 + fixture.options.cacheDirPath = tmpDir.newFolder().absolutePath + + val replay = fixture.getSut(context) + replay.register(fixture.scopes, fixture.options) + replay.start() + + val taskBlocked = CountDownLatch(1) + val lockReleased = CountDownLatch(1) + + // hold lifecycleLock on this thread + val token = replay.lifecycleLock.acquire() + + // submit a task on the executor that tries to acquire the same lock — it will block + replay.replayExecutor.submit { + taskBlocked.countDown() + replay.lifecycleLock.acquire().use {} + } + + // wait for the executor task to actually be running and blocked + assertTrue(taskBlocked.await(2, TimeUnit.SECONDS)) + + // release the lock, then close — if shutdown were inside the lock this would deadlock + token.close() + + // close() must complete within a reasonable time + val closedInTime = AtomicBoolean(false) + val closeThread = Thread { + replay.close() + closedInTime.set(true) + } + closeThread.start() + closeThread.join(5000) + + assertTrue(closedInTime.get(), "close() deadlocked") + } } private class ExampleActivity : Activity() {