Keep lifecycle callbacks registered while a dialog is tracked so the strong dialog reference cannot leak its activity

Author: markushi

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

@@ -43,10 +43,12 @@ public final class FeedbackShakeIntegration
   private volatile boolean enabled = false;
   private boolean detecting = false;
   // Strong reference on purpose: for a per-form opt-in the caller may not retain the created
-  // form, so the controller must keep it alive to be able to show it on shake. Cleared when the
-  // host activity is destroyed, when replaced by another dialog, and on close().
+  // form, so the controller must keep it alive to be able to show it on shake. Lifecycle
+  // callbacks stay registered as long as a dialog is tracked, so the reference is guaranteed
+  // to be cleared once the host activity goes away (or another activity is created on top).
   private volatile @Nullable SentryFeedbackOptions.IShakeDialog trackedDialog;
   private boolean dialogRequestedShakeDetection = false;
+  private boolean callbacksRegistered = false;
   private volatile @Nullable WeakReference<Activity> currentActivityRef;
 
   public FeedbackShakeIntegration(final @NotNull Application application) {
@@ -113,6 +115,7 @@ public synchronized void setDialog(
       if (!enabled) {
         stopDetecting();
       }
+      updateCallbackRegistration();
       return;
     }
     trackedDialog = dialog;
@@ -123,6 +126,24 @@ public synchronized void setDialog(
       // The previous dialog may have been the only reason detection was running.
       stopDetecting();
     }
+    // Even without detection, keep listening for the tracked dialog's host activity being
+    // destroyed, so the strong dialog reference can never outlive it (no activity leak).
+    updateCallbackRegistration();
+  }
+
+  /**
+   * Lifecycle callbacks are needed while shake detection runs (to follow the current activity) or
+   * while a dialog is tracked (to release it when its host activity goes away).
+   */
+  private synchronized void updateCallbackRegistration() {
+    final boolean needed = detecting || trackedDialog != null;
+    if (needed && !callbacksRegistered) {
+      callbacksRegistered = true;
+      application.registerActivityLifecycleCallbacks(this);
+    } else if (!needed && callbacksRegistered) {
+      callbacksRegistered = false;
+      application.unregisterActivityLifecycleCallbacks(this);
+    }
   }
 
   private synchronized void startDetecting(final @NotNull SentryAndroidOptions options) {
@@ -150,7 +171,7 @@ private synchronized void startDetecting(final @NotNull SentryAndroidOptions opt
     }
 
     addIntegrationToSdkVersion("FeedbackShake");
-    application.registerActivityLifecycleCallbacks(this);
+    updateCallbackRegistration();
     options.getLogger().log(SentryLevel.DEBUG, "FeedbackShakeIntegration installed.");
 
     // In case of a deferred init or runtime enable, hook into any already-resumed activity
@@ -167,7 +188,7 @@ private synchronized void stopDetecting() {
     }
     detecting = false;
 
-    application.unregisterActivityLifecycleCallbacks(this);
+    updateCallbackRegistration();
     shakeDetector.close();
     currentActivityRef = null;
   }
@@ -178,6 +199,9 @@ public synchronized void close() throws IOException {
     trackedDialog = null;
     dialogRequestedShakeDetection = false;
     stopDetecting();
+    // stopDetecting is a no-op when detection wasn't running, but a tracking-only dialog may
+    // still have kept the callbacks registered.
+    updateCallbackRegistration();
   }
 
   @Override
@@ -201,7 +225,15 @@ public void onActivityPaused(final @NotNull Activity activity) {
 
   @Override
   public void onActivityCreated(
-      final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) {}
+      final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) {
+    // The user is navigating to a new activity: a dialog hosted by a different activity can't
+    // be shown there, so stop tracking it (also releasing the strong reference early instead
+    // of waiting for the host activity to be destroyed).
+    final @Nullable SentryFeedbackOptions.IShakeDialog dialog = trackedDialog;
+    if (dialog != null && findDialogActivity(dialog) != activity) {
+      setDialog(null, false);
+    }
+  }
 
   @Override
   public void onActivityStarted(final @NotNull Activity activity) {}

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

@@ -296,13 +296,66 @@ class FeedbackShakeIntegrationTest {
   }
 
   @Test
-  fun `setDialog without startShakeDetection only tracks the dialog`() {
+  fun `setDialog without startShakeDetection tracks the dialog but does not start detection`() {
+    whenever(fixture.application.getSystemService(any())).thenReturn(null)
     val sut = fixture.getSut(useShakeGesture = false)
     sut.register(fixture.scopes, fixture.options)
 
     sut.setDialog(createShakeDialog(), false)
 
-    verify(fixture.application, never()).registerActivityLifecycleCallbacks(any())
+    // Callbacks are registered to release the dialog on activity destroy, but the shake
+    // detector itself is not started.
+    verify(fixture.application).registerActivityLifecycleCallbacks(any())
+    verify(fixture.application, never()).getSystemService(eq(Context.SENSOR_SERVICE))
+  }
+
+  @Test
+  fun `clearing a tracking-only dialog unregisters the callbacks`() {
+    val sut = fixture.getSut(useShakeGesture = false)
+    sut.register(fixture.scopes, fixture.options)
+    sut.setDialog(createShakeDialog(), false)
+
+    sut.setDialog(null, false)
+
+    verify(fixture.application).unregisterActivityLifecycleCallbacks(any())
+  }
+
+  @Test
+  fun `destroying the host activity of a tracking-only dialog releases it`() {
+    // Guards against leaking the dialog (and its activity) through the strong reference when
+    // detection is not running.
+    val sut = fixture.getSut(useShakeGesture = false)
+    sut.register(fixture.scopes, fixture.options)
+    val dialog = createShakeDialog()
+    sut.setDialog(dialog, false)
+
+    sut.onActivityDestroyed(dialog.activity)
+
+    verify(fixture.application).unregisterActivityLifecycleCallbacks(any())
+  }
+
+  @Test
+  fun `creating a different activity releases the tracked dialog`() {
+    val sut = fixture.getSut(useShakeGesture = false)
+    sut.register(fixture.scopes, fixture.options)
+    sut.setDialog(createShakeDialog(), false)
+
+    val otherActivity = Robolectric.buildActivity(Activity::class.java).setup().get()
+    sut.onActivityCreated(otherActivity, null)
+
+    verify(fixture.application).unregisterActivityLifecycleCallbacks(any())
+  }
+
+  @Test
+  fun `creating the dialog's own host activity keeps the tracked dialog`() {
+    val sut = fixture.getSut(useShakeGesture = false)
+    sut.register(fixture.scopes, fixture.options)
+    val dialog = createShakeDialog()
+    sut.setDialog(dialog, false)
+
+    sut.onActivityCreated(dialog.activity, null)
+
+    verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any())
   }
 
   @Test
@@ -345,13 +398,18 @@ class FeedbackShakeIntegrationTest {
   }
 
   @Test
-  fun `disable stops shake detection when the tracked dialog did not opt in`() {
+  fun `disable keeps callbacks registered while a tracking-only dialog is set`() {
     val sut = fixture.getSut(useShakeGesture = true)
     sut.register(fixture.scopes, fixture.options)
-    sut.setDialog(createShakeDialog(), false)
+    val dialog = createShakeDialog()
+    sut.setDialog(dialog, false)
 
     sut.disable()
 
+    // Detection stops, but the callbacks stay registered to release the tracked dialog once
+    // its host activity goes away.
+    verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any())
+    sut.onActivityDestroyed(dialog.activity)
     verify(fixture.application).unregisterActivityLifecycleCallbacks(any())
   }
 
@@ -370,14 +428,18 @@ class FeedbackShakeIntegrationTest {
   }
 
   @Test
-  fun `replacing an opted-in dialog with a tracking-only one stops detection when globally disabled`() {
+  fun `replacing an opted-in dialog with a tracking-only one drops the opt-in`() {
     val sut = fixture.getSut(useShakeGesture = false)
     sut.register(fixture.scopes, fixture.options)
     sut.setDialog(createShakeDialog(), true)
 
-    // A different dialog only reporting visibility no longer justifies detection
-    sut.setDialog(createShakeDialog(), false)
+    // A different dialog only reporting visibility no longer justifies detection; the
+    // callbacks stay registered only to track the new dialog's host activity.
+    val dialog = createShakeDialog()
+    sut.setDialog(dialog, false)
+    verify(fixture.application, never()).unregisterActivityLifecycleCallbacks(any())
 
+    sut.setDialog(null, false)
     verify(fixture.application).unregisterActivityLifecycleCallbacks(any())
   }