Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 7, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c75be29
fix(android): Ensure frame metrics listeners are registered/unregiste…
markushi Jul 31, 2025
e1e6946
Fix race conditions
markushi Aug 4, 2025
7f5590e
Update Changelog
markushi Aug 4, 2025
221e9db
Merge branch 'main' into markushi/fix/frame-metrics-listener
markushi Aug 5, 2025
4f2da73
Update CHANGELOG.md
markushi Aug 5, 2025
489cf5c
Merge branch 'main' into markushi/fix/frame-metrics-listener
markushi Aug 5, 2025
2048699
Address PR feedback
markushi Aug 7, 2025
2c0ef48
Merge branch 'main' into markushi/fix/frame-metrics-listener
markushi Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
57 changes: 39 additions & 18 deletions ...-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public final class SentryFrameMetricsCollector implements Application.ActivityLi

private final @NotNull BuildInfoProvider buildInfoProvider;
private final @NotNull Set<Window> trackedWindows = new CopyOnWriteArraySet<>();

private final @NotNull ILogger logger;
private @Nullable Handler handler;
private @Nullable WeakReference<Window> currentWindow;
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
}
}
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: SentryFrameMetricsCollector: Race Conditions, Resource Leaks, Error Handling

The SentryFrameMetricsCollector has several issues:

  • Race Conditions:
    • A race between stopTrackingWindow() and trackCurrentWindow() can lead to windows being incorrectly untracked (missing frame metrics) if their main looper-posted operations execute out of order during rapid activity state changes.
    • In trackCurrentWindow(), listenerMap.isEmpty() is not re-checked inside the posted task, potentially adding a listener unnecessarily and causing a temporary resource leak.
  • Defensive Programming: Explicit API level checks for Build.VERSION_CODES.N were removed from stopTrackingWindow() and trackCurrentWindow(), reducing code robustness.
  • Error Handling: trackCurrentWindow()'s try-catch block does not cover trackedWindowsLock.acquire(), meaning lock acquisition failures would crash the app instead of being logged.
Fix in Cursor Fix in Web

}
}

Expand Down Expand Up @@ -373,13 +388,19 @@ default void addOnFrameMetricsAvailableListener(
final @NotNull Window window,
final @Nullable Window.OnFrameMetricsAvailableListener frameMetricsAvailableListener,
final @Nullable Handler handler) {
if (frameMetricsAvailableListener == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably also worth to add this check to removeOnFrameMetricsAvailableListener?

return;
}
window.addOnFrameMetricsAvailableListener(frameMetricsAvailableListener, handler);
}

@RequiresApi(api = Build.VERSION_CODES.N)
default void removeOnFrameMetricsAvailableListener(
final @NotNull Window window,
final @Nullable Window.OnFrameMetricsAvailableListener frameMetricsAvailableListener) {
if (frameMetricsAvailableListener == null) {
return;
}
window.removeOnFrameMetricsAvailableListener(frameMetricsAvailableListener);
}
}
Expand Down
66 changes: 66 additions & 0 deletions ...ore/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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)
Expand All @@ -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)
}

Expand All @@ -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)
}

Expand Down Expand Up @@ -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<Set<Window>>("trackedWindows").size)
Copy link
Preview

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using reflection to access private property 'trackedWindows' with a magic string makes the test brittle. Consider adding a package-private getter method or using a test-specific interface to access this state.

Suggested change
assertEquals(0, collector.getProperty<Set<Window>>("trackedWindows").size)
assertEquals(0, collector.trackedWindows.size)

Copilot uses AI. Check for mistakes.

}

private fun createMockWindow(refreshRate: Float = 60F): Window {
val mockWindow = mock<Window>()
val mockDisplay = mock<Display>()
Expand Down
Loading