1311091 - Firefox Profile Refresh does not preserve Sync Preferences

Status: RESOLVED DUPLICATE of bug 1395332

(Firefox :: Migration, defect, P3)

Description

[Alex Davis [:adavis]]

When a user refreshes his profile on the /new download page, we should not reset their Sync preferences to default. These preferences should be transferred to the new profile.

https://twitter.com/sadmonc/status/785850640538931200

Comment 1

[:Gijs (he/him)]

Which "sync preferences" exactly? As a general rule, 0 preferences are retained.

Comment 2

[:Gijs (he/him)]

Mark, how does sync even work without copying services.sync.username (which we use for telemetry and UITour state about whether or not the user is using fxa, so it'd underreport such users) ?

Comment 3

[Lina Butler [:lina]]

(In reply to :Gijs Kruitbosch from comment #2)
> Mark, how does sync even work without copying services.sync.username (which
> we use for telemetry and UITour state about whether or not the user is using
> fxa, so it'd underreport such users) ?

I think it works because Sync pulls that value from FxA, which reads it from `signedInUser.json`. If I change "services.sync.username" to a different value and sync, it'll reset that pref (and "services.sync.account") to my FxA email.

Comment 4

[Richard Newman [:rnewman]]

See Bug 972196.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #2)
> Mark, how does sync even work without copying services.sync.username (which
> we use for telemetry and UITour state about whether or not the user is using
> fxa, so it'd underreport such users) ?

As Kit mentioned, most preferences we care about will be re-created after the reset.

For this particular bug, I think the fact we are not copying the preferences for which engines are enabled is causing Sync to get confused and assume that the user explicitly re-enabled all engines via about:preferences#sync. We should fix that (and at the same time, consider if there are other preferences we should copy across - eg, if a user is configured to use a self-hosted (or any non-default) suite of servers, I don't think they will currently be retained either).

Kit has kindly offered to dig into this some more, so moving the ni? to him so it doesn't get forgotten.

Comment 6

[Alex Davis [:adavis]]

Thanks for looking into this. I admit I had not tested but we had a user publicly complain about it on Twitter so I thought it would be worth investigating since forgetting this preference would be a pretty bad UX and potentially a security flaw if users had intentionally opted-out of password sync for a given reason.

Comment 7

[Lina Butler [:lina]]

I guess we can read and migrate Sync-related prefs from the old profile. Or we could store engine state in a JSON file if pref migration is impractical. Deciding what to keep and what to discard gets tricky, though. The current device name? The last sync time for each engine (so we don't do a full sync after refreshing)? The list of synced prefs?

Comment 8

[Mark Hammond [:markh] [:mhammond]]

FirefoxProfileMigrator.jsm does a few preferences, so I think adding a few extra shouldn't cause too much trouble. I'd be inclined to just do the enabled engines, and I guess the device name can't really hurt. I think we should leave the prefs for things like lastSync alone - eg, sync will probably not pick up addons if we do that - and we haven't had reports of bad things happening by us not doing that.

Comment 9

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #8)
> FirefoxProfileMigrator.jsm does a few preferences,

It sets some. I don't think there's current code to read the old prefs, and it might be tricky to write such code without importing all prefs, which we definitely do not want to do (part of why reset/refresh works is because most of the prefs get wiped, and it's often those prefs that cause issues). Not impossible, just annoying - you might need to actually manually read/~grep the old prefs file or something.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Mark Hammond [:markh] from comment #8)
> > FirefoxProfileMigrator.jsm does a few preferences,
> 
> It sets some. I don't think there's current code to read the old prefs

Hrm - yeah. I think we should abandon the idea doing anything other than the engine selections in this bug, in the hope that will allow us to avoid reading old prefs entirely. I admit I don't understand why this is failing in practice (ie, why this is somehow different to a brand new profile), but I'm sure Kit will enlighten us :)

Comment 11

[Richard Newman [:rnewman]]

It would be nice to either:

1. Delete the old client record from the server prior to Refresh, or
2. Preserve the client ID.

Otherwise there will be a stale duplicate client on the server for 3 weeks…

Comment 12

[Lina Butler [:lina]]

I just started looking at this, but now I'm confused how this works at all.

Sync pulls and updates `sync.services.username` from FxA (which, in turn, loads it from ``signedInUser.json`). But, if I'm reading http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/services/sync/Weave.js#147-153 right, Sync shouldn't start if that pref isn't set.

So does something else actually load Sync and start syncing? Does this also mean a user that signed in to FxA, but not Sync—Hello or Pocket users, for instance—will be automatically set up to use Sync after refresh?

More investigation is required.

Comment 13

[Lina Butler [:lina]]

TL;DR: We're not doing the wrong thing (\o/); it just looks like we are ((╯°□°）╯︵ ┻━┻).

(1) Refreshing Firefox does reset the declined engine preferences, until the next successful sync. At that point, we'll download and use the declined engine preferences from the server. Even though all the checkboxes will be checked in about:preferences#sync, we'll clear them during the first sync, and *won't* actually sync them. This is good, but makes for a very confusing experience, because it appears we reset the preferences even though we really didn't.

As a workaround, we could disable the checkboxes until `Services.prefs.prefHasUserValue("services.sync.declinedEngines") === true`.

(2) On my Ubuntu VM, refreshing Firefox consistently causes issues with key3.db. (I haven't been able to reproduce on macOS). Those manifest as "Services.logins is undefined" errors (bug 1295122), and prevent Sync from working at all. We also don't show the reconnect UI at first. This UX is worse: not only does it look like we've reset your Sync prefs, but we're silently failing to sync.

Interestingly, this only happens in the session after the refresh, when Firefox auto-restarts. If I manually restart Firefox after that, the issue goes away, we sync successfully, and fix the engine preferences. I'm going to leave a more detailed comment on bug 1295122, because we've seen that error a number of times in the sync ping, and it suggests something else is going on.

Comment 14

[Lina Butler [:lina]]

(In reply to Kit Cambridge [:kitcambridge] from comment #13)
> We also don't show the reconnect UI at first. This UX is
> worse: not only does it look like we've reset your Sync prefs, but we're
> silently failing to sync.

That is, we don't show it until the first sync. I can sign in again after that, but we can't store the key fetch token or keys due to the same key3.db issue.

Comment 15

[Lina Butler [:lina]]

(In reply to Kit Cambridge [:kitcambridge] from comment #13)
> (1) Refreshing Firefox does reset the declined engine preferences, until the
> next successful sync. At that point, we'll download and use the declined
> engine preferences from the server. Even though all the checkboxes will be
> checked in about:preferences#sync, we'll clear them during the first sync,
> and *won't* actually sync them. This is good, but makes for a very confusing
> experience, because it appears we reset the preferences even though we
> really didn't.
> 
> As a workaround, we could disable the checkboxes until
> `Services.prefs.prefHasUserValue("services.sync.declinedEngines") === true`.

Ryan, how do you feel about this? I'm suggesting we disable the engine checkboxes in Sync prefs until the first sync after a refresh.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kit Cambridge [:kitcambridge] from comment #15)
> Ryan, how do you feel about this? I'm suggesting we disable the engine
> checkboxes in Sync prefs until the first sync after a refresh.

Or instead of disabling them, replace them with text saying something that indicates what state we are in - eg, "Waiting for the first sync of this newly reset profile..." or similar. This is a bit of an edge-case - if not for the other bug, the check-boxes would only be in that state for 10 seconds after the reset, so I don't think it's worth anything too elaborate.

I also think we should get a bug on file for the key3.db issue on Linux (and check if it also exists on Windows) as there's a reasonable chance the original twitter reporter may not have noticed otherwise (ie, that first sync might otherwise have completed before they checked, at which point the engine selections would have been correct)

Comment 17

[Lina Butler [:lina]]

The key3.db issue is tracked in bug 1124553, but the checkboxes are still wrong before the first sync.

Comment 18

[Ryan Feeley [:rfeeley]]

Disabling sounds like the best approach, and luckily we have that front end code working well. Good reuse!

Comment 19

[Lina Butler [:lina]]

Folding this into bug 1395332, since I think it's another symptom of Sync temporarily forgetting it's signed in after a refresh.