Skip to content

Commit 13ff16d

Browse files
romtsnromtsn
authored
Merge 447c665 into 6335e35 
2 parents 6335e35 + 447c665 commit 13ff16d

4 files changed

Lines changed: 429 additions & 68 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
### Fixes
3030

31+
- Prevent concurrent PixelCopy access during Session Replay masking and bitmap cleanup ([#5808](https://github.com/getsentry/sentry-java/pull/5808))
3132
- Reduce main-thread work during `Sentry.init` by resolving the shake-detector accelerometer off the main thread (~1.75ms on a Pixel 10) ([#5784](https://github.com/getsentry/sentry-java/pull/5784))
3233
- 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))
3334
- `SentryTagModifierNode.isImportantForBounds` now matches the default behavior and returns `true` ([#5789](https://github.com/getsentry/sentry-java/pull/5789))

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

Lines changed: 142 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ internal class PixelCopyStrategy(
5959
private val contentChanged = AtomicBoolean(false)
6060
private val unstableCaptures = AtomicInteger(0)
6161
private val isClosed = AtomicBoolean(false)
62+
private val frameInFlight = AtomicBoolean(false)
6263
private val dstOverPaint by
6364
lazy(NONE) { Paint().apply { xfermode = PorterDuffXfermode(PorterDuff.Mode.DST_OVER) } }
6465
private val screenshotCanvas by lazy(NONE) { Canvas(screenshot) }
@@ -77,8 +78,15 @@ internal class PixelCopyStrategy(
7778
return
7879
}
7980

81+
if (!frameInFlight.compareAndSet(false, true)) {
82+
options.logger.log(DEBUG, "PixelCopyStrategy capture is already in flight, skipping")
83+
markContentChanged()
84+
return
85+
}
86+
8087
if (isClosed.get()) {
8188
options.logger.log(DEBUG, "PixelCopyStrategy is closed, not capturing screenshot")
89+
finishFrame()
8290
return
8391
}
8492

@@ -90,51 +98,72 @@ internal class PixelCopyStrategy(
9098
{ copyResult: Int ->
9199
if (isClosed.get()) {
92100
options.logger.log(DEBUG, "PixelCopyStrategy is closed, ignoring capture result")
101+
finishFrame()
93102
return@request
94103
}
95104

96105
if (copyResult != PixelCopy.SUCCESS) {
97106
options.logger.log(INFO, "Failed to capture replay recording: %d", copyResult)
98107
unstableCaptures.set(0)
99108
lastCaptureSuccessful.set(false)
109+
finishFrame()
100110
return@request
101111
}
102112

103113
val changedDuringCapture = contentChanged.get()
104114
if (changedDuringCapture && shouldSkipUnstableCapture()) {
115+
finishFrame()
105116
return@request
106117
}
107118

108-
// TODO: disableAllMasking here and dont traverse?
109-
val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options.sessionReplay)
110-
val surfaceViewNodes =
111-
if (options.sessionReplay.isCaptureSurfaceViews) {
112-
mutableListOf<ViewHierarchyNode.SurfaceViewHierarchyNode>()
113-
} else {
114-
null
115-
}
116-
root.traverse(viewHierarchy, options.sessionReplay, options.logger, surfaceViewNodes)
117-
118-
if (surfaceViewNodes.isNullOrEmpty()) {
119-
executor.submit(
120-
ReplayRunnable("screenshot_recorder.mask") {
121-
applyMaskingAndNotify(
122-
root,
123-
viewHierarchy,
124-
resetUnstableCaptures = !changedDuringCapture,
119+
// Release the frame gate if anything below throws before we hand work off to the
120+
// executor — otherwise a single failure wedges captures forever.
121+
try {
122+
// TODO: disableAllMasking here and dont traverse?
123+
val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options.sessionReplay)
124+
val surfaceViewNodes =
125+
if (options.sessionReplay.isCaptureSurfaceViews) {
126+
mutableListOf<ViewHierarchyNode.SurfaceViewHierarchyNode>()
127+
} else {
128+
null
129+
}
130+
root.traverse(viewHierarchy, options.sessionReplay, options.logger, surfaceViewNodes)
131+
132+
if (surfaceViewNodes.isNullOrEmpty()) {
133+
val submitted =
134+
executor.submit(
135+
ReplayRunnable("screenshot_recorder.mask") {
136+
try {
137+
applyMaskingAndNotify(
138+
root,
139+
viewHierarchy,
140+
resetUnstableCaptures = !changedDuringCapture,
141+
)
142+
} finally {
143+
finishFrame()
144+
}
145+
}
125146
)
147+
if (submitted == null) {
148+
finishFrame()
126149
}
127-
)
128-
} else {
129-
// Re-arm the recorder's contentChanged gate; SurfaceView redraws don't trigger
130-
// ViewTreeObserver.OnDrawListener, so we'd otherwise emit the same frame forever.
131-
markContentChanged()
132-
captureSurfaceViews(
133-
root,
134-
surfaceViewNodes,
135-
viewHierarchy,
136-
resetUnstableCaptures = !changedDuringCapture,
137-
)
150+
} else {
151+
// Re-arm the recorder's contentChanged gate; SurfaceView redraws don't trigger
152+
// ViewTreeObserver.OnDrawListener, so we'd otherwise emit the same frame forever.
153+
markContentChanged()
154+
captureSurfaceViews(
155+
root,
156+
surfaceViewNodes,
157+
viewHierarchy,
158+
resetUnstableCaptures = !changedDuringCapture,
159+
)
160+
}
161+
} catch (e: RuntimeException) {
162+
// OEM View subclasses have been observed throwing during hierarchy traversal
163+
// (e.g. Redmi's TextView NPE). Release the frame gate so a single bad frame
164+
// doesn't wedge the recorder. Errors (OOM, LinkageError) intentionally propagate.
165+
options.logger.log(WARNING, "Failed to process replay frame", e)
166+
finishFrame()
138167
}
139168
},
140169
mainLooperHandler.handler,
@@ -143,6 +172,7 @@ internal class PixelCopyStrategy(
143172
options.logger.log(WARNING, "Failed to capture replay recording", e)
144173
unstableCaptures.set(0)
145174
lastCaptureSuccessful.set(false)
175+
finishFrame()
146176
}
147177
}
148178

@@ -272,37 +302,46 @@ internal class PixelCopyStrategy(
272302
windowY: Int,
273303
resetUnstableCaptures: Boolean,
274304
) {
275-
executor.submit(
276-
ReplayRunnable("screenshot_recorder.composite") {
277-
if (isClosed.get() || screenshot.isRecycled) {
278-
options.logger.log(DEBUG, "PixelCopyStrategy is closed, skipping compositing")
279-
recycleCaptures(captures)
280-
return@ReplayRunnable
281-
}
305+
val submitted =
306+
executor.submit(
307+
ReplayRunnable("screenshot_recorder.composite") {
308+
try {
309+
if (isClosed.get() || screenshot.isRecycled) {
310+
options.logger.log(DEBUG, "PixelCopyStrategy is closed, skipping compositing")
311+
recycleCaptures(captures)
312+
return@ReplayRunnable
313+
}
282314

283-
for (capture in captures) {
284-
if (capture == null) continue
285-
if (capture.bitmap.isRecycled) continue
286-
287-
compositeSurfaceViewInto(
288-
screenshotCanvas,
289-
dstOverPaint,
290-
tmpSrcRect,
291-
tmpDstRect,
292-
capture.bitmap,
293-
capture.x,
294-
capture.y,
295-
windowX,
296-
windowY,
297-
config.scaleFactorX,
298-
config.scaleFactorY,
299-
)
300-
capture.bitmap.recycle()
301-
}
315+
for (capture in captures) {
316+
if (capture == null) continue
317+
if (capture.bitmap.isRecycled) continue
318+
319+
compositeSurfaceViewInto(
320+
screenshotCanvas,
321+
dstOverPaint,
322+
tmpSrcRect,
323+
tmpDstRect,
324+
capture.bitmap,
325+
capture.x,
326+
capture.y,
327+
windowX,
328+
windowY,
329+
config.scaleFactorX,
330+
config.scaleFactorY,
331+
)
332+
capture.bitmap.recycle()
333+
}
302334

303-
applyMaskingAndNotify(root, viewHierarchy, resetUnstableCaptures)
304-
}
305-
)
335+
applyMaskingAndNotify(root, viewHierarchy, resetUnstableCaptures)
336+
} finally {
337+
finishFrame()
338+
}
339+
}
340+
)
341+
if (submitted == null) {
342+
recycleCaptures(captures)
343+
finishFrame()
344+
}
306345
}
307346

308347
private fun recycleCaptures(captures: Array<SurfaceViewCapture?>) {
@@ -322,15 +361,52 @@ internal class PixelCopyStrategy(
322361
}
323362

324363
override fun emitLastScreenshot() {
325-
if (lastCaptureSuccessful() && !screenshot.isRecycled) {
326-
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()
327380
}
328381
}
329382

330383
override fun close() {
331384
isClosed.set(true)
332385
unstableCaptures.set(0)
333-
executor.submit(
386+
cleanUpIfIdle()
387+
}
388+
389+
private fun finishFrame() {
390+
frameInFlight.set(false)
391+
if (isClosed.get()) {
392+
cleanUpIfIdle()
393+
}
394+
}
395+
396+
/**
397+
* Schedules cleanup only for the caller that owns the gate. Whoever wins [frameInFlight]'s CAS
398+
* (close when no frame is running, or the finishFrame of the last in-flight frame after close)
399+
* runs cleanup exactly once; a racing capture that took the gate loses the CAS and backs off, so
400+
* we never recycle the shared screenshot while that capture is still using it.
401+
*/
402+
private fun cleanUpIfIdle() {
403+
if (frameInFlight.compareAndSet(false, true)) {
404+
scheduleCleanup()
405+
}
406+
}
407+
408+
private fun scheduleCleanup() {
409+
val cleanup =
334410
ReplayRunnable(
335411
"PixelCopyStrategy.close",
336412
{
@@ -344,7 +420,12 @@ internal class PixelCopyStrategy(
344420
maskRenderer.close()
345421
},
346422
)
347-
)
423+
// ReplayExecutorService.submit returns null only on genuine rejection (post-shutdown);
424+
// inline execution on the worker thread returns a completed future. Fall back to running
425+
// cleanup here so the bitmap + mask renderer are freed even when the executor is dead.
426+
if (executor.submit(cleanup) == null) {
427+
cleanup.run()
428+
}
348429
}
349430
}
350431

‎sentry-android-replay/src/main/java/io/sentry/android/replay/util/ReplayExecutorService.kt‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import io.sentry.SentryLevel.ERROR
44
import io.sentry.SentryOptions
55
import java.util.concurrent.Future
66
import java.util.concurrent.ScheduledExecutorService
7+
import java.util.concurrent.TimeUnit
78
import java.util.concurrent.TimeUnit.MILLISECONDS
89

910
/**
@@ -14,11 +15,20 @@ internal class ReplayExecutorService(
1415
private val delegate: ScheduledExecutorService,
1516
private val options: SentryOptions,
1617
) : ScheduledExecutorService by delegate {
18+
/**
19+
* Submits [task] for execution and returns a [Future] describing what happened. The return value
20+
* has three distinct outcomes callers can rely on:
21+
* - [CompletedFuture] — the caller is already on the replay worker thread, so the task was run
22+
* inline before this method returned. Skips the queue.
23+
* - A regular [Future] from the underlying [ScheduledExecutorService] — the task was queued and
24+
* will run asynchronously.
25+
* - `null` — the underlying executor rejected the submission (typically because it has been shut
26+
* down). The task did NOT run; callers that need cleanup must handle it themselves.
27+
*/
1728
override fun submit(task: Runnable): Future<*>? {
1829
if (Thread.currentThread().name.startsWith("SentryReplayIntegration")) {
19-
// we're already on the worker thread, no need to submit
2030
task.run()
21-
return null
31+
return CompletedFuture
2232
}
2333
return try {
2434
delegate.submit {
@@ -68,3 +78,16 @@ internal class ReplayExecutorService(
6878
}
6979

7080
internal class ReplayRunnable(val taskName: String, delegate: Runnable) : Runnable by delegate
81+
82+
/** A Future that represents an already-completed inline execution — never used as null. */
83+
internal object CompletedFuture : Future<Unit> {
84+
override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false
85+
86+
override fun isCancelled(): Boolean = false
87+
88+
override fun isDone(): Boolean = true
89+
90+
override fun get() {}
91+
92+
override fun get(timeout: Long, unit: TimeUnit) {}
93+
}

0 commit comments

Comments
 (0)