Fix ConcurrentModificationException due to FrameMetricsAggregator manipulation

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+
+- 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,29 @@ public synchronized void addActivity(final @NotNull Activity activity) {
     if (!isFrameMetricsAggregatorAvailable()) {
       return;
     }
-    frameMetricsAggregator.add(activity);
+
+    try {
+      if (MainThreadChecker.isMainThread()) {
+        frameMetricsAggregator.add(activity);
+      } else {
+        handler.post(
+            () -> {
+              try {
+                frameMetricsAggregator.add(activity);
+              } catch (Throwable t) {
+                if (logger != null) {
+                  logger.log(
+                      SentryLevel.ERROR, "Failed to add Activity to FrameMetricsAggregator", t);
+                }
+              }
+            });
+      }
+    } catch (Throwable t) {
+      if (logger != null) {
+        logger.log(SentryLevel.ERROR, "Failed to add Activity to FrameMetricsAggregator", t);
+      }
+    }
+
     snapshotFrameCountsAtStart(activity);
   }
 
@@ -76,7 +116,8 @@ private void snapshotFrameCountsAtStart(final @NotNull Activity activity) {
     if (frameMetricsAggregator == null) {
       return null;
     }
-    final @Nullable SparseIntArray[] framesRates = frameMetricsAggregator.getMetrics();
+
+    @Nullable final SparseIntArray[] framesRates = frameMetricsAggregator.getMetrics();
 
     int totalFrames = 0;
     int slowFrames = 0;
@@ -112,16 +153,37 @@ public synchronized void setMetrics(
     }
 
     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
+      if (MainThreadChecker.isMainThread()) {
+        frameMetricsAggregator.remove(activity);
+      } else {
+        handler.post(
+            () -> {
+              try {
+                // NOTE: removing an activity does not reset the frame counts, only reset() does
+                frameMetricsAggregator.remove(activity);
+              } catch (Throwable t) {
+                // 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
+                if (logger != null) {
+                  logger.log(
+                      SentryLevel.ERROR,
+                      "Failed to remove Activity from FrameMetricsAggregator",
+                      t);
+                }
+              }
+            });
+      }
+    } catch (Throwable t) {
+      if (logger != null) {
+        logger.log(SentryLevel.ERROR, "Failed to remove Activity from FrameMetricsAggregator", t);
+      }
     }
 
     final @Nullable FrameCounts frameCounts = diffFrameCountsAtEnd(activity);
@@ -178,7 +240,27 @@ public synchronized void setMetrics(
   @SuppressWarnings("NullAway")
   public synchronized void stop() {
     if (isFrameMetricsAggregatorAvailable()) {
-      frameMetricsAggregator.stop();
+      try {
+        if (MainThreadChecker.isMainThread()) {
+          frameMetricsAggregator.stop();
+        } else {
+          handler.post(
+              () -> {
+                try {
+                  frameMetricsAggregator.stop();
+                } catch (Throwable t) {
+                  if (logger != null) {
+                    logger.log(SentryLevel.ERROR, "Failed to stop FrameMetricsAggregator", t);
+                  }
+                }
+              });
+        }
+      } catch (Throwable t) {
+        if (logger != null) {
+          logger.log(SentryLevel.ERROR, "Failed to stop FrameMetricsAggregator", t);
+        }
+      }
+
       frameMetricsAggregator.reset();
     }
     activityMeasurements.clear();

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)