Merge branch 'master' into mrschmidt/simplifylocalstore

Author: schmidt-sebastian

ci/fireci/fireciplugins/macrobenchmark.py

@@ -184,7 +184,7 @@ async def _execute_benchmark_tests(self):
     args += ['--type', 'instrumentation']
     args += ['--app', app_apk_path]
     args += ['--test', test_apk_path]
-    args += ['--device', 'model=flame,version=30,locale=en,orientation=portrait']
+    args += ['--device', 'model=redfin,version=30,locale=en,orientation=portrait']
     args += ['--directories-to-pull', '/sdcard/Download']
     args += ['--results-bucket', f'gs://{self.test_results_bucket}']
     args += ['--results-dir', self.test_results_dir]

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/AabUpdater.java

@@ -16,10 +16,11 @@
 
 import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.DOWNLOAD_FAILURE;
 import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.NETWORK_FAILURE;
+import static com.google.firebase.appdistribution.TaskUtils.combineWithResultOf;
+import static com.google.firebase.appdistribution.TaskUtils.runAsyncInTask;
 import static com.google.firebase.appdistribution.TaskUtils.safeSetTaskException;
 
 import android.app.Activity;
-import android.content.Context;
 import android.content.Intent;
 import android.net.Uri;
 import androidx.annotation.GuardedBy;
@@ -42,29 +43,28 @@ class AabUpdater {
   private final HttpsUrlConnectionFactory httpsUrlConnectionFactory;
   private final Executor executor;
 
+  private final Object updateAabLock = new Object();
+
   @GuardedBy("updateAabLock")
   private UpdateTaskImpl cachedUpdateTask;
 
   @GuardedBy("updateAabLock")
   private AppDistributionReleaseInternal aabReleaseInProgress;
 
-  private final Object updateAabLock = new Object();
-  private final Context context;
+  @GuardedBy("updateAabLock")
+  private boolean hasBeenSentToPlayForCurrentTask = false;
 
-  AabUpdater(@NonNull Context context) {
+  AabUpdater() {
     this(
-        context,
         FirebaseAppDistributionLifecycleNotifier.getInstance(),
         new HttpsUrlConnectionFactory(),
         Executors.newSingleThreadExecutor());
   }
 
   AabUpdater(
-      @NonNull Context context,
       @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
       @NonNull HttpsUrlConnectionFactory httpsUrlConnectionFactory,
       @NonNull Executor executor) {
-    this.context = context;
     this.lifecycleNotifier = lifecycleNotifier;
     this.httpsUrlConnectionFactory = httpsUrlConnectionFactory;
     lifecycleNotifier.addOnActivityStartedListener(this::onActivityStarted);
@@ -76,9 +76,13 @@ void onActivityStarted(Activity activity) {
     if (activity instanceof SignInResultActivity || activity instanceof InstallActivity) {
       return;
     }
-    // If app resumes and aab update task is in progress, assume that installation didn't happen so
-    // cancel the task
-    this.tryCancelAabUpdateTask();
+    // If app resumes and update is in progress, assume that installation didn't happen and cancel
+    // the task
+    synchronized (updateAabLock) {
+      if (awaitingUpdateFromPlay()) {
+        this.tryCancelAabUpdateTask();
+      }
+    }
   }
 
   UpdateTaskImpl updateAab(@NonNull AppDistributionReleaseInternal newRelease) {
@@ -89,7 +93,15 @@ UpdateTaskImpl updateAab(@NonNull AppDistributionReleaseInternal newRelease) {
 
       cachedUpdateTask = new UpdateTaskImpl();
       aabReleaseInProgress = newRelease;
-      redirectToPlayForAabUpdate(newRelease.getDownloadUrl());
+      hasBeenSentToPlayForCurrentTask = false;
+
+      // On a background thread, fetch the redirect URL and open it in the Play app
+      runAsyncInTask(executor, () -> fetchDownloadRedirectUrl(newRelease.getDownloadUrl()))
+          .onSuccessTask(combineWithResultOf(() -> lifecycleNotifier.getForegroundActivity()))
+          .addOnSuccessListener(
+              urlAndActivity ->
+                  openRedirectUrlInPlay(urlAndActivity.first(), urlAndActivity.second()))
+          .addOnFailureListener(this::setUpdateTaskCompletionError);
 
       return cachedUpdateTask;
     }
@@ -138,38 +150,30 @@ private static boolean isRedirectResponse(int responseCode) {
     return responseCode >= 300 && responseCode < 400;
   }
 
-  private void redirectToPlayForAabUpdate(String downloadUrl) {
-    // The 302 redirect is obtained here to open the play store directly and avoid opening chrome
-    executor.execute( // Execute the network calls on a background thread
-        () -> {
-          String redirectUrl;
-          try {
-            redirectUrl = fetchDownloadRedirectUrl(downloadUrl);
-          } catch (FirebaseAppDistributionException e) {
-            setUpdateTaskCompletionError(e);
-            return;
-          }
-
-          Intent updateIntent = new Intent(Intent.ACTION_VIEW);
-          updateIntent.setData(Uri.parse(redirectUrl));
-          updateIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
-          LogWrapper.getInstance().v(TAG + "Redirecting to play");
-
-          synchronized (updateAabLock) {
-            context.startActivity(updateIntent);
-            cachedUpdateTask.updateProgress(
-                UpdateProgress.builder()
-                    .setApkBytesDownloaded(-1)
-                    .setApkFileTotalBytes(-1)
-                    .setUpdateStatus(UpdateStatus.REDIRECTED_TO_PLAY)
-                    .build());
-          }
-        });
+  private void openRedirectUrlInPlay(String redirectUrl, Activity hostActivity) {
+    Intent updateIntent = new Intent(Intent.ACTION_VIEW);
+    updateIntent.setData(Uri.parse(redirectUrl));
+    updateIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
+    LogWrapper.getInstance().v(TAG + "Redirecting to play");
+
+    // Launch the intent before the synchronized block to avoid failing to update in the rare
+    // scenario where the activity moves to the background while we're awaiting the lock.
+    hostActivity.startActivity(updateIntent);
+
+    synchronized (updateAabLock) {
+      cachedUpdateTask.updateProgress(
+          UpdateProgress.builder()
+              .setApkBytesDownloaded(-1)
+              .setApkFileTotalBytes(-1)
+              .setUpdateStatus(UpdateStatus.REDIRECTED_TO_PLAY)
+              .build());
+      hasBeenSentToPlayForCurrentTask = true;
+    }
   }
 
-  private void setUpdateTaskCompletionError(FirebaseAppDistributionException e) {
+  private void setUpdateTaskCompletionError(Exception e) {
     synchronized (updateAabLock) {
-      safeSetTaskException(cachedUpdateTask, e);
+      safeSetTaskException(cachedUpdateTask, FirebaseAppDistributionException.wrap(e));
     }
   }
 
@@ -183,4 +187,12 @@ void tryCancelAabUpdateTask() {
               ReleaseUtils.convertToAppDistributionRelease(aabReleaseInProgress)));
     }
   }
+
+  private boolean awaitingUpdateFromPlay() {
+    synchronized (updateAabLock) {
+      return cachedUpdateTask != null
+          && !cachedUpdateTask.isComplete()
+          && hasBeenSentToPlayForCurrentTask;
+    }
+  }
 }

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

@@ -64,6 +64,8 @@ public class FirebaseAppDistribution {
   private Task<AppDistributionRelease> cachedCheckForNewReleaseTask;
   private AlertDialog updateDialog;
   private AlertDialog signInConfirmationDialog;
+  private boolean remakeUpdateDialog = false;
+  private boolean remakeSignInConfirmationDialog = false;
 
   /** Constructor for FirebaseAppDistribution */
   @VisibleForTesting
@@ -83,6 +85,7 @@ public class FirebaseAppDistribution {
     this.signInStorage = signInStorage;
     this.lifecycleNotifier = lifecycleNotifier;
     lifecycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
+    lifecycleNotifier.addOnActivityCreatedListener(this::onActivityCreated);
   }
 
   /** Constructor for FirebaseAppDistribution */
@@ -99,7 +102,7 @@ public class FirebaseAppDistribution {
             new FirebaseAppDistributionTesterApiClient(),
             firebaseInstallationsApiProvider),
         new ApkUpdater(firebaseApp, new ApkInstaller()),
-        new AabUpdater(firebaseApp.getApplicationContext()),
+        new AabUpdater(),
         signInStorage,
         lifecycleNotifier);
   }
@@ -326,6 +329,14 @@ void onActivityDestroyed(@NonNull Activity activity) {
       // SignInResult is internal to the SDK and is destroyed after creation
       return;
     }
+    if (activity.isChangingConfigurations()) {
+      remakeSignInConfirmationDialog =
+          signInConfirmationDialog != null && signInConfirmationDialog.isShowing();
+      remakeUpdateDialog = updateDialog != null && updateDialog.isShowing();
+      dismissDialogs();
+      return;
+    }
+
     if (signInConfirmationDialog != null && signInConfirmationDialog.isShowing()) {
       setCachedUpdateIfNewReleaseCompletionError(
           new FirebaseAppDistributionException(
@@ -339,6 +350,19 @@ void onActivityDestroyed(@NonNull Activity activity) {
     }
   }
 
+  void onActivityCreated(Activity activity) {
+    if (remakeSignInConfirmationDialog) {
+      remakeSignInConfirmationDialog = false;
+      showSignInConfirmationDialog(activity);
+    } else if (remakeUpdateDialog) {
+      remakeUpdateDialog = false;
+      synchronized (cachedNewReleaseLock) {
+        showUpdateAlertDialog(
+            activity, ReleaseUtils.convertToAppDistributionRelease(cachedNewRelease));
+      }
+    }
+  }
+
   @VisibleForTesting
   void setCachedNewRelease(@Nullable AppDistributionReleaseInternal newRelease) {
     synchronized (cachedNewReleaseLock) {

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistributionException.java

@@ -17,6 +17,7 @@
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import com.google.firebase.FirebaseException;
+import com.google.firebase.appdistribution.Constants.ErrorMessages;
 
 /** Possible exceptions thrown in FirebaseAppDistribution */
 public class FirebaseAppDistributionException extends FirebaseException {
@@ -98,4 +99,13 @@ public AppDistributionRelease getRelease() {
   public Status getErrorCode() {
     return status;
   }
+
+  static FirebaseAppDistributionException wrap(Throwable t) {
+    // We never want to wrap a FirebaseAppDistributionException
+    if (t instanceof FirebaseAppDistributionException) {
+      return (FirebaseAppDistributionException) t;
+    }
+    return new FirebaseAppDistributionException(
+        String.format("%s: %s", ErrorMessages.UNKNOWN_ERROR, t.getMessage()), Status.UNKNOWN, t);
+  }
 }

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistributionLifecycleNotifier.java

@@ -83,6 +83,13 @@ interface OnActivityDestroyedListener {
     void onDestroyed(Activity activity);
   }
 
+  /**
+   * Get a {@link Task} that will succeed with a result of the app's foregrounded {@link Activity},
+   * when one is available.
+   *
+   * <p>The returned task will never fail. It will instead remain pending indefinitely until some
+   * activity comes to the foreground.
+   */
   Task<Activity> getForegroundActivity() {
     synchronized (lock) {
       if (currentActivity != null) {

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/TaskUtils.java

@@ -14,21 +14,31 @@
 
 package com.google.firebase.appdistribution;
 
+import com.google.android.gms.tasks.SuccessContinuation;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.android.gms.tasks.Tasks;
-import com.google.firebase.appdistribution.Constants.ErrorMessages;
+import com.google.auto.value.AutoValue;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
 import com.google.firebase.appdistribution.internal.LogWrapper;
 import java.util.concurrent.Executor;
 
 class TaskUtils {
   private static final String TAG = "TaskUtils:";
 
+  /**
+   * A functional interface to wrap a function that returns some result of a possibly long-running
+   * operation, and could potentially throw a {@link FirebaseAppDistributionException}.
+   */
   interface Operation<TResult> {
     TResult run() throws FirebaseAppDistributionException;
   }
 
+  /** A functional interface to wrap a function that produces a {@link Task}. */
+  interface TaskSource<TResult> {
+    Task<TResult> get();
+  }
+
   /**
    * Runs a long running operation inside a {@link Task}, wrapping any errors in {@link
    * FirebaseAppDistributionException}.
@@ -51,10 +61,8 @@ static <TResult> Task<TResult> runAsyncInTask(Executor executor, Operation<TResu
         () -> {
           try {
             taskCompletionSource.setResult(operation.run());
-          } catch (FirebaseAppDistributionException e) {
-            taskCompletionSource.setException(e);
           } catch (Throwable t) {
-            taskCompletionSource.setException(wrapException(t));
+            taskCompletionSource.setException(FirebaseAppDistributionException.wrap(t));
           }
         });
     return taskCompletionSource.getTask();
@@ -79,14 +87,61 @@ static <TResult> Task<TResult> handleTaskFailure(Task<TResult> task) {
       LogWrapper.getInstance().e(TAG + "Task failed to complete due to " + e.getMessage(), e);
       return e instanceof FirebaseAppDistributionException
           ? task
-          : Tasks.forException(wrapException(e));
+          : Tasks.forException(FirebaseAppDistributionException.wrap(e));
     }
     return task;
   }
 
-  private static FirebaseAppDistributionException wrapException(Throwable t) {
-    return new FirebaseAppDistributionException(
-        String.format("%s: %s", ErrorMessages.UNKNOWN_ERROR, t.getMessage()), Status.UNKNOWN, t);
+  /**
+   * An @{link AutoValue} class to hold the result of two Tasks, combined using {@link
+   * #combineWithResultOf}.
+   *
+   * @param <T1> The result type of the first task
+   * @param <T2> The result type of the second task
+   */
+  @AutoValue
+  abstract static class CombinedTaskResults<T1, T2> {
+    abstract T1 first();
+
+    abstract T2 second();
+
+    static <T1, T2> CombinedTaskResults<T1, T2> create(T1 first, T2 second) {
+      return new AutoValue_TaskUtils_CombinedTaskResults(first, second);
+    }
+  }
+
+  /**
+   * Returns a {@link SuccessContinuation} to be chained off of a {@link Task}, that will run
+   * another task in sequence and combine both results together.
+   *
+   * <p>This is useful when you want to run two tasks and use the results of each, but those tasks
+   * need to be run sequentially. If they can be run in parallel, use {@link Tasks#whenAll} or one
+   * of its variations.
+   *
+   * <p>Usage:
+   *
+   * <pre>{@code
+   * runFirstAsyncTask()
+   *   .onSuccessTask(combineWithResultOf(executor, () -> startSecondAsyncTask())
+   *   .addOnSuccessListener(
+   *       results ->
+   *           doSomethingWithBothResults(results.result1(), results.result2()));
+   * }</pre>
+   *
+   * @param secondTaskSource A {@link TaskSource} providing the next task to run
+   * @param <T1> The result type of the first task
+   * @param <T2> The result type of the second task
+   * @return A {@link SuccessContinuation} that will return a new task with result type {@link
+   *     CombinedTaskResults}, combining the results of both tasks
+   */
+  static <T1, T2> SuccessContinuation<T1, CombinedTaskResults<T1, T2>> combineWithResultOf(
+      TaskSource<T2> secondTaskSource) {
+    return firstResult ->
+        secondTaskSource
+            .get()
+            .onSuccessTask(
+                secondResult ->
+                    Tasks.forResult(CombinedTaskResults.create(firstResult, secondResult)));
   }
 
   static void safeSetTaskException(TaskCompletionSource taskCompletionSource, Exception e) {