1337993 - Bookmark sync broken after disabling bookmarks, syncing, and re-enabling them.

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

After disabling bookmark sync, syncing, and re-enabling it, my bookmarks are no longer synced. This can be checked via aboutsync, or by inspecting the logs.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1337993 - Ensure we mark all current bookmarks for reupload when bookmark engine enabled state changes.

Review commit: https://reviewboard.mozilla.org/r/110902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110902/

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

I don't feel great about this since I don't understand why this works but the old way doesn't.

Comment 3

[Lina Butler [:lina]]

The patch looks good, but now I have a lot of questions about how declining works. AFAICT, the only times we call `resetClient` are:

* `wipeRemote`, which is controlled by the `firstSync` pref.
* `handleFetchedKeys`, when a collection key or the default encryption key changes. We seem to only do this for the first post-initialization sync.
* `verifyAndFetchSymmetricKeys`, when the server doesn't have encryption keys yet. Fresh account, or node reassignment?
* `startOver`, when we disconnect from Sync.
* `remoteSetup`, when we detect a sync ID change, or the server doesn't have a `meta` record. Again, I think we only do this for the first post-init sync.

So, does that mean that declining an engine doesn't currently delete its data from the server? Do we also remember all local state, too, like the last sync time?

On the one hand, I could see why we would want to keep it on the server: if you change your mind, we don't want to do a full sync for large collections. But it's possible for local data to diverge quite a bit from remote data, and I can imagine a slew of conflicts if you do eventually re-enable the engine.

I'm curious if I understand the declined engine logic correctly, and if there's a reason it works the way it does. Richard, any insights?

Comment 4

[Richard Newman [:rnewman]]

"declined" simply means: this engine is not enabled, and it was marked as not enabled -- that is, it was omitted from the meta/global engines map -- on a client that knows how to sync that engine.

If we add a new sync engine, "diesel", to desktop in Firefox 55, we have a question: how do we decide whether to offer it to clients?

If you create your account in Firefox 54, or on Android, there's no mention of "diesel" in meta/global. Easy, you say: we'll have Firefox 55 on all platforms offer Diesel Syncing if "diesel" isn't already in meta/global!

But what if you already opted out somewhere else? How can we avoid prompting? meta/global's engine map doesn't represent disabled engines at all. The declined list was added to do that.

Declining has nothing to do with wiping data (that's engine enablement). It has nothing to do with timestamps (that's resetting).

Keeping data on the server is a tradeoff, and one that can only be addressed by per-device datatype elections. I've been wanting that to happen for about four years now, and I think Ryan's on board, so maybe that'll happen some time.

Comment 5

[Richard Newman [:rnewman]]

The decision to do a full upload and download is, as Thom correctly noted, handled by resetting. Each client is supposed to be reset when a server state is encountered that requires us to do a full re-sync: it's a new server, a new user, a new syncID, new crypto keys, or a new meta/global.

The fix, then, is probably not Thom's patch: that resets everything when the engine is disabled locally. (It's a belt-and-braces thing to do, but it's probably masking another bug elsewhere.)

The expected flow is that the engine disablement propagates to the server as part of a collection wipe and a reuploaded meta/global -- when you turn off bookmark sync you tell the server. If you turn on bookmark sync again, "bookmarks" will be missing from meta/global, so we do a reset as part of that sync, and we upload a new meta/global.

It should be relatively straightforward to read the logs and find out what part of that process is no longer occurring.

Comment 6

[Lina Butler [:lina]]

Thank you for the clarification, that's very helpful! IIUC, http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/services/sync/modules/stages/enginesync.js#409-422 is the part that wipes the collection, either because we disabled the engine locally, or it was disabled on another device?

Comment 7

[Richard Newman [:rnewman]]

Yes. You'll notice line 386 is where we short-circuit if the engine doesn't exist locally, avoiding marking it as declined.

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8835207 [details]
Bug 1337993 - Ensure we mark all current bookmarks for reupload when bookmark engine enabled state changes.

https://reviewboard.mozilla.org/r/110902/#review113062

Clearing review, since this isn't quite the right approach per comment 5.

Comment 9

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8835207 [details]
Bug 1337993 - Ensure we mark all current bookmarks for reupload when bookmark engine enabled state changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110902/diff/1-2/

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

This was hard to track down. Let me know if instead we should be calling the non-underscored version of this (https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines.js#705) from wipeServer (https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines.js#1717) instead.

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8835207 [details]
Bug 1337993 - Ensure we mark all current bookmarks for reupload when bookmark engine enabled state changes.

https://reviewboard.mozilla.org/r/110902/#review113442

Ah, great find! I can't remember why I overrode the public method in bug 1258127, but that was obviously wrong. Let's also uplift this to Aurora.

::: services/sync/tests/unit/test_bookmark_decline_undecline.js:74
(Diff revision 2)
> +  let server = serverForFoo(engine);
> +  await SyncTestingInfrastructure(server);
> +
> +  try {
> +
> +    let bzid = PlacesUtils.bookmarks.insertBookmark(

Nit: Let's use the async API to insert bookmarks in new code.

Comment 12

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8835207 [details]
Bug 1337993 - Ensure we mark all current bookmarks for reupload when bookmark engine enabled state changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110902/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56cd74ccf2c3
Ensure we mark all current bookmarks for reupload when bookmark engine enabled state changes. r=kitcambridge

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56cd74ccf2c3

Comment 15

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8835207 [details]
Bug 1337993 - Ensure we mark all current bookmarks for reupload when bookmark engine enabled state changes.

Sorry, should have requested this before merge

Approval Request Comment
[Feature/Bug causing the regression]: bug 1258127
[User impact if declined]: Users who disable sync of bookmarks and turn it back on won't ever reupload their old bookmarks, they won't be synced.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes.
1. Sign into a firefox account with some number of bookmarks and sync.
2. Disable bookmark sync from about:preferences#sync (uncheck the box)
3. Manually sync (sync button in hamburger menu is good enough)
4. Re-enable bookmarks
5. Manually sync again
6. Verify bookmarks are on the server by installing https://addons.mozilla.org/en-US/firefox/addon/about-sync/ (sorry this is so convoluted!), and checking the bookmarks section, which should say "N records (M deleted)" for some N != 0 (see http://i.imgur.com/SIHEPqh.png for example). If N isn't 0, then everything works, otherwise it doesn't.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only effects bookmark sync, and only on enable/disable.
[String changes made/needed]: N/A

Comment 16

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Since it's a regression from 53 I'd like to fix it in 53. Ideally this would have had verification a couple of weeks ago after it landed. Right now I'm leaning towards: uplift to 53 beta 2 and verify the fix in beta. 

Andrei, can your team take a look next week?

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8835207 [details]
Bug 1337993 - Ensure we mark all current bookmarks for reupload when bookmark engine enabled state changes.

Regression from 53, let's try not to ship it to release.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc91a19ee809

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> Since it's a regression from 53 I'd like to fix it in 53. Ideally this would
> have had verification a couple of weeks ago after it landed. Right now I'm
> leaning towards: uplift to 53 beta 2 and verify the fix in beta. 
> 
> Andrei, can your team take a look next week?

Looping in Kanchan, who's usually handling sync related work on desktop. Instructions available in Comment 15.

Comment 20

[Kanchan Kumari QA]

I have tested this bug fix on following versions and found it to be working fine.
Version    53.0b2
Build ID   20170310174836 (CI build)

Version 	54.0a2
Build ID 	20170313004011

As expected I don't see any validation error or bookmark missing in aboutsync and bookmarks are uploading correctly when bookmark engine enabled state changes.