Skip to content

Commit 3ecc6a6

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. CAS the gate and release it via a fence task on the single-threaded executor (FIFO guarantees it runs after the queued read). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 07af15e commit 3ecc6a6

2 files changed

Lines changed: 69 additions & 2 deletions

File tree

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

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

363363
override fun emitLastScreenshot() {
364-
if (!frameInFlight.get() && lastCaptureSuccessful() && !screenshot.isRecycled) {
365-
screenshotRecorderCallback?.onScreenshotRecorded(screenshot)
364+
// Take the gate: emit runs on the main thread, so the downstream consumer queues its bitmap
365+
// read (JPEG compress) to the executor asynchronously instead of running it inline. Without the
366+
// gate, the next capture tick's PixelCopy.request would write into the shared screenshot while
367+
// that queued read is still in flight. Hold the gate and release it via a fence task — FIFO on
368+
// the single-threaded executor guarantees it runs after the queued read.
369+
if (!frameInFlight.compareAndSet(false, true)) {
370+
return
371+
}
372+
if (!lastCaptureSuccessful() || screenshot.isRecycled) {
373+
finishFrame()
374+
return
375+
}
376+
screenshotRecorderCallback?.onScreenshotRecorded(screenshot)
377+
val fence = ReplayRunnable("PixelCopyStrategy.emit_fence", { finishFrame() })
378+
if (executor.submit(fence) == null) {
379+
finishFrame()
366380
}
367381
}
368382

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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 consumer fence runs`() {
235+
// emit runs on the main thread, so the downstream consumer (JPEG compress reading the shared
236+
// screenshot) is queued to the executor, not run inline. emit must hold the gate across that
237+
// window and release it via a fence task queued after the consumer; otherwise the next capture
238+
// tick's PixelCopy.request writes into the bitmap while the queued read is still in flight.
239+
// Invariant: while the fence is still queued (gate held), a capture racing in is dropped.
240+
// Without the gate (old `if (!frameInFlight.get())`) that capture proceeds -> extra frame.
241+
val activity = buildActivity(SimpleActivity::class.java).setup()
242+
shadowOf(Looper.getMainLooper()).idle()
243+
val root = activity.get().findViewById<View>(android.R.id.content)
244+
val tasks = mutableListOf<Runnable>()
245+
val executor = mock<ScheduledExecutorService>()
246+
whenever(executor.submit(any<Runnable>())).doAnswer {
247+
tasks.add(it.arguments[0] as Runnable)
248+
mock<Future<*>>()
249+
}
250+
val strategy = fixture.getSut(executor)
251+
252+
// Set up a successful last capture: capture -> queued mask task -> drain releases the gate.
253+
strategy.capture(root)
254+
DeferredWindowPixelCopyShadow.flush()
255+
shadowOf(Looper.getMainLooper()).idle()
256+
tasks.removeAll {
257+
it.run()
258+
true
259+
}
260+
verify(fixture.callback, times(1)).onScreenshotRecorded(any<Bitmap>())
261+
262+
// Emit takes the gate, hands the bitmap to the consumer, and queues the fence (still pending).
263+
strategy.emitLastScreenshot()
264+
verify(fixture.callback, times(2)).onScreenshotRecorded(any<Bitmap>())
265+
266+
// A capture racing in before the fence drains must be dropped (gate held), so no new frame.
267+
strategy.capture(root)
268+
DeferredWindowPixelCopyShadow.flush()
269+
shadowOf(Looper.getMainLooper()).idle()
270+
verify(fixture.callback, times(2)).onScreenshotRecorded(any<Bitmap>())
271+
272+
// Fence drains -> gate released -> captures resume normally.
273+
tasks.removeAll {
274+
it.run()
275+
true
276+
}
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)