Skip to content

Commit 63eb6f1

Browse files
romtsnclaude
andcommitted
fix(replay): Hold frame gate across emitLastScreenshot to prevent concurrent bitmap access
emitLastScreenshot runs on the main thread, so the downstream consumer queues its bitmap read (JPEG compress) to the executor asynchronously. Without the gate, the next capture tick's PixelCopy.request writes into the shared screenshot while the queued read is still in flight. Submit the consumer call to the executor so it runs inline on the worker thread while the gate is held, same pattern as the masked capture path. Also fixes the close-race test mock to return CompletedFuture instead of null, matching the executor contract (same fix as inlineExecutor in cd3a7c0). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 07af15e commit 63eb6f1

2 files changed

Lines changed: 75 additions & 3 deletions

File tree

sentry-android-replay/src/main/java/io/sentry/android/replay/screenshot/PixelCopyStrategy.kt

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -361,8 +361,27 @@ internal class PixelCopyStrategy(
361361
}
362362

363363
override fun emitLastScreenshot() {
364-
if (!frameInFlight.get() && lastCaptureSuccessful() && !screenshot.isRecycled) {
365-
screenshotRecorderCallback?.onScreenshotRecorded(screenshot)
364+
if (!frameInFlight.compareAndSet(false, true)) {
365+
return
366+
}
367+
if (!lastCaptureSuccessful() || screenshot.isRecycled) {
368+
finishFrame()
369+
return
370+
}
371+
// Submit to the executor so the downstream consumer's bitmap read (JPEG compress) runs inline
372+
// on the worker thread while the gate is held, same as the masked capture path.
373+
val submitted =
374+
executor.submit(
375+
ReplayRunnable("PixelCopyStrategy.emit") {
376+
try {
377+
screenshotRecorderCallback?.onScreenshotRecorded(screenshot)
378+
} finally {
379+
finishFrame()
380+
}
381+
}
382+
)
383+
if (submitted == null) {
384+
finishFrame()
366385
}
367386
}
368387

sentry-android-replay/src/test/java/io/sentry/android/replay/screenshot/PixelCopyStrategyTest.kt

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ class PixelCopyStrategyTest {
141141
// PixelCopyStrategy swallows the exception, so we have to capture it here and rethrow later
142142
failure.set(e)
143143
}
144-
null
144+
CompletedFuture
145145
}
146146

147147
strategy = fixture.getSut(executor = executorThatClosesFirst)
@@ -229,6 +229,59 @@ class PixelCopyStrategyTest {
229229
verify(fixture.callback, times(2)).onScreenshotRecorded(any<Bitmap>())
230230
}
231231

232+
@Test
233+
@Config(shadows = [DeferredWindowPixelCopyShadow::class])
234+
fun `emitLastScreenshot holds the frame gate until the emit task drains`() {
235+
// emit submits the consumer call to the executor so the bitmap read (JPEG compress) runs
236+
// inline on the worker thread while the gate is held — same pattern as the masked capture path.
237+
// Invariant: while the emit task is still queued (gate held), a racing capture is dropped.
238+
// Without the gate (old `if (!frameInFlight.get())`) that capture proceeds -> extra frame.
239+
val activity = buildActivity(SimpleActivity::class.java).setup()
240+
shadowOf(Looper.getMainLooper()).idle()
241+
val root = activity.get().findViewById<View>(android.R.id.content)
242+
val tasks = mutableListOf<Runnable>()
243+
val executor = mock<ScheduledExecutorService>()
244+
whenever(executor.submit(any<Runnable>())).doAnswer {
245+
tasks.add(it.arguments[0] as Runnable)
246+
mock<Future<*>>()
247+
}
248+
val strategy = fixture.getSut(executor)
249+
250+
// Set up a successful last capture: capture -> queued mask task -> drain releases the gate.
251+
strategy.capture(root)
252+
DeferredWindowPixelCopyShadow.flush()
253+
shadowOf(Looper.getMainLooper()).idle()
254+
tasks.removeAll {
255+
it.run()
256+
true
257+
}
258+
verify(fixture.callback, times(1)).onScreenshotRecorded(any<Bitmap>())
259+
260+
// Emit takes the gate and queues the consumer task (still pending).
261+
strategy.emitLastScreenshot()
262+
// Callback hasn't fired yet — the task is queued, not drained.
263+
verify(fixture.callback, times(1)).onScreenshotRecorded(any<Bitmap>())
264+
265+
// A capture racing in before the emit task drains must be dropped (gate held).
266+
strategy.capture(root)
267+
DeferredWindowPixelCopyShadow.flush()
268+
shadowOf(Looper.getMainLooper()).idle()
269+
verify(fixture.callback, times(1)).onScreenshotRecorded(any<Bitmap>())
270+
271+
// Drain the emit task -> callback fires, gate released -> captures resume.
272+
tasks.removeAll {
273+
it.run()
274+
true
275+
}
276+
verify(fixture.callback, times(2)).onScreenshotRecorded(any<Bitmap>())
277+
captureStableFrame(strategy, root)
278+
tasks.removeAll {
279+
it.run()
280+
true
281+
}
282+
verify(fixture.callback, times(3)).onScreenshotRecorded(any<Bitmap>())
283+
}
284+
232285
@Test
233286
@Config(shadows = [DeferredWindowPixelCopyShadow::class])
234287
fun `close defers cleanup until PixelCopy completes`() {

0 commit comments

Comments
 (0)