Ensure screenshots and view hierarchies are captured on the main thread #2712
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8ac2eae | 300.22 ms | 353.71 ms | 53.49 ms |
| 4187d3a | 358.60 ms | 372.38 ms | 13.78 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8ac2eae | 1.72 MiB | 2.28 MiB | 570.45 KiB |
| 4187d3a | 1.72 MiB | 2.28 MiB | 570.45 KiB |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #2712 +/- ##
=========================================
Coverage 81.11% 81.11%
Complexity 4445 4445
=========================================
Files 345 345
Lines 16387 16387
Branches 2226 2226
=========================================
Hits 13293 13293
Misses 2166 2166
Partials 928 928 ☔ View full report in Codecov by Sentry. |
| } else { | ||
| final CountDownLatch latch = new CountDownLatch(1); | ||
| final AtomicReference<ViewHierarchy> viewHierarchy = new AtomicReference<>(null); | ||
| activity.runOnUiThread( |
There was a problem hiding this comment.
l: potentially we could do handler.postAtFrontOfQueue instead to capture the VH/SS as soon as possible, but I don't know what could be the potential side effects of this. WDYT?
There was a problem hiding this comment.
Hmm that's a good one. Reading up on the docs, I wouldn't do it - at least not within this PR - maybe as a follow up investigation, as it could be relevant for other parts within our code as well?
There was a problem hiding this comment.
Avoid activity.runOnUiThread in favor of MainLooperHandler for testability.
See the previous comment related to this issue as well:
#2281 (comment)
Added a new comment here #2515 (comment)
There was a problem hiding this comment.
Looks good, but it's a breaking change for react native. @krystofwoldrich is that fine for you to just adapt it with the new Android SDK version, or do we have to take a less invasive path (e.g. introducing another method overload to avoid the breaking change)?
I brought back the old signatures relevant for RN, as the new signatures were mainly relevant for testing. Should be a non-breaking change for RN now. |
📜 Description
Right now screenshots/VH are created on the calling thread. According to the docs it's not safe to modify or just access the UI toolkit from a background thread.
Jetpack Compose specifically may makes things worse, as
View.draw()might initiates a layout pass, causing modifications on the calling thread.💡 Motivation and Context
Fixes #2674
💚 How did you test it?
Add unit tests
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps