Host activity interrupted error & dialog dismissal #3341

rachaprince · 2022-01-21T19:37:00Z

This PR makes it so that if the host activity is interrupted and put on pause then we want for it to resume to show a dialog. If there is a new activity that resumes then we cancel the task.

rachaprince · 2022-01-21T19:37:40Z

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

-
-  @GuardedBy("cachedNewReleaseLock")
-  private AppDistributionReleaseInternal cachedNewRelease;
+  AtomicReference<AppDistributionReleaseInternal> cachedNewRelease = new AtomicReference();


Since this had a separate lock from the cached task, I thought we could reduce the number of blocks by adding an atomic reference

google-oss-bot · 2022-01-21T19:43:10Z

Coverage Report ¹

Affected Products

`firebase-appdistribution`

Overall coverage changed from 76.31% (c35626a) to 76.45% (0d688a6) by +0.14%.

Filename	Base (`c35626a`)	Merge (0d688a6)	Diff
FirebaseAppDistribution.java	95.60%	94.44%	-1.16%
FirebaseAppDistributionException.java	88.24%	88.57%	+0.34%
FirebaseAppDistributionLifecycleNotifier.java	72.53%	73.68%	+1.16%

Test Logs

Notes

Commit (0d688a6) is created by Prow via merging PR base commit (c35626a) and head commit (24b1ff4).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/oakBUUBwqr.html

google-oss-bot · 2022-01-21T19:44:42Z

The public api surface has changed for the subproject firebase-appdistribution:
error: Added enum constant com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.HOST_ACTIVITY_INTERRUPTED [AddedField]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2022-01-21T20:11:00Z

Size Report ¹

Affected Products

firebase-appdistribution
Type Base (c35626a) Merge (0d688a6) Diff
aar 123 kB 124 kB +548 B (+0.4%)
apk (aggressive) 743 kB 743 kB +280 B (+0.0%)
apk (release) 1.55 MB 1.55 MB +500 B (+0.0%)

Test Logs

Notes

Commit (0d688a6) is created by Prow via merging PR base commit (c35626a) and head commit (24b1ff4).

https://storage.googleapis.com/firebase-sdk-metric-reports/YSu5cASYJ0.html

manny-jimenez · 2022-01-25T22:17:29Z

Wanted to get this up before EOD still needs some cleaning before review

google-oss-bot · 2022-01-25T22:41:35Z

The public api surface has changed for the subproject firebase-appdistribution:
error: Added enum constant com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.HOST_ACTIVITY_INTERRUPTED [AddedField]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

manny-jimenez · 2022-01-26T20:00:19Z

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+      if (dialogHostActivityHasChanged(activity)) {
+        setUpdateIfNewReleaseWithHostError();
+      } else {
+        signInConfirmationDialog.show();


@lfkellogg Would like to discuss this with you. If we kept the past implementation of calling the method the task hangs on failure. If we set the task to fail then if the user has some failure listener that will be called. I believe we ran into this before not sure if there was a resolution.

lfkellogg · 2022-01-27T22:07:45Z

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java


    return showUpdateDialogTask.getTask();
  }

+  private void setRemakeDialogsToFalse() {


Since we're just using this in one place, and it's already being called from a synchronized block, we might as well just inline it.

lfkellogg · 2022-01-27T22:11:47Z

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

-  private boolean remakeUpdateDialog = false;
+  @Nullable private Activity dialogHostActivity = null;
+
+  @GuardedBy("updateIfNewReleaseTaskLock")


I don't think either of these booleans need to be protected by this lock, or any other lock. They're almost always read or written from the main thread (in lifecycle callbacks), and in the case where they're being set to false in updateIfNewReleaseAvailable, it won't practically affect anything because there's no task in progress at that point.

That is true I agree calling the updateIfNewRelease would not be an issue. I don't see a problem with onPause being called multiple times from different places but if those are all on the main thread there shouldn't be a deadlock issue so as long as we are sure of that we can change it.

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

lfkellogg · 2022-01-27T22:16:16Z

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+
+  private void updateConfirmationDialogClosedCallback(TaskCompletionSource showUpdateDialogTask) {
+    synchronized (updateIfNewReleaseTaskLock) {
+      remakeUpdateConfirmationDialog = false;


We don't need this anymore, right? I'd just drop it and inline this method.

Will remove for now. We don't have this on signIn but it might be nice to have since it does set the remake variable to false when the dialog is cancelled. That may be unnecessary but it makes sense to set it after that since it does not need to be remade at that point. Curious on your thoughts about that.

…d-error' into fad-add-host-activity-interrupted-error

manny-jimenez · 2022-01-28T18:02:38Z

/test smoke-tests

lfkellogg · 2022-01-28T18:41:11Z

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+  @VisibleForTesting
+  void onActivityDestroyed(@NonNull Activity activity) {
+    // If the dialogHostActivity is being destroyed it is set to null. This is to ensure that
+    // onResume shows the dialog on a configuration change and not check the activity reference


"and does not check" ? And please put a . on the end of the comment.

lfkellogg · 2022-01-28T18:43:02Z

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+  }
+
+  private boolean awaitingSignInDialogConfirmation() {
+    return (updateIfNewReleaseAvailableIsTaskInProgress() && remakeSignInConfirmationDialog);


Even though it wouldn't make a practical difference, wouldn't it make more sense if this checked if showSignInDialogTask was in progress, instead of cachedUpdateIfNewReleaseTask?

That does make more sense and it shouldn't change anything will change

lfkellogg · 2022-01-28T18:43:28Z

...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+  }
+
+  private boolean awaitingUpdateDialogConfirmation() {
+    return (updateIfNewReleaseAvailableIsTaskInProgress() && remakeUpdateConfirmationDialog);


Same here, but showUpdateDialogTask

lfkellogg · 2022-01-28T18:45:35Z

...tribution/src/test/java/com/google/firebase/appdistribution/FirebaseAppDistributionTest.java


    assertAlertDialogShown();
    assertFalse(updateTask.isComplete());
  }

  @Test
-  public void taskCancelledOnDestroy_whenSignInDialogShowing() {
+  public void
+      updateIfNewReleaseAvailable_signInTaskCancelled_whenSignInDialogShowingAndNewActivityStarts() {


Nit: for this and the next test, could you change the name of the test to put the "when" clause in the middle of the name, and the expected behavior at the end? E.g. updateIfNewReleaseAvailable_whenSignInDialogShowingAndNewActivityStarts_signInTaskCancelled

...ution/test-app/src/main/java/com/googletest/firebase/appdistribution/testapp/MainActivity.kt

lfkellogg

Some nits, but this looks great!

google-oss-bot · 2022-01-28T20:28:00Z

@rachaprince: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests	`24b1ff4`	link	`/test smoke-tests`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

WIP host activity interrupted error & dialog dismissal

c6bcec9

google-oss-bot added do-not-merge/work-in-progress size/L labels Jan 21, 2022

rachaprince commented Jan 21, 2022

View reviewed changes

Removing atomic variable and combining with screen rotation

660f9ee

Adding synchro blocks

acb1d3f

Changing onResume to accomodate rotation and reentry

9fd2b94

manny-jimenez reviewed Jan 26, 2022

View reviewed changes

Manny Jimenez added 3 commits January 26, 2022 15:36

Adding in synch blocks

e0cd1e7

Moving task completion source to be a class variable

d3a4ae4

Adding in tests

e1b77fc

manny-jimenez removed the do-not-merge/work-in-progress label Jan 27, 2022

Manny Jimenez and others added 2 commits January 27, 2022 17:03

Adding lock

17a2518

Merge branch 'master' into fad-add-host-activity-interrupted-error

1e53d03

manny-jimenez requested a review from lfkellogg January 27, 2022 22:15

lfkellogg reviewed Jan 27, 2022

View reviewed changes

Manny Jimenez added 3 commits January 27, 2022 17:40

Responding to comments

55245e0

Merge remote-tracking branch 'origin/fad-add-host-activity-interrupte…

8c76714

…d-error' into fad-add-host-activity-interrupted-error

Responding to feedback

fc79ea7

lfkellogg reviewed Jan 28, 2022

View reviewed changes

...ution/test-app/src/main/java/com/googletest/firebase/appdistribution/testapp/MainActivity.kt Show resolved Hide resolved

lfkellogg approved these changes Jan 28, 2022

View reviewed changes

lfkellogg changed the title ~~WIP host activity interrupted error & dialog dismissal~~ Host activity interrupted error & dialog dismissal Jan 28, 2022

Manny Jimenez added 2 commits January 28, 2022 14:02

Responding to feedback

4cfeed1

Fixing test

24b1ff4

manny-jimenez merged commit 66797a8 into master Jan 30, 2022

manny-jimenez deleted the fad-add-host-activity-interrupted-error branch January 30, 2022 22:52

firebase locked and limited conversation to collaborators Mar 2, 2022

Host activity interrupted error & dialog dismissal #3341

Host activity interrupted error & dialog dismissal #3341

Uh oh!

Conversation

rachaprince commented Jan 21, 2022 • edited by manny-jimenez Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-appdistribution

Test Logs

Notes

Uh oh!

google-oss-bot commented Jan 21, 2022

Uh oh!

google-oss-bot commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-appdistribution

Test Logs

Notes

Uh oh!

manny-jimenez commented Jan 25, 2022

Uh oh!

google-oss-bot commented Jan 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manny-jimenez Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manny-jimenez commented Jan 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lfkellogg left a comment

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Jan 28, 2022

Uh oh!

Uh oh!

rachaprince commented Jan 21, 2022 •

edited by manny-jimenez

Loading

google-oss-bot commented Jan 21, 2022 •

edited

Loading

Coverage Report ¹

`firebase-appdistribution`

google-oss-bot commented Jan 21, 2022 •

edited

Loading

Size Report ¹

`firebase-appdistribution`

manny-jimenez Jan 27, 2022 •

edited

Loading