1276998 - Add-ons are restored after Refresh if sync is enabled

Status: NEW

(Firefox :: Migration, defect, P3)

Description

[John Karahalis [:openjck]]

Steps to reproduce:

1. Create a new Firefox profile
2. Sign up for and enable sync
3. Install some add-ons
4. Perform a successful sync
5. Refresh Firefox (https://support.mozilla.org/en-US/kb/refresh-firefox-reset-add-ons-and-settings)

Actual results:

A few minutes after Firefox is reset, the add-ons that were previously installed are restored. Sync settings are preserved when Firefox is reset, and sync in turn restores the add-ons that were previously installed. I think that's a bit strange, since add-ons sometimes cause the problems that inspire people to reset Firefox in the first place.

Expected result:

If sync restores add-ons after a reset, they should be restored in a disabled state. At least that's one possible solution.

Comment 1

[:Gijs (he/him)]

Mark/Matt, I didn't think that we kept sync things when resetting... am I missing something?

(cross-ref: bug 1017919 would purposefully change the add-ons to be preserved but disabled)

Comment 2

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #1)
> Mark/Matt, I didn't think that we kept sync things when resetting... am I
> missing something?

See bug 972196 and the copying of signedInUser.json at https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/FirefoxProfileMigrator.js?rev=c2e9a445fede&mark=127#125

Comment 3

[Matthew N. [:MattN]]

I agree with John that this is a problem as it defeats a main benefit of a refresh for users with add-on sync enabled. It seems like bug 1017919 would/could probably fix this though I forget if the userDisabled key is synced so that same add-ons are enabled on desktop devices?

Comment 4

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

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #3)
> I agree with John that this is a problem as it defeats a main benefit of a
> refresh for users with add-on sync enabled. It seems like bug 1017919
> would/could probably fix this though I forget if the userDisabled key is
> synced so that same add-ons are enabled on desktop devices?

That userDisabled key is synced, so yeah, I expect that Sync will just enable them.

But this seems tricky to get right. The entire point of Sync is that the user has chosen for changes made on one device to be made on all of them, so I expect there are a group of users who fully *do* expect this behaviour - they also expect their synced bookmarks, preferences, passwords etc to come back too - that's exactly why they use Sync. Note that the bad addons will presumably still be on their other devices - a full "reset firefox" seems a big hammer to use when the intention is just to remove a single addon (and with Sync enabled, removing it on one device will remove it on all).

Really, the best I can come up with is some UI as part of the reset process that asks if you want your Sync credentials copied.

Comment 5

[Matthew N. [:MattN]]

(In reply to Mark Hammond [:markh] from comment #4)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #3)
> > I agree with John that this is a problem as it defeats a main benefit of a
> > refresh for users with add-on sync enabled. It seems like bug 1017919
> > would/could probably fix this though I forget if the userDisabled key is
> > synced so that same add-ons are enabled on desktop devices?
> 
> That userDisabled key is synced, so yeah, I expect that Sync will just
> enable them.

OK, I vaguely recall that and it always seemed wrong to me (for my use cases) as each computer is different. It's also weird when extension have first-run behaviour such as opening a tab. I may want one set of extensions enabled on my work computer and others on my personal one. I want my data everywhere but the extensions should be separate. See also how we don't sync the tabstrip between devices, we only give access to remote tabs through secondary UI. 

I think it's worth re-evaluating whether we should sync userDisabled. We could instead default to userDisabled = true when installing.

> But this seems tricky to get right. The entire point of Sync is that the
> user has chosen for changes made on one device to be made on all of them, so
> I expect there are a group of users who fully *do* expect this behaviour -
> they also expect their synced bookmarks, preferences, passwords etc to come
> back too - that's exactly why they use Sync. Note that the bad addons will
> presumably still be on their other devices - a full "reset firefox" seems a
> big hammer to use when the intention is just to remove a single addon (and
> with Sync enabled, removing it on one device will remove it on all).

I think you're forgetting about non-user-installed extensions which don't get synced. We also only sync AMO extensions IIRC and hopefully we don't have malware extensions on AMO.

Comment 6

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

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #5)
> OK, I vaguely recall that and it always seemed wrong to me (for my use
> cases)

Yeah - I personally disable addon syncing for exactly this reason.

> I think it's worth re-evaluating whether we should sync userDisabled. We
> could instead default to userDisabled = true when installing.

That's worth considering, although I wonder how many people will work out that for addon sync to work they need to visit about:addons and enable them? ie, in the usual case, I'd expect people to complain that syncing addons simply doesn't work (ie, "I've synced addons but my youtube downloader button isn't appearing on my other devices")

> I think you're forgetting about non-user-installed extensions which don't
> get synced. We also only sync AMO extensions IIRC and hopefully we don't
> have malware extensions on AMO.

Yeah exactly - given they are the only addons we Sync I honestly don't understand what risks the current behaviour has. The profile reset would wipe potentially bad addons while restoring the assumed-good ones from AMO that the user has explicitly chosen to install while also explicitly telling Sync that addons should be synced between the devices. Ditto for restoring preferences (ie, a reset will clear "bad" preferences that aren't synced, but Sync will re-update the whitelisted set of "good" prefs we do sync)

Comment 7

[John Karahalis [:openjck]]

(In reply to Mark Hammond [:markh] from comment #6)
> Yeah exactly - given they are the only addons we Sync I honestly don't
> understand what risks the current behaviour has. The profile reset would
> wipe potentially bad addons while restoring the assumed-good ones from AMO
> that the user has explicitly chosen to install while also explicitly telling

I think that's true. At the same time, add-ons can be undesirable for reasons other than executing malicious code. Add-ons can slow Firefox down or change it in unexpected ways. Ideally those problems would also be addressed by a reset.

Comment 8

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

(In reply to John Karahalis [:openjck] from comment #7)
> I think that's true. At the same time, add-ons can be undesirable for
> reasons other than executing malicious code. Add-ons can slow Firefox down
> or change it in unexpected ways. Ideally those problems would also be
> addressed by a reset.

That's true - but the same addon is presumably problematic on *all* Synced devices. It just seems there are 2 irreconcilable (by me) requirements here. Maybe transferring the Sync account is the wrong thing to do after a reset? But even then, the user's likely to reconnect it to Sync later and strike the exact same problem. It still seems to me that Sync is doing exactly what the user has configured it to do - Sync addons to new profiles.

Comment 9

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #4)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #3)
> > I agree with John that this is a problem as it defeats a main benefit of a
> > refresh for users with add-on sync enabled. It seems like bug 1017919
> > would/could probably fix this though I forget if the userDisabled key is
> > synced so that same add-ons are enabled on desktop devices?
> 
> That userDisabled key is synced, so yeah, I expect that Sync will just
> enable them.
> 
> But this seems tricky to get right. The entire point of Sync is that the
> user has chosen for changes made on one device to be made on all of them, so
> I expect there are a group of users who fully *do* expect this behaviour -
> they also expect their synced bookmarks, preferences, passwords etc to come
> back too - that's exactly why they use Sync.

Bookmarks and passwords persist past a reset without sync's help.

> Note that the bad addons will
> presumably still be on their other devices - a full "reset firefox" seems a
> big hammer to use when the intention is just to remove a single addon (and
> with Sync enabled, removing it on one device will remove it on all).

The intention of refresh/reset is "make Firefox work properly again". IME, 'even' things on AMO, in fact, even add-ons we make ourselves (cf. bug 1276990) sometimes break Firefox.

(In reply to Mark Hammond [:markh] from comment #8)
> (In reply to John Karahalis [:openjck] from comment #7)
> > I think that's true. At the same time, add-ons can be undesirable for
> > reasons other than executing malicious code. Add-ons can slow Firefox down
> > or change it in unexpected ways. Ideally those problems would also be
> > addressed by a reset.
> 
> That's true - but the same addon is presumably problematic on *all* Synced
> devices.

That depends. The add-on prefs are likely not synced, and it's not that hard to misuse e.g. ABP or noscript and end up with a working machine in one place and a busted one somewhere else.

> It just seems there are 2 irreconcilable (by me) requirements here.
> Maybe transferring the Sync account is the wrong thing to do after a reset?
> But even then, the user's likely to reconnect it to Sync later and strike
> the exact same problem. It still seems to me that Sync is doing exactly what
> the user has configured it to do - Sync addons to new profiles.

It sounds to me like we should check with UX. :-)

Comment 10

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

(In reply to :Gijs Kruitbosch from comment #9)
> It sounds to me like we should check with UX. :-)

Yeah, thanks :) Another possibility might be to default this sync engine to disabled.

Hopefully :mverdi and :rfeeley can chat about this...

Comment 11

[Matthew N. [:MattN]]

(In reply to Mark Hammond [:markh] from comment #10)
> Yeah, thanks :) Another possibility might be to default this sync engine to
> disabled.

I was also going to suggest that but unchecking a sync engine currently means that no client will sync that data type and the server data will be wiped (until we fix bug 1215415).

Comment 12

[Ryan Feeley [:rfeeley]]

I'm reluctant to suggest we remove any Sync data, but I think it's more helpful to just disable all add-ons, and then prompt users to re-enable them on the "refresh message" messaging. Michael, thoughts?