diff --git a/CHANGELOG.md b/CHANGELOG.md index b9f7fc811bc..1c0bc27e1bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixes +- Prevent concurrent PixelCopy access during Session Replay masking and bitmap cleanup ([#5808](https://github.com/getsentry/sentry-java/pull/5808)) - Reduce main-thread work during `Sentry.init` by resolving the shake-detector accelerometer off the main thread (~1.75ms on a Pixel 10) ([#5784](https://github.com/getsentry/sentry-java/pull/5784)) - Backfill release, environment, distribution, tags, and app version/build—and use the matching replay-on-error sample rate—for `ApplicationExitInfo` ANR and native crash events captured before SDK initialization, without reusing options cached by a later app update ([#5762](https://github.com/getsentry/sentry-java/pull/5762)) - `SentryTagModifierNode.isImportantForBounds` now matches the default behavior and returns `true` ([#5789](https://github.com/getsentry/sentry-java/pull/5789)) diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/screenshot/PixelCopyStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/screenshot/PixelCopyStrategy.kt index 4b9618df6ec..49686b3851a 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/screenshot/PixelCopyStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/screenshot/PixelCopyStrategy.kt @@ -59,6 +59,7 @@ internal class PixelCopyStrategy( private val contentChanged = AtomicBoolean(false) private val unstableCaptures = AtomicInteger(0) private val isClosed = AtomicBoolean(false) + private val frameInFlight = AtomicBoolean(false) private val dstOverPaint by lazy(NONE) { Paint().apply { xfermode = PorterDuffXfermode(PorterDuff.Mode.DST_OVER) } } private val screenshotCanvas by lazy(NONE) { Canvas(screenshot) } @@ -77,8 +78,15 @@ internal class PixelCopyStrategy( return } + if (!frameInFlight.compareAndSet(false, true)) { + options.logger.log(DEBUG, "PixelCopyStrategy capture is already in flight, skipping") + markContentChanged() + return + } + if (isClosed.get()) { options.logger.log(DEBUG, "PixelCopyStrategy is closed, not capturing screenshot") + finishFrame() return } @@ -90,6 +98,7 @@ internal class PixelCopyStrategy( { copyResult: Int -> if (isClosed.get()) { options.logger.log(DEBUG, "PixelCopyStrategy is closed, ignoring capture result") + finishFrame() return@request } @@ -97,44 +106,64 @@ internal class PixelCopyStrategy( options.logger.log(INFO, "Failed to capture replay recording: %d", copyResult) unstableCaptures.set(0) lastCaptureSuccessful.set(false) + finishFrame() return@request } val changedDuringCapture = contentChanged.get() if (changedDuringCapture && shouldSkipUnstableCapture()) { + finishFrame() return@request } - // TODO: disableAllMasking here and dont traverse? - val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options.sessionReplay) - val surfaceViewNodes = - if (options.sessionReplay.isCaptureSurfaceViews) { - mutableListOf() - } else { - null - } - root.traverse(viewHierarchy, options.sessionReplay, options.logger, surfaceViewNodes) - - if (surfaceViewNodes.isNullOrEmpty()) { - executor.submit( - ReplayRunnable("screenshot_recorder.mask") { - applyMaskingAndNotify( - root, - viewHierarchy, - resetUnstableCaptures = !changedDuringCapture, + // Release the frame gate if anything below throws before we hand work off to the + // executor — otherwise a single failure wedges captures forever. + try { + // TODO: disableAllMasking here and dont traverse? + val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options.sessionReplay) + val surfaceViewNodes = + if (options.sessionReplay.isCaptureSurfaceViews) { + mutableListOf() + } else { + null + } + root.traverse(viewHierarchy, options.sessionReplay, options.logger, surfaceViewNodes) + + if (surfaceViewNodes.isNullOrEmpty()) { + val submitted = + executor.submit( + ReplayRunnable("screenshot_recorder.mask") { + try { + applyMaskingAndNotify( + root, + viewHierarchy, + resetUnstableCaptures = !changedDuringCapture, + ) + } finally { + finishFrame() + } + } ) + if (submitted == null) { + finishFrame() } - ) - } else { - // Re-arm the recorder's contentChanged gate; SurfaceView redraws don't trigger - // ViewTreeObserver.OnDrawListener, so we'd otherwise emit the same frame forever. - markContentChanged() - captureSurfaceViews( - root, - surfaceViewNodes, - viewHierarchy, - resetUnstableCaptures = !changedDuringCapture, - ) + } else { + // Re-arm the recorder's contentChanged gate; SurfaceView redraws don't trigger + // ViewTreeObserver.OnDrawListener, so we'd otherwise emit the same frame forever. + markContentChanged() + captureSurfaceViews( + root, + surfaceViewNodes, + viewHierarchy, + resetUnstableCaptures = !changedDuringCapture, + ) + } + } catch (e: RuntimeException) { + // OEM View subclasses have been observed throwing during hierarchy traversal + // (e.g. Redmi's TextView NPE). Release the frame gate so a single bad frame + // doesn't wedge the recorder. Errors (OOM, LinkageError) intentionally propagate. + options.logger.log(WARNING, "Failed to process replay frame", e) + finishFrame() } }, mainLooperHandler.handler, @@ -143,6 +172,7 @@ internal class PixelCopyStrategy( options.logger.log(WARNING, "Failed to capture replay recording", e) unstableCaptures.set(0) lastCaptureSuccessful.set(false) + finishFrame() } } @@ -272,37 +302,46 @@ internal class PixelCopyStrategy( windowY: Int, resetUnstableCaptures: Boolean, ) { - executor.submit( - ReplayRunnable("screenshot_recorder.composite") { - if (isClosed.get() || screenshot.isRecycled) { - options.logger.log(DEBUG, "PixelCopyStrategy is closed, skipping compositing") - recycleCaptures(captures) - return@ReplayRunnable - } + val submitted = + executor.submit( + ReplayRunnable("screenshot_recorder.composite") { + try { + if (isClosed.get() || screenshot.isRecycled) { + options.logger.log(DEBUG, "PixelCopyStrategy is closed, skipping compositing") + recycleCaptures(captures) + return@ReplayRunnable + } - for (capture in captures) { - if (capture == null) continue - if (capture.bitmap.isRecycled) continue - - compositeSurfaceViewInto( - screenshotCanvas, - dstOverPaint, - tmpSrcRect, - tmpDstRect, - capture.bitmap, - capture.x, - capture.y, - windowX, - windowY, - config.scaleFactorX, - config.scaleFactorY, - ) - capture.bitmap.recycle() - } + for (capture in captures) { + if (capture == null) continue + if (capture.bitmap.isRecycled) continue + + compositeSurfaceViewInto( + screenshotCanvas, + dstOverPaint, + tmpSrcRect, + tmpDstRect, + capture.bitmap, + capture.x, + capture.y, + windowX, + windowY, + config.scaleFactorX, + config.scaleFactorY, + ) + capture.bitmap.recycle() + } - applyMaskingAndNotify(root, viewHierarchy, resetUnstableCaptures) - } - ) + applyMaskingAndNotify(root, viewHierarchy, resetUnstableCaptures) + } finally { + finishFrame() + } + } + ) + if (submitted == null) { + recycleCaptures(captures) + finishFrame() + } } private fun recycleCaptures(captures: Array) { @@ -322,7 +361,7 @@ internal class PixelCopyStrategy( } override fun emitLastScreenshot() { - if (lastCaptureSuccessful() && !screenshot.isRecycled) { + if (!frameInFlight.get() && lastCaptureSuccessful() && !screenshot.isRecycled) { screenshotRecorderCallback?.onScreenshotRecorded(screenshot) } } @@ -330,7 +369,20 @@ internal class PixelCopyStrategy( override fun close() { isClosed.set(true) unstableCaptures.set(0) - executor.submit( + if (!frameInFlight.get()) { + scheduleCleanup() + } + } + + private fun finishFrame() { + frameInFlight.set(false) + if (isClosed.get()) { + scheduleCleanup() + } + } + + private fun scheduleCleanup() { + val cleanup = ReplayRunnable( "PixelCopyStrategy.close", { @@ -344,7 +396,12 @@ internal class PixelCopyStrategy( maskRenderer.close() }, ) - ) + // ReplayExecutorService.submit returns null only on genuine rejection (post-shutdown); + // inline execution on the worker thread returns a completed future. Fall back to running + // cleanup here so the bitmap + mask renderer are freed even when the executor is dead. + if (executor.submit(cleanup) == null) { + cleanup.run() + } } } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/ReplayExecutorService.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/ReplayExecutorService.kt index 9e9491f516f..5ba334f8029 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/ReplayExecutorService.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/ReplayExecutorService.kt @@ -4,6 +4,7 @@ import io.sentry.SentryLevel.ERROR import io.sentry.SentryOptions import java.util.concurrent.Future import java.util.concurrent.ScheduledExecutorService +import java.util.concurrent.TimeUnit import java.util.concurrent.TimeUnit.MILLISECONDS /** @@ -14,11 +15,20 @@ internal class ReplayExecutorService( private val delegate: ScheduledExecutorService, private val options: SentryOptions, ) : ScheduledExecutorService by delegate { + /** + * Submits [task] for execution and returns a [Future] describing what happened. The return value + * has three distinct outcomes callers can rely on: + * - [CompletedFuture] — the caller is already on the replay worker thread, so the task was run + * inline before this method returned. Skips the queue. + * - A regular [Future] from the underlying [ScheduledExecutorService] — the task was queued and + * will run asynchronously. + * - `null` — the underlying executor rejected the submission (typically because it has been shut + * down). The task did NOT run; callers that need cleanup must handle it themselves. + */ override fun submit(task: Runnable): Future<*>? { if (Thread.currentThread().name.startsWith("SentryReplayIntegration")) { - // we're already on the worker thread, no need to submit task.run() - return null + return CompletedFuture } return try { delegate.submit { @@ -68,3 +78,16 @@ internal class ReplayExecutorService( } internal class ReplayRunnable(val taskName: String, delegate: Runnable) : Runnable by delegate + +/** A Future that represents an already-completed inline execution — never used as null. */ +internal object CompletedFuture : Future { + override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false + + override fun isCancelled(): Boolean = false + + override fun isDone(): Boolean = true + + override fun get() {} + + override fun get(timeout: Long, unit: TimeUnit) {} +} diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/screenshot/PixelCopyStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/screenshot/PixelCopyStrategyTest.kt index 779cf7d4311..73371277615 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/screenshot/PixelCopyStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/screenshot/PixelCopyStrategyTest.kt @@ -27,6 +27,8 @@ import io.sentry.android.replay.ScreenshotRecorderCallback import io.sentry.android.replay.ScreenshotRecorderConfig import io.sentry.android.replay.util.DebugOverlayDrawable import io.sentry.android.replay.util.MainLooperHandler +import io.sentry.android.replay.util.ReplayRunnable +import java.util.concurrent.Future import java.util.concurrent.ScheduledExecutorService import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicReference @@ -112,18 +114,23 @@ class PixelCopyStrategyTest { } @Test - fun `when close is called while executor task is running, does not crash with recycled bitmap`() { + fun `when close races the mask task, masking is skipped and no screenshot is emitted`() { val activity = buildActivity(SimpleActivity::class.java).setup() shadowOf(Looper.getMainLooper()).idle() var strategy: PixelCopyStrategy? = null val failure = AtomicReference() - // Custom executor that closes the strategy before executing tasks + // Custom executor that closes the strategy right before running the mask task, to simulate + // close() racing an in-flight mask task. We key off the mask task specifically (not "the first + // submit") because close() itself submits the cleanup task — closing again when that runs would + // recurse via close() -> scheduleCleanup() -> submit(), a loop no real code path can produce. val executorThatClosesFirst = mock() whenever(executorThatClosesFirst.submit(any())).doAnswer { val task = it.getArgument(0) - strategy?.close() + if ((task as? ReplayRunnable)?.taskName == "screenshot_recorder.mask") { + strategy?.close() + } try { task.run() } catch (e: Throwable) { @@ -138,6 +145,140 @@ class PixelCopyStrategyTest { shadowOf(Looper.getMainLooper()).idle() if (failure.get() != null) throw failure.get() + // close() landed before masking ran, so applyMaskingAndNotify must bail out early and never + // hand a screenshot to the callback after the strategy is closed. + verify(fixture.callback, never()).onScreenshotRecorded(any()) + } + + @Test + @Config(shadows = [DeferredWindowPixelCopyShadow::class]) + fun `capture drops frame while PixelCopy is in flight`() { + val activity = buildActivity(SimpleActivity::class.java).setup() + shadowOf(Looper.getMainLooper()).idle() + val root = activity.get().findViewById(android.R.id.content) + val strategy = fixture.getSut(executor = fixture.inlineExecutor()) + + strategy.capture(root) + strategy.capture(root) + + assertTrue(fixture.contentChangedMarked.get()) + + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + verify(fixture.callback).onScreenshotRecorded(any()) + + strategy.capture(root) + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + verify(fixture.callback, times(2)).onScreenshotRecorded(any()) + } + + @Test + @Config(shadows = [DeferredWindowPixelCopyShadow::class]) + fun `capture drops frame while masking is in flight`() { + val activity = buildActivity(SimpleActivity::class.java).setup() + shadowOf(Looper.getMainLooper()).idle() + val root = activity.get().findViewById(android.R.id.content) + val tasks = mutableListOf() + val executor = mock() + whenever(executor.submit(any())).doAnswer { + tasks += it.getArgument(0) + mock>() + } + val strategy = fixture.getSut(executor) + + strategy.capture(root) + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + strategy.capture(root) + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + assertEquals(1, tasks.size) + tasks.removeAt(0).run() + + strategy.capture(root) + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + assertEquals(1, tasks.size) + } + + @Test + @Config(shadows = [DeferredWindowPixelCopyShadow::class]) + fun `emitLastScreenshot skips while frame is in flight`() { + val activity = buildActivity(SimpleActivity::class.java).setup() + shadowOf(Looper.getMainLooper()).idle() + val root = activity.get().findViewById(android.R.id.content) + val strategy = fixture.getSut(executor = fixture.inlineExecutor()) + captureStableFrame(strategy, root) + + strategy.capture(root) + strategy.emitLastScreenshot() + + verify(fixture.callback).onScreenshotRecorded(any()) + + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + verify(fixture.callback, times(2)).onScreenshotRecorded(any()) + } + + @Test + @Config(shadows = [DeferredWindowPixelCopyShadow::class]) + fun `close defers cleanup until PixelCopy completes`() { + val activity = buildActivity(SimpleActivity::class.java).setup() + shadowOf(Looper.getMainLooper()).idle() + val root = activity.get().findViewById(android.R.id.content) + val executor = mock() + val strategy = fixture.getSut(executor) + + strategy.capture(root) + strategy.close() + + verify(executor, never()).submit(any()) + + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + verify(executor).submit(any()) + } + + @Test + @Config(shadows = [DeferredWindowPixelCopyShadow::class]) + fun `frame gate is released when masking submit is rejected`() { + val activity = buildActivity(SimpleActivity::class.java).setup() + shadowOf(Looper.getMainLooper()).idle() + val root = activity.get().findViewById(android.R.id.content) + // Simulate an already-shutdown executor: submit returns null. + val executor = mock() + whenever(executor.submit(any())).thenReturn(null) + val strategy = fixture.getSut(executor) + + strategy.capture(root) + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + // Gate must have been released; a follow-up capture should proceed rather than being dropped. + strategy.capture(root) + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + verify(executor, times(2)).submit(any()) + } + + @Test + fun `close cleans up inline when executor is already shut down`() { + // submit returns null → previously the bitmap + maskRenderer would leak. + val executor = mock() + whenever(executor.submit(any())).thenReturn(null) + val strategy = fixture.getSut(executor) + + strategy.close() + + // No crash and the submit was attempted exactly once (cleanup ran inline as fallback). + verify(executor).submit(any()) } @Test