Skip to content

Commit 809d0ae

Browse files
romtsnclaude
andcommitted
fix(replay): Guard finishFrame cleanup with CAS to avoid recycling a bitmap a new capture is using
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 <noreply@anthropic.com>
1 parent ecc8a41 commit 809d0ae

2 files changed

Lines changed: 39 additions & 1 deletion

File tree

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,11 @@ internal class PixelCopyStrategy(
376376

377377
private fun finishFrame() {
378378
frameInFlight.set(false)
379-
if (isClosed.get()) {
379+
// Only clean up if we're closed AND can re-take the gate. If a new capture slipped in after we
380+
// released it (its CAS won), that frame now owns the shared screenshot; its own finishFrame
381+
// will
382+
// clean up. Backing off here avoids recycling the bitmap while that capture is still using it.
383+
if (isClosed.get() && frameInFlight.compareAndSet(false, true)) {
380384
scheduleCleanup()
381385
}
382386
}

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,40 @@ class PixelCopyStrategyTest {
245245
verify(executor).submit(any<Runnable>())
246246
}
247247

248+
@Test
249+
@Config(shadows = [DeferredWindowPixelCopyShadow::class])
250+
fun `close-triggered cleanup keeps the frame gate so a racing capture cannot double-clean up`() {
251+
// Guards the CAS handoff in finishFrame(). The real race is a 3-thread interleave (a new
252+
// capture takes the gate the instant finishFrame releases it, then the old finishFrame recycles
253+
// the bitmap the new capture is writing) and isn't deterministically reproducible single-
254+
// threaded. This exercises its observable invariant instead: when finishFrame cleans up on
255+
// close, it must re-take the gate (frameInFlight stays held), so any later capture is dropped
256+
// rather than sneaking through to schedule a *second* cleanup on the shared screenshot.
257+
// Without the CAS (plain frameInFlight.set(false)) the gate is left free and the follow-up
258+
// capture reaches the isClosed guard and schedules cleanup again -> 2 submits.
259+
val activity = buildActivity(SimpleActivity::class.java).setup()
260+
shadowOf(Looper.getMainLooper()).idle()
261+
val root = activity.get().findViewById<View>(android.R.id.content)
262+
val executor = mock<ScheduledExecutorService>()
263+
whenever(executor.submit(any<Runnable>())).thenReturn(mock<Future<*>>())
264+
val strategy = fixture.getSut(executor)
265+
266+
strategy.capture(root)
267+
strategy.close() // in-flight -> cleanup deferred, no submit yet
268+
269+
// PixelCopy completes; the callback sees isClosed and runs finishFrame -> the one cleanup.
270+
DeferredWindowPixelCopyShadow.flush()
271+
shadowOf(Looper.getMainLooper()).idle()
272+
273+
// A capture racing in after close must be dropped (gate still held), not schedule cleanup
274+
// again.
275+
strategy.capture(root)
276+
DeferredWindowPixelCopyShadow.flush()
277+
shadowOf(Looper.getMainLooper()).idle()
278+
279+
verify(executor, times(1)).submit(any<Runnable>())
280+
}
281+
248282
@Test
249283
@Config(shadows = [DeferredWindowPixelCopyShadow::class])
250284
fun `frame gate is released when masking submit is rejected`() {

0 commit comments

Comments
 (0)