fix(gestures): Prevent duplicate ui.click breadcrumbs from buried window callbacks#5300
Open
fix(gestures): Prevent duplicate ui.click breadcrumbs from buried window callbacks#5300
Conversation
…strumentation via WeakHashMap Track wrapped windows in a thread-safe WeakHashMap so close() can restore each window's original callback chain, preventing an orphaned SentryWindowCallback from persisting after Sentry.close(). Also handle the case where another wrapper (e.g. Session Replay) has been installed on top of ours — we skip chain mutation but still invoke stopTracking() to release resources. Guards against racing lifecycle callbacks (main thread) and close() (possibly bg thread). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Track wrapped windows in a WeakHashMap so GestureRecorder skips re-wrapping already-instrumented windows and can locate its own recorder even when another wrapper (e.g. UserInteractionIntegration) has been installed on top of it. When our wrapper is buried in the callback chain, inert() it instead of mutating the chain so unrelated instrumentation isn't broken; the next replay session wraps on top with a fresh active recorder. 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 |
romtsn
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
romtsn
force-pushed
the
rz/fix/ui-click-duplicate-breadcrumbs
branch
from
55443f7 to
33540c8
Compare
… cache entry on stop Two follow-ups to the buried-wrapper path: - stopTracking() now sets an inert flag that short-circuits handleTouchEvent, so a SentryWindowCallback that can't be cut out of the chain stops forwarding events to its gesture detector and listener. Without this, the "stopped" wrapper kept emitting ui.click breadcrumbs, so as soon as a fresh wrapper was installed on top the duplicates came back. - unwrapWindow removes the wrapped window from the tracking map in the buried path too. Previously only the top-of-chain path cleared it, which meant the next startTracking() found a stale (but alive, since the inert wrapper is still referenced by the chain) entry and returned early, permanently losing gesture tracking for that window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
romtsn
force-pushed
the
rz/fix/ui-click-duplicate-breadcrumbs
branch
from
33540c8 to
b7674b8
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b7674b8. Configure here.
| } | ||
| synchronized (wrappedWindowsLock) { | ||
| wrappedWindows.clear(); | ||
| } |
There was a problem hiding this comment.
Concurrent stopTracking risks double-recycling native resources
Low Severity
The new close() method iterates windows and calls unwrapWindow() from a background thread, which can call stopTracking() on a SentryWindowCallback. If onActivityPaused is already executing on the main thread for the same window, both threads can concurrently call stopTracking() on the same wrapper instance. Since SentryGestureDetector.release() is not thread-safe (non-volatile velocityTracker field with check-then-act on null), both threads may see velocityTracker != null and call recycle() twice on the same VelocityTracker, risking a native double-free crash.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b7674b8. Configure here.
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
1 participant
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
If anything installs a
Window.Callbackon top of ourSentryWindowCallbackafterstartTracking()(a third-party library, our own Session Replay, etc.), ourstopTracking()path couldn't recognize our wrapper —window.getCallback() instanceof SentryWindowCallbackwasfalse— so it was left in the chain and kept firingSentryGestureListener. Every touch then produced twoui.clickbreadcrumbs — one from the currently-installed wrapper and one from the buried stale one.Fix: both integrations now track their wrappers in a
WeakHashMap<Window, WeakReference<…>>:UserInteractionIntegration— onstopTracking(), if our wrapper is no longer at the top of the chain, we still locate it via the map and callstopTracking()on it to release resources. When our wrapper is at the top, we restore the original delegate as before. Map access is guarded by a lock so main-thread lifecycle callbacks don't race bg-threadclose().GestureRecorder(session replay) — same tracking pattern. When our recorder is buried, weinert()it (null thetouchRecorderCallback) instead of mutating the chain. ThetouchRecorderCallbackfield is now@Volatile varsoinert()can safely null it.Both integrations also skip re-wrapping a window that's already instrumented.
💡 Motivation and Context
See screenshot above — duplicate
ui.clickbreadcrumbs.Previous behavior relied on
window.getCallback() instanceof SentryWindowCallbackto detect and remove our wrapper, which only works when our wrapper is at the top. As soon as anything else wraps on top of us, detection fails, our stale wrapper stays installed, and every subsequent touch fires our listener twice.💚 How did you test it?
UserInteractionIntegrationTest.ktandGestureRecorderTest.ktcovering:inert()prevents subsequent dispatches from invoking the recorder callback../gradlew :sentry-android-core:testDebugUnitTest :sentry-android-replay:testDebugUnitTestpasses.📝 Checklist
sendDefaultPIIis enabled.