From c75be29b12c0311463b7f151c7076445fbd7e37e Mon Sep 17 00:00:00 2001 From: markushi Date: Thu, 31 Jul 2025 11:02:17 +0200 Subject: [PATCH 1/5] fix(android): Ensure frame metrics listeners are registered/unregistered on the main thread --- .../util/SentryFrameMetricsCollector.java | 61 ++++++++++++----- .../util/SentryFrameMetricsCollectorTest.kt | 66 +++++++++++++++++++ 2 files changed, 110 insertions(+), 17 deletions(-) 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..a482e01d95a 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 @@ -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); } 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..1b84e8e07ae 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() + + 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>("trackedWindows").size) + } + private fun createMockWindow(refreshRate: Float = 60F): Window { val mockWindow = mock() val mockDisplay = mock() From e1e69468748d6205abb719a9bd186e8ce4f97994 Mon Sep 17 00:00:00 2001 From: markushi Date: Mon, 4 Aug 2025 07:50:57 +0200 Subject: [PATCH 2/5] Fix race conditions --- .../util/SentryFrameMetricsCollector.java | 50 ++++++++++--------- .../util/SentryFrameMetricsCollectorTest.kt | 4 +- 2 files changed, 29 insertions(+), 25 deletions(-) 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 a482e01d95a..fffd50d0321 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 @@ -14,11 +14,13 @@ import android.view.Window; import androidx.annotation.RequiresApi; import io.sentry.ILogger; +import io.sentry.ISentryLifecycleToken; import io.sentry.SentryLevel; import io.sentry.SentryOptions; import io.sentry.SentryUUID; import io.sentry.android.core.BuildInfoProvider; import io.sentry.android.core.ContextUtils; +import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.Objects; import java.lang.ref.WeakReference; import java.lang.reflect.Field; @@ -38,6 +40,8 @@ public final class SentryFrameMetricsCollector implements Application.ActivityLi private final @NotNull BuildInfoProvider buildInfoProvider; private final @NotNull Set trackedWindows = new CopyOnWriteArraySet<>(); + private final @NotNull AutoClosableReentrantLock trackedWindowsLock = + new AutoClosableReentrantLock(); private final @NotNull ILogger logger; private @Nullable Handler handler; private @Nullable WeakReference currentWindow; @@ -282,23 +286,24 @@ public void stopCollection(final @Nullable String listenerId) { @SuppressLint("NewApi") private void stopTrackingWindow(final @NotNull Window 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); + 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 + final boolean shouldRemove; + try (final @NotNull ISentryLifecycleToken ignored = trackedWindowsLock.acquire()) { + shouldRemove = trackedWindows.contains(window) && trackedWindows.remove(window); } - }); - } + if (shouldRemove) { + windowFrameMetricsManager.removeOnFrameMetricsAvailableListener( + window, frameMetricsAvailableListener); + } + } catch (Throwable e) { + logger.log(SentryLevel.ERROR, "Failed to remove frameMetricsAvailableListener", e); + } + }); } private void setCurrentWindow(final @NotNull Window window) { @@ -316,23 +321,22 @@ private void trackCurrentWindow() { return; } - if (trackedWindows.contains(window)) { - return; - } - if (listenerMap.isEmpty()) { return; } 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)) { + // in case stopTrackingWindow was called for the same Window in the meantime + final boolean shouldAdd; + try (final @NotNull ISentryLifecycleToken ignored = trackedWindowsLock.acquire()) { + shouldAdd = !trackedWindows.contains(window) && trackedWindows.add(window); + } + if (shouldAdd) { try { windowFrameMetricsManager.addOnFrameMetricsAvailableListener( window, frameMetricsAvailableListener, handler); 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 1b84e8e07ae..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 @@ -571,8 +571,8 @@ class SentryFrameMetricsCollectorTest { // 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 + // 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) } From 7f5590e8d55de78648fedcf8facf17b90dc082da Mon Sep 17 00:00:00 2001 From: markushi Date: Mon, 4 Aug 2025 07:59:07 +0200 Subject: [PATCH 3/5] Update Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85dc62b5559..d9b92c7ff39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ - Reduce scope forking when using OpenTelemetry ([#4565](https://github.com/getsentry/sentry-java/pull/4565)) - `Sentry.withScope` now has the correct current scope passed to the callback. Previously our OpenTelemetry integration forked scopes an additional. - Overall the SDK is now forking scopes a bit less often. +- Ensure frame metrics listeners are registered/unregistered on the main thread ([#4582](https://github.com/getsentry/sentry-java/pull/4582)) ## 8.17.0 From 4f2da737d4a1bef103f2e654134f41bd804a4b36 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Tue, 5 Aug 2025 13:01:27 +0200 Subject: [PATCH 4/5] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40e64e74a2f..96e3df5e06e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ native ; } ``` +- Ensure frame metrics listeners are registered/unregistered on the main thread ([#4582](https://github.com/getsentry/sentry-java/pull/4582)) ## 8.18.0 @@ -56,7 +57,6 @@ - Reduce scope forking when using OpenTelemetry ([#4565](https://github.com/getsentry/sentry-java/pull/4565)) - `Sentry.withScope` now has the correct current scope passed to the callback. Previously our OpenTelemetry integration forked scopes an additional. - Overall the SDK is now forking scopes a bit less often. -- Ensure frame metrics listeners are registered/unregistered on the main thread ([#4582](https://github.com/getsentry/sentry-java/pull/4582)) ## 8.17.0 From 2048699535b41ef2e82c5ab7538b1f28524af6f8 Mon Sep 17 00:00:00 2001 From: markushi Date: Thu, 7 Aug 2025 12:30:08 +0200 Subject: [PATCH 5/5] Address PR feedback --- .../util/SentryFrameMetricsCollector.java | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) 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 fffd50d0321..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 @@ -14,13 +14,11 @@ import android.view.Window; import androidx.annotation.RequiresApi; import io.sentry.ILogger; -import io.sentry.ISentryLifecycleToken; import io.sentry.SentryLevel; import io.sentry.SentryOptions; import io.sentry.SentryUUID; import io.sentry.android.core.BuildInfoProvider; import io.sentry.android.core.ContextUtils; -import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.Objects; import java.lang.ref.WeakReference; import java.lang.reflect.Field; @@ -40,8 +38,7 @@ public final class SentryFrameMetricsCollector implements Application.ActivityLi private final @NotNull BuildInfoProvider buildInfoProvider; private final @NotNull Set trackedWindows = new CopyOnWriteArraySet<>(); - private final @NotNull AutoClosableReentrantLock trackedWindowsLock = - new AutoClosableReentrantLock(); + private final @NotNull ILogger logger; private @Nullable Handler handler; private @Nullable WeakReference currentWindow; @@ -292,11 +289,7 @@ private void stopTrackingWindow(final @NotNull Window window) { try { // Re-check if we should still remove the listener for this window // in case trackCurrentWindow was called in the meantime - final boolean shouldRemove; - try (final @NotNull ISentryLifecycleToken ignored = trackedWindowsLock.acquire()) { - shouldRemove = trackedWindows.contains(window) && trackedWindows.remove(window); - } - if (shouldRemove) { + if (trackedWindows.remove(window)) { windowFrameMetricsManager.removeOnFrameMetricsAvailableListener( window, frameMetricsAvailableListener); } @@ -330,13 +323,7 @@ private void trackCurrentWindow() { new Handler(Looper.getMainLooper()) .post( () -> { - // Re-check if we should still track this window - // in case stopTrackingWindow was called for the same Window in the meantime - final boolean shouldAdd; - try (final @NotNull ISentryLifecycleToken ignored = trackedWindowsLock.acquire()) { - shouldAdd = !trackedWindows.contains(window) && trackedWindows.add(window); - } - if (shouldAdd) { + if (trackedWindows.add(window)) { try { windowFrameMetricsManager.addOnFrameMetricsAvailableListener( window, frameMetricsAvailableListener, handler); @@ -411,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); } }