Closed Bug 1456487 Opened 6 years ago Closed 6 years ago

Cannot re-connect Firefox Account after "Clear data" in Android Settings

Categories

(Firefox for Android Graveyard :: Firefox Accounts, defect)

ARM
Android
defect
Not set
normal

Tracking

(fennec?, firefox59 unaffected, firefox60blocking verified, firefox61 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
fennec ? ---
firefox59 --- unaffected
firefox60 blocking verified
firefox61 --- verified

People

(Reporter: ohorvath, Assigned: nalexander)

References

Details

Attachments

(1 file)

Device:
Huawei MediaPad M3 Lite 10 (Android 7.0)
Huawei P8 Lite (Android 6.0)

Build: Nightly 61.0a1 (2018-04-24);

Steps to reproduce:
1. Sign in to Sync with a Firefox account.
2. Clear app data from Android settings>Apps>Firefox Nightly>Storage>Clear data.
3. Open the browser and go to Settings>Sign in.
4. Log in again with the same account.

Expected result:
The sync account should not be saved when clearing the app data. 
If the username is saved though, the user should be able to log back in.

Actual result:
The username is saved in Sync, but the user is not logged in. An error is displayed: "Cannot connect. Tap to sign in."
Tapping the message and trying to sign in, you get a success message that the account is connected.
Going back to settings>sync, the account is still not connected, the same error is displayed.
Can only sign in after the previous account is disconnected.
tracking-fennec: --- → ?
[Tracking Requested - why for this release]: worth considering this a blocker for 60. This is not a good state for the user to try to recover from.
Blocks: 1429735
(In reply to Kevin Brosnan [:kbrosnan] from comment #1)
> [Tracking Requested - why for this release]: worth considering this a
> blocker for 60. This is not a good state for the user to try to recover from.

In the temporary absence of Grisha, I will do the first round of investigation here.  Based on Oana's STRs, it rather sounds like the Status Activity is displaying stale information.  This is generally because a sync is needed to drive the state machine forward; it might be that we need to force a sync more aggressively when we witness part of this flow.
Marking as blocker per Kevin's comment.
Installed

http://archive.mozilla.org/pub/mobile/nightly/2018/04/2018-04-25-10-01-18-mozilla-central-android-api-16/fennec-61.0a1.multi.android-arm.apk

Signed in to a freshly created test account; complete verification loop.

Tapped Settings > Sync; verify that account has synced; tapped Sync Now; verify that account syncs again.

Go to Settings > Storage.  Tap Clear Data.

Go to Settings > Accounts.  Observe that test account is still listed as a Firefox Account!

Kill and restart Nightly.  Observe in the logcat:
04-25 10:43:44.348 31116 31200 I FxAccounts: fennec_aurora :: FirefoxAccountUtils :: Pickle file absent; separating the account.
04-25 10:43:44.352 31116 31200 I FxAccounts: fennec_aurora :: AndroidFxAccount :: Moving account named like XXXXXXXXXX@XXXXXXXX.XXX to state Separated
04-25 10:43:44.378 31116 31200 I FxAccounts: fennec_aurora :: FxAccountNotificationManager :: State Separated needs action; offering notification with title: Sync is not connected
04-25 10:43:44.407 31116 31200 I FxAccounts: fennec_aurora :: FxAccountSyncAdapter :: Syncing in a 'first run' scenario without a pickle file; skipping sync & separating the account.
04-25 10:43:44.417 31116 31116 I GeckoLogger: fennec_aurora :: FxAccountNotificationManager :: State Separated needs action; offering notification with title: Sync is not connected
04-25 10:43:44.434 31116 31128 I GeckoLogger: fennec_aurora :: FxAccountNotificationManager :: State Separated needs action; offering notification with title: Sync is not connected
04-25 10:43:49.527 31116 31188 D GeckoFxAccounts: FxAccountsWebChannel registered: account_updates with origin https://accounts.firefox.com
04-25 10:43:49.870 31116 31188 I GeckoLogger: fennec_aurora :: FxAccountNotificationManager :: State Separated needs action; offering notification with title: Sync is not connected
04-25 10:43:51.361 31116 31181 I GeckoPushService: FxA account not in Married state, not proceeding with registration renewal

Go to Settings > Sync; observe error with "Cannot connect.  Tap to sign in."

Tap error; go to about:accounts; observe user name is filled in.  Entered password, transitioned to "Signed in" screen in about:accounts.

Go to Settings > Sync; observe error with "Cannot connect.  Tap to sign in."  Also, "Last synchronized: never."  In the logcat, I observe:

04-25 10:49:53.037 31116 31188 I GeckoLogger: fennec_aurora :: AndroidFxAccount :: Moving account named like XXXXXXXXXX@XXXXXXXX.XXX to state Engaged
04-25 10:49:53.074 31116 31188 I GeckoFxAccounts: Created or updated Firefox Account.

but then immediately:

04-25 10:50:16.099 31116 31127 I GeckoLogger: fennec_aurora :: AndroidFxAccount :: Moving account named like XXXXXXXXXX@XXXXXXXX.XXX to state Separated
04-25 10:50:16.146 31116  1704 I FxAccounts: fennec_aurora :: FxAccountSyncAdapter :: Syncing FxAccount account named like XXXXXXXXXX@XXXXXXXX.XXX for authority org.mozilla.fennec_aurora.db.browser with instance org.mozilla.gecko.fxa.sync.FxAccountSyncAdapter@3be90f6.

So in some way, the sign in is moving the account to Engaged (correct!) but we then immediately move it to Separated (incorrect!).  My guess is that we're doing that on the basis of the "first run flagging" logic that Grisha relatively recently implemented to handle this situation.  My belief is that we assumed that "Clear data" would kill and restart the App, but locally that seems to not be true.  (I'll confirm or deny.)

In any case, I can confirm the poor experience that Oana filed (thanks, Oana!).
OK, a little more investigation.  It does appear that Clear data is killing and restarting the App (phew!).  The "Clear data" flow is being recognized, and the Firefox Account is being moved to Separated based on the mismatched first-run UUIDs.  The issue is that reconnecting the account does not update the first-run UUID, so that the next Sync-related action immediately moves it back to Separated.  That is, we only set the first-run UUID when adding the account, not when reconnecting it.

It's not clear to me exactly what the entry points for "reconnected are"; I'll try to track those through now.
Patch inbound.  I believe the work-around is for the user to start, and then kill the App entirely, start again, and _then_ re-connect the Account.  That should get us out of the First Run session logic that is causing this cycle.
Comment on attachment 8971001 [details]
Bug 1456487 - Update Firefox Account's first run UUID when re-connecting.

https://reviewboard.mozilla.org/r/239738/#review245474

::: commit-message-88abd:6
(Diff revision 1)
> +Bug 1456487 - Update Firefox Account's first run UUID when re-connecting. r=rnewman
> +
> +The behaviour of Android Firefox Account instances recently changed in
> +the face of system "Clear data" commands.  To align more closely with
> +common Apps like Dropbox and Whatsapp (which generally don't use
> +Android Account instances), after a "Clear data" Firefox Account's are

Account's -> Accounts

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:652
(Diff revision 1)
>  
> +  /**
> +   * If there's an active First Run session, tag the given account with it.  If there's no active
> +   * First Run session, remove any existing tag.
> +   * We do this in order to reliably determine if an account was created during the current
> +   * "first run"; this allows us to re-connect an account that was no created during the current

no -> not
Attachment #8971001 - Flags: review?(bugzilla) → review+
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df36fee2b3ac
Update Firefox Account's first run UUID when re-connecting. r=rnewman
Once this lands we should verify on nightly and consider uplift to beta 60.
(In reply to Nick Alexander :nalexander from comment #6)
> Patch inbound.  I believe the work-around is for the user to start, and then
> kill the App entirely, start again, and _then_ re-connect the Account.  That
> should get us out of the First Run session logic that is causing this cycle.

Oana, could you confirm both the fix when it gets to Nightly, and the work-around for affected versions?  Thanks!
Flags: needinfo?(oana.horvath)
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Summary: Cannot sign in after a clear data → Cannot re-connect Firefox Account after "Clear data" in Android Settings
https://hg.mozilla.org/mozilla-central/rev/df36fee2b3ac
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
(In reply to Nick Alexander :nalexander from comment #12)
> (In reply to Nick Alexander :nalexander from comment #6)
> > Patch inbound.  I believe the work-around is for the user to start, and then
> > kill the App entirely, start again, and _then_ re-connect the Account.  That
> > should get us out of the First Run session logic that is causing this cycle.
> 
> Oana, could you confirm both the fix when it gets to Nightly, and the
> work-around for affected versions?  Thanks!

Verified the fix in Nightly 61 (2018-04-26), works as expected: the user can reconnect after a clear data.
That workaround doesn't seem to work on Beta 60.b15. However, there is a simple workaround that works: to disconnect an re-connect the account from the Sync menu.

Devices:
Pixel XL (Android 8.1.0)
Asus Nexus 7 (Android 5.1.1)
Flags: needinfo?(oana.horvath)
Hi Nick, can you please nominate this for uplift to Beta60? Thanks!
Flags: needinfo?(nalexander)
(In reply to Ritu Kothari (:ritu) from comment #15)
> Hi Nick, can you please nominate this for uplift to Beta60? Thanks!

Yes.  Per discussion with lizzard, we have the Nightly verification we wanted, although I don't fully understand why the simpler work-around I proposed doesn't work.  I'm not going to worry about it right now, and will nominate.
Flags: needinfo?(nalexander)
Comment on attachment 8971001 [details]
Bug 1456487 - Update Firefox Account's first run UUID when re-connecting.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1429735.

[User impact if declined]: after Clear data, the user needs to disconnect their Sync account (delete it entirely) and reconnect it before they can start syncing again.  This is a burden but _should_ not cause visible issues to the user other than polluting their list of devices to which they can Send Tab.  This pollution is because internally the device re-syncs *everything* and starts fresh, which is both hard on our service (lots of downloads) and very likely to actually cause visible issues (since Sync 1.5 is just not that robust corruption and bad merges are likely).

[Is this code covered by automated tests?]:

No.

[Has the fix been verified in Nightly?]:

Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

No, I don't think additional testing on Beta would be that useful -- the code in this area is relatively stable.

[List of other uplifts needed for the feature/fix]:

None that I am aware of.

[Is the change risky?]:

Not really.

[Why is the change risky/not risky?]:

The situation is more or less maximally broken, and the proposed fix impacts exactly one flow -- when we reconnect the Account from about:accounts.  Any additional breakage their would both be visible (since it's in response to user action) and unlikely (since the change is minimal).

[String changes made/needed]:

None.
Attachment #8971001 - Flags: approval-mozilla-beta?
Comment on attachment 8971001 [details]
Bug 1456487 - Update Firefox Account's first run UUID when re-connecting.

FxA fix, approved for 60.0b17 and 60.0 rc
Attachment #8971001 - Flags: approval-mozilla-release+
Attachment #8971001 - Flags: approval-mozilla-beta?
Attachment #8971001 - Flags: approval-mozilla-beta+
Flags: qe-verify+
Verified on Beta 60.0b17:
Google Pixel (Android 8.1.0)
Sony Xperia Z5 Premium (Android 6.0.1)
Flags: qe-verify+
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.