Ensure that on-demand fatal events are never processed on the main thread (#6113)

Ensure that on-demand fatal events are never processed on the main
thread.

Also decreased the timeout on main to under 3 seconds, to give more time
to other uncaught exception handlers in the chain.

Also fixed some formatting.

Tested cases:
Uncaught exception thrown from the main thread
Uncaught exception thrown from a background thread
ODF triggered on the main thread
ODF triggered on a background thread

Author: mrober

firebase-crashlytics/CHANGELOG.md

@@ -1,11 +1,11 @@
 # Unreleased
+* [feature] Added the `isCrashlyticsCollectionEnabled` API to check if Crashlytics collection is enabled.
+  ([Github #5919](//github.com/firebase/firebase-android-sdk/issues/5919))
+* [fixed] Ensure that on-demand fatal events are never processed on the main thread.
 
 
 # 19.0.3
 * [changed] Update the internal file system to handle long file names.
-* [feature] Added the `isCrashlyticsCollectionEnabled` API to check if Crashlytics collection is enabled. 
-  ([Github #5919](//github.com/firebase/firebase-android-sdk/issues/5919))
-
 
 ## Kotlin
 The Kotlin extensions library transitively includes the updated

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java

@@ -250,9 +250,13 @@ public Task<Void> then(@Nullable Settings settings) throws Exception {
               }
             });
 
+    // Block until the task completes, to prevent the process from ending before logging the report.
+    // There might be other uncaught exception handlers in the chain, so do not block very long.
     try {
-      // TODO(mrober): Don't block the main thread ever for on-demand fatals.
-      Utils.awaitEvenIfOnMainThread(handleUncaughtExceptionTask);
+      // Never block on ODFs, in case they get triggered from the main thread.
+      if (!isOnDemand) {
+        Utils.awaitEvenIfOnMainThread(handleUncaughtExceptionTask);
+      }
     } catch (TimeoutException e) {
       Logger.getLogger().e("Cannot send reports. Timed out while fetching settings.");
     } catch (Exception e) {

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java

@@ -31,9 +31,15 @@
 /** Utils */
 @SuppressWarnings({"ResultOfMethodCallIgnored", "UnusedReturnValue"})
 public final class Utils {
-  private static final int TIMEOUT_SEC = 4;
+  /** Timeout in milliseconds for blocking on background threads. */
+  private static final int BACKGROUND_TIMEOUT_MILLIS = 4_000;
 
-  /** @return A tasks that is resolved when either of the given tasks is resolved. */
+  /** Timeout in milliseconds for blocking on the main thread. Be careful about ANRs. */
+  private static final int MAIN_TIMEOUT_MILLIS = 2_750;
+
+  /**
+   * @return A tasks that is resolved when either of the given tasks is resolved.
+   */
   // TODO(b/261014167): Use an explicit executor in continuations.
   @SuppressLint("TaskMainThread")
   public static <T> Task<T> race(Task<T> t1, Task<T> t2) {
@@ -52,7 +58,9 @@ public static <T> Task<T> race(Task<T> t1, Task<T> t2) {
     return result.getTask();
   }
 
-  /** @return A tasks that is resolved when either of the given tasks is resolved. */
+  /**
+   * @return A tasks that is resolved when either of the given tasks is resolved.
+   */
   public static <T> Task<T> race(Executor executor, Task<T> t1, Task<T> t2) {
     final TaskCompletionSource<T> result = new TaskCompletionSource<>();
     Continuation<T, Void> continuation =
@@ -119,9 +127,9 @@ public static <T> T awaitEvenIfOnMainThread(Task<T> task)
         });
 
     if (Looper.getMainLooper() == Looper.myLooper()) {
-      latch.await(CrashlyticsCore.DEFAULT_MAIN_HANDLER_TIMEOUT_SEC, TimeUnit.SECONDS);
+      latch.await(MAIN_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS);
     } else {
-      latch.await(TIMEOUT_SEC, TimeUnit.SECONDS);
+      latch.await(BACKGROUND_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS);
     }
 
     if (task.isSuccessful()) {

firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiterRobolectricTest.java

@@ -15,16 +15,14 @@
 package com.google.firebase.crashlytics.internal.common;
 
 import static androidx.test.core.app.ApplicationProvider.getApplicationContext;
+import static com.google.common.truth.Truth.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
-import static com.google.common.truth.Truth.assertThat;
 import static org.robolectric.Shadows.shadowOf;
 
 import android.content.Context;
 import android.os.Bundle;
-
 import com.google.firebase.FirebaseApp;
-
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -37,7 +35,7 @@ public class DataCollectionArbiterRobolectricTest {
   private FirebaseApp firebaseApp;
 
   private static final String FIREBASE_CRASHLYTICS_COLLECTION_ENABLED =
-          "firebase_crashlytics_collection_enabled";
+      "firebase_crashlytics_collection_enabled";
 
   @Before
   public void setUp() {
@@ -54,7 +52,7 @@ private DataCollectionArbiter getDataCollectionArbiter(FirebaseApp app) {
   public void testSetCrashlyticsDataCollectionEnabled_overridesOtherSettings() {
     // Ensure that Manifest metadata is set to false.
     editManifestApplicationMetadata(testContext)
-            .putBoolean(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED, false);
+        .putBoolean(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED, false);
 
     // Mock FirebaseApp to return default data collection as false.
     when(firebaseApp.isDataCollectionDefaultEnabled()).thenReturn(false);
@@ -70,21 +68,21 @@ public void testSetCrashlyticsDataCollectionEnabled_overridesOtherSettings() {
     assertThat(arbiter.isAutomaticDataCollectionEnabled()).isFalse();
 
     arbiter.setCrashlyticsDataCollectionEnabled(null);
-    //Expecting `false` result since manifest metadata value is `false`
+    // Expecting `false` result since manifest metadata value is `false`
     assertThat(arbiter.isAutomaticDataCollectionEnabled()).isFalse();
   }
 
   @Test
   public void testManifestMetadata_respectedWhenNoOverride() {
     editManifestApplicationMetadata(testContext)
-            .putBoolean(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED, true);
+        .putBoolean(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED, true);
 
     DataCollectionArbiter arbiter = getDataCollectionArbiter(firebaseApp);
 
     assertThat(arbiter.isAutomaticDataCollectionEnabled()).isTrue();
 
     editManifestApplicationMetadata(testContext)
-            .putBoolean(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED, false);
+        .putBoolean(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED, false);
 
     arbiter = getDataCollectionArbiter(firebaseApp);
 
@@ -93,8 +91,7 @@ public void testManifestMetadata_respectedWhenNoOverride() {
 
   @Test
   public void testDefaultDataCollection_usedWhenNoOverrideOrManifestSetting() {
-    editManifestApplicationMetadata(testContext)
-            .remove(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED);
+    editManifestApplicationMetadata(testContext).remove(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED);
 
     DataCollectionArbiter arbiter = getDataCollectionArbiter(firebaseApp);
 
@@ -104,13 +101,14 @@ public void testDefaultDataCollection_usedWhenNoOverrideOrManifestSetting() {
     when(firebaseApp.isDataCollectionDefaultEnabled()).thenReturn(false);
     assertThat(arbiter.isAutomaticDataCollectionEnabled()).isFalse();
 
-    //No Test of `null` return for firebaseApp.isDataCollectionDefaultEnabled(), since it will never return `null` value
+    // No Test of `null` return for firebaseApp.isDataCollectionDefaultEnabled(), since it will
+    // never return `null` value
   }
 
   private Bundle editManifestApplicationMetadata(Context context) {
     return shadowOf(context.getPackageManager())
-            .getInternalMutablePackageInfo(context.getPackageName())
-            .applicationInfo
-            .metaData;
+        .getInternalMutablePackageInfo(context.getPackageName())
+        .applicationInfo
+        .metaData;
   }
-}
+}