Skip to content

Commit 952b180

Browse files
romtsnclaude
andauthored
fix(gestures): Prevent duplicate ui.click breadcrumbs from buried window callbacks (#5300)
* fix(user-interaction): Restore window callbacks on close and dedup instrumentation 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> * fix(replay): Track wrapped windows and inert buried recorders on stop 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> * changelog * fix(user-interaction): Inert buried SentryWindowCallback and drop its 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> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 40234a9 commit 952b180

7 files changed

Lines changed: 217 additions & 24 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
### Fixes
2828

2929
- Fix ANR caused by `GestureDetectorCompat` Handler/MessageQueue lock contention in `SentryWindowCallback` ([#5138](https://github.com/getsentry/sentry-java/pull/5138))
30+
- Fix duplicate `ui.click` breadcrumbs when another `Window.Callback` wraps `SentryWindowCallback` ([#5300](https://github.com/getsentry/sentry-java/pull/5300))
3031

3132
### Internal
3233

sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@
1818
import io.sentry.util.Objects;
1919
import java.io.Closeable;
2020
import java.io.IOException;
21+
import java.lang.ref.WeakReference;
22+
import java.util.ArrayList;
23+
import java.util.WeakHashMap;
2124
import org.jetbrains.annotations.NotNull;
2225
import org.jetbrains.annotations.Nullable;
2326

@@ -30,6 +33,16 @@ public final class UserInteractionIntegration
3033

3134
private final boolean isAndroidxLifecycleAvailable;
3235

36+
// WeakReference value, because the callback chain strongly references the wrapper — a strong
37+
// value would prevent the window from ever being GC'd.
38+
//
39+
// All access must be guarded by wrappedWindowsLock — lifecycle callbacks fire on the main
40+
// thread, but close() may be called from a background thread (e.g. Sentry.close()).
41+
private final @NotNull WeakHashMap<Window, WeakReference<SentryWindowCallback>> wrappedWindows =
42+
new WeakHashMap<>();
43+
44+
private final @NotNull Object wrappedWindowsLock = new Object();
45+
3346
public UserInteractionIntegration(
3447
final @NotNull Application application, final @NotNull io.sentry.util.LoadClass classLoader) {
3548
this.application = Objects.requireNonNull(application, "Application is required");
@@ -47,19 +60,26 @@ private void startTracking(final @NotNull Activity activity) {
4760
}
4861

4962
if (scopes != null && options != null) {
63+
synchronized (wrappedWindowsLock) {
64+
final @Nullable WeakReference<SentryWindowCallback> cached = wrappedWindows.get(window);
65+
if (cached != null && cached.get() != null) {
66+
return;
67+
}
68+
}
69+
5070
Window.Callback delegate = window.getCallback();
5171
if (delegate == null) {
5272
delegate = new NoOpWindowCallback();
5373
}
5474

55-
if (delegate instanceof SentryWindowCallback) {
56-
// already instrumented
57-
return;
58-
}
59-
6075
final SentryGestureListener gestureListener =
6176
new SentryGestureListener(activity, scopes, options);
62-
window.setCallback(new SentryWindowCallback(delegate, activity, gestureListener, options));
77+
final SentryWindowCallback wrapper =
78+
new SentryWindowCallback(delegate, activity, gestureListener, options);
79+
window.setCallback(wrapper);
80+
synchronized (wrappedWindowsLock) {
81+
wrappedWindows.put(window, new WeakReference<>(wrapper));
82+
}
6383
}
6484
}
6585

@@ -71,7 +91,10 @@ private void stopTracking(final @NotNull Activity activity) {
7191
}
7292
return;
7393
}
94+
unwrapWindow(window);
95+
}
7496

97+
private void unwrapWindow(final @NotNull Window window) {
7598
final Window.Callback current = window.getCallback();
7699
if (current instanceof SentryWindowCallback) {
77100
((SentryWindowCallback) current).stopTracking();
@@ -80,6 +103,23 @@ private void stopTracking(final @NotNull Activity activity) {
80103
} else {
81104
window.setCallback(((SentryWindowCallback) current).getDelegate());
82105
}
106+
synchronized (wrappedWindowsLock) {
107+
wrappedWindows.remove(window);
108+
}
109+
return;
110+
}
111+
112+
// Another wrapper (e.g. Session Replay) sits on top of ours — cutting it out of the chain
113+
// would break its instrumentation, so we leave the chain alone and just call stopTracking()
114+
// to release our resources. The upstream wrapper holds a reference to ours, so it'll be
115+
// GC'd whenever that upstream holder is (typically when the window is destroyed).
116+
final @Nullable SentryWindowCallback ours;
117+
synchronized (wrappedWindowsLock) {
118+
final @Nullable WeakReference<SentryWindowCallback> cached = wrappedWindows.remove(window);
119+
ours = cached != null ? cached.get() : null;
120+
}
121+
if (ours != null) {
122+
ours.stopTracking();
83123
}
84124
}
85125

@@ -146,6 +186,21 @@ public void register(@NotNull IScopes scopes, @NotNull SentryOptions options) {
146186
public void close() throws IOException {
147187
application.unregisterActivityLifecycleCallbacks(this);
148188

189+
// Restore original callbacks so a subsequent Sentry.init() starts from a clean chain instead
190+
// of wrapping on top of our orphaned callback.
191+
final ArrayList<Window> snapshot;
192+
synchronized (wrappedWindowsLock) {
193+
snapshot = new ArrayList<>(wrappedWindows.keySet());
194+
}
195+
for (final Window window : snapshot) {
196+
if (window != null) {
197+
unwrapWindow(window);
198+
}
199+
}
200+
synchronized (wrappedWindowsLock) {
201+
wrappedWindows.clear();
202+
}
203+
149204
if (options != null) {
150205
options.getLogger().log(SentryLevel.DEBUG, "UserInteractionIntegration removed.");
151206
}

sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryWindowCallback.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ public final class SentryWindowCallback extends WindowCallbackAdapter {
1919
private final @Nullable SentryOptions options;
2020
private final @NotNull MotionEventObtainer motionEventObtainer;
2121

22+
// When we can't be removed from the callback chain (see UserInteractionIntegration),
23+
// stopTracking() flips this so handleTouchEvent short-circuits.
24+
private volatile boolean inert;
25+
2226
public SentryWindowCallback(
2327
final @NotNull Window.Callback delegate,
2428
final @NotNull Context context,
@@ -64,6 +68,9 @@ public boolean dispatchTouchEvent(final @Nullable MotionEvent motionEvent) {
6468
}
6569

6670
private void handleTouchEvent(final @NotNull MotionEvent motionEvent) {
71+
if (inert) {
72+
return;
73+
}
6774
gestureDetector.onTouchEvent(motionEvent);
6875
int action = motionEvent.getActionMasked();
6976
if (action == MotionEvent.ACTION_UP) {
@@ -72,6 +79,7 @@ private void handleTouchEvent(final @NotNull MotionEvent motionEvent) {
7279
}
7380

7481
public void stopTracking() {
82+
inert = true;
7583
gestureListener.stopTracing(SpanStatus.CANCELLED);
7684
gestureDetector.release();
7785
}

sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt

Lines changed: 58 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import kotlin.test.BeforeTest
1414
import kotlin.test.Test
1515
import kotlin.test.assertIs
1616
import kotlin.test.assertIsNot
17-
import kotlin.test.assertNotEquals
17+
import kotlin.test.assertNotSame
1818
import kotlin.test.assertSame
1919
import org.junit.runner.RunWith
2020
import org.mockito.kotlin.any
@@ -149,15 +149,63 @@ class UserInteractionIntegrationTest {
149149
}
150150

151151
@Test
152-
fun `does not instrument if the callback is already ours`() {
153-
val existingCallback =
154-
SentryWindowCallback(NoOpWindowCallback(), fixture.activity, mock(), mock())
155-
val sut = fixture.getSut(existingCallback)
152+
fun `resume after buried pause installs a fresh wrapper on top`() {
153+
val sut = fixture.getSut()
154+
sut.register(fixture.scopes, fixture.options)
155+
156+
sut.onActivityResumed(fixture.activity)
157+
val originalSentryCallback = fixture.window.callback
158+
assertIs<SentryWindowCallback>(originalSentryCallback)
159+
160+
// Third-party wraps on top of us mid-activity.
161+
val outerWrapper = WrapperCallback(originalSentryCallback)
162+
fixture.window.callback = outerWrapper
163+
164+
sut.onActivityPaused(fixture.activity)
165+
sut.onActivityResumed(fixture.activity)
166+
167+
val newTop = fixture.window.callback
168+
assertIs<SentryWindowCallback>(newTop)
169+
assertNotSame(originalSentryCallback, newTop)
170+
assertSame(outerWrapper, newTop.delegate)
171+
}
172+
173+
@Test
174+
fun `close unwraps windows so re-init does not double-wrap`() {
175+
val mockCallback = mock<Window.Callback>()
176+
fixture.window.callback = mockCallback
156177

178+
val sutA = fixture.getSut()
179+
sutA.register(fixture.scopes, fixture.options)
180+
sutA.onActivityResumed(fixture.activity)
181+
assertIs<SentryWindowCallback>(fixture.window.callback)
182+
183+
sutA.close()
184+
assertSame(mockCallback, fixture.window.callback)
185+
186+
val sutB = UserInteractionIntegration(fixture.application, fixture.loadClass)
187+
sutB.register(fixture.scopes, fixture.options)
188+
sutB.onActivityResumed(fixture.activity)
189+
190+
val newWrapper = fixture.window.callback
191+
assertIs<SentryWindowCallback>(newWrapper)
192+
assertSame(mockCallback, newWrapper.delegate)
193+
}
194+
195+
@Test
196+
fun `paused with another wrapper on top does not cut it out of the chain`() {
197+
val sut = fixture.getSut()
157198
sut.register(fixture.scopes, fixture.options)
199+
158200
sut.onActivityResumed(fixture.activity)
201+
val sentryCallback = fixture.window.callback as SentryWindowCallback
202+
203+
val outerWrapper = WrapperCallback(sentryCallback)
204+
fixture.window.callback = outerWrapper
159205

160-
assertNotEquals(existingCallback, (fixture.window.callback as SentryWindowCallback).delegate)
206+
sut.onActivityPaused(fixture.activity)
207+
208+
assertSame(outerWrapper, fixture.window.callback)
161209
}
162210

163211
@Test
@@ -205,3 +253,7 @@ class UserInteractionIntegrationTest {
205253
private class EmptyActivity : Activity(), LifecycleOwner {
206254
override val lifecycle: Lifecycle = mock<Lifecycle>()
207255
}
256+
257+
/** Simulates a third-party callback wrapper (e.g. Session Replay's FixedWindowCallback). */
258+
private open class WrapperCallback(@JvmField val delegate: Window.Callback) :
259+
Window.Callback by delegate

sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryWindowCallbackTest.kt

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,4 +82,18 @@ class SentryWindowCallbackTest {
8282

8383
verify(fixture.gestureDetector, never()).onTouchEvent(any())
8484
}
85+
86+
@Test
87+
fun `after stopTracking does not forward touches to detector or listener`() {
88+
val event = mock<MotionEvent> { whenever(it.actionMasked).thenReturn(MotionEvent.ACTION_UP) }
89+
val sut = fixture.getSut()
90+
91+
sut.stopTracking()
92+
sut.dispatchTouchEvent(event)
93+
94+
verify(fixture.gestureDetector, never()).onTouchEvent(any())
95+
verify(fixture.gestureListener, never()).onUp(any())
96+
// super.dispatchTouchEvent still delegates to the wrapped delegate so the chain keeps working.
97+
verify(fixture.delegate).dispatchTouchEvent(event)
98+
}
8599
}

sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/GestureRecorder.kt

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import io.sentry.android.replay.phoneWindow
1111
import io.sentry.android.replay.util.FixedWindowCallback
1212
import io.sentry.util.AutoClosableReentrantLock
1313
import java.lang.ref.WeakReference
14+
import java.util.WeakHashMap
1415

1516
internal class GestureRecorder(
1617
private val options: SentryOptions,
@@ -19,6 +20,11 @@ internal class GestureRecorder(
1920
private val rootViews = ArrayList<WeakReference<View>>()
2021
private val rootViewsLock = AutoClosableReentrantLock()
2122

23+
// WeakReference value, because the callback chain strongly references the wrapper — a strong
24+
// value would prevent the window from ever being GC'd.
25+
private val wrappedWindows = WeakHashMap<Window, WeakReference<SentryReplayGestureRecorder>>()
26+
private val wrappedWindowsLock = AutoClosableReentrantLock()
27+
2228
override fun onRootViewsChanged(root: View, added: Boolean) {
2329
rootViewsLock.acquire().use {
2430
if (added) {
@@ -45,10 +51,16 @@ internal class GestureRecorder(
4551
return
4652
}
4753

48-
val delegate = window.callback
49-
if (delegate !is SentryReplayGestureRecorder) {
50-
window.callback = SentryReplayGestureRecorder(options, touchRecorderCallback, delegate)
54+
wrappedWindowsLock.acquire().use {
55+
if (wrappedWindows[window]?.get() != null) {
56+
return
57+
}
5158
}
59+
60+
val delegate = window.callback
61+
val wrapper = SentryReplayGestureRecorder(options, touchRecorderCallback, delegate)
62+
window.callback = wrapper
63+
wrappedWindowsLock.acquire().use { wrappedWindows[window] = WeakReference(wrapper) }
5264
}
5365

5466
private fun View.stopGestureTracking() {
@@ -60,14 +72,25 @@ internal class GestureRecorder(
6072

6173
val callback = window.callback
6274
if (callback is SentryReplayGestureRecorder) {
63-
val delegate = callback.delegate
64-
window.callback = delegate
75+
window.callback = callback.delegate
76+
wrappedWindowsLock.acquire().use { wrappedWindows.remove(window) }
77+
return
78+
}
79+
80+
// Another wrapper (e.g. UserInteractionIntegration) sits on top of ours — cutting it out of
81+
// the chain would break its instrumentation, so we inert our buried wrapper instead. The
82+
// next replay session will then wrap on top with a fresh active instance.
83+
val ours: SentryReplayGestureRecorder?
84+
wrappedWindowsLock.acquire().use {
85+
ours = wrappedWindows[window]?.get()
86+
wrappedWindows.remove(window)
6587
}
88+
ours?.inert()
6689
}
6790

6891
internal class SentryReplayGestureRecorder(
6992
private val options: SentryOptions,
70-
private val touchRecorderCallback: TouchRecorderCallback?,
93+
@Volatile private var touchRecorderCallback: TouchRecorderCallback?,
7194
delegate: Window.Callback?,
7295
) : FixedWindowCallback(delegate) {
7396
override fun dispatchTouchEvent(event: MotionEvent?): Boolean {
@@ -83,6 +106,14 @@ internal class GestureRecorder(
83106
}
84107
return super.dispatchTouchEvent(event)
85108
}
109+
110+
/**
111+
* Turns this wrapper into a passthrough when it can't be removed from the chain (another
112+
* wrapper sits on top). Subsequent dispatches only delegate, skipping the recorder callback.
113+
*/
114+
fun inert() {
115+
touchRecorderCallback = null
116+
}
86117
}
87118
}
88119

0 commit comments

Comments
 (0)