[QA] Fix potential ANRs due to default integrations (#3778)

* Fix don't register any full drawn listeners if feature is not enabled * Offload app components breadcrumbs to background thread * Offload system events breadcrumbs to background thread * Update Changelog * Fix flaky tests * Remove session breadcrumbs * Fix tests * Address PR feedback
getsentry · Oct 9, 2024 · 1607621 · 1607621
1 parent 503f916
commit 1607621
Show file tree

Hide file tree

Showing 13 changed files with 237 additions and 135 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@
     - If you're using code obfuscation, adjust your proguard-rules accordingly, so your custom view class name is not minified
 - Fix ensure Application Context is used even when SDK is initialized via Activity Context ([#3669](https://github.com/getsentry/sentry-java/pull/3669))
 - Fix potential ANRs due to `Calendar.getInstance` usage in Breadcrumbs constructor ([#3736](https://github.com/getsentry/sentry-java/pull/3736))
+- Fix potential ANRs due to default integrations ([#3778](https://github.com/getsentry/sentry-java/pull/3778))
 - Lazily initialize heavy `SentryOptions` members to avoid ANRs on app start ([#3749](https://github.com/getsentry/sentry-java/pull/3749))
 
 *Breaking changes*:

diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
@@ -382,7 +382,7 @@ public synchronized void onActivityCreated(
 
     firstActivityCreated = true;
 
-    if (fullyDisplayedReporter != null) {
+    if (performanceEnabled && ttfdSpan != null && fullyDisplayedReporter != null) {
       fullyDisplayedReporter.registerFullyDrawnListener(() -> onFullFrameDrawn(ttfdSpan));
     }
   }

diff --git a/...ndroid-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java b/...ndroid-core/src/main/java/io/sentry/android/core/AppComponentsBreadcrumbsIntegration.java
@@ -85,43 +85,25 @@ public void close() throws IOException {
   @SuppressWarnings("deprecation")
   @Override
   public void onConfigurationChanged(@NotNull Configuration newConfig) {
-    if (hub != null) {
-      final Device.DeviceOrientation deviceOrientation =
-          DeviceOrientations.getOrientation(context.getResources().getConfiguration().orientation);
-
-      String orientation;
-      if (deviceOrientation != null) {
-        orientation = deviceOrientation.name().toLowerCase(Locale.ROOT);
-      } else {
-        orientation = "undefined";
-      }
-
-      final Breadcrumb breadcrumb = new Breadcrumb();
-      breadcrumb.setType("navigation");
-      breadcrumb.setCategory("device.orientation");
-      breadcrumb.setData("position", orientation);
-      breadcrumb.setLevel(SentryLevel.INFO);
-
-      final Hint hint = new Hint();
-      hint.set(ANDROID_CONFIGURATION, newConfig);
-
-      hub.addBreadcrumb(breadcrumb, hint);
-    }
+    final long now = System.currentTimeMillis();
+    executeInBackground(() -> captureConfigurationChangedBreadcrumb(now, newConfig));
   }
 
   @Override
   public void onLowMemory() {
-    createLowMemoryBreadcrumb(null);
+    final long now = System.currentTimeMillis();
+    executeInBackground(() -> captureLowMemoryBreadcrumb(now, null));
   }
 
   @Override
   public void onTrimMemory(final int level) {
-    createLowMemoryBreadcrumb(level);
+    final long now = System.currentTimeMillis();
+    executeInBackground(() -> captureLowMemoryBreadcrumb(now, level));
   }
 
-  private void createLowMemoryBreadcrumb(final @Nullable Integer level) {
+  private void captureLowMemoryBreadcrumb(final long timeMs, final @Nullable Integer level) {
     if (hub != null) {
-      final Breadcrumb breadcrumb = new Breadcrumb();
+      final Breadcrumb breadcrumb = new Breadcrumb(timeMs);
       if (level != null) {
         // only add breadcrumb if TRIM_MEMORY_BACKGROUND, TRIM_MEMORY_MODERATE or
         // TRIM_MEMORY_COMPLETE.
@@ -147,4 +129,42 @@ private void createLowMemoryBreadcrumb(final @Nullable Integer level) {
       hub.addBreadcrumb(breadcrumb);
     }
   }
+
+  private void captureConfigurationChangedBreadcrumb(
+      final long timeMs, final @NotNull Configuration newConfig) {
+    if (hub != null) {
+      final Device.DeviceOrientation deviceOrientation =
+          DeviceOrientations.getOrientation(context.getResources().getConfiguration().orientation);
+
+      String orientation;
+      if (deviceOrientation != null) {
+        orientation = deviceOrientation.name().toLowerCase(Locale.ROOT);
+      } else {
+        orientation = "undefined";
+      }
+
+      final Breadcrumb breadcrumb = new Breadcrumb(timeMs);
+      breadcrumb.setType("navigation");
+      breadcrumb.setCategory("device.orientation");
+      breadcrumb.setData("position", orientation);
+      breadcrumb.setLevel(SentryLevel.INFO);
+
+      final Hint hint = new Hint();
+      hint.set(ANDROID_CONFIGURATION, newConfig);
+
+      hub.addBreadcrumb(breadcrumb, hint);
+    }
+  }
+
+  private void executeInBackground(final @NotNull Runnable runnable) {
+    if (options != null) {
+      try {
+        options.getExecutorService().submit(runnable);
+      } catch (Throwable t) {
+        options
+            .getLogger()
+            .log(SentryLevel.ERROR, t, "Failed to submit app components breadcrumb task");
+      }
+    }
+  }
 }
diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java
@@ -6,7 +6,6 @@
 import io.sentry.IHub;
 import io.sentry.SentryLevel;
 import io.sentry.Session;
-import io.sentry.android.core.internal.util.BreadcrumbFactory;
 import io.sentry.transport.CurrentDateProvider;
 import io.sentry.transport.ICurrentDateProvider;
 import java.util.Timer;
@@ -90,7 +89,6 @@ private void startSession() {
     if (lastUpdatedSession == 0L
         || (lastUpdatedSession + sessionIntervalMillis) <= currentTimeMillis) {
       if (enableSessionTracking) {
-        addSessionBreadcrumb("start");
         hub.startSession();
       }
       hub.getOptions().getReplayController().start();
@@ -125,7 +123,6 @@ private void scheduleEndSession() {
               @Override
               public void run() {
                 if (enableSessionTracking) {
-                  addSessionBreadcrumb("end");
                   hub.endSession();
                 }
                 hub.getOptions().getReplayController().stop();
@@ -157,11 +154,6 @@ private void addAppBreadcrumb(final @NotNull String state) {
     }
   }
 
-  private void addSessionBreadcrumb(final @NotNull String state) {
-    final Breadcrumb breadcrumb = BreadcrumbFactory.forSession(state);
-    hub.addBreadcrumb(breadcrumb);
-  }
-
   @TestOnly
   @Nullable
   TimerTask getTimerTask() {

diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/NetworkBreadcrumbsIntegration.java
@@ -31,12 +31,13 @@
 public final class NetworkBreadcrumbsIntegration implements Integration, Closeable {
 
   private final @NotNull Context context;
-
   private final @NotNull BuildInfoProvider buildInfoProvider;
-
   private final @NotNull ILogger logger;
+  private final @NotNull Object lock = new Object();
+  private volatile boolean isClosed;
+  private @Nullable SentryOptions options;
 
-  @TestOnly @Nullable NetworkBreadcrumbsNetworkCallback networkCallback;
+  @TestOnly @Nullable volatile NetworkBreadcrumbsNetworkCallback networkCallback;
 
   public NetworkBreadcrumbsIntegration(
       final @NotNull Context context,
@@ -63,40 +64,74 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio
         "NetworkBreadcrumbsIntegration enabled: %s",
         androidOptions.isEnableNetworkEventBreadcrumbs());
 
+    this.options = options;
+
     if (androidOptions.isEnableNetworkEventBreadcrumbs()) {
 
       // The specific error is logged in the ConnectivityChecker method
-      if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) {
-        networkCallback = null;
-        logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration requires Android 5+");
+      if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.N) {
+        logger.log(SentryLevel.DEBUG, "NetworkCallbacks need Android N+.");
         return;
       }
 
-      networkCallback =
-          new NetworkBreadcrumbsNetworkCallback(hub, buildInfoProvider, options.getDateProvider());
-      final boolean registered =
-          AndroidConnectionStatusProvider.registerNetworkCallback(
-              context, logger, buildInfoProvider, networkCallback);
+      try {
+        options
+            .getExecutorService()
+            .submit(
+                new Runnable() {
+                  @Override
+                  public void run() {
+                    // in case integration is closed before the task is executed, simply return
+                    if (isClosed) {
+                      return;
+                    }
 
-      // The specific error is logged in the ConnectivityChecker method
-      if (!registered) {
-        networkCallback = null;
-        logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed.");
-        return;
+                    synchronized (lock) {
+                      networkCallback =
+                          new NetworkBreadcrumbsNetworkCallback(
+                              hub, buildInfoProvider, options.getDateProvider());
+
+                      final boolean registered =
+                          AndroidConnectionStatusProvider.registerNetworkCallback(
+                              context, logger, buildInfoProvider, networkCallback);
+                      if (registered) {
+                        logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration installed.");
+                        addIntegrationToSdkVersion(getClass());
+                      } else {
+                        logger.log(
+                            SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed.");
+                        // The specific error is logged by AndroidConnectionStatusProvider
+                      }
+                    }
+                  }
+                });
+      } catch (Throwable t) {
+        logger.log(SentryLevel.ERROR, "Error submitting NetworkBreadcrumbsIntegration task.", t);
       }
-      logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration installed.");
-      addIntegrationToSdkVersion(getClass());
     }
   }
 
   @Override
   public void close() throws IOException {
-    if (networkCallback != null) {
-      AndroidConnectionStatusProvider.unregisterNetworkCallback(
-          context, logger, buildInfoProvider, networkCallback);
-      logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration remove.");
+    isClosed = true;
+
+    try {
+      Objects.requireNonNull(options, "Options is required")
+          .getExecutorService()
+          .submit(
+              () -> {
+                synchronized (lock) {
+                  if (networkCallback != null) {
+                    AndroidConnectionStatusProvider.unregisterNetworkCallback(
+                        context, logger, buildInfoProvider, networkCallback);
+                    logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration removed.");
+                  }
+                  networkCallback = null;
+                }
+              });
+    } catch (Throwable t) {
+      logger.log(SentryLevel.ERROR, "Error submitting NetworkBreadcrumbsIntegration task.", t);
     }
-    networkCallback = null;
   }
 
   @SuppressLint("ObsoleteSdkInt")

diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java
@@ -13,7 +13,6 @@
 import io.sentry.SentryLevel;
 import io.sentry.SentryOptions;
 import io.sentry.Session;
-import io.sentry.android.core.internal.util.BreadcrumbFactory;
 import io.sentry.android.core.performance.AppStartMetrics;
 import io.sentry.android.core.performance.TimeSpan;
 import io.sentry.android.fragment.FragmentLifecycleIntegration;
@@ -173,7 +172,6 @@ public static synchronized void init(
                 }
               });
           if (!sessionStarted.get()) {
-            hub.addBreadcrumb(BreadcrumbFactory.forSession("session.start"));
             hub.startSession();
           }
         }

diff --git a/...android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java b/...android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java
@@ -211,7 +211,7 @@ static final class SystemEventsBroadcastReceiver extends BroadcastReceiver {
     private static final long DEBOUNCE_WAIT_TIME_MS = 60 * 1000;
     private final @NotNull IHub hub;
     private final @NotNull SentryAndroidOptions options;
-    private final @NotNull Debouncer debouncer =
+    private final @NotNull Debouncer batteryChangedDebouncer =
         new Debouncer(AndroidCurrentDateProvider.getInstance(), DEBOUNCE_WAIT_TIME_MS, 0);
 
     SystemEventsBroadcastReceiver(
@@ -221,19 +221,43 @@ static final class SystemEventsBroadcastReceiver extends BroadcastReceiver {
     }
 
     @Override
-    public void onReceive(Context context, Intent intent) {
-      final boolean shouldDebounce = debouncer.checkForDebounce();
-      final String action = intent.getAction();
+    public void onReceive(final Context context, final @NotNull Intent intent) {
+      final @Nullable String action = intent.getAction();
       final boolean isBatteryChanged = ACTION_BATTERY_CHANGED.equals(action);
-      if (isBatteryChanged && shouldDebounce) {
-        // aligning with iOS which only captures battery status changes every minute at maximum
+
+      // aligning with iOS which only captures battery status changes every minute at maximum
+      if (isBatteryChanged && batteryChangedDebouncer.checkForDebounce()) {
         return;
       }
 
-      final Breadcrumb breadcrumb = new Breadcrumb();
+      final long now = System.currentTimeMillis();
+      try {
+        options
+            .getExecutorService()
+            .submit(
+                () -> {
+                  final Breadcrumb breadcrumb =
+                      createBreadcrumb(now, intent, action, isBatteryChanged);
+                  final Hint hint = new Hint();
+                  hint.set(ANDROID_INTENT, intent);
+                  hub.addBreadcrumb(breadcrumb, hint);
+                });
+      } catch (Throwable t) {
+        options
+            .getLogger()
+            .log(SentryLevel.ERROR, t, "Failed to submit system event breadcrumb action.");
+      }
+    }
+
+    private @NotNull Breadcrumb createBreadcrumb(
+        final long timeMs,
+        final @NotNull Intent intent,
+        final @Nullable String action,
+        boolean isBatteryChanged) {
+      final Breadcrumb breadcrumb = new Breadcrumb(timeMs);
       breadcrumb.setType("system");
       breadcrumb.setCategory("device.event");
-      String shortAction = StringUtils.getStringAfterDot(action);
+      final String shortAction = StringUtils.getStringAfterDot(action);
       if (shortAction != null) {
         breadcrumb.setData("action", shortAction);
       }
@@ -273,11 +297,7 @@ public void onReceive(Context context, Intent intent) {
         }
       }
       breadcrumb.setLevel(SentryLevel.INFO);
-
-      final Hint hint = new Hint();
-      hint.set(ANDROID_INTENT, intent);
-
-      hub.addBreadcrumb(breadcrumb, hint);
+      return breadcrumb;
     }
   }
 }
diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt
@@ -78,7 +78,6 @@ class ActivityLifecycleIntegrationTest {
         }
         val bundle = mock<Bundle>()
         val activityFramesTracker = mock<ActivityFramesTracker>()
-        val fullyDisplayedReporter = FullyDisplayedReporter.getInstance()
         val transactionFinishedCallback = mock<TransactionFinishedCallback>()
         lateinit var shadowActivityManager: ShadowActivityManager
 
@@ -619,11 +618,30 @@ class ActivityLifecycleIntegrationTest {
         sut.onActivityCreated(activity, mock())
         val ttfdSpan = sut.ttfdSpanMap[activity]
         sut.ttidSpanMap.values.first().finish()
-        fixture.fullyDisplayedReporter.reportFullyDrawn()
+        fixture.options.fullyDisplayedReporter.reportFullyDrawn()
         assertTrue(ttfdSpan!!.isFinished)
         assertNotEquals(SpanStatus.CANCELLED, ttfdSpan.status)
     }
 
+    @Test
+    fun `if ttfd is disabled, no listener is registered for FullyDisplayedReporter`() {
+        val ttfdReporter = mock<FullyDisplayedReporter>()
+
+        val sut = fixture.getSut()
+        fixture.options.apply {
+            tracesSampleRate = 1.0
+            isEnableTimeToFullDisplayTracing = false
+            fullyDisplayedReporter = ttfdReporter
+        }
+
+        sut.register(fixture.hub, fixture.options)
+
+        val activity = mock<Activity>()
+        sut.onActivityCreated(activity, mock())
+
+        verify(ttfdReporter, never()).registerFullyDrawnListener(any())
+    }
+
     @Test
     fun `App start is Cold when savedInstanceState is null`() {
         val sut = fixture.getSut()