fix(gestures): Prevent duplicate ui.click breadcrumbs from buried window callbacks

CHANGELOG.md

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

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

@@ -18,6 +18,9 @@
 import io.sentry.util.Objects;
 import java.io.Closeable;
 import java.io.IOException;
+import java.lang.ref.WeakReference;
+import java.util.ArrayList;
+import java.util.WeakHashMap;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
@@ -30,6 +33,16 @@ public final class UserInteractionIntegration
 
   private final boolean isAndroidxLifecycleAvailable;
 
+  // WeakReference value, because the callback chain strongly references the wrapper — a strong
+  // value would prevent the window from ever being GC'd.
+  //
+  // All access must be guarded by wrappedWindowsLock — lifecycle callbacks fire on the main
+  // thread, but close() may be called from a background thread (e.g. Sentry.close()).
+  private final @NotNull WeakHashMap<Window, WeakReference<SentryWindowCallback>> wrappedWindows =
+      new WeakHashMap<>();
+
+  private final @NotNull Object wrappedWindowsLock = new Object();
+
   public UserInteractionIntegration(
       final @NotNull Application application, final @NotNull io.sentry.util.LoadClass classLoader) {
     this.application = Objects.requireNonNull(application, "Application is required");
@@ -47,19 +60,26 @@ private void startTracking(final @NotNull Activity activity) {
     }
 
     if (scopes != null && options != null) {
+      synchronized (wrappedWindowsLock) {
+        final @Nullable WeakReference<SentryWindowCallback> cached = wrappedWindows.get(window);
+        if (cached != null && cached.get() != null) {
+          return;
+        }
+      }
+
       Window.Callback delegate = window.getCallback();
       if (delegate == null) {
         delegate = new NoOpWindowCallback();
       }
 
-      if (delegate instanceof SentryWindowCallback) {
-        // already instrumented
-        return;
-      }
-
       final SentryGestureListener gestureListener =
           new SentryGestureListener(activity, scopes, options);
-      window.setCallback(new SentryWindowCallback(delegate, activity, gestureListener, options));
+      final SentryWindowCallback wrapper =
+          new SentryWindowCallback(delegate, activity, gestureListener, options);
+      window.setCallback(wrapper);
+      synchronized (wrappedWindowsLock) {
+        wrappedWindows.put(window, new WeakReference<>(wrapper));
+      }
     }
   }
 
@@ -71,7 +91,10 @@ private void stopTracking(final @NotNull Activity activity) {
       }
       return;
     }
+    unwrapWindow(window);
+  }
 
+  private void unwrapWindow(final @NotNull Window window) {
     final Window.Callback current = window.getCallback();
     if (current instanceof SentryWindowCallback) {
       ((SentryWindowCallback) current).stopTracking();
@@ -80,6 +103,23 @@ private void stopTracking(final @NotNull Activity activity) {
       } else {
         window.setCallback(((SentryWindowCallback) current).getDelegate());
       }
+      synchronized (wrappedWindowsLock) {
+        wrappedWindows.remove(window);
+      }
+      return;
+    }
+
+    // Another wrapper (e.g. Session Replay) sits on top of ours — cutting it out of the chain
+    // would break its instrumentation, so we leave the chain alone and just call stopTracking()
+    // to release our resources. The upstream wrapper holds a reference to ours, so it'll be
+    // GC'd whenever that upstream holder is (typically when the window is destroyed).
+    final @Nullable SentryWindowCallback ours;
+    synchronized (wrappedWindowsLock) {
+      final @Nullable WeakReference<SentryWindowCallback> cached = wrappedWindows.remove(window);
+      ours = cached != null ? cached.get() : null;
+    }
+    if (ours != null) {
+      ours.stopTracking();
     }
   }
 
@@ -146,6 +186,21 @@ public void register(@NotNull IScopes scopes, @NotNull SentryOptions options) {
   public void close() throws IOException {
     application.unregisterActivityLifecycleCallbacks(this);
 
+    // Restore original callbacks so a subsequent Sentry.init() starts from a clean chain instead
+    // of wrapping on top of our orphaned callback.
+    final ArrayList<Window> snapshot;
+    synchronized (wrappedWindowsLock) {
+      snapshot = new ArrayList<>(wrappedWindows.keySet());
+    }
+    for (final Window window : snapshot) {
+      if (window != null) {
+        unwrapWindow(window);
+      }
+    }
+    synchronized (wrappedWindowsLock) {
+      wrappedWindows.clear();
+    }
+
     if (options != null) {
       options.getLogger().log(SentryLevel.DEBUG, "UserInteractionIntegration removed.");
     }

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

@@ -19,6 +19,10 @@ public final class SentryWindowCallback extends WindowCallbackAdapter {
   private final @Nullable SentryOptions options;
   private final @NotNull MotionEventObtainer motionEventObtainer;
 
+  // When we can't be removed from the callback chain (see UserInteractionIntegration),
+  // stopTracking() flips this so handleTouchEvent short-circuits.
+  private volatile boolean inert;
+
   public SentryWindowCallback(
       final @NotNull Window.Callback delegate,
       final @NotNull Context context,
@@ -64,6 +68,9 @@ public boolean dispatchTouchEvent(final @Nullable MotionEvent motionEvent) {
   }
 
   private void handleTouchEvent(final @NotNull MotionEvent motionEvent) {
+    if (inert) {
+      return;
+    }
     gestureDetector.onTouchEvent(motionEvent);
     int action = motionEvent.getActionMasked();
     if (action == MotionEvent.ACTION_UP) {
@@ -72,6 +79,7 @@ private void handleTouchEvent(final @NotNull MotionEvent motionEvent) {
   }
 
   public void stopTracking() {
+    inert = true;
     gestureListener.stopTracing(SpanStatus.CANCELLED);
     gestureDetector.release();
   }

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

@@ -14,7 +14,7 @@ import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertIs
 import kotlin.test.assertIsNot
-import kotlin.test.assertNotEquals
+import kotlin.test.assertNotSame
 import kotlin.test.assertSame
 import org.junit.runner.RunWith
 import org.mockito.kotlin.any
@@ -149,15 +149,63 @@ class UserInteractionIntegrationTest {
   }
 
   @Test
-  fun `does not instrument if the callback is already ours`() {
-    val existingCallback =
-      SentryWindowCallback(NoOpWindowCallback(), fixture.activity, mock(), mock())
-    val sut = fixture.getSut(existingCallback)
+  fun `resume after buried pause installs a fresh wrapper on top`() {
+    val sut = fixture.getSut()
+    sut.register(fixture.scopes, fixture.options)
+
+    sut.onActivityResumed(fixture.activity)
+    val originalSentryCallback = fixture.window.callback
+    assertIs<SentryWindowCallback>(originalSentryCallback)
+
+    // Third-party wraps on top of us mid-activity.
+    val outerWrapper = WrapperCallback(originalSentryCallback)
+    fixture.window.callback = outerWrapper
+
+    sut.onActivityPaused(fixture.activity)
+    sut.onActivityResumed(fixture.activity)
+
+    val newTop = fixture.window.callback
+    assertIs<SentryWindowCallback>(newTop)
+    assertNotSame(originalSentryCallback, newTop)
+    assertSame(outerWrapper, newTop.delegate)
+  }
+
+  @Test
+  fun `close unwraps windows so re-init does not double-wrap`() {
+    val mockCallback = mock<Window.Callback>()
+    fixture.window.callback = mockCallback
 
+    val sutA = fixture.getSut()
+    sutA.register(fixture.scopes, fixture.options)
+    sutA.onActivityResumed(fixture.activity)
+    assertIs<SentryWindowCallback>(fixture.window.callback)
+
+    sutA.close()
+    assertSame(mockCallback, fixture.window.callback)
+
+    val sutB = UserInteractionIntegration(fixture.application, fixture.loadClass)
+    sutB.register(fixture.scopes, fixture.options)
+    sutB.onActivityResumed(fixture.activity)
+
+    val newWrapper = fixture.window.callback
+    assertIs<SentryWindowCallback>(newWrapper)
+    assertSame(mockCallback, newWrapper.delegate)
+  }
+
+  @Test
+  fun `paused with another wrapper on top does not cut it out of the chain`() {
+    val sut = fixture.getSut()
     sut.register(fixture.scopes, fixture.options)
+
     sut.onActivityResumed(fixture.activity)
+    val sentryCallback = fixture.window.callback as SentryWindowCallback
+
+    val outerWrapper = WrapperCallback(sentryCallback)
+    fixture.window.callback = outerWrapper
 
-    assertNotEquals(existingCallback, (fixture.window.callback as SentryWindowCallback).delegate)
+    sut.onActivityPaused(fixture.activity)
+
+    assertSame(outerWrapper, fixture.window.callback)
   }
 
   @Test
@@ -205,3 +253,7 @@ class UserInteractionIntegrationTest {
 private class EmptyActivity : Activity(), LifecycleOwner {
   override val lifecycle: Lifecycle = mock<Lifecycle>()
 }
+
+/** Simulates a third-party callback wrapper (e.g. Session Replay's FixedWindowCallback). */
+private open class WrapperCallback(@JvmField val delegate: Window.Callback) :
+  Window.Callback by delegate

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

@@ -82,4 +82,18 @@ class SentryWindowCallbackTest {
 
     verify(fixture.gestureDetector, never()).onTouchEvent(any())
   }
+
+  @Test
+  fun `after stopTracking does not forward touches to detector or listener`() {
+    val event = mock<MotionEvent> { whenever(it.actionMasked).thenReturn(MotionEvent.ACTION_UP) }
+    val sut = fixture.getSut()
+
+    sut.stopTracking()
+    sut.dispatchTouchEvent(event)
+
+    verify(fixture.gestureDetector, never()).onTouchEvent(any())
+    verify(fixture.gestureListener, never()).onUp(any())
+    // super.dispatchTouchEvent still delegates to the wrapped delegate so the chain keeps working.
+    verify(fixture.delegate).dispatchTouchEvent(event)
+  }
 }

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

@@ -11,6 +11,7 @@ import io.sentry.android.replay.phoneWindow
 import io.sentry.android.replay.util.FixedWindowCallback
 import io.sentry.util.AutoClosableReentrantLock
 import java.lang.ref.WeakReference
+import java.util.WeakHashMap
 
 internal class GestureRecorder(
   private val options: SentryOptions,
@@ -19,6 +20,11 @@ internal class GestureRecorder(
   private val rootViews = ArrayList<WeakReference<View>>()
   private val rootViewsLock = AutoClosableReentrantLock()
 
+  // WeakReference value, because the callback chain strongly references the wrapper — a strong
+  // value would prevent the window from ever being GC'd.
+  private val wrappedWindows = WeakHashMap<Window, WeakReference<SentryReplayGestureRecorder>>()
+  private val wrappedWindowsLock = AutoClosableReentrantLock()
+
   override fun onRootViewsChanged(root: View, added: Boolean) {
     rootViewsLock.acquire().use {
       if (added) {
@@ -45,10 +51,16 @@ internal class GestureRecorder(
       return
     }
 
-    val delegate = window.callback
-    if (delegate !is SentryReplayGestureRecorder) {
-      window.callback = SentryReplayGestureRecorder(options, touchRecorderCallback, delegate)
+    wrappedWindowsLock.acquire().use {
+      if (wrappedWindows[window]?.get() != null) {
+        return
+      }
     }
+
+    val delegate = window.callback
+    val wrapper = SentryReplayGestureRecorder(options, touchRecorderCallback, delegate)
+    window.callback = wrapper
+    wrappedWindowsLock.acquire().use { wrappedWindows[window] = WeakReference(wrapper) }
   }
 
   private fun View.stopGestureTracking() {
@@ -60,14 +72,25 @@ internal class GestureRecorder(
 
     val callback = window.callback
     if (callback is SentryReplayGestureRecorder) {
-      val delegate = callback.delegate
-      window.callback = delegate
+      window.callback = callback.delegate
+      wrappedWindowsLock.acquire().use { wrappedWindows.remove(window) }
+      return
+    }
+
+    // Another wrapper (e.g. UserInteractionIntegration) sits on top of ours — cutting it out of
+    // the chain would break its instrumentation, so we inert our buried wrapper instead. The
+    // next replay session will then wrap on top with a fresh active instance.
+    val ours: SentryReplayGestureRecorder?
+    wrappedWindowsLock.acquire().use {
+      ours = wrappedWindows[window]?.get()
+      wrappedWindows.remove(window)
     }
+    ours?.inert()
   }
 
   internal class SentryReplayGestureRecorder(
     private val options: SentryOptions,
-    private val touchRecorderCallback: TouchRecorderCallback?,
+    @Volatile private var touchRecorderCallback: TouchRecorderCallback?,
     delegate: Window.Callback?,
   ) : FixedWindowCallback(delegate) {
     override fun dispatchTouchEvent(event: MotionEvent?): Boolean {
@@ -83,6 +106,14 @@ internal class GestureRecorder(
       }
       return super.dispatchTouchEvent(event)
     }
+
+    /**
+     * Turns this wrapper into a passthrough when it can't be removed from the chain (another
+     * wrapper sits on top). Subsequent dispatches only delegate, skipping the recorder callback.
+     */
+    fun inert() {
+      touchRecorderCallback = null
+    }
   }
 }