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

markushi · 2025-07-31T09:05:01Z

📜 Description

Fixes #4577

💡 Motivation and Context

The Android View system expects add/remove calls to happen on the main thread.

💚 How did you test it?

Added unit tests

📝 Checklist

I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
I updated the wizard if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

…red on the main thread

Copilot

Pull Request Overview

This PR ensures that frame metrics listeners are properly registered and unregistered on the Android main thread to comply with the Android View system requirements. The changes prevent potential threading issues when adding/removing frame metrics listeners.

Wraps addOnFrameMetricsAvailableListener and removeOnFrameMetricsAvailableListener calls in Handler.post() to execute on main thread
Adds race condition protection by re-checking tracking state before executing listener operations
Updates unit tests to handle asynchronous listener operations by idling the main looper

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
SentryFrameMetricsCollector.java	Modified listener registration/unregistration to execute on main thread with race condition protection
SentryFrameMetricsCollectorTest.kt	Added main looper idle calls to existing tests and new tests to verify main thread execution

Copilot · 2025-07-31T09:06:24Z

...oid-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java

+                    windowFrameMetricsManager.removeOnFrameMetricsAvailableListener(
+                        window, frameMetricsAvailableListener);
+                  }
+                } catch (Throwable e) {


Catching Throwable is overly broad and can mask programming errors. Consider catching more specific exceptions like IllegalStateException or RuntimeException that might be thrown by removeOnFrameMetricsAvailableListener.

Suggested change

} catch (Throwable e) {

} catch (RuntimeException e) {

...oid-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java

Copilot · 2025-07-31T09:06:25Z

...d-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt

+    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<Set<Window>>("trackedWindows").size)


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)

github-actions · 2025-07-31T09:14:48Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	491.28 ms	557.98 ms	66.70 ms
Size	1.58 MiB	2.09 MiB	521.83 KiB

Previous results on branch: markushi/fix/frame-metrics-listener

Startup times

Revision	Plain	With Sentry	Diff
`13818f8`	429.13 ms	471.91 ms	42.79 ms
`668c641`	432.42 ms	458.46 ms	26.04 ms
`961343d`	328.70 ms	365.08 ms	36.39 ms
`134c911`	402.52 ms	423.61 ms	21.09 ms

App size

Revision	Plain	With Sentry	Diff
`13818f8`	1.58 MiB	2.09 MiB	521.44 KiB
`668c641`	1.58 MiB	2.09 MiB	521.84 KiB
`961343d`	1.58 MiB	2.09 MiB	521.76 KiB
`134c911`	1.58 MiB	2.09 MiB	521.84 KiB

romtsn · 2025-08-06T14:54:22Z

...oid-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java

+                // 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);


I think should be enough to just call remove because it will return false if the set did not contain the element?

Suggested change

shouldRemove = trackedWindows.contains(window) && trackedWindows.remove(window);

shouldRemove = trackedWindows.remove(window);

romtsn · 2025-08-07T07:17:33Z

...oid-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java

@@ -373,6 +401,9 @@ default void addOnFrameMetricsAvailableListener(
        final @NotNull Window window,
        final @Nullable Window.OnFrameMetricsAvailableListener frameMetricsAvailableListener,
        final @Nullable Handler handler) {
+      if (frameMetricsAvailableListener == null) {


probably also worth to add this check to removeOnFrameMetricsAvailableListener?

romtsn · 2025-08-07T07:20:01Z

...oid-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java

+                // 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);


same here I guess, could omit contains since it's a set?

romtsn · 2025-08-07T07:20:52Z

...oid-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java

+                // 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()) {


do we actually need this lock considering we only add/remove windows on the main thread now?

Oh true, that's some leftover, trackedWindows is only accessed within the main thread - thanks!

romtsn

a few comments, but nothing blocking

…red on the main thread (#4582) * fix(android): Ensure frame metrics listeners are registered/unregistered on the main thread * Fix race conditions * Update Changelog * Update CHANGELOG.md * Address PR feedback

* Do not report cached events as lost * E2E tests for OpenTelemetry based console sample (#4563) * e2e tests for console app * fix test failures by waiting for 10s after first try to find envelopes * add system-test-runner.py script to replace bash scripts for running e2e / system tests * use py script for ci, cleanup, makefile * Format code * remove bash scripts * install requests module * api * fix gh script * Implement E2E tests for OTel based console sample * fixes after merge * Format code * e2e tests for console app * Implement E2E tests for OTel based console sample * fixes after merge * Format code * api * Reduce scope forking when using OpenTelemetry (#4565) * Reduce scope forking in OpenTelemetry * Format code * api * changelog --------- Co-authored-by: Sentry Github Bot <[email protected]> * SDKs send queue is no longer shutdown immediately on re-init (#4564) * Let queue drain on a restart * Format code * Format code * Update sentry-samples/sentry-samples-console-opentelemetry-noagent/src/test/kotlin/sentry/systemtest/ConsoleApplicationSystemTest.kt * Let queue drain on a restart * Format code * Format code * Update sentry-samples/sentry-samples-console-opentelemetry-noagent/src/test/kotlin/sentry/systemtest/ConsoleApplicationSystemTest.kt * adapt tests * changelog --------- Co-authored-by: Sentry Github Bot <[email protected]> --------- Co-authored-by: Sentry Github Bot <[email protected]> * release: 8.18.0 * ref(replay): Use main thread to schedule capture (#4542) * perf(connectivity): Cache network capabilities and status to reduce IPC calls (#4560) * fix(breadcrumbs): Deduplicate battery breadcrumbs (#4561) * fix(ci): remove obsolete NDK debug symbols (#4581) As they don't exist anymore and this is done within sentry-native directly: https://github.com/getsentry/sentry-native/pull/1327/files * fix(android): Remove unused method (#4585) * fix(android): Remove unused method * Update Changelog * Add rules file for documenting SDK offline behaviour (#4572) #skip-changelog ## 📜 Description  Add rules file for documenting SDK offline behaviour ## 💡 Motivation and Context   Should help speed up AI reasoning about the SDK offline/retry behaviour. ## 💚 How did you test it? ## 📝 Checklist  - [ ] I added tests to verify the changes. - [ ] No new PII added or SDK only sends newly added PII if `sendDefaultPII` is enabled. - [ ] I updated the docs if needed. - [ ] I updated the wizard if needed. - [ ] Review from the native team if needed. - [ ] No breaking change or entry added to the changelog. - [ ] No breaking change for hybrid SDKs or communicated to hybrid SDKs. ## 🔮 Next steps * perf(connectivity): Have only one NetworkCallback active at a time (#4562) * fix(scripts): update-gradle script set-version (#4591) * fix: sentry-android-ndk proguard rule keeps all native class (#4427) * fix: sentry-androi-ndk proguard rule keeps all native class * docs: update CHANGELOG * fix: update CHANGELOG * Update CHANGELOG.md * Update CHANGELOG.md --------- Co-authored-by: Markus Hintersteiner <[email protected]> Co-authored-by: Markus Hintersteiner <[email protected]> * refactor(lifecycle): Use single lifecycle observer (#4567) * perf(connectivity): Cache network capabilities and status to reduce IPC calls * changelog * Changelog * revert * fix(breadcrumbs): Deduplicate battery breadcrumbs * ref * Changelog * Fix test * perf(connectivity): Have only one NetworkCallback active at a time * Changelog * perf(integrations): Use single lifecycle observer * Add tests * Changelog * Fix tests * Improve callback handling and test visibility (#4593) * Null-check lifecycleObserver --------- Co-authored-by: Markus Hintersteiner <[email protected]> * fix(sqlite): Fix abstract method error (#4597) * fix(sqlite): Fix abstract method error * Update CHANGELOG.md * Suppress metadata version checks * perf(integrations): Do not register for SystemEvents and NetworkCallbacks when launched with background importance (#4579) * fix(android): Ensure frame metrics listeners are registered/unregistered on the main thread (#4582) * fix(android): Ensure frame metrics listeners are registered/unregistered on the main thread * Fix race conditions * Update Changelog * Update CHANGELOG.md * Address PR feedback * perf(executor): Prewarm SentryExecutorService (#4606) * review feedback * changelog * pass through whether cache stored in AndroidEnvelopeCache + test * Format code --------- Co-authored-by: Sentry Github Bot <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: Roman Zavarnitsyn <[email protected]> Co-authored-by: Markus Hintersteiner <[email protected]> Co-authored-by: Ghasem Shirdel <[email protected]> Co-authored-by: Markus Hintersteiner <[email protected]>

fix(android): Ensure frame metrics listeners are registered/unregiste…

c75be29

…red on the main thread

markushi requested review from adinauer, romtsn, stefanosiano and lcian as code owners July 31, 2025 09:05

markushi requested a review from Copilot July 31, 2025 09:05

Copilot AI reviewed Jul 31, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

Fix race conditions

e1e6946

This comment was marked as outdated.

Sign in to view

markushi added 2 commits August 4, 2025 07:59

Update Changelog

7f5590e

Merge branch 'main' into markushi/fix/frame-metrics-listener

221e9db

This comment was marked as outdated.

Sign in to view

markushi added 2 commits August 5, 2025 13:01

Update CHANGELOG.md

4f2da73

Merge branch 'main' into markushi/fix/frame-metrics-listener

489cf5c

This comment was marked as outdated.

Sign in to view

romtsn reviewed Aug 6, 2025

View reviewed changes

romtsn reviewed Aug 7, 2025

View reviewed changes

romtsn approved these changes Aug 7, 2025

View reviewed changes

markushi added 2 commits August 7, 2025 12:30

Address PR feedback

2048699

Merge branch 'main' into markushi/fix/frame-metrics-listener

2c0ef48

markushi merged commit 616af61 into main Aug 7, 2025
35 of 37 checks passed

markushi deleted the markushi/fix/frame-metrics-listener branch August 7, 2025 12:09

antonis mentioned this pull request Aug 11, 2025

chore(deps): v7: update Android SDK to v8.19.0 getsentry/sentry-react-native#5063

Draft

10 tasks

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

	shouldRemove = trackedWindows.contains(window) && trackedWindows.remove(window);
	shouldRemove = trackedWindows.remove(window);

Uh oh!

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

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

Uh oh!

Conversation

markushi commented Jul 31, 2025

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: markushi/fix/frame-metrics-listener

Startup times

App size

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

romtsn Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

romtsn Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

romtsn Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

romtsn Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

markushi Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

romtsn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 31, 2025 •

edited

Loading