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

Author: markushi

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

@@ -282,16 +282,22 @@ 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);
+    final boolean wasTracked = trackedWindows.remove(window);
+    if (wasTracked) {
+      new Handler(Looper.getMainLooper())
+          .post(
+              () -> {
+                try {
+                  // Re-check if we should still stop tracking this window
+                  if (!trackedWindows.contains(window)) {
+                    windowFrameMetricsManager.removeOnFrameMetricsAvailableListener(
+                        window, frameMetricsAvailableListener);
+                  }
+                } catch (Throwable e) {
+                  logger.log(
+                      SentryLevel.ERROR, "Failed to remove frameMetricsAvailableListener", e);
+                }
+              });
     }
   }
 
@@ -305,18 +311,36 @@ 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 (trackedWindows.contains(window)) {
+      return;
+    }
+
+    if (listenerMap.isEmpty()) {
+      return;
+    }
 
-      if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.N && handler != null) {
-        trackedWindows.add(window);
-        windowFrameMetricsManager.addOnFrameMetricsAvailableListener(
-            window, frameMetricsAvailableListener, handler);
-      }
+    if (handler != null) {
+      trackedWindows.add(window);
+      // Ensure the addOnFrameMetricsAvailableListener is called on the main thread
+      new Handler(Looper.getMainLooper())
+          .post(
+              () -> {
+                // Re-check if we should still track this window
+                // in case stopTrackingWindow was called in the meantime
+                if (trackedWindows.contains(window)) {
+                  try {
+                    windowFrameMetricsManager.addOnFrameMetricsAvailableListener(
+                        window, frameMetricsAvailableListener, handler);
+                  } catch (Throwable e) {
+                    logger.log(SentryLevel.ERROR, "Failed to add frameMetricsAvailableListener", e);
+                  }
+                }
+              });
     }
   }
 
@@ -373,6 +397,9 @@ default void addOnFrameMetricsAvailableListener(
         final @NotNull Window window,
         final @Nullable Window.OnFrameMetricsAvailableListener frameMetricsAvailableListener,
         final @Nullable Handler handler) {
+      if (frameMetricsAvailableListener == null) {
+        return;
+      }
       window.addOnFrameMetricsAvailableListener(frameMetricsAvailableListener, handler);
     }
 

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()
+
+    assertEquals(0, fixture.addOnFrameMetricsAvailableListenerCounter)
+    // remove will still execute as it has no clue that add bailed on the main thread
+    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>()