Fix ConcurrentModificationException due to FrameMetricsAggregator manipulation

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Fix ConcurrentModificationException due to FrameMetricsAggregator manipulation ([#2282](https://github.com/getsentry/sentry-java/pull/2282))
+
 ## 6.4.3
 
 - Fix slow and frozen frames tracking ([#2271](https://github.com/getsentry/sentry-java/pull/2271))

sentry-android-core/api/sentry-android-core.api

@@ -1,6 +1,7 @@
 public final class io/sentry/android/core/ActivityFramesTracker {
 	public fun <init> (Lio/sentry/android/core/LoadClass;)V
 	public fun <init> (Lio/sentry/android/core/LoadClass;Lio/sentry/ILogger;)V
+	public fun <init> (Lio/sentry/android/core/LoadClass;Lio/sentry/ILogger;Lio/sentry/android/core/MainLooperHandler;)V
 	public fun addActivity (Landroid/app/Activity;)V
 	public fun setMetrics (Landroid/app/Activity;Lio/sentry/protocol/SentryId;)V
 	public fun stop ()V

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

@@ -6,6 +6,8 @@
 import android.util.SparseIntArray;
 import androidx.core.app.FrameMetricsAggregator;
 import io.sentry.ILogger;
+import io.sentry.SentryLevel;
+import io.sentry.android.core.internal.util.MainThreadChecker;
 import io.sentry.protocol.MeasurementValue;
 import io.sentry.protocol.SentryId;
 import java.util.HashMap;
@@ -31,21 +33,37 @@ public final class ActivityFramesTracker {
   private final @NotNull Map<Activity, FrameCounts> frameCountAtStartSnapshots =
       new WeakHashMap<>();
 
-  public ActivityFramesTracker(final @NotNull LoadClass loadClass, final @Nullable ILogger logger) {
+  private final @Nullable ILogger logger;
+  private final @NotNull MainLooperHandler handler;
+
+  public ActivityFramesTracker(
+      final @NotNull LoadClass loadClass,
+      final @Nullable ILogger logger,
+      final @NotNull MainLooperHandler handler) {
     androidXAvailable =
         loadClass.isClassAvailable("androidx.core.app.FrameMetricsAggregator", logger);
     if (androidXAvailable) {
       frameMetricsAggregator = new FrameMetricsAggregator();
     }
+    this.logger = logger;
+    this.handler = handler;
+  }
+
+  public ActivityFramesTracker(final @NotNull LoadClass loadClass, final @Nullable ILogger logger) {
+    this(loadClass, logger, new MainLooperHandler());
   }
 
   public ActivityFramesTracker(final @NotNull LoadClass loadClass) {
     this(loadClass, null);
   }
 
   @TestOnly
-  ActivityFramesTracker(final @Nullable FrameMetricsAggregator frameMetricsAggregator) {
+  ActivityFramesTracker(
+      final @Nullable FrameMetricsAggregator frameMetricsAggregator,
+      final @NotNull MainLooperHandler handler) {
     this.frameMetricsAggregator = frameMetricsAggregator;
+    this.logger = null;
+    this.handler = handler;
   }
 
   private boolean isFrameMetricsAggregatorAvailable() {
@@ -57,7 +75,8 @@ public synchronized void addActivity(final @NotNull Activity activity) {
     if (!isFrameMetricsAggregatorAvailable()) {
       return;
     }
-    frameMetricsAggregator.add(activity);
+
+    runSafelyOnUiThread(() -> frameMetricsAggregator.add(activity), "FrameMetricsAggregator.add");
     snapshotFrameCountsAtStart(activity);
   }
 
@@ -76,6 +95,7 @@ private void snapshotFrameCountsAtStart(final @NotNull Activity activity) {
     if (frameMetricsAggregator == null) {
       return null;
     }
+
     final @Nullable SparseIntArray[] framesRates = frameMetricsAggregator.getMetrics();
 
     int totalFrames = 0;
@@ -111,18 +131,18 @@ public synchronized void setMetrics(
       return;
     }
 
-    try {
-      // NOTE: removing an activity does not reset the frame counts, only reset() does
-      frameMetricsAggregator.remove(activity);
-    } catch (Throwable ignored) {
-      // throws IllegalArgumentException when attempting to remove OnFrameMetricsAvailableListener
-      // that was never added.
-      // there's no contains method.
-      // throws NullPointerException when attempting to remove OnFrameMetricsAvailableListener and
-      // there was no
-      // Observers, See
-      // https://android.googlesource.com/platform/frameworks/base/+/140ff5ea8e2d99edc3fbe63a43239e459334c76b
-    }
+    // NOTE: removing an activity does not reset the frame counts, only reset() does
+    // throws IllegalArgumentException when attempting to remove
+    // OnFrameMetricsAvailableListener
+    // that was never added.
+    // there's no contains method.
+    // throws NullPointerException when attempting to remove
+    // OnFrameMetricsAvailableListener and
+    // there was no
+    // Observers, See
+    // https://android.googlesource.com/platform/frameworks/base/+/140ff5ea8e2d99edc3fbe63a43239e459334c76b
+    runSafelyOnUiThread(
+        () -> frameMetricsAggregator.remove(activity), "FrameMetricsAggregator.remove");
 
     final @Nullable FrameCounts frameCounts = diffFrameCountsAtEnd(activity);
 
@@ -178,12 +198,35 @@ public synchronized void setMetrics(
   @SuppressWarnings("NullAway")
   public synchronized void stop() {
     if (isFrameMetricsAggregatorAvailable()) {
-      frameMetricsAggregator.stop();
+      runSafelyOnUiThread(() -> frameMetricsAggregator.stop(), "FrameMetricsAggregator.stop");
       frameMetricsAggregator.reset();
     }
     activityMeasurements.clear();
   }
 
+  private void runSafelyOnUiThread(final Runnable runnable, final String tag) {
+    try {
+      if (MainThreadChecker.isMainThread()) {
+        runnable.run();
+      } else {
+        handler.post(
+            () -> {
+              try {
+                runnable.run();
+              } catch (Throwable t) {
+                if (logger != null) {
+                  logger.log(SentryLevel.ERROR, "Failed to execute " + tag, t);
+                }
+              }
+            });
+      }
+    } catch (Throwable t) {
+      if (logger != null) {
+        logger.log(SentryLevel.ERROR, "Failed to execute " + tag, t);
+      }
+    }
+  }
+
   private static final class FrameCounts {
     private final int totalFrames;
     private final int slowFrames;

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

@@ -24,9 +24,10 @@ class ActivityFramesTrackerTest {
         val activity = mock<Activity>()
         val sentryId = SentryId()
         val loadClass = mock<LoadClass>()
+        val handler = mock<MainLooperHandler>()
 
         fun getSut(): ActivityFramesTracker {
-            return ActivityFramesTracker(aggregator)
+            return ActivityFramesTracker(aggregator, handler)
         }
     }
     private val fixture = Fixture()
@@ -341,6 +342,42 @@ class ActivityFramesTrackerTest {
         assertNull(sut.takeMetrics(fixture.sentryId))
     }
 
+    @Test
+    fun `addActivity call to FrameMetricsTracker is done on the main thread, even when being called from a background thread`() {
+        val sut = fixture.getSut()
+
+        val addThread = Thread {
+            sut.addActivity(fixture.activity)
+        }
+        addThread.start()
+        addThread.join(500)
+        verify(fixture.handler).post(any())
+    }
+
+    @Test
+    fun `setMetrics call to FrameMetricsTracker is done on the main thread, even when being called from a background thread`() {
+        val sut = fixture.getSut()
+
+        val setMetricsThread = Thread {
+            sut.setMetrics(fixture.activity, fixture.sentryId)
+        }
+        setMetricsThread.start()
+        setMetricsThread.join(500)
+        verify(fixture.handler).post(any())
+    }
+
+    @Test
+    fun `stop call to FrameMetricsTracker is done on the main thread, even when being called from a background thread`() {
+        val sut = fixture.getSut()
+
+        val stopThread = Thread {
+            sut.stop()
+        }
+        stopThread.start()
+        stopThread.join(500)
+        verify(fixture.handler).post(any())
+    }
+
     private fun getArray(frameTime: Int = 1, numFrames: Int = 1): Array<SparseIntArray?> {
         val totalArray = SparseIntArray()
         totalArray.put(frameTime, numFrames)