diff --git a/CHANGELOG.md b/CHANGELOG.md index 74219c17d9d..bfad1aef26c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java index c0dd3f9eb71..0d77625c718 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java +++ b/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> 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 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 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 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."); } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryWindowCallback.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryWindowCallback.java index 557cd4e7a29..e69756e506a 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryWindowCallback.java +++ b/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(); } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt index f558841e6f5..8f20dbb1539 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt +++ b/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(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(newTop) + assertNotSame(originalSentryCallback, newTop) + assertSame(outerWrapper, newTop.delegate) + } + + @Test + fun `close unwraps windows so re-init does not double-wrap`() { + val mockCallback = mock() + fixture.window.callback = mockCallback + val sutA = fixture.getSut() + sutA.register(fixture.scopes, fixture.options) + sutA.onActivityResumed(fixture.activity) + assertIs(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(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() } + +/** 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 diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryWindowCallbackTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryWindowCallbackTest.kt index 8afc1b39304..be0438d9345 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryWindowCallbackTest.kt +++ b/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 { 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) + } } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/GestureRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/GestureRecorder.kt index 945a0be5156..cee75fb06c0 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/GestureRecorder.kt +++ b/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>() 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>() + 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 + } } } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/gestures/GestureRecorderTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/gestures/GestureRecorderTest.kt index bf3f9cb8443..6f5a02b54c7 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/gestures/GestureRecorderTest.kt +++ b/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