Skip to content

Forbid creating ui thread Handler. #4385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
() -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down
1 change: 1 addition & 0 deletions firebase-perf/firebase-perf.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ android {

dependencies {
// Firebase Deps
implementation project(':firebase-annotations')
implementation project(':firebase-common')
implementation project(':firebase-components')
implementation project(':firebase-config')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
25 changes: 19 additions & 6 deletions tools/lint/src/main/kotlin/ThreadPoolDetector.kt
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,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) {
Expand All @@ -50,11 +51,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 {
Expand Down