fix(android): Ensure frame metrics listeners are registered/unregistered on the main thread (#4582)

* fix(android): Ensure frame metrics listeners are registered/unregistered on the main thread

* Fix race conditions

* Update Changelog

* Update CHANGELOG.md

* Address PR feedback

Author: markushi

CHANGELOG.md

@@ -22,6 +22,7 @@
   }
   ```
 - Fix abstract method error in `SentrySupportSQLiteDatabase` ([#4597](https://github.com/getsentry/sentry-java/pull/4597))
+- Ensure frame metrics listeners are registered/unregistered on the main thread ([#4582](https://github.com/getsentry/sentry-java/pull/4582))
 
 ## 8.18.0
 

sentry-android-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java

@@ -38,6 +38,7 @@ public final class SentryFrameMetricsCollector implements Application.ActivityLi
 
   private final @NotNull BuildInfoProvider buildInfoProvider;
   private final @NotNull Set<Window> trackedWindows = new CopyOnWriteArraySet<>();
+
   private final @NotNull ILogger logger;
   private @Nullable Handler handler;
   private @Nullable WeakReference<Window> currentWindow;
@@ -282,17 +283,20 @@ public void stopCollection(final @Nullable String listenerId) {
 
   @SuppressLint("NewApi")
   private void stopTrackingWindow(final @NotNull Window window) {
-    if (trackedWindows.contains(window)) {
-      if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.N) {
-        try {
-          windowFrameMetricsManager.removeOnFrameMetricsAvailableListener(
-              window, frameMetricsAvailableListener);
-        } catch (Exception e) {
-          logger.log(SentryLevel.ERROR, "Failed to remove frameMetricsAvailableListener", e);
-        }
-      }
-      trackedWindows.remove(window);
-    }
+    new Handler(Looper.getMainLooper())
+        .post(
+            () -> {
+              try {
+                // Re-check if we should still remove the listener for this window
+                // in case trackCurrentWindow was called in the meantime
+                if (trackedWindows.remove(window)) {
+                  windowFrameMetricsManager.removeOnFrameMetricsAvailableListener(
+                      window, frameMetricsAvailableListener);
+                }
+              } catch (Throwable e) {
+                logger.log(SentryLevel.ERROR, "Failed to remove frameMetricsAvailableListener", e);
+              }
+            });
   }
 
   private void setCurrentWindow(final @NotNull Window window) {
@@ -305,18 +309,29 @@ private void setCurrentWindow(final @NotNull Window window) {
 
   @SuppressLint("NewApi")
   private void trackCurrentWindow() {
-    Window window = currentWindow != null ? currentWindow.get() : null;
+    @Nullable Window window = currentWindow != null ? currentWindow.get() : null;
     if (window == null || !isAvailable) {
       return;
     }
 
-    if (!trackedWindows.contains(window) && !listenerMap.isEmpty()) {
+    if (listenerMap.isEmpty()) {
+      return;
+    }
 
-      if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.N && handler != null) {
-        trackedWindows.add(window);
-        windowFrameMetricsManager.addOnFrameMetricsAvailableListener(
-            window, frameMetricsAvailableListener, handler);
-      }
+    if (handler != null) {
+      // Ensure the addOnFrameMetricsAvailableListener is called on the main thread
+      new Handler(Looper.getMainLooper())
+          .post(
+              () -> {
+                if (trackedWindows.add(window)) {
+                  try {
+                    windowFrameMetricsManager.addOnFrameMetricsAvailableListener(
+                        window, frameMetricsAvailableListener, handler);
+                  } catch (Throwable e) {
+                    logger.log(SentryLevel.ERROR, "Failed to add frameMetricsAvailableListener", e);
+                  }
+                }
+              });
     }
   }
 
@@ -373,13 +388,19 @@ default void addOnFrameMetricsAvailableListener(
         final @NotNull Window window,
         final @Nullable Window.OnFrameMetricsAvailableListener frameMetricsAvailableListener,
         final @Nullable Handler handler) {
+      if (frameMetricsAvailableListener == null) {
+        return;
+      }
       window.addOnFrameMetricsAvailableListener(frameMetricsAvailableListener, handler);
     }
 
     @RequiresApi(api = Build.VERSION_CODES.N)
     default void removeOnFrameMetricsAvailableListener(
         final @NotNull Window window,
         final @Nullable Window.OnFrameMetricsAvailableListener frameMetricsAvailableListener) {
+      if (frameMetricsAvailableListener == null) {
+        return;
+      }
       window.removeOnFrameMetricsAvailableListener(frameMetricsAvailableListener);
     }
   }

sentry-android-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt

@@ -148,6 +148,9 @@ class SentryFrameMetricsCollectorTest {
     collector.startCollection(mock())
     assertEquals(0, fixture.addOnFrameMetricsAvailableListenerCounter)
     collector.onActivityStarted(fixture.activity)
+    // Execute pending main looper tasks since addOnFrameMetricsAvailableListener is posted to main
+    // thread
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
   }
 
@@ -157,8 +160,12 @@ class SentryFrameMetricsCollectorTest {
 
     collector.startCollection(mock())
     collector.onActivityStarted(fixture.activity)
+    // Execute pending add operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(0, fixture.removeOnFrameMetricsAvailableListenerCounter)
     collector.onActivityStopped(fixture.activity)
+    // Execute pending remove operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
   }
 
@@ -181,6 +188,8 @@ class SentryFrameMetricsCollectorTest {
     collector.onActivityStarted(fixture.activity)
     assertEquals(0, fixture.addOnFrameMetricsAvailableListenerCounter)
     collector.startCollection(mock())
+    // Execute pending main looper tasks
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
   }
 
@@ -189,9 +198,13 @@ class SentryFrameMetricsCollectorTest {
     val collector = fixture.getSut(context)
     val id = collector.startCollection(mock())
     collector.onActivityStarted(fixture.activity)
+    // Execute pending add operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
 
     assertEquals(0, fixture.removeOnFrameMetricsAvailableListenerCounter)
     collector.stopCollection(id)
+    // Execute pending remove operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
   }
 
@@ -205,9 +218,13 @@ class SentryFrameMetricsCollectorTest {
 
     collector.onActivityStarted(fixture.activity)
     collector.onActivityStarted(fixture.activity)
+    // Execute pending add operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
 
     collector.onActivityStopped(fixture.activity)
     collector.onActivityStopped(fixture.activity)
+    // Execute pending remove operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
 
     assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
     assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
@@ -228,9 +245,13 @@ class SentryFrameMetricsCollectorTest {
     collector.startCollection(mock())
     collector.onActivityStarted(fixture.activity)
     collector.onActivityStarted(fixture.activity2)
+    // Execute pending add operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(2, fixture.addOnFrameMetricsAvailableListenerCounter)
     collector.onActivityStopped(fixture.activity)
     collector.onActivityStopped(fixture.activity2)
+    // Execute pending remove operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(2, fixture.removeOnFrameMetricsAvailableListenerCounter)
   }
 
@@ -240,10 +261,13 @@ class SentryFrameMetricsCollectorTest {
     val id1 = collector.startCollection(mock())
     val id2 = collector.startCollection(mock())
     collector.onActivityStarted(fixture.activity)
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
     collector.stopCollection(id1)
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(0, fixture.removeOnFrameMetricsAvailableListenerCounter)
     collector.stopCollection(id2)
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
     assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
   }
 
@@ -511,6 +535,48 @@ class SentryFrameMetricsCollectorTest {
     )
   }
 
+  @Test
+  fun `collector calls addOnFrameMetricsAvailableListener on main thread`() {
+    val collector = fixture.getSut(context)
+
+    collector.startCollection(mock())
+    collector.onActivityStarted(fixture.activity)
+
+    assertEquals(0, fixture.addOnFrameMetricsAvailableListenerCounter)
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
+    assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
+  }
+
+  @Test
+  fun `collector calls removeOnFrameMetricsAvailableListener on main thread`() {
+    val collector = fixture.getSut(context)
+    collector.startCollection(mock())
+    collector.onActivityStarted(fixture.activity)
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
+
+    collector.onActivityStopped(fixture.activity)
+    assertEquals(0, fixture.removeOnFrameMetricsAvailableListenerCounter)
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
+    assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
+  }
+
+  @Test
+  fun `collector prevents race condition when stop is called immediately after start`() {
+    val collector = fixture.getSut(context)
+
+    collector.startCollection(mock())
+    collector.onActivityStarted(fixture.activity)
+    collector.onActivityStopped(fixture.activity)
+
+    // Now execute all pending operations
+    Shadows.shadowOf(Looper.getMainLooper()).idle()
+
+    // as the listeners are posted to the main thread, we expect an add followed by a remove
+    assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
+    assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
+    assertEquals(0, collector.getProperty<Set<Window>>("trackedWindows").size)
+  }
+
   private fun createMockWindow(refreshRate: Float = 60F): Window {
     val mockWindow = mock<Window>()
     val mockDisplay = mock<Display>()