Fix user action blocking behaviour when data collection disabled (#6218)

Make doBackgroundInitializationAsync submit a callable instead of submit
task

Submit task results in a blocking behaviour when `SettingsTask` cannot
be resolved during initialization
Remove un-necessary task return

Author: themiswang

firebase-crashlytics-ndk/gradle.properties

@@ -1,2 +1,2 @@
 version=19.1.1
-latestReleasedVersion=19.1.0
+latestReleasedVersion=19.1.0

firebase-crashlytics/gradle.properties

@@ -1,2 +1,2 @@
 version=19.1.1
-latestReleasedVersion=19.1.0
+latestReleasedVersion=19.1.0

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java

@@ -410,9 +410,9 @@ public void testUploadWithNoReports() throws Exception {
 
     final CrashlyticsController controller = createController();
 
-    Task<Void> task = controller.submitAllReports(testSettingsProvider.getSettingsAsync());
+    controller.submitAllReports(testSettingsProvider.getSettingsAsync());
 
-    await(task);
+    crashlyticsWorkers.common.await();
 
     verify(mockSessionReportingCoordinator).hasReportsToSend();
     verifyNoMoreInteractions(mockSessionReportingCoordinator);
@@ -426,9 +426,9 @@ public void testUploadWithDataCollectionAlwaysEnabled() throws Exception {
 
     final CrashlyticsController controller = createController();
 
-    final Task<Void> task = controller.submitAllReports(testSettingsProvider.getSettingsAsync());
+    controller.submitAllReports(testSettingsProvider.getSettingsAsync());
 
-    await(task);
+    crashlyticsWorkers.common.await();
 
     verify(mockSessionReportingCoordinator).hasReportsToSend();
     verify(mockDataCollectionArbiter).isAutomaticDataCollectionEnabled();
@@ -452,15 +452,13 @@ public void testUploadDisabledThenOptIn() throws Exception {
     final ControllerBuilder builder = builder();
     builder.setDataCollectionArbiter(arbiter);
     final CrashlyticsController controller = builder.build();
-
-    final Task<Void> task = controller.submitAllReports(testSettingsProvider.getSettingsAsync());
-
+    controller.submitAllReports(testSettingsProvider.getSettingsAsync());
     verify(arbiter).isAutomaticDataCollectionEnabled();
     verify(mockSessionReportingCoordinator).hasReportsToSend();
     verify(mockSessionReportingCoordinator, never()).sendReports(any(Executor.class));
 
     await(controller.sendUnsentReports());
-    await(task);
+    crashlyticsWorkers.common.await();
 
     verify(mockSessionReportingCoordinator).sendReports(any(Executor.class));
     verifyNoMoreInteractions(mockSessionReportingCoordinator);
@@ -481,14 +479,14 @@ public void testUploadDisabledThenOptOut() throws Exception {
     builder.setDataCollectionArbiter(arbiter);
     final CrashlyticsController controller = builder.build();
 
-    final Task<Void> task = controller.submitAllReports(testSettingsProvider.getSettingsAsync());
+    controller.submitAllReports(testSettingsProvider.getSettingsAsync());
 
     verify(arbiter).isAutomaticDataCollectionEnabled();
     verify(mockSessionReportingCoordinator).hasReportsToSend();
     verify(mockSessionReportingCoordinator, never()).removeAllReports();
 
     await(controller.deleteUnsentReports());
-    await(task);
+    crashlyticsWorkers.common.await();
 
     crashlyticsWorkers.diskWrite.await();
 
@@ -525,7 +523,7 @@ public void testUploadDisabledThenEnabled() throws Exception {
     builder.setDataCollectionArbiter(arbiter);
     final CrashlyticsController controller = builder.build();
 
-    final Task<Void> task = controller.submitAllReports(testSettingsProvider.getSettingsAsync());
+    controller.submitAllReports(testSettingsProvider.getSettingsAsync());
 
     verify(mockSessionReportingCoordinator).hasReportsToSend();
     verify(mockSessionReportingCoordinator, never()).sendReports(any(Executor.class));
@@ -547,7 +545,7 @@ public void testUploadDisabledThenEnabled() throws Exception {
     when(app.isDataCollectionDefaultEnabled()).thenReturn(false);
     assertFalse(arbiter.isAutomaticDataCollectionEnabled());
 
-    await(task);
+    crashlyticsWorkers.common.await();
 
     verify(mockSessionReportingCoordinator).sendReports(any(Executor.class));
     verifyNoMoreInteractions(mockSessionReportingCoordinator);

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

@@ -339,16 +339,16 @@ Task<Void> deleteUnsentReports() {
     return unsentReportsHandled.getTask();
   }
 
-  Task<Void> submitAllReports(Task<Settings> settingsDataTask) {
+  void submitAllReports(Task<Settings> settingsDataTask) {
     if (!reportingCoordinator.hasReportsToSend()) {
       // Just notify the user that there are no reports and stop.
       Logger.getLogger().v("No crash reports are available to be sent.");
       unsentReportsAvailable.trySetResult(false);
-      return Tasks.forResult(null);
+      return;
     }
     Logger.getLogger().v("Crash reports are available to be sent.");
 
-    return waitForReportAction()
+    waitForReportAction()
         .onSuccessTask(
             crashlyticsWorkers.common,
             new SuccessContinuation<Boolean, Void>() {
@@ -360,6 +360,7 @@ public Task<Void> then(@Nullable Boolean send) throws Exception {
                   deleteFiles(listAppExceptionMarkerFiles());
                   reportingCoordinator.removeAllReports();
                   unsentReportsHandled.trySetResult(null);
+
                   return Tasks.forResult(null);
                 }
 
@@ -371,7 +372,6 @@ public Task<Void> then(@Nullable Boolean send) throws Exception {
                 // Signal to the settings fetch and onboarding that we have explicit
                 // permission.
                 dataCollectionArbiter.grantDataCollectionPermission(dataCollectionToken);
-
                 return settingsDataTask.onSuccessTask(
                     crashlyticsWorkers.common,
                     new SuccessContinuation<Settings, Void>() {

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

@@ -21,7 +21,6 @@
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 import com.google.android.gms.tasks.Task;
-import com.google.android.gms.tasks.Tasks;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.crashlytics.BuildConfig;
@@ -224,13 +223,12 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
 
   /** Performs background initialization asynchronously on the common worker. */
   @CanIgnoreReturnValue
-  public Task<Settings> doBackgroundInitializationAsync(SettingsProvider settingsProvider) {
-    return crashlyticsWorkers.common.submitTask(() -> doBackgroundInitialization(settingsProvider));
+  public Task<Void> doBackgroundInitializationAsync(SettingsProvider settingsProvider) {
+    return crashlyticsWorkers.common.submit(() -> doBackgroundInitialization(settingsProvider));
   }
 
   /** Performs background initialization synchronously on the calling thread. */
-  @CanIgnoreReturnValue
-  private Task<Settings> doBackgroundInitialization(SettingsProvider settingsProvider) {
+  private void doBackgroundInitialization(SettingsProvider settingsProvider) {
     CrashlyticsWorkers.checkBackgroundThread();
     // create the marker for this run
     markInitializationStarted();
@@ -246,21 +244,17 @@ private Task<Settings> doBackgroundInitialization(SettingsProvider settingsProvi
         Logger.getLogger().d("Collection of crash reports disabled in Crashlytics settings.");
         // TODO: This isn't actually an error condition, so figure out the right way to
         // handle this case.
-        return Tasks.forException(
-            new RuntimeException("Collection of crash reports disabled in Crashlytics settings."));
+        throw new RuntimeException("Collection of crash reports disabled in Crashlytics settings.");
       }
 
       if (!controller.finalizeSessions(settingsProvider)) {
         Logger.getLogger().w("Previous sessions could not be finalized.");
       }
 
-      Task<Settings> settings = settingsProvider.getSettingsAsync();
-      controller.submitAllReports(settings);
-      return settings;
+      controller.submitAllReports(settingsProvider.getSettingsAsync());
     } catch (Exception e) {
       Logger.getLogger()
           .e("Crashlytics encountered a problem during asynchronous initialization.", e);
-      return Tasks.forException(e);
     } finally {
       // The only thing that compels us to leave the marker and start synchronously next time
       // is not executing all the way through this method. That would indicate that we perhaps

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

@@ -341,6 +341,7 @@ private void persistEvent(
     if (!isFatal) {
       crashlyticsWorkers.diskWrite.submit(
           () -> {
+            Logger.getLogger().d("disk worker: log non-fatal event to persistence");
             reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority);
           });
       return;

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java

@@ -162,7 +162,6 @@ public Task<Void> loadSettingsData(
       SettingsCacheBehavior cacheBehavior, CrashlyticsWorkers crashlyticsWorkers) {
     // TODO: Refactor this so that it doesn't do the cache lookup twice when settings are
     // expired.
-
     // We need to bypass the cache if this is the first time a new build has run so the
     // backend will know about it.
     if (!buildInstanceIdentifierChanged()) {