Bug 1349703 - (part 1) Fix issue where sync would fail misreconcile special folders in fresh syncs r?kitcambridge draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 23 Mar 2017 16:32:40 -0400
changeset 551909 ff1dacc44cc91bac8824d59fecfd22be0c8f6bd9
parent 503869 c309f93f5c33eaaff2294ef70ab18158d3fbb25d
child 551910 be89e71076bbd0e6815597312e5a1cbf21d4b3d0
push id51196
push userbmo:tchiovoloni@mozilla.com
push dateMon, 27 Mar 2017 17:35:45 +0000
reviewerskitcambridge
bugs1349703
milestone55.0a1
Bug 1349703 - (part 1) Fix issue where sync would fail misreconcile special folders in fresh syncs r?kitcambridge MozReview-Commit-ID: Izy9JgMYpom
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -1355,16 +1355,23 @@ SyncEngine.prototype = {
    * to have a different GUID
    *
    * @return GUID of the similar item; falsy otherwise
    */
   _findDupe(item) {
     // By default, assume there's no dupe items for the engine
   },
 
+  /**
+   * Called before a remote record is discarded due to failed reconciliation.
+   * Used by bookmark sync to note the child ordering of special folders.
+   */
+  beforeRecordDiscard(record) {
+  },
+
   // Called when the server has a record marked as deleted, but locally we've
   // changed it more recently than the deletion. If we return false, the
   // record will be deleted locally. If we return true, we'll reupload the
   // record to the server -- any extra work that's needed as part of this
   // process should be done at this point (such as mark the record's parent
   // for reuploading in the case of bookmarks).
   _shouldReviveRemotelyDeletedRecord(remoteItem) {
     return true;
@@ -1564,16 +1571,19 @@ SyncEngine.prototype = {
     }
 
     // At this point, records are different and the local record is modified.
     // We resolve conflicts by record age, where the newest one wins. This does
     // result in data loss and should be handled by giving the engine an
     // opportunity to merge the records. Bug 720592 tracks this feature.
     this._log.warn("DATA LOSS: Both local and remote changes to record: " +
                    item.id);
+    if (!remoteIsNewer) {
+      this.beforeRecordDiscard(item);
+    }
     return remoteIsNewer;
   },
 
   // Upload outgoing records.
   _uploadOutgoing() {
     this._log.trace("Uploading local changes to server.");
 
     // collection we'll upload
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -625,16 +625,27 @@ BookmarksEngine.prototype = {
 
   // Cleans up the Places root, reading list items (ignored in bug 762118,
   // removed in bug 1155684), and pinned sites.
   _shouldDeleteRemotely(incomingItem) {
     return FORBIDDEN_INCOMING_IDS.includes(incomingItem.id) ||
            FORBIDDEN_INCOMING_PARENT_IDS.includes(incomingItem.parentid);
   },
 
+  beforeRecordDiscard(record) {
+    let isSpecial = PlacesSyncUtils.bookmarks.ROOTS.includes(record.id);
+    if (isSpecial && record.children && !this._store._childrenToOrder[record.id]) {
+      if (this._modified.getStatus(record.id) != PlacesUtils.bookmarks.SYNC_STATUS.NEW) {
+        return;
+      }
+      this._log.debug("Recording children of " + record.id + " as " + JSON.stringify(record.children));
+      this._store._childrenToOrder[record.id] = record.children;
+    }
+  },
+
   getValidator() {
     return new BookmarkValidator();
   }
 };
 
 function BookmarksStore(name, engine) {
   Store.call(this, name, engine);
   this._itemsToDelete = new Set();
@@ -1117,16 +1128,24 @@ BookmarksTracker.prototype = {
 class BookmarksChangeset extends Changeset {
   constructor() {
     super();
     // Weak changes are part of the changeset, but don't bump the change
     // counter, and aren't persisted anywhere.
     this.weakChanges = {};
   }
 
+  getStatus(id) {
+    let change = this.changes[id];
+    if (!change) {
+      return PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN;
+    }
+    return change.status;
+  }
+
   getModifiedTimestamp(id) {
     let change = this.changes[id];
     if (change) {
       // Pretend the change doesn't exist if we've already synced or
       // reconciled it.
       return change.synced ? Number.NaN : change.modified;
     }
     if (this.weakChanges[id]) {