Skip to content

Conversation

@romtsn
Copy link
Member

@romtsn romtsn commented Nov 4, 2025

📜 Description

  • Switch to PixelCopy for capturing surface backed by the Picture in CanvasStrategy
  • Use the same background handler for both PixelCopy and pictureRenderTask to avoid thread context switch

Example replay: https://sentry-sdks.sentry.io/explore/replays/09456bd8f08c43eb862a23759d1dc640

💡 Motivation and Context

Closes #4857

💚 How did you test it?

📝 Checklist

  • I added GH Issue ID & Linear ID
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Comment on lines 104 to 110
{ result ->
if (result == PixelCopy.SUCCESS) {
lastCaptureSuccessful.set(true)
val bitmap = screenshot
if (bitmap != null && !bitmap.isRecycled) {
screenshotRecorderCallback?.onScreenshotRecorded(bitmap)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Unsynchronized access to screenshot Bitmap by multiple threads while another thread recycles it.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The screenshot Bitmap is accessed without synchronization by emitLastScreenshot() on the main thread and by the PixelCopy callback on a background handler thread. Concurrently, the close() method, executing on a thread pool, recycles the same screenshot Bitmap. This creates a data race where the Bitmap can be accessed after it has been recycled, which can lead to native crashes according to repository guidelines.

💡 Suggested Fix

Protect all accesses to the screenshot Bitmap with synchronized(bitmap) blocks to prevent concurrent access during recycling or other operations.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location:
sentry-android-replay/src/main/java/io/sentry/android/replay/screenshot/CanvasStrategy.kt#L104-L110

Potential issue: The `screenshot` Bitmap is accessed without synchronization by
`emitLastScreenshot()` on the main thread and by the PixelCopy callback on a background
handler thread. Concurrently, the `close()` method, executing on a thread pool, recycles
the same `screenshot` Bitmap. This creates a data race where the Bitmap can be accessed
after it has been recycled, which can lead to native crashes according to repository
guidelines.

Did we get this right? 👍 / 👎 to inform future reviews.

cursor[bot]

This comment was marked as outdated.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 319.29 ms 412.76 ms 93.47 ms
Size 1.58 MiB 2.12 MiB 551.78 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
2124a46 319.19 ms 415.04 ms 95.85 ms
b3d8889 420.46 ms 453.71 ms 33.26 ms
d217708 409.83 ms 474.72 ms 64.89 ms
3d205d0 352.15 ms 432.53 ms 80.38 ms
fcec2f2 357.47 ms 447.32 ms 89.85 ms
889ecea 367.58 ms 437.52 ms 69.94 ms
1df7eb6 397.04 ms 429.64 ms 32.60 ms
ce0a49e 532.00 ms 609.96 ms 77.96 ms
ee747ae 357.79 ms 421.84 ms 64.05 ms
b750b96 408.98 ms 480.32 ms 71.34 ms

App size

Revision Plain With Sentry Diff
2124a46 1.58 MiB 2.12 MiB 551.51 KiB
b3d8889 1.58 MiB 2.10 MiB 535.07 KiB
d217708 1.58 MiB 2.10 MiB 532.97 KiB
3d205d0 1.58 MiB 2.10 MiB 532.97 KiB
fcec2f2 1.58 MiB 2.12 MiB 551.50 KiB
889ecea 1.58 MiB 2.11 MiB 539.75 KiB
1df7eb6 1.58 MiB 2.10 MiB 532.97 KiB
ce0a49e 1.58 MiB 2.10 MiB 532.94 KiB
ee747ae 1.58 MiB 2.10 MiB 530.95 KiB
b750b96 1.58 MiB 2.10 MiB 533.19 KiB

Previous results on branch: rz/fix/session-replay-canvas-artifacts

Startup times

Revision Plain With Sentry Diff
a47b2db 372.31 ms 463.36 ms 91.05 ms
51bd5cc 360.74 ms 464.40 ms 103.65 ms
bd9a863 300.79 ms 361.92 ms 61.13 ms
7e17e61 314.77 ms 337.06 ms 22.29 ms

App size

Revision Plain With Sentry Diff
a47b2db 1.58 MiB 2.12 MiB 551.78 KiB
51bd5cc 1.58 MiB 2.12 MiB 551.66 KiB
bd9a863 1.58 MiB 2.12 MiB 551.65 KiB
7e17e61 1.58 MiB 2.12 MiB 551.75 KiB

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@romtsn romtsn merged commit b6702b0 into main Nov 5, 2025
63 checks passed
@romtsn romtsn deleted the rz/fix/session-replay-canvas-artifacts branch November 5, 2025 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Canvas Capture Causes Artifacts

3 participants