Skip to content

Commit 52011c1

Browse files
romtsnclaude
andcommitted
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 <noreply@anthropic.com>
1 parent 809d0ae commit 52011c1

2 files changed

Lines changed: 38 additions & 8 deletions

File tree

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

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -369,18 +369,24 @@ internal class PixelCopyStrategy(
369369
override fun close() {
370370
isClosed.set(true)
371371
unstableCaptures.set(0)
372-
if (!frameInFlight.get()) {
373-
scheduleCleanup()
374-
}
372+
cleanUpIfIdle()
375373
}
376374

377375
private fun finishFrame() {
378376
frameInFlight.set(false)
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)) {
377+
if (isClosed.get()) {
378+
cleanUpIfIdle()
379+
}
380+
}
381+
382+
/**
383+
* Schedules cleanup only for the caller that owns the gate. Whoever wins [frameInFlight]'s CAS
384+
* (close when no frame is running, or the finishFrame of the last in-flight frame after close)
385+
* runs cleanup exactly once; a racing capture that took the gate loses the CAS and backs off, so
386+
* we never recycle the shared screenshot while that capture is still using it.
387+
*/
388+
private fun cleanUpIfIdle() {
389+
if (frameInFlight.compareAndSet(false, true)) {
384390
scheduleCleanup()
385391
}
386392
}

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,30 @@ class PixelCopyStrategyTest {
279279
verify(executor, times(1)).submit(any<Runnable>())
280280
}
281281

282+
@Test
283+
@Config(shadows = [DeferredWindowPixelCopyShadow::class])
284+
fun `idle close claims the gate so a racing capture cannot schedule a second cleanup`() {
285+
// Mirror of the finishFrame guard, but for close()'s idle path (no frame in flight). close()
286+
// must atomically claim the gate before scheduling cleanup; otherwise a capture racing in right
287+
// after the check can take the gate, see isClosed, run finishFrame and schedule cleanup a
288+
// second
289+
// time. Both cleanups are idempotent, but a single submit is the invariant we keep uniform.
290+
val activity = buildActivity(SimpleActivity::class.java).setup()
291+
shadowOf(Looper.getMainLooper()).idle()
292+
val root = activity.get().findViewById<View>(android.R.id.content)
293+
val executor = mock<ScheduledExecutorService>()
294+
whenever(executor.submit(any<Runnable>())).thenReturn(mock<Future<*>>())
295+
val strategy = fixture.getSut(executor)
296+
297+
strategy.close() // idle -> claims gate, schedules the one cleanup
298+
// A capture landing after close must be dropped (gate held), not schedule cleanup again.
299+
strategy.capture(root)
300+
DeferredWindowPixelCopyShadow.flush()
301+
shadowOf(Looper.getMainLooper()).idle()
302+
303+
verify(executor, times(1)).submit(any<Runnable>())
304+
}
305+
282306
@Test
283307
@Config(shadows = [DeferredWindowPixelCopyShadow::class])
284308
fun `frame gate is released when masking submit is rejected`() {

0 commit comments

Comments
 (0)