Skip to content

Commit fcda53a

Browse files
romtsnromtsn
authored
Merge b5bf05f into f210d11 
2 parents f210d11 + b5bf05f commit fcda53a

3 files changed

Lines changed: 255 additions & 60 deletions

File tree

‎CHANGELOG.md‎

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

99
### Fixes
1010

11+
- Prevent concurrent PixelCopy access during Session Replay masking and bitmap cleanup ([#5808](https://github.com/getsentry/sentry-java/pull/5808))
1112
- 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))
1213
- 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))
1314
- `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: 122 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ 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)
63+
private val cleanupScheduled = AtomicBoolean(false)
6264
private val dstOverPaint by
6365
lazy(NONE) { Paint().apply { xfermode = PorterDuffXfermode(PorterDuff.Mode.DST_OVER) } }
6466
private val screenshotCanvas by lazy(NONE) { Canvas(screenshot) }
@@ -77,8 +79,15 @@ internal class PixelCopyStrategy(
7779
return
7880
}
7981

82+
if (!frameInFlight.compareAndSet(false, true)) {
83+
options.logger.log(DEBUG, "PixelCopyStrategy capture is already in flight, skipping")
84+
markContentChanged()
85+
return
86+
}
87+
8088
if (isClosed.get()) {
8189
options.logger.log(DEBUG, "PixelCopyStrategy is closed, not capturing screenshot")
90+
finishFrame()
8291
return
8392
}
8493

@@ -90,51 +99,74 @@ internal class PixelCopyStrategy(
9099
{ copyResult: Int ->
91100
if (isClosed.get()) {
92101
options.logger.log(DEBUG, "PixelCopyStrategy is closed, ignoring capture result")
102+
finishFrame()
93103
return@request
94104
}
95105

96106
if (copyResult != PixelCopy.SUCCESS) {
97107
options.logger.log(INFO, "Failed to capture replay recording: %d", copyResult)
98108
unstableCaptures.set(0)
99109
lastCaptureSuccessful.set(false)
110+
finishFrame()
100111
return@request
101112
}
102113

103114
val changedDuringCapture = contentChanged.get()
104115
if (changedDuringCapture && shouldSkipUnstableCapture()) {
116+
finishFrame()
105117
return@request
106118
}
107119

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,
120+
// Ensure the frame gate is always released if anything below throws before we hand
121+
// work off to the executor — otherwise a single failure wedges captures forever.
122+
var handedOff = false
123+
try {
124+
// TODO: disableAllMasking here and dont traverse?
125+
val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options.sessionReplay)
126+
val surfaceViewNodes =
127+
if (options.sessionReplay.isCaptureSurfaceViews) {
128+
mutableListOf<ViewHierarchyNode.SurfaceViewHierarchyNode>()
129+
} else {
130+
null
131+
}
132+
root.traverse(viewHierarchy, options.sessionReplay, options.logger, surfaceViewNodes)
133+
134+
if (surfaceViewNodes.isNullOrEmpty()) {
135+
val submitted =
136+
executor.submit(
137+
ReplayRunnable("screenshot_recorder.mask") {
138+
try {
139+
applyMaskingAndNotify(
140+
root,
141+
viewHierarchy,
142+
resetUnstableCaptures = !changedDuringCapture,
143+
)
144+
} finally {
145+
finishFrame()
146+
}
147+
}
125148
)
149+
if (submitted != null) {
150+
handedOff = true
126151
}
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-
)
152+
} else {
153+
// Re-arm the recorder's contentChanged gate; SurfaceView redraws don't trigger
154+
// ViewTreeObserver.OnDrawListener, so we'd otherwise emit the same frame forever.
155+
markContentChanged()
156+
captureSurfaceViews(
157+
root,
158+
surfaceViewNodes,
159+
viewHierarchy,
160+
resetUnstableCaptures = !changedDuringCapture,
161+
)
162+
handedOff = true
163+
}
164+
} catch (e: Throwable) {
165+
options.logger.log(WARNING, "Failed to process replay frame", e)
166+
} finally {
167+
if (!handedOff) {
168+
finishFrame()
169+
}
138170
}
139171
},
140172
mainLooperHandler.handler,
@@ -143,6 +175,7 @@ internal class PixelCopyStrategy(
143175
options.logger.log(WARNING, "Failed to capture replay recording", e)
144176
unstableCaptures.set(0)
145177
lastCaptureSuccessful.set(false)
178+
finishFrame()
146179
}
147180
}
148181

@@ -272,37 +305,46 @@ internal class PixelCopyStrategy(
272305
windowY: Int,
273306
resetUnstableCaptures: Boolean,
274307
) {
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-
}
308+
val submitted =
309+
executor.submit(
310+
ReplayRunnable("screenshot_recorder.composite") {
311+
try {
312+
if (isClosed.get() || screenshot.isRecycled) {
313+
options.logger.log(DEBUG, "PixelCopyStrategy is closed, skipping compositing")
314+
recycleCaptures(captures)
315+
return@ReplayRunnable
316+
}
282317

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-
}
318+
for (capture in captures) {
319+
if (capture == null) continue
320+
if (capture.bitmap.isRecycled) continue
321+
322+
compositeSurfaceViewInto(
323+
screenshotCanvas,
324+
dstOverPaint,
325+
tmpSrcRect,
326+
tmpDstRect,
327+
capture.bitmap,
328+
capture.x,
329+
capture.y,
330+
windowX,
331+
windowY,
332+
config.scaleFactorX,
333+
config.scaleFactorY,
334+
)
335+
capture.bitmap.recycle()
336+
}
302337

303-
applyMaskingAndNotify(root, viewHierarchy, resetUnstableCaptures)
304-
}
305-
)
338+
applyMaskingAndNotify(root, viewHierarchy, resetUnstableCaptures)
339+
} finally {
340+
finishFrame()
341+
}
342+
}
343+
)
344+
if (submitted == null) {
345+
recycleCaptures(captures)
346+
finishFrame()
347+
}
306348
}
307349

308350
private fun recycleCaptures(captures: Array<SurfaceViewCapture?>) {
@@ -322,15 +364,31 @@ internal class PixelCopyStrategy(
322364
}
323365

324366
override fun emitLastScreenshot() {
325-
if (lastCaptureSuccessful() && !screenshot.isRecycled) {
367+
if (!frameInFlight.get() && lastCaptureSuccessful() && !screenshot.isRecycled) {
326368
screenshotRecorderCallback?.onScreenshotRecorded(screenshot)
327369
}
328370
}
329371

330372
override fun close() {
331373
isClosed.set(true)
332374
unstableCaptures.set(0)
333-
executor.submit(
375+
if (!frameInFlight.get()) {
376+
scheduleCleanup()
377+
}
378+
}
379+
380+
private fun finishFrame() {
381+
frameInFlight.set(false)
382+
if (isClosed.get()) {
383+
scheduleCleanup()
384+
}
385+
}
386+
387+
private fun scheduleCleanup() {
388+
if (!cleanupScheduled.compareAndSet(false, true)) {
389+
return
390+
}
391+
val cleanup =
334392
ReplayRunnable(
335393
"PixelCopyStrategy.close",
336394
{
@@ -344,7 +402,11 @@ internal class PixelCopyStrategy(
344402
maskRenderer.close()
345403
},
346404
)
347-
)
405+
// close() typically runs after ReplayIntegration has shut down the executor, so submit may
406+
// return null. Fall back to running cleanup inline so the bitmap + mask renderer are freed.
407+
if (executor.submit(cleanup) == null) {
408+
cleanup.run()
409+
}
348410
}
349411
}
350412

0 commit comments

Comments
 (0)