From 45c33ca36b8d2d15b568f585f874c8cce6c655db Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Tue, 21 Jul 2026 18:23:24 +0200 Subject: [PATCH 01/10] fix(replay): Prevent concurrent PixelCopy frame access Keep PixelCopy, masking, compositing, and cleanup from accessing the shared bitmap concurrently. Fixes GH-5340 Co-Authored-By: Codex --- .../replay/screenshot/PixelCopyStrategy.kt | 123 ++++++++++++------ .../screenshot/PixelCopyStrategyTest.kt | 93 +++++++++++++ 2 files changed, 177 insertions(+), 39 deletions(-) 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..281b099ee79 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,8 @@ 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 cleanupScheduled = 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 +79,14 @@ internal class PixelCopyStrategy( return } + if (!frameInFlight.compareAndSet(false, true)) { + options.logger.log(DEBUG, "PixelCopyStrategy capture is already in flight, skipping") + 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,11 +106,13 @@ 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 } @@ -116,15 +127,23 @@ internal class PixelCopyStrategy( root.traverse(viewHierarchy, options.sessionReplay, options.logger, surfaceViewNodes) if (surfaceViewNodes.isNullOrEmpty()) { - executor.submit( - ReplayRunnable("screenshot_recorder.mask") { - applyMaskingAndNotify( - root, - viewHierarchy, - resetUnstableCaptures = !changedDuringCapture, - ) - } - ) + 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. @@ -143,6 +162,7 @@ internal class PixelCopyStrategy( options.logger.log(WARNING, "Failed to capture replay recording", e) unstableCaptures.set(0) lastCaptureSuccessful.set(false) + finishFrame() } } @@ -272,37 +292,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 +351,7 @@ internal class PixelCopyStrategy( } override fun emitLastScreenshot() { - if (lastCaptureSuccessful() && !screenshot.isRecycled) { + if (!frameInFlight.get() && lastCaptureSuccessful() && !screenshot.isRecycled) { screenshotRecorderCallback?.onScreenshotRecorded(screenshot) } } @@ -330,6 +359,22 @@ internal class PixelCopyStrategy( override fun close() { isClosed.set(true) unstableCaptures.set(0) + if (!frameInFlight.get()) { + scheduleCleanup() + } + } + + private fun finishFrame() { + frameInFlight.set(false) + if (isClosed.get()) { + scheduleCleanup() + } + } + + private fun scheduleCleanup() { + if (!cleanupScheduled.compareAndSet(false, true)) { + return + } executor.submit( ReplayRunnable( "PixelCopyStrategy.close", 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..14e0706407f 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,7 @@ 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 java.util.concurrent.Future import java.util.concurrent.ScheduledExecutorService import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicReference @@ -140,6 +141,98 @@ class PixelCopyStrategyTest { if (failure.get() != null) throw failure.get() } + @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) + 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 `capture skips the first unstable PixelCopy result`() { From d6e09451728d85a88447a5443bc46c87eea328fc Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 22 Jul 2026 12:50:13 +0200 Subject: [PATCH 02/10] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d37da44d4b..48ce2126e61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixes +- Prevent concurrent PixelCopy access during Session Replay masking and bitmap cleanup ([#5808](https://github.com/getsentry/sentry-java/pull/5808)) - 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)) - Prevent a `StackOverflowError` when a `beforeSend`, `beforeBreadcrumb`, `beforeSendLog`, or `beforeEnvelope` callback triggers another capture (directly or through a logging integration such as Timber) ([#5737](https://github.com/getsentry/sentry-java/pull/5737)) From 98aeb0ce87071baa3dc049ed953bfceae9065244 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 22 Jul 2026 13:57:05 +0200 Subject: [PATCH 03/10] fix(android-replay): Preserve pending PixelCopy capture Re-arm the recorder's content-change gate when a capture is skipped because another frame is still in flight. This ensures the latest UI state is retried on the next capture interval. Refs GH-5340 Co-Authored-By: OpenAI Codex --- .../io/sentry/android/replay/screenshot/PixelCopyStrategy.kt | 1 + .../sentry/android/replay/screenshot/PixelCopyStrategyTest.kt | 3 +++ 2 files changed, 4 insertions(+) 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 281b099ee79..00a64dbfadc 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 @@ -81,6 +81,7 @@ internal class PixelCopyStrategy( if (!frameInFlight.compareAndSet(false, true)) { options.logger.log(DEBUG, "PixelCopyStrategy capture is already in flight, skipping") + markContentChanged() return } 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 14e0706407f..834998da707 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 @@ -151,6 +151,9 @@ class PixelCopyStrategyTest { strategy.capture(root) strategy.capture(root) + + assertTrue(fixture.contentChangedMarked.get()) + DeferredWindowPixelCopyShadow.flush() shadowOf(Looper.getMainLooper()).idle() From b5bf05faeed67d1a46d2774631aa5c48b6f33333 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 22 Jul 2026 17:46:18 +0200 Subject: [PATCH 04/10] fix(replay): Release PixelCopy frame gate and cleanup on failure paths Two bugbot findings on #5808: - Frame gate stuck on callback errors: viewhierarchy traversal or captureSurfaceViews throwing between the PixelCopy success check and the executor submit left frameInFlight = true forever, silently wedging all future captures. Wrap the post-success block in a try/finally that releases the gate unless work was successfully handed off. - Cleanup lost after executor shutdown: close() typically runs after ReplayIntegration has shut down the replay executor, so submit() returns null and the bitmap + maskRenderer were never released. Fall back to running cleanup inline in that case. --- .../replay/screenshot/PixelCopyStrategy.kt | 88 +++++++++++-------- .../screenshot/PixelCopyStrategyTest.kt | 36 ++++++++ 2 files changed, 88 insertions(+), 36 deletions(-) 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 00a64dbfadc..c3ab6765359 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 @@ -117,44 +117,56 @@ internal class PixelCopyStrategy( 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()) { - val submitted = - executor.submit( - ReplayRunnable("screenshot_recorder.mask") { - try { - applyMaskingAndNotify( - root, - viewHierarchy, - resetUnstableCaptures = !changedDuringCapture, - ) - } finally { - finishFrame() + // Ensure the frame gate is always released if anything below throws before we hand + // work off to the executor — otherwise a single failure wedges captures forever. + var handedOff = false + 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) { + handedOff = true + } + } 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, ) - if (submitted == null) { + handedOff = true + } + } catch (e: Throwable) { + options.logger.log(WARNING, "Failed to process replay frame", e) + } finally { + if (!handedOff) { 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, - ) } }, mainLooperHandler.handler, @@ -376,7 +388,7 @@ internal class PixelCopyStrategy( if (!cleanupScheduled.compareAndSet(false, true)) { return } - executor.submit( + val cleanup = ReplayRunnable( "PixelCopyStrategy.close", { @@ -390,7 +402,11 @@ internal class PixelCopyStrategy( maskRenderer.close() }, ) - ) + // close() typically runs after ReplayIntegration has shut down the executor, so submit may + // return null. Fall back to running cleanup inline so the bitmap + mask renderer are freed. + if (executor.submit(cleanup) == null) { + cleanup.run() + } } } 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 834998da707..9802c3d9df0 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 @@ -236,6 +236,42 @@ class PixelCopyStrategyTest { 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 @Config(shadows = [DeferredWindowPixelCopyShadow::class]) fun `capture skips the first unstable PixelCopy result`() { From aee9b3073528ab61dea72aba742c797d7563c273 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 22 Jul 2026 17:56:53 +0200 Subject: [PATCH 05/10] chore(replay): Drop redundant handedOff flag in capture callback The mask branch already knows via 'submitted == null' whether the executor took ownership; the surface-view branch always hands off. Flatten to explicit finishFrame() calls on the two failure paths (mask null-submit, outer catch) instead of tracking handoff state across a try/finally. --- .../replay/screenshot/PixelCopyStrategy.kt | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) 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 c3ab6765359..215ec31a261 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 @@ -117,9 +117,8 @@ internal class PixelCopyStrategy( return@request } - // Ensure the frame gate is always released if anything below throws before we hand - // work off to the executor — otherwise a single failure wedges captures forever. - var handedOff = false + // 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) @@ -146,8 +145,8 @@ internal class PixelCopyStrategy( } } ) - if (submitted != null) { - handedOff = true + if (submitted == null) { + finishFrame() } } else { // Re-arm the recorder's contentChanged gate; SurfaceView redraws don't trigger @@ -159,14 +158,10 @@ internal class PixelCopyStrategy( viewHierarchy, resetUnstableCaptures = !changedDuringCapture, ) - handedOff = true } } catch (e: Throwable) { options.logger.log(WARNING, "Failed to process replay frame", e) - } finally { - if (!handedOff) { - finishFrame() - } + finishFrame() } }, mainLooperHandler.handler, From 241129ab901df3339cb4e371b40275ee6a92ea1f Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 22 Jul 2026 20:54:58 +0200 Subject: [PATCH 06/10] fix(replay): Distinguish inline execution from executor rejection ReplayExecutorService.submit previously returned null both when the caller was on the worker thread (task ran inline) and when the executor rejected the submission (task did NOT run). Callers had no way to tell them apart. Return a CompletedFuture sentinel for inline execution; null now means only rejection. Also narrow PixelCopyStrategy's frame-processing catch from Throwable to RuntimeException so OOM/LinkageError still propagate. --- .../replay/screenshot/PixelCopyStrategy.kt | 10 ++++--- .../replay/util/ReplayExecutorService.kt | 27 +++++++++++++++++-- 2 files changed, 32 insertions(+), 5 deletions(-) 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 215ec31a261..f1a4ed80fe1 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 @@ -159,7 +159,10 @@ internal class PixelCopyStrategy( resetUnstableCaptures = !changedDuringCapture, ) } - } catch (e: Throwable) { + } 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() } @@ -397,8 +400,9 @@ internal class PixelCopyStrategy( maskRenderer.close() }, ) - // close() typically runs after ReplayIntegration has shut down the executor, so submit may - // return null. Fall back to running cleanup inline so the bitmap + mask renderer are freed. + // 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) {} +} From 68f4e8b9aaf89bc786d516c6055bf48e59bd7a46 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 22 Jul 2026 20:58:04 +0200 Subject: [PATCH 07/10] chore(replay): Drop redundant cleanupScheduled guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cleanup body is already idempotent (screenshot.isRecycled check, MaskRenderer.close guards on isInitialized + isRecycled). A stray extra scheduleCleanup would just submit a no-op — not worth the AtomicBoolean. --- .../io/sentry/android/replay/screenshot/PixelCopyStrategy.kt | 4 ---- 1 file changed, 4 deletions(-) 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 f1a4ed80fe1..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 @@ -60,7 +60,6 @@ internal class PixelCopyStrategy( private val unstableCaptures = AtomicInteger(0) private val isClosed = AtomicBoolean(false) private val frameInFlight = AtomicBoolean(false) - private val cleanupScheduled = AtomicBoolean(false) private val dstOverPaint by lazy(NONE) { Paint().apply { xfermode = PorterDuffXfermode(PorterDuff.Mode.DST_OVER) } } private val screenshotCanvas by lazy(NONE) { Canvas(screenshot) } @@ -383,9 +382,6 @@ internal class PixelCopyStrategy( } private fun scheduleCleanup() { - if (!cleanupScheduled.compareAndSet(false, true)) { - return - } val cleanup = ReplayRunnable( "PixelCopyStrategy.close", From 08d009a065613b4558ea6d10a926b37fb3bc6f38 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Thu, 23 Jul 2026 00:00:21 +0200 Subject: [PATCH 08/10] test(replay): Fix recursive close and assert masking is skipped on close race The mock executor closed the strategy on every submit(); since close() itself submits the cleanup task, this recursed close() -> scheduleCleanup() -> submit() until the stack overflowed. Close only when the mask task is submitted. The prior "does not crash" assertion was also vacuous under the frameInFlight gate (passed even with the isClosed guard removed). Assert instead that no screenshot is emitted once close() races masking, which fails if the guard in applyMaskingAndNotify is removed. Co-Authored-By: Claude Opus 4.8 --- .../replay/screenshot/PixelCopyStrategyTest.kt | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) 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 9802c3d9df0..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,7 @@ 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 @@ -113,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) { @@ -139,6 +145,9 @@ 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 From bc8d24203869d78ac2c7c54ce7888a6cdfc19a9c Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Thu, 23 Jul 2026 11:38:45 +0200 Subject: [PATCH 09/10] fix(replay): Guard finishFrame cleanup with CAS to avoid recycling a bitmap a new capture is using MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit finishFrame cleared frameInFlight before checking isClosed, so a new capture could take the gate and start PixelCopy into the shared screenshot while the old finishFrame went on to scheduleCleanup after close() flipped isClosed — recycling the bitmap mid-write. Re-take the gate with compareAndSet before cleaning up so the losing frame backs off. Co-Authored-By: Claude Opus 4.8 --- .../replay/screenshot/PixelCopyStrategy.kt | 6 +++- .../screenshot/PixelCopyStrategyTest.kt | 34 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) 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 49686b3851a..95f5e3b1ff2 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 @@ -376,7 +376,11 @@ internal class PixelCopyStrategy( private fun finishFrame() { frameInFlight.set(false) - if (isClosed.get()) { + // Only clean up if we're closed AND can re-take the gate. If a new capture slipped in after we + // released it (its CAS won), that frame now owns the shared screenshot; its own finishFrame + // will + // clean up. Backing off here avoids recycling the bitmap while that capture is still using it. + if (isClosed.get() && frameInFlight.compareAndSet(false, true)) { scheduleCleanup() } } 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 73371277615..9e960501730 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 @@ -245,6 +245,40 @@ class PixelCopyStrategyTest { verify(executor).submit(any()) } + @Test + @Config(shadows = [DeferredWindowPixelCopyShadow::class]) + fun `close-triggered cleanup keeps the frame gate so a racing capture cannot double-clean up`() { + // Guards the CAS handoff in finishFrame(). The real race is a 3-thread interleave (a new + // capture takes the gate the instant finishFrame releases it, then the old finishFrame recycles + // the bitmap the new capture is writing) and isn't deterministically reproducible single- + // threaded. This exercises its observable invariant instead: when finishFrame cleans up on + // close, it must re-take the gate (frameInFlight stays held), so any later capture is dropped + // rather than sneaking through to schedule a *second* cleanup on the shared screenshot. + // Without the CAS (plain frameInFlight.set(false)) the gate is left free and the follow-up + // capture reaches the isClosed guard and schedules cleanup again -> 2 submits. + val activity = buildActivity(SimpleActivity::class.java).setup() + shadowOf(Looper.getMainLooper()).idle() + val root = activity.get().findViewById(android.R.id.content) + val executor = mock() + whenever(executor.submit(any())).thenReturn(mock>()) + val strategy = fixture.getSut(executor) + + strategy.capture(root) + strategy.close() // in-flight -> cleanup deferred, no submit yet + + // PixelCopy completes; the callback sees isClosed and runs finishFrame -> the one cleanup. + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + // A capture racing in after close must be dropped (gate still held), not schedule cleanup + // again. + strategy.capture(root) + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + verify(executor, times(1)).submit(any()) + } + @Test @Config(shadows = [DeferredWindowPixelCopyShadow::class]) fun `frame gate is released when masking submit is rejected`() { From f63aff6b995ebf7778a884e4307134c7823ce306 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Thu, 23 Jul 2026 12:13:56 +0200 Subject: [PATCH 10/10] fix(replay): Make close's idle cleanup claim the gate atomically close() checked !frameInFlight.get() non-atomically before scheduleCleanup, so a capture racing in after the check could take the gate, see isClosed, and have its finishFrame schedule a second cleanup. Extract the shared "claim the gate, then the winner cleans up once" invariant into cleanUpIfIdle() so close() and finishFrame() use the same CAS. Co-Authored-By: Claude Opus 4.8 --- .../replay/screenshot/PixelCopyStrategy.kt | 22 ++++++++++------- .../screenshot/PixelCopyStrategyTest.kt | 24 +++++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) 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 95f5e3b1ff2..2bd38e8447d 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 @@ -369,18 +369,24 @@ internal class PixelCopyStrategy( override fun close() { isClosed.set(true) unstableCaptures.set(0) - if (!frameInFlight.get()) { - scheduleCleanup() - } + cleanUpIfIdle() } private fun finishFrame() { frameInFlight.set(false) - // Only clean up if we're closed AND can re-take the gate. If a new capture slipped in after we - // released it (its CAS won), that frame now owns the shared screenshot; its own finishFrame - // will - // clean up. Backing off here avoids recycling the bitmap while that capture is still using it. - if (isClosed.get() && frameInFlight.compareAndSet(false, true)) { + if (isClosed.get()) { + cleanUpIfIdle() + } + } + + /** + * Schedules cleanup only for the caller that owns the gate. Whoever wins [frameInFlight]'s CAS + * (close when no frame is running, or the finishFrame of the last in-flight frame after close) + * runs cleanup exactly once; a racing capture that took the gate loses the CAS and backs off, so + * we never recycle the shared screenshot while that capture is still using it. + */ + private fun cleanUpIfIdle() { + if (frameInFlight.compareAndSet(false, true)) { scheduleCleanup() } } 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 9e960501730..0e276d3affe 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 @@ -279,6 +279,30 @@ class PixelCopyStrategyTest { verify(executor, times(1)).submit(any()) } + @Test + @Config(shadows = [DeferredWindowPixelCopyShadow::class]) + fun `idle close claims the gate so a racing capture cannot schedule a second cleanup`() { + // Mirror of the finishFrame guard, but for close()'s idle path (no frame in flight). close() + // must atomically claim the gate before scheduling cleanup; otherwise a capture racing in right + // after the check can take the gate, see isClosed, run finishFrame and schedule cleanup a + // second + // time. Both cleanups are idempotent, but a single submit is the invariant we keep uniform. + val activity = buildActivity(SimpleActivity::class.java).setup() + shadowOf(Looper.getMainLooper()).idle() + val root = activity.get().findViewById(android.R.id.content) + val executor = mock() + whenever(executor.submit(any())).thenReturn(mock>()) + val strategy = fixture.getSut(executor) + + strategy.close() // idle -> claims gate, schedules the one cleanup + // A capture landing after close must be dropped (gate held), not schedule cleanup again. + strategy.capture(root) + DeferredWindowPixelCopyShadow.flush() + shadowOf(Looper.getMainLooper()).idle() + + verify(executor, times(1)).submit(any()) + } + @Test @Config(shadows = [DeferredWindowPixelCopyShadow::class]) fun `frame gate is released when masking submit is rejected`() {