1429735 - Move Firefox Account into Separated state after user performed "Clear Data" for the app

Status: RESOLVED FIXED

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

Description

[u549602]

Device: 
Nexus 6P - Android 8.0.0
Google Pixel XL - Android 8.0.0
Google Pixel 2 - Android 8.1

Build: 
Beta 58.0b15
Release: 57.0.4

Steps to reproduce:

1. On two devices install nightly, Beta and Release builds
2. Log in with the same FxA on all builds
3. Wait for the Sync action to be completed on all builds
4. Uninstall Beta build/Release from any device
5. Reinstall the previous uninstalled version
6. Launch fennec

Actual results: 
Upon reaching the sign in onboarding page, the user is already signed in with the pre-used FxA

Expected results:
The user should reach the mandatory onboarding log in page, and not be already logged in.

In addition, it can be observed that, right after the fennec launc, the Account is added to the Users& Accounts section in the Android's settings.
Could not reproduce on Nightly and on devices with Android less than 8.0

For further details please check:  https://www.youtube.com/watch?v=pw4PhWU9rx8

Comment 1

[u549602]

Attachment: Logcat for the FxA prelogged issue.txt

Comment 2

[u549602]

After further investigations and a Log-cat study I've seen this warning, first one when starting the reinstalled build
"01-11 12:34:40.446: W/FxAccounts(12529): firefox_beta :: GlobalSession :: Remote syncID different from local syncID: resetting client and assuming remote syncID." 
I believe that this might be the trigger for this bug.
Attached, the logcat with the FxAccounts Tag filter

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Please help advise.
Thanks!

Comment 4

[:Grisha Kruglov]

Moving components and marking it as sensitive out of an abundance of caution. I will clear the flag if necessary after investigation.

Comment 5

[:Grisha Kruglov]

I can replicate this in an API27 emulator, with Release and Beta installed. Sign in on both, then remove Beta, then re-install Beta: its local Firefox Account is auto-created. If I remove Beta, then remove Release, then install Beta - it's in correct state.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #5)
> I can replicate this in an API27 emulator, with Release and Beta installed.
> Sign in on both, then remove Beta, then re-install Beta: its local Firefox
> Account is auto-created. If I remove Beta, then remove Release, then install
> Beta - it's in correct state.

This really sounds like Android 8 is doing something new: perhaps its keeping old App files around; perhaps it is rehydrating old App Android Account details upon re-install.

There's literally no way for Firefox Release to "talk to" Firefox Beta.  It has to be the system, or something left behind by Firefox Beta that then gets used to recreate (un-pickle) the account.

Grisha, didn't we remove pickling entirely?

Comment 7

[:Grisha Kruglov]

Aye, I'm investigating locally what's going on. It's pretty weird. Context on pickling and friends in Bug 1408555.

Comment 8

[:Grisha Kruglov]

Interesting - after installing Release & Beta, signing in on both, and then uninstalling Release, I see the following in /data/system/sync/accounts.xml:

<?xml version='1.0' encoding='utf-8' standalone='yes' ?>
<accounts version="3" nextAuthorityId="7" offsetInSeconds="1816">
    <listenForTickles user="0" enabled="true" />
    <authority id="4" user="0" enabled="true" account="[e-mail]" type="org.mozilla.firefox_beta_fxaccount" authority="org.mozilla.firefox_beta.db.browser" syncable="1" />
    <authority id="5" user="0" enabled="true" account="[e-mail]" type="org.mozilla.firefox_fxaccount" authority="org.mozilla.firefox.db.browser" syncable="1" />
</accounts>

... note the second entry, the account we supposedly just removed.

Comment 9

[:Grisha Kruglov]

It appears that in certain circumstances, after an app uninstall, accounts_de.db and accounts_ce.db files (in /data/system_de/0... and /data/system_ce/0...) are not purged of the account information for that app, and still contain the account in its entirety, along with any "user data" we set for the account (in accounts_ce.db#extras table).

As far as I can tell, we don't recreate that account on launch manually. There are only two places [0] at which we add an Android Account in our codebase, and none of them would be hit on first launch by themselves if there is no pickle file left (uninstall does still delete the pickle file along with any other app data, so at least that's good).

Neither do we need to recreate it - it's still there in the internal accounts db, along with all of its data. It seems that OS->Settings->User Accounts filters out accounts in that DB by which applications are installed. Simply re-installing the app makes the account visible again, and kicks off a sync - without having to launch Firefox.

After manually removing accounts data for the internal accounts db, everything is back to normal.

So at first sight, this seems like a "Android O broke account deletion on app removal under certain circumstances" bug.

Android's account manager considers the accounts_de & accounts_ce db files as authoritative - see [1] and around that call stack.

My minimal current STRs are:
- install Release
- install Beta
- login on Beta
- remove Beta
--> accounts info is still in internal db :(
- install Beta, it's logged-in from the get-go

I can't seem repro this without having both versions installed. I can't repro with a local build as the second app.

Perhaps Android's account manager removal process is stumbling on our account type names, somehow? They are: 'org.mozilla.firefox_beta_fxaccount' and 'org.mozilla.firefox_fxaccount' for beta and release.

Investigating this further.

[0] https://dxr.mozilla.org/mozilla-central/search?q=addAccountExplicitly&=mozilla-central
[1] https://github.com/aosp-mirror/platform_frameworks_base/blob/766f0a4981478ff63854df70ba50e6420d19c02b/services/core/java/com/android/server/accounts/AccountManagerService.java#L1166

Comment 10

[:Grisha Kruglov]

tldr; I don't think we can reliably distinguish uninstallation from 'clear data' at runtime without ugly/unreliable/risky hacks/involving a server. Thus, I don't think we can reliably detect the "account shouldn't be there" state - which means we can't really mitigate this? If so, one avenue we have is to figure what's triggering this apparent OS bug in the first place, and see if we can avoid it somehow. Another is to change the problem: force account removal after 'clear data'.

Assuming that it's possible for account information to be erroneously left over on the device after application removal, our goal is to:
1) detect that this happened after app has been re-installed
2) mitigate it - either delete it, or drive it into Doghouse state

For (1), we need to detect that an account shouldn't actually exist when we first encounter it. This seems tricky: we can detect clearing of our app data, but we can't really distinguish that from uninstallation reliably and without remote server support of sorts. There's a myriad of hacks people employ: sending empty push messages and monitoring for 'NotRegistered' message states on the server; spying on system intents and tracking foreground activities (did user trigger the uninstall activity for us?), etc. None of these seem even remotely reliable and applicable to us.
 
In short, I don't think we can answer the following question: "are we in first run & didn't just have our data cleared?", and so we can't know for certain that an account shouldn't be there.

_If_ we also deleted the account whenever user clears app data, _then_ this problem becomes quite easy. But that's a different can of worms.

Assuming that we can't easily affect OS accounts behaviour in the way we'd like, this bug turns into answering a question, "does this potential security quirk affect our thinking about account removal on 'clear data' enough to make it worth it?". If no: this is a wontfix; if yes, wipe accounts on 'clear data', and we're done. We can wipe account on 'clear data' only if we're on affected versions of Android...

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #10)
> tldr; I don't think we can reliably distinguish uninstallation from 'clear
> data' at runtime without ugly/unreliable/risky hacks/involving a server.
> Thus, I don't think we can reliably detect the "account shouldn't be there"
> state - which means we can't really mitigate this? If so, one avenue we have
> is to figure what's triggering this apparent OS bug in the first place, and
> see if we can avoid it somehow. Another is to change the problem: force
> account removal after 'clear data'.
> 
> Assuming that it's possible for account information to be erroneously left
> over on the device after application removal, our goal is to:
> 1) detect that this happened after app has been re-installed
> 2) mitigate it - either delete it, or drive it into Doghouse state
> 
> For (1), we need to detect that an account shouldn't actually exist when we
> first encounter it. This seems tricky: we can detect clearing of our app
> data, but we can't really distinguish that from uninstallation reliably and
> without remote server support of sorts. There's a myriad of hacks people
> employ: sending empty push messages and monitoring for 'NotRegistered'
> message states on the server; spying on system intents and tracking
> foreground activities (did user trigger the uninstall activity for us?),
> etc. None of these seem even remotely reliable and applicable to us.
>  
> In short, I don't think we can answer the following question: "are we in
> first run & didn't just have our data cleared?", and so we can't know for
> certain that an account shouldn't be there.
> 
> _If_ we also deleted the account whenever user clears app data, _then_ this
> problem becomes quite easy. But that's a different can of worms.
> 
> Assuming that we can't easily affect OS accounts behaviour in the way we'd
> like, this bug turns into answering a question, "does this potential
> security quirk affect our thinking about account removal on 'clear data'
> enough to make it worth it?". If no: this is a wontfix; if yes, wipe
> accounts on 'clear data', and we're done. We can wipe account on 'clear
> data' only if we're on affected versions of Android...

I don't think we get told about clear data explicitly -- we'd need to detect it (basically, firstrun).

Is this really specific to two *Firefox* versions installed?  Or is it triggered by multiple Android Accounts, and the types and owning packages are a red herring?

Comment 12

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #11)
> I don't think we get told about clear data explicitly -- we'd need to detect
> it (basically, firstrun).

Right; if our default behaviour is "there shouldn't be an account on first run" (regardless of how we got to first run), then this bug is resolved as a side-effect of that behaviour.

> Is this really specific to two *Firefox* versions installed?  Or is it
> triggered by multiple Android Accounts, and the types and owning packages
> are a red herring?

I couldn't trigger it without having a different firefox release also installed (it didn't matter if that other release had an account). I couldn't trigger it with a local build either. Hence my guess about similarity of appid or account type names having something to do with the bug. Ideally I should actually understand this behaviour more, which means further digging into the android source code. It'd also be useful to test this locally on an O device.

Comment 13

[:Grisha Kruglov]

From Android's AccountManagerService:

> A side effect of this is that an authenticator sharing a uid with
> multiple apps won't get its credentials wiped as long as some app with
> that uid is still on the device. But I suspect that this is a rare case.
> And it isn't clear to me how an attacker could really exploit that
> feature.

We're using the same sharedUserId for release and beta, and so this seems to be the expected behaviour. Unless I'm misreading their code though, it does appear that this behaviour was introduced in Android 7, so in theory we should be able to reproduce the problem there (although Comment 1 states that it's observed only on 8, so perhaps I _am_ misreading their code).

Mihai, you did test this on Android 7 devices, right?

Comment 14

[u549602]

Hi Grisha,

I've tried also on Android 7 devices in the testing process of this issue but could not reproduce it at that time. 
I will try it again on Android 7 devices exclusively today and come back with a result.

Comment 15

[:Grisha Kruglov]

Thanks, Mihai. It's possible I've missed some detail while reading their codebase, and more changes where introduced going from Android 7 to 8.

Regardless, I think our current thinking is to move the local Firefox Account into a Doghouse state if user clears application data. That is, keep the local firefox account around, but require users to re-login. I believe this carries the intent of "clearing data" much better than our current "do nothing" behaviour.

For bonus points, this change will a lot with our Sync+Master Password story. (e.g. see Bug 1410753).

Comment 16

[u549602]

Hi Grisha,

It seems that you were right on Android 7. I can reproduce the Auto log-in on two Android 7 devices Samsung Galaxy S6 edge and Samsung Galaxy S6. I've tried this scenario with 57.0.4 Release build and 59.0b3 in the first session. I switched to 58.0 Build 2(next release) and 59.0b3 and the result was the same.

So officially i can reproduce on both Android 8 and Android 7 devices

Comment 17

[Nick Alexander :nalexander [he/him]]

Grisha, here's a thought -- what if we baked the current android:versionCode into the account and used it to try to detect this situation?  Hmm.  The more I think about this, the more I think that it doesn't help us determine the pattern we're trying to catch, because an upgrade looks the same as a remove-and-reinstall (of any version).

I think we should try to push the account to the Doghouse on App removal, and accept this weird edge case in that state.

Comment 18

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #17)
> I think we should try to push the account to the Doghouse on App removal,
> and accept this weird edge case in that state.

From my explorations in Comment 10, I don't think we can reliably detect app removal, or distinguish it from "clear data" event. So this is going to be more broad. I'll post a patch soon.

Comment 19

[:Grisha Kruglov]

(In reply to Mihai Ninu {:Ninu} from comment #16)
> So officially i can reproduce on both Android 8 and Android 7 devices

Thanks for confirming. This makes sense to me, and also makes this problem a worse (larger number of currently affected devices), even if it is a strange edge case.

Comment 20

[:Grisha Kruglov]

Attachment: separate-account-on-clear-data.patch

Will request an actual review once I put tests in place.

(couldn't "push to review" due to this bug's confidentiality status. Nick, do you think it's warranted?)

Comment 21

[:Grisha Kruglov]

On a second thought, I might just land this as part of Bug 1410753.

Comment 22

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8944972 [details] [diff] [review]
separate-account-on-clear-data.patch

Review of attachment 8944972 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty confident something like this will work, but can you explain what the pros and cons of this approach are with respect to Instance IDs or similar from Android itself?  https://developer.android.com/training/articles/user-data-ids.html#working_with_instance_ids_&_guids

::: mobile/android/base/java/org/mozilla/gecko/Telemetry.java
@@ +53,4 @@
>      private static native void nativeAddUiEvent(String action, String method,
>                                                  long timestamp, String extras);
>  
> +    @Nullable public static UUID firstRunSessionUUID;

This doesn't seem to respect a meaningful domain abstraction.  Why not make `Session` have an identifier, and collect the identifiers of sessions that are active?  (I'm assuming that `FIRSTRUN` isn't exited until the first session is actually finished.)

If you want to inject this hack, then you need to add a nice comment explaining exactly why and what this is doing.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java
@@ +729,5 @@
>    public synchronized State getState() {
> +    return getState(true);
> +  }
> +
> +  public synchronized State getState(boolean refreshAccount) {

You introduce this, but there is only caller with `refreshAccount = false`, and I don't see why it hurts to get the account again.  Why bother with the flag?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java
@@ +17,4 @@
>  import android.os.SystemClock;
>  import android.support.v4.content.LocalBroadcastManager;
>  
> +import org.mozilla.gecko.GeckoApp;

This will fail in the non-Gradle builds, since services are built separately from Fennec itself.  And we'd really prefer to keep these separate, since we want to eventually ship Android Sync as an independent thing for consumption elsewhere, probably in Focus/Focus 2.0 -- see https://bugzilla.mozilla.org/show_bug.cgi?id=1229149, which tracks extracting a omg.db+Sync/services Gradle project out of Fennec.

The way to achieve this type of thing is to bloat further FirefoxAccounts, which is shared, IIRC.

And actually I see you're just using GSP -- that can just be hard-coded *if you ignore the profile*, which in this case probably makes sense.  That is, use App-wide prefs for this first run thing, since there's no meaningful profile management at work here.

@@ +508,4 @@
>      final Context context = getContext();
>      final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
>  
> +    // Does pickle file for this account exist, and does this look like a first run?

This comment isn't correct -- you're separating if the pickle file does _not_ exist, and we're in first run.

@@ +513,5 @@
> +    // TODO what are the chances of sync vs pickle creation races?
> +    final FirefoxAccounts.PickleFileState pickleFileState = FirefoxAccounts.queryPickleFileState(getContext());
> +    final boolean isFirstRun = GeckoSharedPrefs.forProfile(getContext()).getBoolean(GeckoApp.PREFS_IS_FIRST_RUN, true);
> +    final boolean pickleFilePresent;
> +    switch (pickleFileState) {

Consider pushing this onto `PickleFileState` as a helper method.  It's unfortunate to use the pickle file as a sentinel, since we want to remove pickling entirely :(

@@ +537,5 @@
> +              FxAccountSyncAdapter.NOTIFICATION_ID);
> +      notificationManager.update(context, fxAccount);
> +      return;
> +    } else {
> +      Logger.info(LOG_TAG, "Not in first run, and pickle file present; leaving account in its current state.");

This is all racing against the pickler, so can you bring this logic and the "write pickle" logic closer together in this file?  Can this go write before the "write pickle" logic?

Comment 23

[:Grisha Kruglov]

Thanks, Nick!

(In reply to Nick Alexander :nalexander from comment #22)
> Comment on attachment 8944972 [details] [diff] [review]
> separate-account-on-clear-data.patch
> 
> Review of attachment 8944972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm pretty confident something like this will work, but can you explain what
> the pros and cons of this approach are with respect to Instance IDs or
> similar from Android itself? 
> https://developer.android.com/training/articles/user-data-ids.
> html#working_with_instance_ids_&_guids

Instance ID "persists as long as the app is installed", which is longer than I'd like here.

The goal is to uniquely identify each first run. Consider multiple "clear data" events in a lifetime of an application - using Instance ID, we wouldn't be able to tell them apart - and so, wouldn't be able to tell apart an account added in the current first run from an account added in the previous "first run" of the same app install.

> 
> ::: mobile/android/base/java/org/mozilla/gecko/Telemetry.java
> @@ +53,4 @@
> >      private static native void nativeAddUiEvent(String action, String method,
> >                                                  long timestamp, String extras);
> >  
> > +    @Nullable public static UUID firstRunSessionUUID;
> 
> This doesn't seem to respect a meaningful domain abstraction.  Why not make
> `Session` have an identifier, and collect the identifiers of sessions that
> are active?  (I'm assuming that `FIRSTRUN` isn't exited until the first
> session is actually finished.)

See above for why I need a different way to identify this. "firstrun" ui session is actually never exited explicitly.

> If you want to inject this hack, then you need to add a nice comment
> explaining exactly why and what this is doing.

Aye.

> 
> :::
> mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/
> AndroidFxAccount.java
> @@ +729,5 @@
> >    public synchronized State getState() {
> > +    return getState(true);
> > +  }
> > +
> > +  public synchronized State getState(boolean refreshAccount) {
> 
> You introduce this, but there is only caller with `refreshAccount = false`,
> and I don't see why it hurts to get the account again.  Why bother with the
> flag?

That was just me being lazy in avoiding an infinite loop; I'll fix that up.

> 
> :::
> mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/
> FxAccountSyncAdapter.java
> @@ +17,4 @@
> >  import android.os.SystemClock;
> >  import android.support.v4.content.LocalBroadcastManager;
> >  
> > +import org.mozilla.gecko.GeckoApp;
> 
> The way to achieve this type of thing is to bloat further FirefoxAccounts,
> which is shared, IIRC.
> 
> And actually I see you're just using GSP -- that can just be hard-coded *if
> you ignore the profile*, which in this case probably makes sense.  That is,
> use App-wide prefs for this first run thing, since there's no meaningful
> profile management at work here.

I think bloating the shared FirefoxAccounts accounts is a better bet here: we'd still be coupling,
but at the very edge. Hard-coding pref access will make that coupling a few steps removed - we'll be
depending on patterns in which an external action (flip of a pref by GeckoApp) happens to our shared memory.

I mean, that dependency stays the same - but moving it out to boundaries is probably a good thing.

> 
> @@ +513,5 @@
> > +    // TODO what are the chances of sync vs pickle creation races?
> > +    final FirefoxAccounts.PickleFileState pickleFileState = FirefoxAccounts.queryPickleFileState(getContext());
> > +    final boolean isFirstRun = GeckoSharedPrefs.forProfile(getContext()).getBoolean(GeckoApp.PREFS_IS_FIRST_RUN, true);
> > +    final boolean pickleFilePresent;
> > +    switch (pickleFileState) {
> 
> Consider pushing this onto `PickleFileState` as a helper method.  It's
> unfortunate to use the pickle file as a sentinel, since we want to remove
> pickling entirely :(

We could pick anything else that's a by-product of a sync and isn't subject to races against the first sync. Pickle file fulfills that role, but so could a sync prefs file, for example (except that we don't own its creation). Probably a better fit might be a new sentinel specifically for this purpose?

> 
> @@ +537,5 @@
> > +              FxAccountSyncAdapter.NOTIFICATION_ID);
> > +      notificationManager.update(context, fxAccount);
> > +      return;
> > +    } else {
> > +      Logger.info(LOG_TAG, "Not in first run, and pickle file present; leaving account in its current state.");
> 
> This is all racing against the pickler, so can you bring this logic and the
> "write pickle" logic closer together in this file?  Can this go write before
> the "write pickle" logic?

I'm primarily concerned here with the first sync racing against pickling started when account was first added. Account is (as of somewhat recently) pickled right when it's added. So the race I'm wary about here is:
- account is added in the UI while we're in "first run", with auto-sync enabled; pickling is kicked off
- OS kicks off a sync before pickling could complete (seems quite improbably)
- ... which leads to account being erroneously switched to "separate" (persisted in account's user data)
- ... while the pickle operation completes resulting in a divergent state ("engaged", persisted on disk)

While I suspect this will work just fine most of the time (and likely for our purposes), it's brittle around the edges. The whole concept of a pickle file (or rather, attempting to maintain two representations of a state in light of inevitable failures and inability to synchronize events) is problematic.

To answer your question: I should add a clarification comment there to explain the current "pickle flow".

Comment 24

[:Grisha Kruglov]

(after flipping on --without-gradle)

Well, I can't use the Telemetry trick as-is, nor directly access GSP. Both are fixed, I think, by going directly through a sharedpref.

Comment 25

[:Grisha Kruglov]

Attachment: separate-account-on-clear-data.patch

Comment 26

[:Grisha Kruglov]

Attachment: separate-account-on-clear-data.patch

Comment 27

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #23)
> Thanks, Nick!
> 
> (In reply to Nick Alexander :nalexander from comment #22)
> > Comment on attachment 8944972 [details] [diff] [review]
> > separate-account-on-clear-data.patch
> > 
> > Review of attachment 8944972 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm pretty confident something like this will work, but can you explain what
> > the pros and cons of this approach are with respect to Instance IDs or
> > similar from Android itself? 
> > https://developer.android.com/training/articles/user-data-ids.
> > html#working_with_instance_ids_&_guids
> 
> Instance ID "persists as long as the app is installed", which is longer than
> I'd like here.
> 
> The goal is to uniquely identify each first run. Consider multiple "clear
> data" events in a lifetime of an application - using Instance ID, we
> wouldn't be able to tell them apart - and so, wouldn't be able to tell apart
> an account added in the current first run from an account added in the
> previous "first run" of the same app install.

Uh, I didn't realize this was the goal of this patch series.  That's a big change, and I'm not sure we want to do that.  Why do we want to do this?  Do we have product agreement that accounts should be removed on "clear data"?

It's unfortunate to move to that world and not simplify all of the Account-ContentProvider-SyncAdapter complications at the same time.  That is, we have close to the worst of all worlds:

- the simple user experience...
- with the complex Android abstraction
- and none of the simplicity one might get from managing your own Sync process

I don't think we _ever_ want to do this simplification, to be honest, at least not for Fennec -- but that suggests that we shouldn't try to switch the fundamental model of the implementation.

> > 
> > ::: mobile/android/base/java/org/mozilla/gecko/Telemetry.java
> > @@ +53,4 @@
> > >      private static native void nativeAddUiEvent(String action, String method,
> > >                                                  long timestamp, String extras);
> > >  
> > > +    @Nullable public static UUID firstRunSessionUUID;
> > 
> > This doesn't seem to respect a meaningful domain abstraction.  Why not make
> > `Session` have an identifier, and collect the identifiers of sessions that
> > are active?  (I'm assuming that `FIRSTRUN` isn't exited until the first
> > session is actually finished.)
> 
> See above for why I need a different way to identify this. "firstrun" ui
> session is actually never exited explicitly.

I'll re-read, trying to interpret in this new light.

But there's a much simpler way to answer "am I a lingering account (from a previous install) or/and am I a lingering account (after a clear data)?"

Tag every newly created Android Account object with the Instance ID.  (If we don't have Google Play Services, fine -- ignore this.)

Write a sentinel on Android Account creation -- SharedPreferences will do, since it's as good as any other file.

If the existing Account doesn't have the right Instance ID, it's lingering from a previous install.  If it doesn't have the SharedPreferences sentinel, it's lingering after a clear data.

No fuss, no muss, no tie in to the Gecko lifecycle and UI Telemetry.  You could even add the sentinels (if missing) in the App upgrade handler, easing your migration forward.

> > If you want to inject this hack, then you need to add a nice comment
> > explaining exactly why and what this is doing.
> 
> Aye.
> 
> > 
> > :::
> > mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/
> > AndroidFxAccount.java
> > @@ +729,5 @@
> > >    public synchronized State getState() {
> > > +    return getState(true);
> > > +  }
> > > +
> > > +  public synchronized State getState(boolean refreshAccount) {
> > 
> > You introduce this, but there is only caller with `refreshAccount = false`,
> > and I don't see why it hurts to get the account again.  Why bother with the
> > flag?
> 
> That was just me being lazy in avoiding an infinite loop; I'll fix that up.
> 
> > 
> > :::
> > mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/
> > FxAccountSyncAdapter.java
> > @@ +17,4 @@
> > >  import android.os.SystemClock;
> > >  import android.support.v4.content.LocalBroadcastManager;
> > >  
> > > +import org.mozilla.gecko.GeckoApp;
> > 
> > The way to achieve this type of thing is to bloat further FirefoxAccounts,
> > which is shared, IIRC.
> > 
> > And actually I see you're just using GSP -- that can just be hard-coded *if
> > you ignore the profile*, which in this case probably makes sense.  That is,
> > use App-wide prefs for this first run thing, since there's no meaningful
> > profile management at work here.
> 
> I think bloating the shared FirefoxAccounts accounts is a better bet here:
> we'd still be coupling,
> but at the very edge. Hard-coding pref access will make that coupling a few
> steps removed - we'll be
> depending on patterns in which an external action (flip of a pref by
> GeckoApp) happens to our shared memory.
> 
> I mean, that dependency stays the same - but moving it out to boundaries is
> probably a good thing.
> 
> > 
> > @@ +513,5 @@
> > > +    // TODO what are the chances of sync vs pickle creation races?
> > > +    final FirefoxAccounts.PickleFileState pickleFileState = FirefoxAccounts.queryPickleFileState(getContext());
> > > +    final boolean isFirstRun = GeckoSharedPrefs.forProfile(getContext()).getBoolean(GeckoApp.PREFS_IS_FIRST_RUN, true);
> > > +    final boolean pickleFilePresent;
> > > +    switch (pickleFileState) {
> > 
> > Consider pushing this onto `PickleFileState` as a helper method.  It's
> > unfortunate to use the pickle file as a sentinel, since we want to remove
> > pickling entirely :(
> 
> We could pick anything else that's a by-product of a sync and isn't subject
> to races against the first sync. Pickle file fulfills that role, but so
> could a sync prefs file, for example (except that we don't own its
> creation). Probably a better fit might be a new sentinel specifically for
> this purpose?

Yes, I think we're agreed here :)

> > 
> > @@ +537,5 @@
> > > +              FxAccountSyncAdapter.NOTIFICATION_ID);
> > > +      notificationManager.update(context, fxAccount);
> > > +      return;
> > > +    } else {
> > > +      Logger.info(LOG_TAG, "Not in first run, and pickle file present; leaving account in its current state.");
> > 
> > This is all racing against the pickler, so can you bring this logic and the
> > "write pickle" logic closer together in this file?  Can this go write before
> > the "write pickle" logic?
> 
> I'm primarily concerned here with the first sync racing against pickling
> started when account was first added. Account is (as of somewhat recently)
> pickled right when it's added. So the race I'm wary about here is:
> - account is added in the UI while we're in "first run", with auto-sync
> enabled; pickling is kicked off
> - OS kicks off a sync before pickling could complete (seems quite improbably)
> - ... which leads to account being erroneously switched to "separate"
> (persisted in account's user data)
> - ... while the pickle operation completes resulting in a divergent state
> ("engaged", persisted on disk)

This is a valid flow to be concerned about.

> While I suspect this will work just fine most of the time (and likely for
> our purposes), it's brittle around the edges. The whole concept of a pickle
> file (or rather, attempting to maintain two representations of a state in
> light of inevitable failures and inability to synchronize events) is
> problematic.
> 
> To answer your question: I should add a clarification comment there to
> explain the current "pickle flow".

Or prefer the dedicated sentinel.  But your concern is valid, and this might be the correct action.

Recall that SharedPreferences is synchronized across the application, _and_ you can synchronize pickle reading and writing (since everything is in the same process), so you can ensure that all pickle handling is in a critical section.  That is, you can solve this problem in a number of different ways; and I think you can ensure code locality for the read and write during a Sync this way.

Thanks for additional context!  I'll read the details again after we hammer out the sentinel strategy on this ticket.

Comment 28

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #27)
> Uh, I didn't realize this was the goal of this patch series.  That's a big
> change, and I'm not sure we want to do that.  Why do we want to do this?  Do
> we have product agreement that accounts should be removed on "clear data"?

Well, as for product buy in - there's this: https://bugzilla.mozilla.org/show_bug.cgi?id=1410753#c13

My desire to also address the broken Master Password+Sync situation certainly complicates matters here a little, but (fingers crossed) I do think that I've worked out the details (once we settle on a forward-looking sentinel).

As "Part 2" to this work, we can also modify the first-run pager to make it obvious what happened - or at the very least, don't hide the "set-up sync" page as we currently do in presence of an account.

Coupled with a notification which is thrown up it should give us a pretty acceptable user experience.

Comment 29

[:Grisha Kruglov]

(In reply to :Grisha Kruglov from comment #28)
> don't hide the "set-up sync" page as we currently do in presence of an account.

... that requires further attention from user.

Comment 30

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8945040 [details] [diff] [review]
separate-account-on-clear-data.patch

Review of attachment 8945040 [details] [diff] [review]:
-----------------------------------------------------------------

Great work on this patch!  I found this version much more clear.

Two comments:

1) the commit comment needs to be updated.

2) the key technical lever that we are using here is that Sync and the browser activity run in the same process, and therefore can access the static variables safely.  Everything about the UUID flows from that, and that reality should be in the comments _somewhere_.

Also, are you really confident that everything is thread safe?  I'd sprinkle `synchronized` around the relevant pieces, just in case we're running things at the same time...

Great work!

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +234,4 @@
>      public static final String EXTRA_SKIP_STARTPANE = "skipstartpane";
>      private static final String EOL_NOTIFIED = "eol_notified";
>  
> +    // Beware of org.mozilla.gecko.fxa.EnvironmentUtils.GECKO_PREFS_FIRSTRUN_UUID

nit: "Be aware", trailing period (always full sentences, please).

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +146,4 @@
>      //
>      // Originally, this was only used for the telemetry core ping logic. To avoid
>      // having to write custom migration logic, we just keep the original pref key.
> +    // Beware of org.mozilla.gecko.fxa.EnvironmentUtils.GECKO_PREFS_IS_FIRST_RUN

Ditto.  Another way is to use Javadoc to link these two things, so that usage shows both in the IDE...

::: mobile/android/base/java/org/mozilla/gecko/Telemetry.java
@@ +12,4 @@
>  import org.mozilla.gecko.TelemetryContract.Reason;
>  import org.mozilla.gecko.TelemetryContract.Session;
>  import org.mozilla.gecko.mma.MmaDelegate;
> +import org.mozilla.gecko.util.UUIDUtil;

These look unused.  Cull them?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccountsUtils.java
@@ +56,5 @@
> +            }
> +
> +            // Otherwise:
> +            // Account is present while in "First Run" and it wasn't created during this "First Run".
> +            // Normally this indicates either that application data was cleared in a signed-in browser.

nit: drop "either".

@@ +58,5 @@
> +            // Otherwise:
> +            // Account is present while in "First Run" and it wasn't created during this "First Run".
> +            // Normally this indicates either that application data was cleared in a signed-in browser.
> +            // More rarely, this state might indicate that account shouldn't be there at all (Bug 1429735).
> +            // Either way, we move this account into a Separated state, requiring user to re-enter their

Maybe just "requiring the user to re-connect the Account".  In the future, it'll read our minds to re-connect!

Comment 31

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #30)
> 2) the key technical lever that we are using here is that Sync and the
> browser activity run in the same process, and therefore can access the
> static variables safely.  Everything about the UUID flows from that, and
> that reality should be in the comments _somewhere_.

This comment is almost correct - latest version of this patch is using SharedPreferences as shared memory, which isn't safe to access reliably from different processes. So we're depending on sync & browser activity to run in the same process, or things won't work as expected.

Comment 32

[:Grisha Kruglov]

Attachment: separate-account-on-clear-data-final.patch

Rebased, addressed all of your feedback. Only added comments, with an exception of synchronizing two methods.

Doesn't seem like Splinter can carry over the r+, so flagging for another review.

Comment 33

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8952267 [details] [diff] [review]
separate-account-on-clear-data-final.patch

Review of attachment 8952267 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.  Make sure it builds with and without Gradle (B and Bng).

Comment 34

[:Grisha Kruglov]

Requesting checkin + renaming this bug to better reflect the most common affected scenario.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e1cc926177
Pre: cleanup some unused helper methods r=nalexander

Comment 37

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a5e1cc926177