Forbid creating ui thread Handler. (#4385)

* Forbid creating ui thread Handler. * fix perf tests * ktfmt * actually fix perf
firebase · Dec 1, 2022 · 0fc9a70 · 0fc9a70
1 parent 0955f12
commit 0fc9a70
Show file tree

Hide file tree

Showing 14 changed files with 72 additions and 19 deletions.
diff --git a/firebase-common/src/main/java/com/google/firebase/concurrent/UiExecutor.java b/firebase-common/src/main/java/com/google/firebase/concurrent/UiExecutor.java
@@ -14,6 +14,7 @@
 
 package com.google.firebase.concurrent;
 
+import android.annotation.SuppressLint;
 import android.os.Handler;
 import android.os.Looper;
 import java.util.concurrent.Executor;
@@ -22,6 +23,8 @@
 public enum UiExecutor implements Executor {
   INSTANCE;
 
+  // This is the only UI handler that is allowed in Firebase SDK.
+  @SuppressLint("ThreadPoolCreation")
   private static final Handler HANDLER = new Handler(Looper.getMainLooper());
 
   @Override

diff --git a/firebase-database/src/main/java/com/google/firebase/database/android/AndroidEventTarget.java b/firebase-database/src/main/java/com/google/firebase/database/android/AndroidEventTarget.java
@@ -14,13 +14,16 @@
 
 package com.google.firebase.database.android;
 
+import android.annotation.SuppressLint;
 import android.os.Handler;
 import android.os.Looper;
 import com.google.firebase.database.core.EventTarget;
 
 public class AndroidEventTarget implements EventTarget {
   private final Handler handler;
 
+  // TODO(b/258277572): Migrate to go/firebase-android-executors
+  @SuppressLint("ThreadPoolCreation")
   public AndroidEventTarget() {
     this.handler = new Handler(Looper.getMainLooper());
   }

diff --git a/firebase-database/src/main/java/com/google/firebase/database/android/AndroidPlatform.java b/firebase-database/src/main/java/com/google/firebase/database/android/AndroidPlatform.java
@@ -14,6 +14,7 @@
 
 package com.google.firebase.database.android;
 
+import android.annotation.SuppressLint;
 import android.content.Context;
 import android.os.Build;
 import android.os.Handler;
@@ -88,6 +89,8 @@ public void handleException(final Throwable e) {
         // Rethrow on main thread, so the application will crash
         // The exception might indicate that there is something seriously wrong and better crash,
         // than continue run in an undefined state...
+        // TODO(b/258277572): Migrate to go/firebase-android-executors
+        @SuppressLint("ThreadPoolCreation")
         Handler handler = new Handler(applicationContext.getMainLooper());
         handler.post(
             new Runnable() {

diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java
@@ -522,6 +522,9 @@ public void skipDelaysForTimerId(TimerId timerId) {
    */
   public void panic(Throwable t) {
     executor.shutdownNow();
+
+    // TODO(b/258277574): Migrate to go/firebase-android-executors
+    @SuppressLint("ThreadPoolCreation")
     Handler handler = new Handler(Looper.getMainLooper());
     handler.post(
         () -> {

diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java
@@ -14,6 +14,7 @@
 
 package com.google.firebase.firestore.util;
 
+import android.annotation.SuppressLint;
 import android.os.Handler;
 import android.os.Looper;
 import androidx.annotation.Nullable;
@@ -206,6 +207,8 @@ public static String typeName(@Nullable Object obj) {
   }
 
   /** Raises an exception on Android's UI Thread and crashes the end user's app. */
+  // TODO(b/258277574): Migrate to go/firebase-android-executors
+  @SuppressLint("ThreadPoolCreation")
   public static void crashMainThread(RuntimeException exception) {
     new Handler(Looper.getMainLooper())
         .post(

diff --git a/firebase-messaging/src/main/java/com/google/firebase/messaging/FcmLifecycleCallbacks.java b/firebase-messaging/src/main/java/com/google/firebase/messaging/FcmLifecycleCallbacks.java
@@ -15,6 +15,7 @@
 
 import static com.google.firebase.messaging.Constants.TAG;
 
+import android.annotation.SuppressLint;
 import android.app.Activity;
 import android.app.Application;
 import android.content.Intent;
@@ -34,6 +35,8 @@ class FcmLifecycleCallbacks implements Application.ActivityLifecycleCallbacks {
   private final Set<Intent> seenIntents =
       Collections.newSetFromMap(new WeakHashMap<Intent, Boolean>());
 
+  // TODO(b/258424124): Migrate to go/firebase-android-executors
+  @SuppressLint("ThreadPoolCreation")
   @Override
   public void onActivityCreated(Activity createdActivity, Bundle instanceState) {
     Intent startingIntent = createdActivity.getIntent();

diff --git a/firebase-perf/firebase-perf.gradle b/firebase-perf/firebase-perf.gradle
@@ -92,6 +92,7 @@ android {
 
 dependencies {
     // Firebase Deps
+    implementation project(':firebase-annotations')
     implementation project(':firebase-common')
     implementation project(':firebase-components')
     implementation project(':firebase-config')

diff --git a/firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerfEarly.java b/firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerfEarly.java
@@ -15,26 +15,26 @@
 package com.google.firebase.perf;
 
 import android.content.Context;
-import android.os.Handler;
-import android.os.Looper;
-import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.StartupTime;
 import com.google.firebase.perf.application.AppStateMonitor;
 import com.google.firebase.perf.config.ConfigResolver;
 import com.google.firebase.perf.metrics.AppStartTrace;
 import com.google.firebase.perf.session.SessionManager;
+import java.util.concurrent.Executor;
 
 /**
  * The Firebase Performance early initialization.
  *
  * <p>Responsible for initializing the AppStartTrace, and early initialization of ConfigResolver
+ *
+ * @hide
  */
 public class FirebasePerfEarly {
-  @NonNull private final Handler mainHandler = new Handler(Looper.getMainLooper());
 
-  public FirebasePerfEarly(@NonNull FirebaseApp app, @Nullable StartupTime startupTime) {
+  public FirebasePerfEarly(
+      FirebaseApp app, @Nullable StartupTime startupTime, Executor uiExecutor) {
     Context context = app.getApplicationContext();
 
     // Initialize ConfigResolver early for accessing device caching layer.
@@ -48,7 +48,7 @@ public FirebasePerfEarly(@NonNull FirebaseApp app, @Nullable StartupTime startup
     if (startupTime != null) {
       AppStartTrace appStartTrace = AppStartTrace.getInstance();
       appStartTrace.registerActivityLifecycleCallbacks(context);
-      mainHandler.post(new AppStartTrace.StartFromBackgroundRunnable(appStartTrace));
+      uiExecutor.execute(new AppStartTrace.StartFromBackgroundRunnable(appStartTrace));
     }
 
     // In the case of cold start, we create a session and start collecting gauges as early as

diff --git a/firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerfRegistrar.java b/firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerfRegistrar.java
@@ -18,10 +18,12 @@
 import com.google.android.datatransport.TransportFactory;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.StartupTime;
+import com.google.firebase.annotations.concurrent.UiThread;
 import com.google.firebase.components.Component;
 import com.google.firebase.components.ComponentContainer;
 import com.google.firebase.components.ComponentRegistrar;
 import com.google.firebase.components.Dependency;
+import com.google.firebase.components.Qualified;
 import com.google.firebase.installations.FirebaseInstallationsApi;
 import com.google.firebase.perf.injection.components.DaggerFirebasePerformanceComponent;
 import com.google.firebase.perf.injection.components.FirebasePerformanceComponent;
@@ -30,6 +32,7 @@
 import com.google.firebase.remoteconfig.RemoteConfigComponent;
 import java.util.Arrays;
 import java.util.List;
+import java.util.concurrent.Executor;
 
 /**
  * {@link com.google.firebase.components.ComponentRegistrar} for the Firebase Performance SDK.
@@ -47,6 +50,7 @@ public class FirebasePerfRegistrar implements ComponentRegistrar {
   @Override
   @Keep
   public List<Component<?>> getComponents() {
+    Qualified<Executor> uiExecutor = Qualified.qualified(UiThread.class, Executor.class);
     return Arrays.asList(
         Component.builder(FirebasePerformance.class)
             .name(LIBRARY_NAME)
@@ -61,8 +65,14 @@ public List<Component<?>> getComponents() {
             .name(EARLY_LIBRARY_NAME)
             .add(Dependency.required(FirebaseApp.class))
             .add(Dependency.optionalProvider(StartupTime.class))
+            .add(Dependency.required(uiExecutor))
             .eagerInDefaultApp()
-            .factory(FirebasePerfRegistrar::providesFirebasePerformanceEarly)
+            .factory(
+                container ->
+                    new FirebasePerfEarly(
+                        container.get(FirebaseApp.class),
+                        container.getProvider(StartupTime.class).get(),
+                        container.get(uiExecutor)))
             .build(),
         /**
          * Fireperf SDK is lazily by {@link FirebasePerformanceInitializer} during {@link
@@ -89,9 +99,4 @@ private static FirebasePerformance providesFirebasePerformance(ComponentContaine
 
     return component.getFirebasePerformance();
   }
-
-  private static FirebasePerfEarly providesFirebasePerformanceEarly(ComponentContainer container) {
-    return new FirebasePerfEarly(
-        container.get(FirebaseApp.class), container.getProvider(StartupTime.class).get());
-  }
 }
diff --git a/firebase-perf/src/main/java/com/google/firebase/perf/util/FirstDrawDoneListener.java b/firebase-perf/src/main/java/com/google/firebase/perf/util/FirstDrawDoneListener.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.firebase.perf.util;
 
+import android.annotation.SuppressLint;
 import android.os.Build;
 import android.os.Handler;
 import android.os.Looper;
@@ -25,7 +26,10 @@
  * 16+ implementation is an approximation of the initial-display-time defined by Android Vitals.
  */
 public class FirstDrawDoneListener implements ViewTreeObserver.OnDrawListener {
+  // TODO(b/258263016): Migrate to go/firebase-android-executors
+  @SuppressLint("ThreadPoolCreation")
   private final Handler mainThreadHandler = new Handler(Looper.getMainLooper());
+
   private final AtomicReference<View> viewReference;
   private final Runnable callback;
 

diff --git a/firebase-perf/src/main/java/com/google/firebase/perf/util/PreDrawListener.java b/firebase-perf/src/main/java/com/google/firebase/perf/util/PreDrawListener.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.firebase.perf.util;
 
+import android.annotation.SuppressLint;
 import android.os.Handler;
 import android.os.Looper;
 import android.view.View;
@@ -24,7 +25,10 @@
  * OnPreDraw. This is an approximation of the initial-display time defined by Android Vitals.
  */
 public class PreDrawListener implements ViewTreeObserver.OnPreDrawListener {
+  // TODO(b/258263016): Migrate to go/firebase-android-executors
+  @SuppressLint("ThreadPoolCreation")
   private final Handler mainThreadHandler = new Handler(Looper.getMainLooper());
+
   private final AtomicReference<View> viewReference;
   private final Runnable callback;
 

diff --git a/firebase-perf/src/test/java/com/google/firebase/perf/FirebasePerfRegistrarTest.java b/firebase-perf/src/test/java/com/google/firebase/perf/FirebasePerfRegistrarTest.java
@@ -19,11 +19,14 @@
 import com.google.android.datatransport.TransportFactory;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.StartupTime;
+import com.google.firebase.annotations.concurrent.UiThread;
 import com.google.firebase.components.Component;
 import com.google.firebase.components.Dependency;
+import com.google.firebase.components.Qualified;
 import com.google.firebase.installations.FirebaseInstallationsApi;
 import com.google.firebase.remoteconfig.RemoteConfigComponent;
 import java.util.List;
+import java.util.concurrent.Executor;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.RobolectricTestRunner;
@@ -54,7 +57,9 @@ public void testGetComponents() {
 
     assertThat(firebasePerfEarlyComponent.getDependencies())
         .containsExactly(
-            Dependency.required(FirebaseApp.class), Dependency.optionalProvider(StartupTime.class));
+            Dependency.required(Qualified.qualified(UiThread.class, Executor.class)),
+            Dependency.required(FirebaseApp.class),
+            Dependency.optionalProvider(StartupTime.class));
 
     assertThat(firebasePerfEarlyComponent.isLazy()).isFalse();
   }

diff --git a/firebase-storage/src/main/java/com/google/firebase/storage/internal/SmartHandler.java b/firebase-storage/src/main/java/com/google/firebase/storage/internal/SmartHandler.java
@@ -14,6 +14,7 @@
 
 package com.google.firebase.storage.internal;
 
+import android.annotation.SuppressLint;
 import android.os.Handler;
 import android.os.Looper;
 import androidx.annotation.NonNull;
@@ -43,6 +44,8 @@ public class SmartHandler {
   /*package*/ static boolean testMode = false;
 
   /** Constructs a SmartHandler */
+  // TODO(b/258426744): Migrate to go/firebase-android-executors
+  @SuppressLint("ThreadPoolCreation")
   public SmartHandler(@Nullable Executor executor) {
     this.executor = executor;
     if (this.executor == null) {

diff --git a/tools/lint/src/main/kotlin/ThreadPoolDetector.kt b/tools/lint/src/main/kotlin/ThreadPoolDetector.kt
@@ -44,7 +44,8 @@ class ThreadPoolDetector : Detector(), SourceCodeScanner {
       "java.lang.Thread",
       "java.util.concurrent.ForkJoinPool",
       "java.util.concurrent.ThreadPoolExecutor",
-      "java.util.concurrent.ScheduledThreadPoolExecutor"
+      "java.util.concurrent.ScheduledThreadPoolExecutor",
+      "android.os.Handler"
     )
 
   override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) {
@@ -64,11 +65,23 @@ class ThreadPoolDetector : Detector(), SourceCodeScanner {
     node: UCallExpression,
     constructor: PsiMethod
   ) {
-    context.report(
-      THREAD_POOL_CREATION,
-      context.getCallLocation(node, includeReceiver = false, includeArguments = true),
-      "Creating threads or thread pools is not allowed"
-    )
+    val cls = (constructor.parent as? PsiClass) ?: return
+    if (cls.qualifiedName == "android.os.Handler") {
+      if (node.valueArgumentCount == 0) return
+      if (node.valueArguments[0].toString().endsWith("getMainLooper()")) {
+        context.report(
+          THREAD_POOL_CREATION,
+          context.getCallLocation(node, includeReceiver = false, includeArguments = true),
+          "Creating Ui thread loopers is not allowed, use a `@UiThread Executor` instead"
+        )
+      }
+    } else {
+      context.report(
+        THREAD_POOL_CREATION,
+        context.getCallLocation(node, includeReceiver = false, includeArguments = true),
+        "Creating threads or thread pools is not allowed"
+      )
+    }
   }
 
   private fun isExecutorMethod(method: PsiMethod): Boolean {