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.

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

(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!

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.

(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 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 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!

Kit has rebased it and it will land today.

Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f6bd9857f97d Try to preserve synced folder child order in case of conflicts. r=tcsc