Skip to content

Add lint check that detects UI thread continuations. #4363

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 2 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 @@ -21,14 +21,12 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import com.google.android.gms.tasks.Continuation;
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.FirebaseApp;
import com.google.firebase.appcheck.AppCheckProvider;
import com.google.firebase.appcheck.AppCheckToken;
import com.google.firebase.appcheck.internal.AppCheckTokenResponse;
import com.google.firebase.appcheck.internal.DefaultAppCheckToken;
import com.google.firebase.appcheck.internal.NetworkClient;
import com.google.firebase.appcheck.internal.RetryManager;
Expand Down Expand Up @@ -95,35 +93,31 @@ static Task<String> determineDebugSecret(
return taskCompletionSource.getTask();
}

// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
@Override
public Task<AppCheckToken> getToken() {
return debugSecretTask
.continueWithTask(
new Continuation<String, Task<AppCheckTokenResponse>>() {
@Override
public Task<AppCheckTokenResponse> then(@NonNull Task<String> task) throws Exception {
ExchangeDebugTokenRequest request = new ExchangeDebugTokenRequest(task.getResult());
return Tasks.call(
backgroundExecutor,
() ->
networkClient.exchangeAttestationForAppCheckToken(
request.toJsonString().getBytes(UTF_8),
NetworkClient.DEBUG,
retryManager));
}
task -> {
ExchangeDebugTokenRequest request = new ExchangeDebugTokenRequest(task.getResult());
return Tasks.call(
backgroundExecutor,
() ->
networkClient.exchangeAttestationForAppCheckToken(
request.toJsonString().getBytes(UTF_8),
NetworkClient.DEBUG,
retryManager));
})
.continueWithTask(
new Continuation<AppCheckTokenResponse, Task<AppCheckToken>>() {
@Override
public Task<AppCheckToken> then(@NonNull Task<AppCheckTokenResponse> task) {
if (task.isSuccessful()) {
return Tasks.forResult(
DefaultAppCheckToken.constructFromAppCheckTokenResponse(task.getResult()));
}
// TODO: Surface more error details.
return Tasks.forException(task.getException());
task -> {
if (task.isSuccessful()) {
return Tasks.forResult(
DefaultAppCheckToken.constructFromAppCheckTokenResponse(task.getResult()));
}
// TODO: Surface more error details.
return Tasks.forException(task.getException());
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ public PlayIntegrityAppCheckProvider(@NonNull FirebaseApp firebaseApp) {
this.retryManager = retryManager;
}

// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
@Override
public Task<AppCheckToken> getToken() {
Expand All @@ -90,6 +92,8 @@ public Task<AppCheckToken> getToken() {
});
}

// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
private Task<IntegrityTokenResponse> getPlayIntegrityAttestation() {
GeneratePlayIntegrityChallengeRequest generateChallengeRequest =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.android.gms.safetynet.SafetyNet;
import com.google.android.gms.safetynet.SafetyNetApi;
import com.google.android.gms.safetynet.SafetyNetClient;
import com.google.android.gms.tasks.Continuation;
import com.google.android.gms.tasks.Task;
import com.google.android.gms.tasks.TaskCompletionSource;
import com.google.android.gms.tasks.Tasks;
Expand Down Expand Up @@ -140,39 +139,35 @@ Task<SafetyNetClient> getSafetyNetClientTask() {
return safetyNetClientTask;
}

// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
@Override
public Task<AppCheckToken> getToken() {
return safetyNetClientTask
.continueWithTask(
new Continuation<SafetyNetClient, Task<SafetyNetApi.AttestationResponse>>() {
@Override
public Task<SafetyNetApi.AttestationResponse> then(
@NonNull Task<SafetyNetClient> task) {
if (task.isSuccessful()) {
return task.getResult().attest(NONCE.getBytes(), apiKey);
}
return Tasks.forException(task.getException());
task -> {
if (task.isSuccessful()) {
return task.getResult().attest(NONCE.getBytes(), apiKey);
}
return Tasks.forException(task.getException());
})
.continueWithTask(
new Continuation<SafetyNetApi.AttestationResponse, Task<AppCheckToken>>() {
@Override
public Task<AppCheckToken> then(
@NonNull Task<SafetyNetApi.AttestationResponse> task) {
if (!task.isSuccessful()) {
// Proxies errors to the client directly; need to wrap to get the
// types right.
// TODO: more specific error mapping to help clients debug more
// easily.
return Tasks.forException(task.getException());
} else {
return exchangeSafetyNetAttestationResponseForToken(task.getResult());
}
task -> {
if (!task.isSuccessful()) {
// Proxies errors to the client directly; need to wrap to get the
// types right.
// TODO: more specific error mapping to help clients debug more
// easily.
return Tasks.forException(task.getException());
} else {
return exchangeSafetyNetAttestationResponseForToken(task.getResult());
}
});
}

// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
Task<AppCheckToken> exchangeSafetyNetAttestationResponseForToken(
@NonNull SafetyNetApi.AttestationResponse attestationResponse) {
Expand All @@ -191,16 +186,13 @@ Task<AppCheckToken> exchangeSafetyNetAttestationResponseForToken(
NetworkClient.SAFETY_NET,
retryManager));
return networkTask.continueWithTask(
new Continuation<AppCheckTokenResponse, Task<AppCheckToken>>() {
@Override
public Task<AppCheckToken> then(@NonNull Task<AppCheckTokenResponse> task) {
if (task.isSuccessful()) {
return Tasks.forResult(
DefaultAppCheckToken.constructFromAppCheckTokenResponse(task.getResult()));
}
// TODO: Surface more error details.
return Tasks.forException(task.getException());
task -> {
if (task.isSuccessful()) {
return Tasks.forResult(
DefaultAppCheckToken.constructFromAppCheckTokenResponse(task.getResult()));
}
// TODO: Surface more error details.
return Tasks.forException(task.getException());
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import com.google.android.gms.tasks.Continuation;
import com.google.android.gms.tasks.Task;
import com.google.android.gms.tasks.TaskCompletionSource;
import com.google.android.gms.tasks.Tasks;
Expand Down Expand Up @@ -176,6 +175,8 @@ public void removeAppCheckListener(@NonNull AppCheckListener listener) {
appCheckTokenListenerList.size() + appCheckListenerList.size());
}

// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
@Override
public Task<AppCheckTokenResult> getToken(boolean forceRefresh) {
Expand Down Expand Up @@ -210,6 +211,8 @@ public Task<AppCheckTokenResult> getToken(boolean forceRefresh) {
});
}

// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
@Override
public Task<AppCheckToken> getAppCheckToken(boolean forceRefresh) {
Expand All @@ -226,27 +229,26 @@ public Task<AppCheckToken> getAppCheckToken(boolean forceRefresh) {
}

/** Fetches an {@link AppCheckToken} via the installed {@link AppCheckProvider}. */
// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
Task<AppCheckToken> fetchTokenFromProvider() {
return appCheckProvider
.getToken()
.continueWithTask(
new Continuation<AppCheckToken, Task<AppCheckToken>>() {
@Override
public Task<AppCheckToken> then(@NonNull Task<AppCheckToken> task) {
if (task.isSuccessful()) {
AppCheckToken token = task.getResult();
updateStoredToken(token);
for (AppCheckListener listener : appCheckListenerList) {
listener.onAppCheckTokenChanged(token);
}
AppCheckTokenResult tokenResult =
DefaultAppCheckTokenResult.constructFromAppCheckToken(token);
for (AppCheckTokenListener listener : appCheckTokenListenerList) {
listener.onAppCheckTokenChanged(tokenResult);
}
task -> {
if (task.isSuccessful()) {
AppCheckToken token = task.getResult();
updateStoredToken(token);
for (AppCheckListener listener : appCheckListenerList) {
listener.onAppCheckTokenChanged(token);
}
AppCheckTokenResult tokenResult =
DefaultAppCheckTokenResult.constructFromAppCheckToken(token);
for (AppCheckTokenListener listener : appCheckTokenListenerList) {
listener.onAppCheckTokenChanged(tokenResult);
}
return task;
}
return task;
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import android.annotation.SuppressLint;
import androidx.annotation.NonNull;
import androidx.annotation.VisibleForTesting;
import com.google.android.gms.tasks.OnFailureListener;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
Expand Down Expand Up @@ -93,16 +92,12 @@ private long getNextRefreshMillis() {
}
}

// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
private void onRefresh() {
firebaseAppCheck
.fetchTokenFromProvider()
.addOnFailureListener(
new OnFailureListener() {
@Override
public void onFailure(@NonNull Exception e) {
scheduleRefreshAfterFailure();
}
});
.addOnFailureListener(e -> scheduleRefreshAfterFailure());
}

/** Cancels the in-flight scheduled refresh. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.firebase.appdistribution.internal;

import android.annotation.SuppressLint;
import android.app.Activity;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
Expand All @@ -38,6 +39,8 @@
* Tasks}/{@link UpdateTask UpdateTasks} with {@link
* FirebaseAppDistributionException.Status#NOT_IMPLEMENTED}.
*/
// TODO(b/261013680): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
public class FirebaseAppDistributionStub implements FirebaseAppDistribution {
@NonNull
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class AabUpdater {
@GuardedBy("updateAabLock")
private boolean hasBeenSentToPlayForCurrentTask = false;

// TODO(b/258264924): Migrate to go/firebase-android-executors
// TODO(b/261014422): Migrate to go/firebase-android-executors
@SuppressLint("ThreadPoolCreation")
AabUpdater() {
this(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class ApkUpdater {

private final Object updateTaskLock = new Object();

// TODO(b/258264924): Migrate to go/firebase-android-executors
// TODO(b/261014422): Migrate to go/firebase-android-executors
@SuppressLint("ThreadPoolCreation")
public ApkUpdater(@NonNull FirebaseApp firebaseApp, @NonNull ApkInstaller apkInstaller) {
this(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskResult;

import android.annotation.SuppressLint;
import android.app.Activity;
import android.app.AlertDialog;
import android.content.Context;
Expand Down Expand Up @@ -101,6 +102,8 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {

@Override
@NonNull
// TODO(b/261014422): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
public UpdateTask updateIfNewReleaseAvailable() {
synchronized (updateIfNewReleaseTaskLock) {
if (updateIfNewReleaseAvailableIsTaskInProgress()) {
Expand All @@ -111,7 +114,6 @@ public UpdateTask updateIfNewReleaseAvailable() {
remakeUpdateConfirmationDialog = false;
dialogHostActivity = null;
}

lifecycleNotifier
.applyToForegroundActivityTask(this::showSignInConfirmationDialog)
.onSuccessTask(unused -> signInTester())
Expand Down Expand Up @@ -219,6 +221,8 @@ public void signOutTester() {

@Override
@NonNull
// TODO(b/261014422): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
public synchronized Task<AppDistributionRelease> checkForNewRelease() {
if (cachedCheckForNewReleaseTask != null && !cachedCheckForNewReleaseTask.isComplete()) {
LogWrapper.getInstance().v("Response in progress");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ private interface FidDependentJob<TResult> {
private final Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider;
private final TesterApiHttpClient testerApiHttpClient;

// TODO(b/258264924): Migrate to go/firebase-android-executors
// TODO(b/261014422): Migrate to go/firebase-android-executors
@SuppressLint("ThreadPoolCreation")
private final Executor taskExecutor = Executors.newSingleThreadExecutor();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static com.google.firebase.appdistribution.impl.PackageInfoUtils.getPackageInfo;

import android.annotation.SuppressLint;
import android.content.Context;
import androidx.annotation.NonNull;
import androidx.annotation.VisibleForTesting;
Expand Down Expand Up @@ -48,6 +49,8 @@ class NewReleaseFetcher {
this.releaseIdentifier = releaseIdentifier;
}

// TODO(b/261014422): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
public synchronized Task<AppDistributionReleaseInternal> checkForNewRelease() {
if (cachedCheckForNewRelease != null && !cachedCheckForNewRelease.isComplete()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskResult;

import android.annotation.SuppressLint;
import android.app.Activity;
import android.content.Context;
import android.content.Intent;
Expand Down Expand Up @@ -113,6 +114,8 @@ void onActivityResumed(Activity activity) {
}
}

// TODO(b/261014422): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
public Task<Void> signInTester() {
if (signInStorage.getSignInStatus()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.firebase.appdistribution.impl;

import android.annotation.SuppressLint;
import android.app.Activity;
import androidx.annotation.GuardedBy;
import androidx.annotation.NonNull;
Expand All @@ -34,6 +35,8 @@
import java.util.concurrent.Executor;

/** Implementation of UpdateTask, the return type of updateApp. */
// TODO(b/261013814): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
class UpdateTaskImpl extends UpdateTask {
@Nullable
@GuardedBy("lock")
Expand Down
Loading