diff --git a/CHANGELOG.md b/CHANGELOG.md index c617242fc89..a7b1c62f37f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java index fed9dd5b2d0..55342c0e4c0 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java +++ b/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 trackedWindows = new CopyOnWriteArraySet<>(); + private final @NotNull ILogger logger; private @Nullable Handler handler; private @Nullable WeakReference 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,6 +388,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); } @@ -380,6 +398,9 @@ default void addOnFrameMetricsAvailableListener( default void removeOnFrameMetricsAvailableListener( final @NotNull Window window, final @Nullable Window.OnFrameMetricsAvailableListener frameMetricsAvailableListener) { + if (frameMetricsAvailableListener == null) { + return; + } window.removeOnFrameMetricsAvailableListener(frameMetricsAvailableListener); } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt index 3f8fea1b935..b3b018e87b2 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt +++ b/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>("trackedWindows").size) + } + private fun createMockWindow(refreshRate: Float = 60F): Window { val mockWindow = mock() val mockDisplay = mock()