fix(gestures): Thread-safe SentryGestureDetector with per-gesture VelocityTracker recycle#5301
Open
fix(gestures): Thread-safe SentryGestureDetector with per-gesture VelocityTracker recycle#5301
Conversation
…th a lock Recycle VelocityTracker on every ACTION_UP/ACTION_CANCEL instead of only when the detector is torn down, so the pooled native tracker isn't held across gestures (matches Android's framework GestureDetector behavior). Merges endGesture() and release() into a single recycle() method. Guard onTouchEvent and recycle with an AutoClosableReentrantLock — SentryWindowCallback.stopTracking() can be invoked from a bg thread via Sentry.close(), which would otherwise race with the UI thread's touch dispatch and cause use-after-recycle on the native MotionEvent/VelocityTracker pools. recycle() captures the native handles under the lock and performs the JNI recycle() calls outside it to keep the bg thread's critical section to a pointer swap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
🤖 This preview updates automatically when you update the PR. |
📲 Install BuildsAndroid
|
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d15471f | 310.66 ms | 368.19 ms | 57.53 ms |
| 9054d65 | 330.94 ms | 403.24 ms | 72.30 ms |
| b77456b | 393.26 ms | 441.10 ms | 47.84 ms |
| 96449e8 | 361.30 ms | 423.39 ms | 62.09 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| a416a65 | 295.53 ms | 373.74 ms | 78.21 ms |
| ab8a72d | 316.24 ms | 356.38 ms | 40.14 ms |
| 1564554 | 323.06 ms | 336.68 ms | 13.62 ms |
| b3d8889 | 371.69 ms | 432.96 ms | 61.26 ms |
| d15471f | 315.20 ms | 370.22 ms | 55.02 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| 9054d65 | 1.58 MiB | 2.29 MiB | 723.38 KiB |
| b77456b | 1.58 MiB | 2.12 MiB | 548.11 KiB |
| 96449e8 | 1.58 MiB | 2.11 MiB | 539.35 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| ab8a72d | 1.58 MiB | 2.12 MiB | 551.55 KiB |
| 1564554 | 1.58 MiB | 2.20 MiB | 635.33 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.06 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
Three related fixes to
SentryGestureDetector, the lightweight gesture detector introduced in #5138:VelocityTrackerper gesture. On everyACTION_UP/ACTION_CANCELthe pooled native tracker is recycled and nulled, and re-obtained lazily on the nextACTION_DOWN. Matches frameworkGestureDetectorbehavior. Previously the tracker was held for the detector's entire lifetime, keeping one pooled instance out of circulation.endGesture()+release()into a singlerecycle(). They did the same thing after the fix above.onTouchEventandrecyclewith anAutoClosableReentrantLock.Sentry.close()is not constrained to the UI thread, soSentryWindowCallback.stopTracking()→SentryGestureDetector.recycle()can race UI-thread touch dispatch. Without synchronization that causes use-after-recycle on the nativeMotionEvent/VelocityTrackerpools — the bg thread can return a pooled instance that UI is still callingaddMovement()/computeCurrentVelocity()on, and unrelated callers that re-obtain that slot get a corrupted state.recycle()captures the native handles under the lock and performs the JNIrecycle()calls outside, keeping the bg thread's critical section to a pointer swap.SentryWindowCallbackis updated for therelease()→recycle()rename.💡 Motivation and Context
Before #5138, the detector delegated to
GestureDetectorCompatwhich didn't own pooled native resources directly, so there was no thread-safety hazard at this layer. The new lightweight detector managesMotionEvent.obtain/recycleandVelocityTracker.obtain/recycleitself, which reopens the race described above onceSentry.close()is called off the main thread.Holding a
VelocityTrackeracross gestures also deviates from framework behavior and unnecessarily ties up a pooled instance.💚 How did you test it?
SentryGestureDetectorTestsuite (tap / scroll / fling / cancel / multi-touch / sequential gestures) still passes.recycle mid-gesture - subsequent gesture still fires onSingleTapUp— callsrecycle()betweenACTION_DOWNand a fresh gesture to verify the lazy re-obtain path and that an external mid-gesture teardown doesn't leave the detector broken../gradlew :sentry-android-core:testDebugUnitTest --tests "*SentryGestureDetectorTest*"→ 11/11 pass../gradlew spotlessApply apiDump— no public API changes.📝 Checklist
sendDefaultPIIis enabled.