Skip to content

Commit c181555

Browse files
authored
Merge branch 'main' into rz/fix/replay-checkcanrecord-deadlock
2 parents 4af06f2 + 3dd4c87 commit c181555

8 files changed

Lines changed: 508 additions & 84 deletions

File tree

.github/workflows/integration-tests-ui-critical.yml

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,22 +63,27 @@ jobs:
6363
target: google_apis
6464
channel: canary # Necessary for ATDs
6565
arch: x86_64
66+
memory: 4096
6667
- api-level: 33 # Android 13
6768
target: google_apis
6869
channel: canary # Necessary for ATDs
6970
arch: x86_64
71+
memory: 4096
7072
- api-level: 35 # Android 15
7173
target: google_apis
7274
channel: canary # Necessary for ATDs
7375
arch: x86_64
76+
memory: 4096
7477
- api-level: 36 # Android 16
7578
target: google_apis
7679
channel: canary # Necessary for ATDs
7780
arch: x86_64
81+
memory: 4096
7882
- api-level: "37.0" # Android 17; API 37 ships only as a minor-versioned image
7983
target: google_apis_ps16k # API 37 has no plain google_apis image
8084
channel: canary # Necessary for ATDs
8185
arch: x86_64
86+
memory: 8192
8287
steps:
8388
- name: Checkout code
8489
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
@@ -112,9 +117,9 @@ jobs:
112117
path: |
113118
~/.android/avd/*
114119
~/.android/adb*
115-
# Keyed on the cmdline-tools version so AVDs created by the old, broken
116-
# avdmanager are invalidated automatically.
117-
key: avd-api-${{ matrix.api-level }}-${{ matrix.arch }}-${{ matrix.target }}-tools${{ steps.cmdline-tools.outputs.version }}
120+
# Keyed on memory and the cmdline-tools version so incompatible snapshots
121+
# and AVDs created by the old, broken avdmanager are invalidated automatically.
122+
key: avd-api-${{ matrix.api-level }}-${{ matrix.arch }}-${{ matrix.target }}-memory${{ matrix.memory }}-tools${{ steps.cmdline-tools.outputs.version }}
118123

119124
- name: Create AVD and generate snapshot for caching
120125
if: steps.avd-cache.outputs.cache-hit != 'true'
@@ -127,7 +132,7 @@ jobs:
127132
force-avd-creation: false
128133
disable-animations: true
129134
disable-spellchecker: true
130-
emulator-options: -memory 4096 -no-window -gpu auto -noaudio -no-boot-anim -camera-back none
135+
emulator-options: -memory ${{ matrix.memory }} -no-window -gpu auto -noaudio -no-boot-anim -camera-back none
131136
disk-size: 4096M
132137
script: echo "Generated AVD snapshot for caching."
133138

@@ -151,7 +156,7 @@ jobs:
151156
force-avd-creation: false
152157
disable-animations: true
153158
disable-spellchecker: true
154-
emulator-options: -memory 4096 -no-window -gpu auto -noaudio -no-boot-anim -camera-back none -no-snapshot-save
159+
emulator-options: -memory ${{ matrix.memory }} -no-window -gpu auto -noaudio -no-boot-anim -camera-back none -no-snapshot-save
155160
script: |
156161
adb uninstall io.sentry.uitest.android.critical || echo "Already uninstalled (or not found)"
157162
adb install -r -d "${{env.APK_NAME}}"

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,13 @@
99
### Fixes
1010

1111
- Fix potential ANR/deadlock in Session Replay when `checkCanRecord` runs on the replay executor thread ([#5837](https://github.com/getsentry/sentry-java/pull/5837))
12+
- Prevent concurrent PixelCopy access during Session Replay masking and bitmap cleanup ([#5808](https://github.com/getsentry/sentry-java/pull/5808))
1213
- Release `MediaMuxer` when the replay video encoder fails to start to avoid a resource leak ([#5607](https://github.com/getsentry/sentry-java/pull/5607))
1314

1415
### Performance
1516

1617
- Reduce the number of SDK threads: `LifecycleWatcher` now schedules the session-end task on the shared timer executor instead of creating a dedicated `java.util.Timer` thread ([#5819](https://github.com/getsentry/sentry-java/pull/5819))
18+
- Speed up deserialization of arbitrary JSON objects by typing numbers without throwing exceptions ([#5783](https://github.com/getsentry/sentry-java/pull/5783))
1719

1820
## 8.50.1
1921

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

Lines changed: 147 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,57 @@ internal class PixelCopyStrategy(
322361
}
323362

324363
override fun emitLastScreenshot() {
325-
if (lastCaptureSuccessful() && !screenshot.isRecycled) {
326-
screenshotRecorderCallback?.onScreenshotRecorded(screenshot)
364+
if (!frameInFlight.compareAndSet(false, true)) {
365+
return
366+
}
367+
if (!lastCaptureSuccessful() || screenshot.isRecycled) {
368+
finishFrame()
369+
return
370+
}
371+
// Submit to the executor so the downstream consumer's bitmap read (JPEG compress) runs inline
372+
// on the worker thread while the gate is held, same as the masked capture path.
373+
val submitted =
374+
executor.submit(
375+
ReplayRunnable("PixelCopyStrategy.emit") {
376+
try {
377+
screenshotRecorderCallback?.onScreenshotRecorded(screenshot)
378+
} finally {
379+
finishFrame()
380+
}
381+
}
382+
)
383+
if (submitted == null) {
384+
finishFrame()
327385
}
328386
}
329387

330388
override fun close() {
331389
isClosed.set(true)
332390
unstableCaptures.set(0)
333-
executor.submit(
391+
cleanUpIfIdle()
392+
}
393+
394+
private fun finishFrame() {
395+
frameInFlight.set(false)
396+
if (isClosed.get()) {
397+
cleanUpIfIdle()
398+
}
399+
}
400+
401+
/**
402+
* Schedules cleanup only for the caller that owns the gate. Whoever wins [frameInFlight]'s CAS
403+
* (close when no frame is running, or the finishFrame of the last in-flight frame after close)
404+
* runs cleanup exactly once; a racing capture that took the gate loses the CAS and backs off, so
405+
* we never recycle the shared screenshot while that capture is still using it.
406+
*/
407+
private fun cleanUpIfIdle() {
408+
if (frameInFlight.compareAndSet(false, true)) {
409+
scheduleCleanup()
410+
}
411+
}
412+
413+
private fun scheduleCleanup() {
414+
val cleanup =
334415
ReplayRunnable(
335416
"PixelCopyStrategy.close",
336417
{
@@ -344,7 +425,12 @@ internal class PixelCopyStrategy(
344425
maskRenderer.close()
345426
},
346427
)
347-
)
428+
// ReplayExecutorService.submit returns null only on genuine rejection (post-shutdown);
429+
// inline execution on the worker thread returns a completed future. Fall back to running
430+
// cleanup here so the bitmap + mask renderer are freed even when the executor is dead.
431+
if (executor.submit(cleanup) == null) {
432+
cleanup.run()
433+
}
348434
}
349435
}
350436

0 commit comments

Comments
 (0)