1352233 - Reconciliation discards folder order if local folder is newer

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler [:lina]]

Folder records store an ordered list of their child GUIDs. Unfortunately, if the local folder has a newer modification time than the remote folder, we'll discard the remote folder record and lose the order. We also use the wall-clock time to determine the age of the two records, so clock skew might cause us to do the wrong thing.

This is particularly a problem for first syncs and node reassignments, where a profile might have new bookmarks in an existing folder. In that case, we'll download the records in whatever order they're stored on the Sync server (folders first), move them to the end of the parent, and then not call `PlacesSyncUtils.bookmarks.order` because the local parent is newer than the remote parent.

Thom fixed this for first syncs and roots in bug 1349703; this bug covers the more general case. I think we'll need to do some kind of structural reconciliation here.

Comment 1

[Richard Newman [:rnewman]]

To clarify: do you mean "if the local folder has a newer modification time" _and the local folder is marked as changed_?

For reference, the iOS structural merger, with half-decent commenting, is here:

https://github.com/mozilla-mobile/firefox-ios/blob/master/Sync/Synchronizers/Bookmarks/ThreeWayTreeMerger.swift#L675

Comment 2

[Lina Butler [:lina]]

(In reply to Richard Newman [:rnewman] from comment #1)
> To clarify: do you mean "if the local folder has a newer modification time"
> _and the local folder is marked as changed_?

Yes.

> For reference, the iOS structural merger, with half-decent commenting, is
> here:
> 
> https://github.com/mozilla-mobile/firefox-ios/blob/master/Sync/Synchronizers/
> Bookmarks/ThreeWayTreeMerger.swift#L675

Thanks, this is very useful!

Comment 3

[Lina Butler [:lina]]

Other cases besides a first sync: profile restore, adding two different bookmarks to the same folder on different devices. In case of a node reassignment, the timestamps and order should already be correct.

Comment 4

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

Attachment: Test changes to demonstrate the problem

(In reply to Kit Cambridge [:kitcambridge] (He/Him; UTC-8) from comment #0)
> Folder records store an ordered list of their child GUIDs. Unfortunately, if
> the local folder has a newer modification time than the remote folder, we'll
> discard the remote folder record and lose the order.

The following test:

1) Creates a folder with 1 bookmark B1, syncs.
2) Simulates another client changing that folder by modifying the server records in-place (ie, inserts a new bookmark B2 into the server collection and changes the folder's children to include the new bookmark) - the order of bookmarks in the parent record is B2,B1
3) Adds another bookmark B3 locally, with the local order being B3,B1.
4) Syncs

At the end, the order of the folder is B3,B1,B2. Given in both cases B1 is the last bookmark - it should be either B3,B2,B1 or B2,B3,B1

I believe this is the general case of the patch that landed in bug 1349703 - IIUC, that fix works because the local record doesn't have children to consider (so the server order is correct), however, in this general case, we probably need to be smarter (ie, consider the order in *both* copies and do the right thing).

I also believe this isn't going to be as rare as we hope - someone adding (say) a bookmark to their toolbars on 2 different devices, but before they have synced, seems something people may well hit - so I'm going to mark this as P1.

Comment 5

[Lina Butler [:lina]]

Attachment: Bug 1352233 - Try to preserve synced folder child order in case of conflicts.

For folders, we resolve conflicts by taking the chronologically newer
list, then appending any missing items from the older list. This
preserves the order of those missing items relative to each other, but
not relative to those appearing in the newer list.

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

Comment 6

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

Comment on attachment 8865643 [details]
Bug 1352233 - Try to preserve synced folder child order in case of conflicts.

https://reviewboard.mozilla.org/r/137258/#review141126

Looks fine with the followup, your call for the test nit, just seems like a lot of logging we might do.

::: services/sync/modules/engines/bookmarks.js:657
(Diff revision 1)
> +    // Some of the children in `order` might have been deleted, or moved to
> +    // other folders. `PlacesSyncUtils.bookmarks.order` ignores them.
> +    let order = newRecord.children ?
> +                [...newRecord.children, ...missingChildren] : missingChildren;
> +    this._log.debug("Recording children of " + localRecord.id, order);
> +    this._store._childrenToOrder[localRecord.id] = order;

As we discussed in IRC, can you file a followup bug for handling duped IDs in the children array? I think this is an issue for all items in childrenToOrder, so it doesn't seem within the scope of this bug.

::: services/sync/tests/unit/head_helpers.js:575
(Diff revision 1)
> +async function assertBookmarksTreeMatches(rootGuid, expected, message) {
> +  let root = await PlacesUtils.promiseBookmarksTree(rootGuid);
> +  let actual = bookmarkNodesToInfos(root.children);
> +
> +  _(`Expected structure for ${rootGuid}`, JSON.stringify(expected));
> +  _(`Actual structure for ${rootGuid}`, JSON.stringify(actual));

Can we avoid logging this much in success cases? Not sure this matters but if we have a lot of these asserts (which we seem to) it might be worth it to wrap in a try/catch, w/ log and rethrow if it fails.

Your call though.

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8865643 [details]
Bug 1352233 - Try to preserve synced folder child order in case of conflicts.

https://reviewboard.mozilla.org/r/137258/#review141126

> As we discussed in IRC, can you file a followup bug for handling duped IDs in the children array? I think this is an issue for all items in childrenToOrder, so it doesn't seem within the scope of this bug.

Filed bug 1366891.

> Can we avoid logging this much in success cases? Not sure this matters but if we have a lot of these asserts (which we seem to) it might be worth it to wrap in a try/catch, w/ log and rethrow if it fails.
> 
> Your call though.

Done!

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8865643 [details]
Bug 1352233 - Try to preserve synced folder child order in case of conflicts.

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

Comment 9

[Julie McCracken (:julie)]

Kit has rebased it and it will land today.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6bd9857f97d
Try to preserve synced folder child order in case of conflicts. r=tcsc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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