Skip to content

Commit 134c911

Browse files
markushimarkushi
authored
Merge 489cf5c into 0048b42 
2 parents 0048b42 + 489cf5c commit 134c911

File tree

3 files changed

+116
-18
lines changed

3 files changed

+116
-18
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
}
2222
```
2323
- Fix abstract method error in `SentrySupportSQLiteDatabase` ([#4597](https://github.com/getsentry/sentry-java/pull/4597))
24+
- Ensure frame metrics listeners are registered/unregistered on the main thread ([#4582](https://github.com/getsentry/sentry-java/pull/4582))
2425

2526
## 8.18.0
2627

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

Lines changed: 49 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,13 @@
1414
import android.view.Window;
1515
import androidx.annotation.RequiresApi;
1616
import io.sentry.ILogger;
17+
import io.sentry.ISentryLifecycleToken;
1718
import io.sentry.SentryLevel;
1819
import io.sentry.SentryOptions;
1920
import io.sentry.SentryUUID;
2021
import io.sentry.android.core.BuildInfoProvider;
2122
import io.sentry.android.core.ContextUtils;
23+
import io.sentry.util.AutoClosableReentrantLock;
2224
import io.sentry.util.Objects;
2325
import java.lang.ref.WeakReference;
2426
import java.lang.reflect.Field;
@@ -38,6 +40,8 @@ public final class SentryFrameMetricsCollector implements Application.ActivityLi
3840

3941
private final @NotNull BuildInfoProvider buildInfoProvider;
4042
private final @NotNull Set<Window> trackedWindows = new CopyOnWriteArraySet<>();
43+
private final @NotNull AutoClosableReentrantLock trackedWindowsLock =
44+
new AutoClosableReentrantLock();
4145
private final @NotNull ILogger logger;
4246
private @Nullable Handler handler;
4347
private @Nullable WeakReference<Window> currentWindow;
@@ -282,17 +286,24 @@ public void stopCollection(final @Nullable String listenerId) {
282286

283287
@SuppressLint("NewApi")
284288
private void stopTrackingWindow(final @NotNull Window window) {
285-
if (trackedWindows.contains(window)) {
286-
if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.N) {
287-
try {
288-
windowFrameMetricsManager.removeOnFrameMetricsAvailableListener(
289-
window, frameMetricsAvailableListener);
290-
} catch (Exception e) {
291-
logger.log(SentryLevel.ERROR, "Failed to remove frameMetricsAvailableListener", e);
292-
}
293-
}
294-
trackedWindows.remove(window);
295-
}
289+
new Handler(Looper.getMainLooper())
290+
.post(
291+
() -> {
292+
try {
293+
// Re-check if we should still remove the listener for this window
294+
// in case trackCurrentWindow was called in the meantime
295+
final boolean shouldRemove;
296+
try (final @NotNull ISentryLifecycleToken ignored = trackedWindowsLock.acquire()) {
297+
shouldRemove = trackedWindows.contains(window) && trackedWindows.remove(window);
298+
}
299+
if (shouldRemove) {
300+
windowFrameMetricsManager.removeOnFrameMetricsAvailableListener(
301+
window, frameMetricsAvailableListener);
302+
}
303+
} catch (Throwable e) {
304+
logger.log(SentryLevel.ERROR, "Failed to remove frameMetricsAvailableListener", e);
305+
}
306+
});
296307
}
297308

298309
private void setCurrentWindow(final @NotNull Window window) {
@@ -305,18 +316,35 @@ private void setCurrentWindow(final @NotNull Window window) {
305316

306317
@SuppressLint("NewApi")
307318
private void trackCurrentWindow() {
308-
Window window = currentWindow != null ? currentWindow.get() : null;
319+
@Nullable Window window = currentWindow != null ? currentWindow.get() : null;
309320
if (window == null || !isAvailable) {
310321
return;
311322
}
312323

313-
if (!trackedWindows.contains(window) && !listenerMap.isEmpty()) {
324+
if (listenerMap.isEmpty()) {
325+
return;
326+
}
314327

315-
if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.N && handler != null) {
316-
trackedWindows.add(window);
317-
windowFrameMetricsManager.addOnFrameMetricsAvailableListener(
318-
window, frameMetricsAvailableListener, handler);
319-
}
328+
if (handler != null) {
329+
// Ensure the addOnFrameMetricsAvailableListener is called on the main thread
330+
new Handler(Looper.getMainLooper())
331+
.post(
332+
() -> {
333+
// Re-check if we should still track this window
334+
// in case stopTrackingWindow was called for the same Window in the meantime
335+
final boolean shouldAdd;
336+
try (final @NotNull ISentryLifecycleToken ignored = trackedWindowsLock.acquire()) {
337+
shouldAdd = !trackedWindows.contains(window) && trackedWindows.add(window);
338+
}
339+
if (shouldAdd) {
340+
try {
341+
windowFrameMetricsManager.addOnFrameMetricsAvailableListener(
342+
window, frameMetricsAvailableListener, handler);
343+
} catch (Throwable e) {
344+
logger.log(SentryLevel.ERROR, "Failed to add frameMetricsAvailableListener", e);
345+
}
346+
}
347+
});
320348
}
321349
}
322350

@@ -373,6 +401,9 @@ default void addOnFrameMetricsAvailableListener(
373401
final @NotNull Window window,
374402
final @Nullable Window.OnFrameMetricsAvailableListener frameMetricsAvailableListener,
375403
final @Nullable Handler handler) {
404+
if (frameMetricsAvailableListener == null) {
405+
return;
406+
}
376407
window.addOnFrameMetricsAvailableListener(frameMetricsAvailableListener, handler);
377408
}
378409

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,9 @@ class SentryFrameMetricsCollectorTest {
148148
collector.startCollection(mock())
149149
assertEquals(0, fixture.addOnFrameMetricsAvailableListenerCounter)
150150
collector.onActivityStarted(fixture.activity)
151+
// Execute pending main looper tasks since addOnFrameMetricsAvailableListener is posted to main
152+
// thread
153+
Shadows.shadowOf(Looper.getMainLooper()).idle()
151154
assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
152155
}
153156

@@ -157,8 +160,12 @@ class SentryFrameMetricsCollectorTest {
157160

158161
collector.startCollection(mock())
159162
collector.onActivityStarted(fixture.activity)
163+
// Execute pending add operations
164+
Shadows.shadowOf(Looper.getMainLooper()).idle()
160165
assertEquals(0, fixture.removeOnFrameMetricsAvailableListenerCounter)
161166
collector.onActivityStopped(fixture.activity)
167+
// Execute pending remove operations
168+
Shadows.shadowOf(Looper.getMainLooper()).idle()
162169
assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
163170
}
164171

@@ -181,6 +188,8 @@ class SentryFrameMetricsCollectorTest {
181188
collector.onActivityStarted(fixture.activity)
182189
assertEquals(0, fixture.addOnFrameMetricsAvailableListenerCounter)
183190
collector.startCollection(mock())
191+
// Execute pending main looper tasks
192+
Shadows.shadowOf(Looper.getMainLooper()).idle()
184193
assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
185194
}
186195

@@ -189,9 +198,13 @@ class SentryFrameMetricsCollectorTest {
189198
val collector = fixture.getSut(context)
190199
val id = collector.startCollection(mock())
191200
collector.onActivityStarted(fixture.activity)
201+
// Execute pending add operations
202+
Shadows.shadowOf(Looper.getMainLooper()).idle()
192203

193204
assertEquals(0, fixture.removeOnFrameMetricsAvailableListenerCounter)
194205
collector.stopCollection(id)
206+
// Execute pending remove operations
207+
Shadows.shadowOf(Looper.getMainLooper()).idle()
195208
assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
196209
}
197210

@@ -205,9 +218,13 @@ class SentryFrameMetricsCollectorTest {
205218

206219
collector.onActivityStarted(fixture.activity)
207220
collector.onActivityStarted(fixture.activity)
221+
// Execute pending add operations
222+
Shadows.shadowOf(Looper.getMainLooper()).idle()
208223

209224
collector.onActivityStopped(fixture.activity)
210225
collector.onActivityStopped(fixture.activity)
226+
// Execute pending remove operations
227+
Shadows.shadowOf(Looper.getMainLooper()).idle()
211228

212229
assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
213230
assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
@@ -228,9 +245,13 @@ class SentryFrameMetricsCollectorTest {
228245
collector.startCollection(mock())
229246
collector.onActivityStarted(fixture.activity)
230247
collector.onActivityStarted(fixture.activity2)
248+
// Execute pending add operations
249+
Shadows.shadowOf(Looper.getMainLooper()).idle()
231250
assertEquals(2, fixture.addOnFrameMetricsAvailableListenerCounter)
232251
collector.onActivityStopped(fixture.activity)
233252
collector.onActivityStopped(fixture.activity2)
253+
// Execute pending remove operations
254+
Shadows.shadowOf(Looper.getMainLooper()).idle()
234255
assertEquals(2, fixture.removeOnFrameMetricsAvailableListenerCounter)
235256
}
236257

@@ -240,10 +261,13 @@ class SentryFrameMetricsCollectorTest {
240261
val id1 = collector.startCollection(mock())
241262
val id2 = collector.startCollection(mock())
242263
collector.onActivityStarted(fixture.activity)
264+
Shadows.shadowOf(Looper.getMainLooper()).idle()
243265
assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
244266
collector.stopCollection(id1)
267+
Shadows.shadowOf(Looper.getMainLooper()).idle()
245268
assertEquals(0, fixture.removeOnFrameMetricsAvailableListenerCounter)
246269
collector.stopCollection(id2)
270+
Shadows.shadowOf(Looper.getMainLooper()).idle()
247271
assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
248272
}
249273

@@ -511,6 +535,48 @@ class SentryFrameMetricsCollectorTest {
511535
)
512536
}
513537

538+
@Test
539+
fun `collector calls addOnFrameMetricsAvailableListener on main thread`() {
540+
val collector = fixture.getSut(context)
541+
542+
collector.startCollection(mock())
543+
collector.onActivityStarted(fixture.activity)
544+
545+
assertEquals(0, fixture.addOnFrameMetricsAvailableListenerCounter)
546+
Shadows.shadowOf(Looper.getMainLooper()).idle()
547+
assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
548+
}
549+
550+
@Test
551+
fun `collector calls removeOnFrameMetricsAvailableListener on main thread`() {
552+
val collector = fixture.getSut(context)
553+
collector.startCollection(mock())
554+
collector.onActivityStarted(fixture.activity)
555+
Shadows.shadowOf(Looper.getMainLooper()).idle()
556+
557+
collector.onActivityStopped(fixture.activity)
558+
assertEquals(0, fixture.removeOnFrameMetricsAvailableListenerCounter)
559+
Shadows.shadowOf(Looper.getMainLooper()).idle()
560+
assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
561+
}
562+
563+
@Test
564+
fun `collector prevents race condition when stop is called immediately after start`() {
565+
val collector = fixture.getSut(context)
566+
567+
collector.startCollection(mock())
568+
collector.onActivityStarted(fixture.activity)
569+
collector.onActivityStopped(fixture.activity)
570+
571+
// Now execute all pending operations
572+
Shadows.shadowOf(Looper.getMainLooper()).idle()
573+
574+
// as the listeners are posted to the main thread, we expect an add followed by a remove
575+
assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter)
576+
assertEquals(1, fixture.removeOnFrameMetricsAvailableListenerCounter)
577+
assertEquals(0, collector.getProperty<Set<Window>>("trackedWindows").size)
578+
}
579+
514580
private fun createMockWindow(refreshRate: Float = 60F): Window {
515581
val mockWindow = mock<Window>()
516582
val mockDisplay = mock<Display>()

0 commit comments

Comments
 (0)