Plumb the executor through waitForDataCollectionPermission in Crashlytics (#2925)

Plumb the executor through waitForDataCollectionPermission to resolve concurrency issue in Crashlytics when racing two tasks.

Author: mrober

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

@@ -413,7 +413,7 @@ public void testUploadDisabledThenOptIn() throws Exception {
 
     final DataCollectionArbiter arbiter = mock(DataCollectionArbiter.class);
     when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(false);
-    when(arbiter.waitForDataCollectionPermission())
+    when(arbiter.waitForDataCollectionPermission(any(Executor.class)))
         .thenReturn(new TaskCompletionSource<Void>().getTask());
     when(arbiter.waitForAutomaticDataCollectionEnabled())
         .thenReturn(new TaskCompletionSource<Void>().getTask());

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java

@@ -142,7 +142,7 @@ public void testCachedSettingsLoad_newInstanceIdentifier() throws Exception {
     when(mockSettingsJsonParser.parseSettingsJson(fetchedJson)).thenReturn(fetchedSettings);
 
     TaskCompletionSource<Void> dataCollectionPermission = new TaskCompletionSource<>();
-    when(mockDataCollectionArbiter.waitForDataCollectionPermission())
+    when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class)))
         .thenReturn(dataCollectionPermission.getTask());
 
     final SettingsRequest requestData = buildSettingsRequest();
@@ -187,7 +187,7 @@ public void testExpiredCachedSettingsLoad() throws Exception {
     when(mockSettingsJsonParser.parseSettingsJson(fetchedJson)).thenReturn(fetchedSettings);
 
     TaskCompletionSource<Void> dataCollectionPermission = new TaskCompletionSource<>();
-    when(mockDataCollectionArbiter.waitForDataCollectionPermission())
+    when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class)))
         .thenReturn(dataCollectionPermission.getTask());
 
     final SettingsRequest requestData = buildSettingsRequest();
@@ -270,7 +270,7 @@ public void testSkipCachedSettingsLoad() throws Exception {
         .thenReturn(expiredCachedSettings);
 
     TaskCompletionSource<Void> dataCollectionPermission = new TaskCompletionSource<>();
-    when(mockDataCollectionArbiter.waitForDataCollectionPermission())
+    when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class)))
         .thenReturn(dataCollectionPermission.getTask());
 
     final SettingsRequest requestData = buildSettingsRequest();
@@ -323,7 +323,7 @@ public void testLastDitchSettingsLoad() throws Exception {
         .thenReturn(expiredCachedSettings);
 
     TaskCompletionSource<Void> dataCollectionPermission = new TaskCompletionSource<>();
-    when(mockDataCollectionArbiter.waitForDataCollectionPermission())
+    when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class)))
         .thenReturn(dataCollectionPermission.getTask());
 
     final SettingsRequest requestData = buildSettingsRequest();
@@ -360,7 +360,7 @@ public void testNoAvailableSettingsLoad() throws Exception {
     when(mockCachedSettingsIo.readCachedSettings()).thenReturn(null);
 
     TaskCompletionSource<Void> dataCollectionPermission = new TaskCompletionSource<>();
-    when(mockDataCollectionArbiter.waitForDataCollectionPermission())
+    when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class)))
         .thenReturn(dataCollectionPermission.getTask());
 
     final SettingsRequest requestData = buildSettingsRequest();

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

@@ -24,6 +24,7 @@
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.crashlytics.internal.Logger;
+import java.util.concurrent.Executor;
 
 // Determines whether automatic data collection is enabled.
 public class DataCollectionArbiter {
@@ -115,9 +116,11 @@ public Task<Void> waitForAutomaticDataCollectionEnabled() {
    * Returns a task which will be resolved when either: 1) automatic data collection has been
    * enabled, or 2) grantDataCollectionPermission has been called.
    */
-  public Task<Void> waitForDataCollectionPermission() {
+  public Task<Void> waitForDataCollectionPermission(Executor executor) {
     return Utils.race(
-        dataCollectionExplicitlyApproved.getTask(), waitForAutomaticDataCollectionEnabled());
+        executor,
+        dataCollectionExplicitlyApproved.getTask(),
+        waitForAutomaticDataCollectionEnabled());
   }
 
   /**

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

@@ -14,6 +14,8 @@
 
 package com.google.firebase.crashlytics.internal.common;
 
+import static java.util.Objects.requireNonNull;
+
 import android.os.Looper;
 import androidx.annotation.NonNull;
 import com.google.android.gms.tasks.Continuation;
@@ -36,22 +38,36 @@ private Utils() {}
   public static <T> Task<T> race(Task<T> t1, Task<T> t2) {
     final TaskCompletionSource<T> result = new TaskCompletionSource<>();
     Continuation<T, Void> continuation =
-        new Continuation<T, Void>() {
-          @Override
-          public Void then(@NonNull Task<T> task) throws Exception {
-            if (task.isSuccessful()) {
-              result.trySetResult(task.getResult());
-            } else {
-              result.trySetException(task.getException());
-            }
-            return null;
+        task -> {
+          if (task.isSuccessful()) {
+            result.trySetResult(task.getResult());
+          } else {
+            result.trySetException(requireNonNull(task.getException()));
           }
+          return null;
         };
     t1.continueWith(continuation);
     t2.continueWith(continuation);
     return result.getTask();
   }
 
+  /** @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 =
+        task -> {
+          if (task.isSuccessful()) {
+            result.trySetResult(task.getResult());
+          } else {
+            result.trySetException(requireNonNull(task.getException()));
+          }
+          return null;
+        };
+    t1.continueWith(executor, continuation);
+    t2.continueWith(executor, continuation);
+    return result.getTask();
+  }
+
   /** Similar to Tasks.call, but takes a Callable that returns a Task. */
   public static <T> Task<T> callTask(Executor executor, Callable<Task<T>> callable) {
     final TaskCompletionSource<T> tcs = new TaskCompletionSource<T>();

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

@@ -186,7 +186,7 @@ public Task<Void> loadSettingsData(SettingsCacheBehavior cacheBehavior, Executor
 
     // Kick off fetching fresh settings.
     return dataCollectionArbiter
-        .waitForDataCollectionPermission()
+        .waitForDataCollectionPermission(executor)
         .onSuccessTask(
             executor,
             new SuccessContinuation<Void, Void>() {