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,22 @@ 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 only release our
+    // resources. The inert wrapper gets GC'd when the window is destroyed.
+    final @Nullable SentryWindowCallback ours;
+    synchronized (wrappedWindowsLock) {
+      final @Nullable WeakReference<SentryWindowCallback> cached = wrappedWindows.get(window);
+      ours = cached != null ? cached.get() : null;
+    }
+    if (ours != null) {
+      ours.stopTracking();
     }
   }
 
@@ -146,6 +185,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/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt

@@ -14,7 +14,6 @@ import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertIs
 import kotlin.test.assertIsNot
-import kotlin.test.assertNotEquals
 import kotlin.test.assertSame
 import org.junit.runner.RunWith
 import org.mockito.kotlin.any
@@ -149,15 +148,59 @@ 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 `does not double-wrap when another callback wraps SentryWindowCallback`() {
+    val sut = fixture.getSut()
+    sut.register(fixture.scopes, fixture.options)
+
+    sut.onActivityResumed(fixture.activity)
+    val sentryCallback = fixture.window.callback
+    assertIs<SentryWindowCallback>(sentryCallback)
+
+    val outerWrapper = WrapperCallback(sentryCallback)
+    fixture.window.callback = outerWrapper
+
+    sut.onActivityPaused(fixture.activity)
+    sut.onActivityResumed(fixture.activity)
+
+    assertSame(outerWrapper, fixture.window.callback)
+  }
+
+  @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 +248,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-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
+    }
   }
 }
 

sentry-android-replay/src/test/java/io/sentry/android/replay/gestures/GestureRecorderTest.kt

@@ -5,6 +5,7 @@ import android.app.Activity
 import android.os.Bundle
 import android.view.MotionEvent
 import android.view.View
+import android.view.Window
 import android.widget.LinearLayout
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import io.sentry.SentryOptions
@@ -14,6 +15,7 @@ import io.sentry.android.replay.phoneWindow
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
+import kotlin.test.assertSame
 import kotlin.test.assertTrue
 import org.junit.runner.RunWith
 import org.robolectric.Robolectric
@@ -37,17 +39,44 @@ class GestureRecorderTest {
   }
 
   @Test
-  fun `when new window added and window callback is already wrapped, does not wrap it again`() {
+  fun `does not double-wrap when root is added twice and another callback wraps on top`() {
     val activity = Robolectric.buildActivity(TestActivity::class.java).setup().get()
     val gestureRecorder = fixture.getSut()
 
-    activity.root.phoneWindow?.callback = SentryReplayGestureRecorder(fixture.options, null, null)
     gestureRecorder.onRootViewsChanged(activity.root, true)
+    val ourWrapper = activity.root.phoneWindow?.callback as SentryReplayGestureRecorder
 
-    assertFalse(
-      (activity.root.phoneWindow?.callback as SentryReplayGestureRecorder).delegate
-        is SentryReplayGestureRecorder
-    )
+    val outer = WrapperCallback(ourWrapper)
+    activity.root.phoneWindow?.callback = outer
+
+    gestureRecorder.onRootViewsChanged(activity.root, true)
+
+    assertSame(outer, activity.root.phoneWindow?.callback)
+  }
+
+  @Test
+  fun `when stopped with another wrapper on top, inerts the buried recorder`() {
+    var called = false
+    val activity = Robolectric.buildActivity(TestActivity::class.java).setup().get()
+    val gestureRecorder =
+      fixture.getSut(
+        touchRecorderCallback =
+          object : TouchRecorderCallback {
+            override fun onTouchEvent(event: MotionEvent) {
+              called = true
+            }
+          }
+      )
+
+    gestureRecorder.onRootViewsChanged(activity.root, true)
+    val ourWrapper = activity.root.phoneWindow?.callback as SentryReplayGestureRecorder
+    activity.root.phoneWindow?.callback = WrapperCallback(ourWrapper)
+
+    gestureRecorder.onRootViewsChanged(activity.root, false)
+
+    val motionEvent = MotionEvent.obtain(0, 0, MotionEvent.ACTION_DOWN, 0f, 0f, 0)
+    ourWrapper.dispatchTouchEvent(motionEvent)
+    assertFalse(called)
   }
 
   @Test
@@ -109,6 +138,9 @@ class GestureRecorderTest {
   }
 }
 
+private open class WrapperCallback(@JvmField val delegate: Window.Callback) :
+  Window.Callback by delegate
+
 private class TestActivity : Activity() {
   lateinit var root: View