1511574 - When setting up Firefox Sync, it will sometimes sync the new install's add-on settings (which are still default) to the server instead of pulling the existing settings

Status: RESOLVED FIXED

(WebExtensions :: Storage, defect, P3)

Description

[notjerl]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Steps to reproduce:

1.  Have Firefox installed on devices with add-ons that sync their settings via your Firefox Account. (in my case, the most noticeable being Tridactyl, since it's my interface with the program)
2.  Have a brand new install of Firefox (or new profile).
3.  Link the new install (or new profile) to your Firefox Account.


Actual results:

Firefox downloads fresh copies of the extensions, then goes ahead and syncs the default settings from those extensions to the Firefox Account before pulling the settings that were already synced, overwriting existing settings synced by previous installs of Firefox.  

Other installs of Firefox will then have the add-on's default settings synced to them, replacing their existing settings with defaults as though the add-on had just been installed for the first time.  

Attempting to sync those old settings back to the Firefox account by using a device that still has them or restoring backups of the profile prove futile, as Sync will one again overwrite the saved settings on those unsynced or backed-up profiles to those of a freshly-installed add-on.

Essentially, for some reason Sync seems to assume that the brand new barely linked install's default app settings should be sent to other installs instead of the other way around.

I've experienced this twice now (both from setting up absolutely brand new installs of Firefox and attempting to link them), and a contributor in the #sync channel instructed me to submit a ticket if I experienced it again.


Expected results:

Firefox downloads copies of the extensions with their settings already set exactly as previously synced

OR

Firefox downloads fresh copies of the extensions, then replaces their default settings with those previously stored in the Firefox Account by existing devices linked to it. Firefox does NOT sync the settings from the newly-linked Firefox install up to the cloud until it has already pulled the existing settings.

Comment 1

[Daniel Bodea [:danibodea]]

I have confirmed this issue by using the following steps:
1. Open browser with a new profile.
2. Install a bunch of webextensions.
3. Sign in with a sync account.
4. Modify the options of each webextension so that it's different from the default settings.
5. Close the browser.
6. Reopen the browser with a new profile.
7. Wait for the previously installed extensions to be reinstalled.

Expected: The webextensions set on the previous profile are picked up by each extension.

Actual: Each extension has its default settings (like at first install).


Please note: I do not know whether this is a Firefox Sync issue, a webextension sync issue or if it even is a valid issue. I will leave it with the previously set component and let some developer decide the fate of this bug.

Comment 2

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

Addon settings are not synced by default. Each addon can choose to store their settings using the browser.storage.sync API, which would cause them to be synced, however, each addon must opt in to this. If a particular addon does opt-in but it still doesn't work, then the appropriate component would be Web Extensions->Storage.

Comment 3

[notjerl]

Yes, the add-ons that this happens for are add-ons that *do* opt-in to sync.  Sync normally does work just fine, the problem only occurs when setting up sync on a brand new profile.

If the problem with the ticket is just that it's in the wrong component, how do I direct it to the right component?

Comment 4

[Andrew Swan [:aswan]]

Mark (or Ethan), can you clarify how storage.sync works and how extensions are meant to use it?
In particular, if an extension calls browser.storage.sync.get(), does that wait to return until it is sure it has up-to-date data?

A pretty common pattern in extensions is something like this, meant to initialize settings when an extension is first used:
```
let settings = await browser.storage.whatever.get(somekey);
if (!settings) {
  settings = DEFAULTS;
  browser.storage.whatever.set(somekey, settings);
}
```

If get() commonly returns no results right after an extension is installed, this is going to be a frequent problem.  Is there any reasonably simple way for extension authors to work around this (short of reimplementing their own write ordering and conflict resolution on top of sync!)?

Comment 5

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

Sorry, but I'm not particularly familiar with that code at all - while I helped review it, all work was done outside our team. However, my understanding is that there is no waiting at all - it simply requests what kinto has at this point in time. I imagine it would be tricky to wait for a first sync given the latency this might introduce, especially in the face of network errors.

Comment 6

[Ethan Glasser-Camp (:glasserc)]

The current approach taken in `ExtensionStorageSync` is to sync any extensions which are "currently active" based on their use of the storage.sync API. See `ExtensionStorageSync#syncAll`. "Use of the storage.sync API" includes calls to get(), set(), and onChanged(). Once those methods are called, data for this extension will be synced, but that could still be up to an hour later. If the pseudocode you post is representative, then yes, this is going to be a frequent problem.

Since the user experience of loading an extension and not already having your settings is unfortunate, maybe we should sync settings for all extensions rather than just the ones "in use". I reread https://bugzilla.mozilla.org/show_bug.cgi?id=1253740 to try to understand why this feature evolved in this way and I think it was because we didn't have any obvious way to get a list of "all extensions", so we wrote our own "track the extensions that use it" logic. But in hindsight maybe we should just sync all the data written by all extensions on all machines (based on their presence in the keyring), on the off chance that an extension will get added in the near future. This might also allow us to clean up or delete the "track current extensions" code.

Comment 7

[David Durst [:ddurst]]

Mark, is #c6 something your team can take?

Comment 8

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

(In reply to David Durst [:ddurst] from comment #7)
> Mark, is #c6 something your team can take?

Sorry, but we only look after core sync stuff, which doesn't include this kinto-based stuff (or anything under toolkit/components/extensions)

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

This is still an issue, and it may become even more important if we actively suggest users to connect to sync (e.g. by promoting in the post-install doorhanger).

When I was looking into this issue sometimes ago, Ethan and I discussed about changing the default conflict resolution on the kinto-client to be server-wins.

Ethan, I recall that there was some ongoing discussion about changing the backend used by locally for storage.sync (migrating it from kinto-client to something else), is that still the case and we should fix the issue in the new backend or can we plan to fix this issue on the current kinto-client one?

Comment 10

[Ethan Glasser-Camp (:glasserc)]

That work is a collaboration between me and the sync team, and relies on their implementation of a "generic sync backend", which is in progress now. Janet Dragojevich, :tcsc or :lina are the people to ask about that. Considering the scale of the change given the existing system, it might be better to just go ahead and make the change now.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

If I remember correctly we briefly discussed about this issue while you were working on the new storage.sync backend, and if I'm not mistaken this should actually have been fixed as part of migrating to the new backend. Do you recall more details and can confirm us this is the case?

Thanks!

Comment 12

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

Yes, the "new" webext-storage backend should have solved this.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Thanks for the confirmation Mark,
I'm closing this as fixed then and add Bug 1634615 as a dependency to reflect the relationship with enabling the new backend on all channels.