Always call foreground activity callbacks on the UI thread (#4531)

* Always call foreground activity callbacks on the UI thread

This will make it easier to reason about what runs on what thread.

* Refactor getActivityWithIgnoredClass to consolidate synchronized blocks

Author: lfkellogg

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

@@ -47,13 +47,10 @@ class AabUpdater {
   private boolean hasBeenSentToPlayForCurrentTask = false;
 
   AabUpdater(
+      @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
       @NonNull @Blocking Executor blockingExecutor,
       @NonNull @Lightweight Executor lightweightExecutor) {
-    this(
-        FirebaseAppDistributionLifecycleNotifier.getInstance(),
-        new HttpsUrlConnectionFactory(),
-        blockingExecutor,
-        lightweightExecutor);
+    this(lifecycleNotifier, new HttpsUrlConnectionFactory(), blockingExecutor, lightweightExecutor);
   }
 
   AabUpdater(

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ApkInstaller.java

@@ -43,10 +43,6 @@ class ApkInstaller {
     lifeCycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
   }
 
-  ApkInstaller(@Lightweight Executor lightweightExecutor) {
-    this(FirebaseAppDistributionLifecycleNotifier.getInstance(), lightweightExecutor);
-  }
-
   void onActivityDestroyed(@Nullable Activity activity) {
     if (activity instanceof InstallActivity) {
       // Since install activity is destroyed but app is still active, installation has failed /

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ApkUpdater.java

@@ -58,27 +58,28 @@ class ApkUpdater {
   public ApkUpdater(
       @NonNull FirebaseApp firebaseApp,
       @NonNull ApkInstaller apkInstaller,
+      @NonNull FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier,
       @NonNull @Blocking Executor blockingExecutor,
       @NonNull @Lightweight Executor lightweightExecutor) {
     this(
-        blockingExecutor,
-        lightweightExecutor,
         firebaseApp.getApplicationContext(),
         apkInstaller,
         new FirebaseAppDistributionNotificationsManager(firebaseApp.getApplicationContext()),
         new HttpsUrlConnectionFactory(),
-        FirebaseAppDistributionLifecycleNotifier.getInstance());
+        lifeCycleNotifier,
+        blockingExecutor,
+        lightweightExecutor);
   }
 
   @VisibleForTesting
   public ApkUpdater(
-      @NonNull @Blocking Executor blockingExecutor,
-      @NonNull @Lightweight Executor lightweightExecutor,
       @NonNull Context context,
       @NonNull ApkInstaller apkInstaller,
       @NonNull FirebaseAppDistributionNotificationsManager appDistributionNotificationsManager,
       @NonNull HttpsUrlConnectionFactory httpsUrlConnectionFactory,
-      @NonNull FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier) {
+      @NonNull FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier,
+      @NonNull @Blocking Executor blockingExecutor,
+      @NonNull @Lightweight Executor lightweightExecutor) {
     this.blockingExecutor = blockingExecutor;
     this.lightweightExecutor = lightweightExecutor;
     this.context = context;

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

@@ -14,9 +14,12 @@
 
 package com.google.firebase.appdistribution.impl;
 
+import static com.google.firebase.appdistribution.impl.TaskUtils.runAsyncInTask;
+
 import android.app.Activity;
 import android.app.Application;
 import android.os.Bundle;
+import android.os.Looper;
 import androidx.annotation.GuardedBy;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
@@ -25,6 +28,7 @@
 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.annotations.concurrent.UiThread;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException;
 import java.util.ArrayDeque;
 import java.util.Queue;
@@ -53,6 +57,7 @@ interface NullableActivityFunction<T> {
   }
 
   private static FirebaseAppDistributionLifecycleNotifier instance;
+  private final Executor uiThreadExecutor;
   private final Object lock = new Object();
 
   @GuardedBy("lock")
@@ -82,11 +87,14 @@ interface NullableActivityFunction<T> {
   private final Queue<OnActivityDestroyedListener> onDestroyedListeners = new ArrayDeque<>();
 
   @VisibleForTesting
-  FirebaseAppDistributionLifecycleNotifier() {}
+  FirebaseAppDistributionLifecycleNotifier(Executor uiThreadExecutor) {
+    this.uiThreadExecutor = uiThreadExecutor;
+  }
 
-  static synchronized FirebaseAppDistributionLifecycleNotifier getInstance() {
+  static synchronized FirebaseAppDistributionLifecycleNotifier getInstance(
+      @UiThread Executor uiThreadExecutor) {
     if (instance == null) {
-      instance = new FirebaseAppDistributionLifecycleNotifier();
+      instance = new FirebaseAppDistributionLifecycleNotifier(uiThreadExecutor);
     }
     return instance;
   }
@@ -115,9 +123,7 @@ interface OnActivityDestroyedListener {
    * Apply a function to a foreground activity, when one is available, returning a {@link Task} that
    * will complete immediately after the function is applied.
    *
-   * <p>The function will be called immediately once the activity is available. This may be main
-   * thread or the calling thread, depending on whether or not there is already a foreground
-   * activity available when this method is called.
+   * <p>The function will always be called on the UI thread.
    */
   <T> Task<T> applyToForegroundActivity(ActivityFunction<T> function) {
     return getForegroundActivity(null)
@@ -141,9 +147,7 @@ <T> Task<T> applyToForegroundActivity(ActivityFunction<T> function) {
    * will be passed to the function, which may be null if there was no previously active activity or
    * the activity has been destroyed.
    *
-   * <p>The function will be called immediately once the activity is available. This may be main
-   * thread or the calling thread, depending on whether or not there is already a foreground
-   * activity available when this method is called.
+   * <p>The function will always be called on the UI thread.
    */
   <T, A extends Activity> Task<T> applyToNullableForegroundActivity(
       Class<A> classToIgnore, NullableActivityFunction<T> function) {
@@ -168,9 +172,7 @@ <T, A extends Activity> Task<T> applyToNullableForegroundActivity(
    * will be passed to the function, which may be null if there was no previously active activity or
    * the activity has been destroyed.
    *
-   * <p>The continuation function will be called immediately once the activity is available. This
-   * may be on the main thread or the calling thread, depending on whether or not there is already a
-   * foreground activity available when this method is called.
+   * <p>The function will always be called on the UI thread.
    */
   <T, A extends Activity> Task<T> applyToNullableForegroundActivityTask(
       Class<A> classToIgnore, SuccessContinuation<Activity, T> continuation) {
@@ -207,9 +209,7 @@ Task<Void> consumeForegroundActivity(ActivityConsumer consumer) {
    * Apply a function to a foreground activity, when one is available, returning a {@link Task} that
    * will complete with the result of the Task returned by that function.
    *
-   * <p>The continuation function will be called immediately once the activity is available. This
-   * may be on the main thread or the calling thread, depending on whether or not there is already a
-   * foreground activity available when this method is called.
+   * <p>The function will always be called on the UI thread.
    */
   <T> Task<T> applyToForegroundActivityTask(SuccessContinuation<Activity, T> continuation) {
     return getForegroundActivity()
@@ -232,40 +232,40 @@ Task<Activity> getForegroundActivity() {
   <A extends Activity> Task<Activity> getForegroundActivity(@Nullable Class<A> classToIgnore) {
     synchronized (lock) {
       if (currentActivity != null) {
-        return Tasks.forResult(
-            getActivityWithIgnoredClass(currentActivity, previousActivity, classToIgnore));
+        Activity foregroundActivity = getCurrentActivityWithIgnoredClass(classToIgnore);
+        if (Looper.myLooper() == Looper.getMainLooper()) {
+          // We're already on the UI thread, so just complete the task with the activity
+          return Tasks.forResult(foregroundActivity);
+        } else {
+          // Run in UI thread so that returned Task will be completed on the UI thread
+          return runAsyncInTask(
+              uiThreadExecutor, () -> getCurrentActivityWithIgnoredClass(classToIgnore));
+        }
       }
     }
 
     TaskCompletionSource<Activity> task = new TaskCompletionSource<>();
-
     addOnActivityResumedListener(
         new OnActivityResumedListener() {
           @Override
           public void onResumed(Activity activity) {
-            task.setResult(
-                getActivityWithIgnoredClass(activity, getPreviousActivity(), classToIgnore));
+            // Since this method is run on the UI thread, the Task is completed on the UI thread
+            task.setResult(getCurrentActivityWithIgnoredClass(classToIgnore));
             removeOnActivityResumedListener(this);
           }
         });
-
     return task.getTask();
   }
 
   @Nullable
-  private Activity getPreviousActivity() {
+  private <A extends Activity> Activity getCurrentActivityWithIgnoredClass(
+      @Nullable Class<A> classToIgnore) {
     synchronized (lock) {
-      return previousActivity;
-    }
-  }
-
-  @Nullable
-  private static <A extends Activity> Activity getActivityWithIgnoredClass(
-      Activity activity, @Nullable Activity fallbackActivity, @Nullable Class<A> classToIgnore) {
-    if (classToIgnore != null && classToIgnore.isInstance(activity)) {
-      return fallbackActivity;
-    } else {
-      return activity;
+      if (classToIgnore != null && classToIgnore.isInstance(currentActivity)) {
+        return previousActivity;
+      } else {
+        return currentActivity;
+      }
     }
   }
 

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java

@@ -22,6 +22,7 @@
 import com.google.firebase.annotations.concurrent.Background;
 import com.google.firebase.annotations.concurrent.Blocking;
 import com.google.firebase.annotations.concurrent.Lightweight;
+import com.google.firebase.annotations.concurrent.UiThread;
 import com.google.firebase.appdistribution.FirebaseAppDistribution;
 import com.google.firebase.components.Component;
 import com.google.firebase.components.ComponentContainer;
@@ -52,6 +53,7 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
     Qualified<Executor> blockingExecutor = Qualified.qualified(Blocking.class, Executor.class);
     Qualified<Executor> lightweightExecutor =
         Qualified.qualified(Lightweight.class, Executor.class);
+    Qualified<Executor> uiThreadExecutor = Qualified.qualified(UiThread.class, Executor.class);
     return Arrays.asList(
         Component.builder(FirebaseAppDistribution.class)
             .name(LIBRARY_NAME)
@@ -60,18 +62,22 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
             .add(Dependency.required(backgroundExecutor))
             .add(Dependency.required(blockingExecutor))
             .add(Dependency.required(lightweightExecutor))
+            .add(Dependency.required(uiThreadExecutor))
             .factory(
                 c ->
                     buildFirebaseAppDistribution(
-                        c, backgroundExecutor, blockingExecutor, lightweightExecutor))
+                        c,
+                        backgroundExecutor,
+                        blockingExecutor,
+                        lightweightExecutor,
+                        uiThreadExecutor))
             // construct FirebaseAppDistribution instance on startup so we can register for
             // activity lifecycle callbacks before the API is called
             .alwaysEager()
             .build(),
         Component.builder(FeedbackSender.class)
             .add(Dependency.required(FirebaseApp.class))
             .add(Dependency.requiredProvider(FirebaseInstallationsApi.class))
-            .add(Dependency.required(backgroundExecutor))
             .add(Dependency.required(blockingExecutor))
             .factory(c -> buildFeedbackSender(c, c.get(blockingExecutor)))
             .build(),
@@ -96,11 +102,13 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
       ComponentContainer container,
       Qualified<Executor> backgroundExecutorType,
       Qualified<Executor> blockingExecutorType,
-      Qualified<Executor> lightweightExecutorType) {
+      Qualified<Executor> lightweightExecutorType,
+      Qualified<Executor> uiThreadExecutorType) {
     FirebaseApp firebaseApp = container.get(FirebaseApp.class);
     Executor backgroundExecutor = container.get(backgroundExecutorType);
     Executor blockingExecutor = container.get(blockingExecutorType);
     Executor lightweightExecutor = container.get(lightweightExecutorType);
+    Executor uiThreadExecutor = container.get(uiThreadExecutorType);
     Context context = firebaseApp.getApplicationContext();
     Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider =
         container.getProvider(FirebaseInstallationsApi.class);
@@ -112,24 +120,29 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
             blockingExecutor);
     SignInStorage signInStorage = new SignInStorage(context, backgroundExecutor);
     FirebaseAppDistributionLifecycleNotifier lifecycleNotifier =
-        FirebaseAppDistributionLifecycleNotifier.getInstance();
+        FirebaseAppDistributionLifecycleNotifier.getInstance(uiThreadExecutor);
     ReleaseIdentifier releaseIdentifier = new ReleaseIdentifier(firebaseApp, testerApiClient);
     FirebaseAppDistribution appDistribution =
         new FirebaseAppDistributionImpl(
             firebaseApp,
             new TesterSignInManager(
-                firebaseApp, firebaseInstallationsApiProvider, signInStorage, lightweightExecutor),
+                firebaseApp,
+                firebaseInstallationsApiProvider,
+                signInStorage,
+                lifecycleNotifier,
+                lightweightExecutor),
             new NewReleaseFetcher(
                 firebaseApp.getApplicationContext(),
                 testerApiClient,
                 releaseIdentifier,
                 lightweightExecutor),
             new ApkUpdater(
                 firebaseApp,
-                new ApkInstaller(lightweightExecutor),
+                new ApkInstaller(lifecycleNotifier, lightweightExecutor),
+                lifecycleNotifier,
                 blockingExecutor,
                 lightweightExecutor),
-            new AabUpdater(blockingExecutor, lightweightExecutor),
+            new AabUpdater(lifecycleNotifier, blockingExecutor, lightweightExecutor),
             signInStorage,
             lifecycleNotifier,
             releaseIdentifier,

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/TesterSignInManager.java

@@ -56,20 +56,6 @@ class TesterSignInManager {
 
   private boolean hasBeenSentToBrowserForCurrentTask = false;
 
-  TesterSignInManager(
-      @NonNull FirebaseApp firebaseApp,
-      @NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,
-      @NonNull final SignInStorage signInStorage,
-      @Lightweight Executor lightweightExecutor) {
-    this(
-        firebaseApp,
-        firebaseInstallationsApiProvider,
-        signInStorage,
-        FirebaseAppDistributionLifecycleNotifier.getInstance(),
-        lightweightExecutor);
-  }
-
-  @VisibleForTesting
   TesterSignInManager(
       @NonNull FirebaseApp firebaseApp,
       @NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,

firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/impl/ApkUpdaterTest.java

@@ -125,13 +125,13 @@ public void setup() throws IOException, FirebaseAppDistributionException {
     apkUpdater =
         Mockito.spy(
             new ApkUpdater(
-                blockingExecutor,
-                lightweightExecutor,
                 ApplicationProvider.getApplicationContext(),
                 mockApkInstaller,
                 mockNotificationsManager,
                 mockHttpsUrlConnectionFactory,
-                mockLifecycleNotifier));
+                mockLifecycleNotifier,
+                blockingExecutor,
+                lightweightExecutor));
   }
 
   @Test

firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionLifecycleNotifierTest.java

@@ -26,11 +26,14 @@
 import com.google.android.gms.tasks.SuccessContinuation;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.Tasks;
+import com.google.firebase.annotations.concurrent.UiThread;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
 import com.google.firebase.appdistribution.impl.FirebaseAppDistributionLifecycleNotifier.ActivityConsumer;
 import com.google.firebase.appdistribution.impl.FirebaseAppDistributionLifecycleNotifier.ActivityFunction;
 import com.google.firebase.appdistribution.impl.FirebaseAppDistributionLifecycleNotifier.NullableActivityFunction;
+import com.google.firebase.concurrent.TestOnlyExecutors;
+import java.util.concurrent.Executor;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -47,6 +50,8 @@ static class TestActivity extends Activity {}
 
   static class OtherTestActivity extends Activity {}
 
+  private final @UiThread Executor uiThreadExecutor = TestOnlyExecutors.ui();
+
   @Captor private ArgumentCaptor<Activity> activityArgCaptor;
   private TestActivity activity;
   private OtherTestActivity otherActivity;
@@ -57,7 +62,7 @@ public void setup() {
     MockitoAnnotations.initMocks(this);
     activity = Robolectric.buildActivity(TestActivity.class).create().get();
     otherActivity = Robolectric.buildActivity(OtherTestActivity.class).create().get();
-    lifecycleNotifier = new FirebaseAppDistributionLifecycleNotifier();
+    lifecycleNotifier = new FirebaseAppDistributionLifecycleNotifier(uiThreadExecutor);
   }
 
   @Test