Skip to content

Commit d1d5b47

Browse files
lfkellogglfkellogg
authored
Await foreground activity during sign in (#3340)
* Await foreground activity during sign in * Some cleanup and better error handling * Add tests * Don't kick off second task until first is finished * Respond to Manny's feedback
1 parent 4de5c56 commit d1d5b47

File tree

6 files changed

+169
-57
lines changed

6 files changed

+169
-57
lines changed

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import androidx.annotation.NonNull;
1818
import androidx.annotation.Nullable;
1919
import com.google.firebase.FirebaseException;
20+
import com.google.firebase.appdistribution.Constants.ErrorMessages;
2021

2122
/** Possible exceptions thrown in FirebaseAppDistribution */
2223
public class FirebaseAppDistributionException extends FirebaseException {
@@ -98,4 +99,13 @@ public AppDistributionRelease getRelease() {
9899
public Status getErrorCode() {
99100
return status;
100101
}
102+
103+
static FirebaseAppDistributionException wrap(Throwable t) {
104+
// We never want to wrap a FirebaseAppDistributionException
105+
if (t instanceof FirebaseAppDistributionException) {
106+
return (FirebaseAppDistributionException) t;
107+
}
108+
return new FirebaseAppDistributionException(
109+
String.format("%s: %s", ErrorMessages.UNKNOWN_ERROR, t.getMessage()), Status.UNKNOWN, t);
110+
}
101111
}

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,13 @@ interface OnActivityDestroyedListener {
8383
void onDestroyed(Activity activity);
8484
}
8585

86+
/**
87+
* Get a {@link Task} that will succeed with a result of the app's foregrounded {@link Activity},
88+
* when one is available.
89+
*
90+
* <p>The returned task will never fail. It will instead remain pending indefinitely until some
91+
* activity comes to the foreground.
92+
*/
8693
Task<Activity> getForegroundActivity() {
8794
synchronized (lock) {
8895
if (currentActivity != null) {

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

Lines changed: 63 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,21 +14,31 @@
1414

1515
package com.google.firebase.appdistribution;
1616

17+
import com.google.android.gms.tasks.SuccessContinuation;
1718
import com.google.android.gms.tasks.Task;
1819
import com.google.android.gms.tasks.TaskCompletionSource;
1920
import com.google.android.gms.tasks.Tasks;
20-
import com.google.firebase.appdistribution.Constants.ErrorMessages;
21+
import com.google.auto.value.AutoValue;
2122
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
2223
import com.google.firebase.appdistribution.internal.LogWrapper;
2324
import java.util.concurrent.Executor;
2425

2526
class TaskUtils {
2627
private static final String TAG = "TaskUtils:";
2728

29+
/**
30+
* A functional interface to wrap a function that returns some result of a possibly long-running
31+
* operation, and could potentially throw a {@link FirebaseAppDistributionException}.
32+
*/
2833
interface Operation<TResult> {
2934
TResult run() throws FirebaseAppDistributionException;
3035
}
3136

37+
/** A functional interface to wrap a function that produces a {@link Task}. */
38+
interface TaskSource<TResult> {
39+
Task<TResult> get();
40+
}
41+
3242
/**
3343
* Runs a long running operation inside a {@link Task}, wrapping any errors in {@link
3444
* FirebaseAppDistributionException}.
@@ -51,10 +61,8 @@ static <TResult> Task<TResult> runAsyncInTask(Executor executor, Operation<TResu
5161
() -> {
5262
try {
5363
taskCompletionSource.setResult(operation.run());
54-
} catch (FirebaseAppDistributionException e) {
55-
taskCompletionSource.setException(e);
5664
} catch (Throwable t) {
57-
taskCompletionSource.setException(wrapException(t));
65+
taskCompletionSource.setException(FirebaseAppDistributionException.wrap(t));
5866
}
5967
});
6068
return taskCompletionSource.getTask();
@@ -79,14 +87,61 @@ static <TResult> Task<TResult> handleTaskFailure(Task<TResult> task) {
7987
LogWrapper.getInstance().e(TAG + "Task failed to complete due to " + e.getMessage(), e);
8088
return e instanceof FirebaseAppDistributionException
8189
? task
82-
: Tasks.forException(wrapException(e));
90+
: Tasks.forException(FirebaseAppDistributionException.wrap(e));
8391
}
8492
return task;
8593
}
8694

87-
private static FirebaseAppDistributionException wrapException(Throwable t) {
88-
return new FirebaseAppDistributionException(
89-
String.format("%s: %s", ErrorMessages.UNKNOWN_ERROR, t.getMessage()), Status.UNKNOWN, t);
95+
/**
96+
* An @{link AutoValue} class to hold the result of two Tasks, combined using {@link
97+
* #combineWithResultOf}.
98+
*
99+
* @param <T1> The result type of the first task
100+
* @param <T2> The result type of the second task
101+
*/
102+
@AutoValue
103+
abstract static class CombinedTaskResults<T1, T2> {
104+
abstract T1 first();
105+
106+
abstract T2 second();
107+
108+
static <T1, T2> CombinedTaskResults<T1, T2> create(T1 first, T2 second) {
109+
return new AutoValue_TaskUtils_CombinedTaskResults(first, second);
110+
}
111+
}
112+
113+
/**
114+
* Returns a {@link SuccessContinuation} to be chained off of a {@link Task}, that will run
115+
* another task in sequence and combine both results together.
116+
*
117+
* <p>This is useful when you want to run two tasks and use the results of each, but those tasks
118+
* need to be run sequentially. If they can be run in parallel, use {@link Tasks#whenAll} or one
119+
* of its variations.
120+
*
121+
* <p>Usage:
122+
*
123+
* <pre>{@code
124+
* runFirstAsyncTask()
125+
* .onSuccessTask(combineWithResultOf(() -> startSecondAsyncTask())
126+
* .addOnSuccessListener(
127+
* results ->
128+
* doSomethingWithBothResults(results.result1(), results.result2()));
129+
* }</pre>
130+
*
131+
* @param secondTaskSource A {@link TaskSource} providing the next task to run
132+
* @param <T1> The result type of the first task
133+
* @param <T2> The result type of the second task
134+
* @return A {@link SuccessContinuation} that will return a new task with result type {@link
135+
* CombinedTaskResults}, combining the results of both tasks
136+
*/
137+
static <T1, T2> SuccessContinuation<T1, CombinedTaskResults<T1, T2>> combineWithResultOf(
138+
TaskSource<T2> secondTaskSource) {
139+
return firstResult ->
140+
secondTaskSource
141+
.get()
142+
.onSuccessTask(
143+
secondResult ->
144+
Tasks.forResult(CombinedTaskResults.create(firstResult, secondResult)));
90145
}
91146

92147
static void safeSetTaskException(TaskCompletionSource taskCompletionSource, Exception e) {

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

Lines changed: 58 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
package com.google.firebase.appdistribution;
1616

1717
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_CANCELED;
18-
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_FAILURE;
18+
import static com.google.firebase.appdistribution.TaskUtils.combineWithResultOf;
1919
import static com.google.firebase.appdistribution.TaskUtils.safeSetTaskException;
2020
import static com.google.firebase.appdistribution.TaskUtils.safeSetTaskResult;
2121

@@ -28,12 +28,13 @@
2828
import androidx.annotation.NonNull;
2929
import androidx.annotation.VisibleForTesting;
3030
import androidx.browser.customtabs.CustomTabsIntent;
31-
import com.google.android.gms.tasks.OnSuccessListener;
31+
import com.google.android.gms.tasks.OnFailureListener;
3232
import com.google.android.gms.tasks.Task;
3333
import com.google.android.gms.tasks.TaskCompletionSource;
3434
import com.google.android.gms.tasks.Tasks;
3535
import com.google.firebase.FirebaseApp;
3636
import com.google.firebase.appdistribution.Constants.ErrorMessages;
37+
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
3738
import com.google.firebase.appdistribution.internal.InstallActivity;
3839
import com.google.firebase.appdistribution.internal.LogWrapper;
3940
import com.google.firebase.appdistribution.internal.SignInResultActivity;
@@ -55,6 +56,9 @@ class TesterSignInManager {
5556

5657
private final Object signInTaskLock = new Object();
5758

59+
@GuardedBy("signInTaskLock")
60+
private boolean hasBeenSentToBrowserForCurrentTask = false;
61+
5862
@GuardedBy("signInTaskLock")
5963
private TaskCompletionSource<Void> signInTaskCompletionSource = null;
6064

@@ -87,8 +91,7 @@ class TesterSignInManager {
8791
@VisibleForTesting
8892
void onActivityCreated(Activity activity) {
8993
// We call finish() in the onCreate method of the SignInResultActivity, so we must set the
90-
// result
91-
// of the signIn Task in the onActivityCreated callback
94+
// result of the signIn Task in the onActivityCreated callback
9295
if (activity instanceof SignInResultActivity) {
9396
LogWrapper.getInstance().v("Sign in completed");
9497
this.setSuccessfulSignInResult();
@@ -104,50 +107,74 @@ void onActivityStarted(Activity activity) {
104107
return;
105108
} else {
106109
// Throw error if app reentered during sign in
107-
if (this.isCurrentlySigningIn()) {
108-
LogWrapper.getInstance().e("App Resumed without sign in flow completing.");
109-
this.setCanceledAuthenticationError();
110+
synchronized (signInTaskLock) {
111+
if (awaitingResultFromBrowser()) {
112+
LogWrapper.getInstance().e("App Resumed without sign in flow completing.");
113+
setSignInTaskCompletionError(
114+
new FirebaseAppDistributionException(
115+
Constants.ErrorMessages.AUTHENTICATION_CANCELED, AUTHENTICATION_CANCELED));
116+
}
110117
}
111118
}
112119
}
113120

114121
@NonNull
115122
public Task<Void> signInTester() {
116-
117123
if (signInStorage.getSignInStatus()) {
118124
LogWrapper.getInstance().v(TAG + "Tester is already signed in.");
119125
return Tasks.forResult(null);
120126
}
121127

122128
synchronized (signInTaskLock) {
123-
if (this.isCurrentlySigningIn()) {
129+
if (signInTaskCompletionSource != null
130+
&& !signInTaskCompletionSource.getTask().isComplete()) {
124131
LogWrapper.getInstance()
125132
.v(TAG + "Detected In-Progress sign in task. Returning the same task.");
126133
return signInTaskCompletionSource.getTask();
127134
}
128135

129136
signInTaskCompletionSource = new TaskCompletionSource<>();
137+
hasBeenSentToBrowserForCurrentTask = false;
130138

131139
firebaseInstallationsApiProvider
132140
.get()
133141
.getId()
134-
.addOnSuccessListener(getFidGenerationOnSuccessListener())
135142
.addOnFailureListener(
136-
e -> {
137-
LogWrapper.getInstance().e(TAG + "Fid retrieval failed.", e);
138-
setSignInTaskCompletionError(
139-
new FirebaseAppDistributionException(
140-
ErrorMessages.AUTHENTICATION_ERROR, AUTHENTICATION_FAILURE, e));
141-
});
143+
handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, Status.AUTHENTICATION_FAILURE))
144+
.onSuccessTask(combineWithResultOf(() -> lifecycleNotifier.getForegroundActivity()))
145+
.addOnSuccessListener(
146+
fidAndActivity -> {
147+
// Launch the intent outside of the synchronized block because we don't need to wait
148+
// for the lock, and we don't want to risk the activity leaving the foreground in
149+
// the meantime.
150+
openSignInFlowInBrowser(fidAndActivity.first(), fidAndActivity.second());
151+
// This synchronized block is required by the @GuardedBy annotation, but is not
152+
// practically required in this case because the only reads of this variable are on
153+
// the main thread, which this callback is also running on.
154+
synchronized (signInTaskLock) {
155+
hasBeenSentToBrowserForCurrentTask = true;
156+
}
157+
})
158+
// No failures expected here, since getForegroundActivity() will wait indefinitely for a
159+
// foreground activity, but catch any unexpected failures to be safe.
160+
.addOnFailureListener(handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, Status.UNKNOWN));
142161

143162
return signInTaskCompletionSource.getTask();
144163
}
145164
}
146165

147-
private boolean isCurrentlySigningIn() {
166+
private OnFailureListener handleTaskFailure(String message, Status status) {
167+
return e -> {
168+
LogWrapper.getInstance().e(TAG + message, e);
169+
setSignInTaskCompletionError(new FirebaseAppDistributionException(message, status, e));
170+
};
171+
}
172+
173+
private boolean awaitingResultFromBrowser() {
148174
synchronized (signInTaskLock) {
149175
return signInTaskCompletionSource != null
150-
&& !signInTaskCompletionSource.getTask().isComplete();
176+
&& !signInTaskCompletionSource.getTask().isComplete()
177+
&& hasBeenSentToBrowserForCurrentTask;
151178
}
152179
}
153180

@@ -157,33 +184,12 @@ private void setSignInTaskCompletionError(FirebaseAppDistributionException e) {
157184
}
158185
}
159186

160-
private void setCanceledAuthenticationError() {
161-
setSignInTaskCompletionError(
162-
new FirebaseAppDistributionException(
163-
Constants.ErrorMessages.AUTHENTICATION_CANCELED, AUTHENTICATION_CANCELED));
164-
}
165-
166187
private void setSuccessfulSignInResult() {
167188
synchronized (signInTaskLock) {
168189
safeSetTaskResult(signInTaskCompletionSource, null);
169190
}
170191
}
171192

172-
private OnSuccessListener<String> getFidGenerationOnSuccessListener() {
173-
return fid -> {
174-
Context context = firebaseApp.getApplicationContext();
175-
Uri uri =
176-
Uri.parse(
177-
String.format(
178-
SIGNIN_REDIRECT_URL,
179-
firebaseApp.getOptions().getApplicationId(),
180-
fid,
181-
getApplicationName(context),
182-
context.getPackageName()));
183-
openSignInFlowInBrowser(context, uri);
184-
};
185-
}
186-
187193
private static String getApplicationName(Context context) {
188194
try {
189195
return context.getApplicationInfo().loadLabel(context.getPackageManager()).toString();
@@ -193,22 +199,30 @@ private static String getApplicationName(Context context) {
193199
}
194200
}
195201

196-
private void openSignInFlowInBrowser(Context applicationContext, Uri uri) {
202+
private void openSignInFlowInBrowser(String fid, Activity activity) {
203+
Context context = firebaseApp.getApplicationContext();
204+
Uri uri =
205+
Uri.parse(
206+
String.format(
207+
SIGNIN_REDIRECT_URL,
208+
firebaseApp.getOptions().getApplicationId(),
209+
fid,
210+
getApplicationName(context),
211+
context.getPackageName()));
197212
LogWrapper.getInstance().v(TAG + "Opening sign in flow in browser at " + uri);
198-
if (supportsCustomTabs(applicationContext)) {
213+
if (supportsCustomTabs(context)) {
199214
// If we can launch a chrome view, try that.
200215
CustomTabsIntent customTabsIntent = new CustomTabsIntent.Builder().build();
201216
Intent intent = customTabsIntent.intent;
202217
intent.addFlags(Intent.FLAG_ACTIVITY_NO_HISTORY);
203218
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
204-
customTabsIntent.launchUrl(applicationContext, uri);
205-
219+
customTabsIntent.launchUrl(activity, uri);
206220
} else {
207221
// If we can't launch a chrome view try to launch anything that can handle a URL.
208222
Intent browserIntent = new Intent(Intent.ACTION_VIEW, uri);
209223
browserIntent.addFlags(Intent.FLAG_ACTIVITY_NO_HISTORY);
210224
browserIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
211-
applicationContext.startActivity(browserIntent);
225+
activity.startActivity(browserIntent);
212226
}
213227
}
214228

‎firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/TestUtils.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,8 @@ static void assertTaskFailure(Task task, Status status, String messageSubstring)
3131
}
3232

3333
static void assertTaskFailure(
34-
UpdateTask updateTask, Status status, String messageSubstring, Throwable cause) {
35-
assertTaskFailure(updateTask, status, messageSubstring);
36-
assertThat(updateTask.getException()).hasCauseThat().isEqualTo(cause);
34+
Task task, Status status, String messageSubstring, Throwable cause) {
35+
assertTaskFailure(task, status, messageSubstring);
36+
assertThat(task.getException()).hasCauseThat().isEqualTo(cause);
3737
}
3838
}

0 commit comments

Comments
 (0)