Bug 1352233 - Try to preserve synced folder child order in case of conflicts. r=tcsc
authorKit Cambridge <kit@yakshaving.ninja>
Fri, 28 Apr 2017 14:08:11 -0700
changeset 360046 f6bd9857f97dc3162b86e01600d755e62f4b63ab
parent 360045 a0e257e346ccf3c1db332ec5903241f4eeb9a7ee
child 360047 2ede14818ea0e817522787f2e5afe61d35370c3a
push id43187
push userkcambridge@mozilla.com
push dateTue, 23 May 2017 00:25:38 +0000
treeherderautoland@f6bd9857f97d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc
bugs1352233
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1352233 - Try to preserve synced folder child order in case of conflicts. r=tcsc 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. MozReview-Commit-ID: 6rimXpV7vLg
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/head_helpers.js
services/sync/tests/unit/test_bookmark_order.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -1360,19 +1360,19 @@ SyncEngine.prototype = {
    * @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.
+   * Used by bookmark sync to merge folder child orders.
    */
-  beforeRecordDiscard(record) {
+  beforeRecordDiscard(localRecord, remoteRecord, remoteIsNewer) {
   },
 
   // 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).
@@ -1575,17 +1575,17 @@ 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);
+      this.beforeRecordDiscard(localRecord, item, remoteIsNewer);
     }
     return remoteIsNewer;
   },
 
   // Upload outgoing records.
   _uploadOutgoing() {
     this._log.trace("Uploading local changes to server.");
 
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -629,25 +629,37 @@ 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;
+  beforeRecordDiscard(localRecord, remoteRecord, remoteIsNewer) {
+    if (localRecord.type != "folder" || remoteRecord.type != "folder") {
+      return;
     }
+    // Resolve child order 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
+    // the items that appear in the newer list.
+    let newRecord = remoteIsNewer ? remoteRecord : localRecord;
+    let newChildren = new Set(newRecord.children);
+
+    let oldChildren = (remoteIsNewer ? localRecord : remoteRecord).children;
+    let missingChildren = oldChildren ? oldChildren.filter(
+      child => !newChildren.has(child)) : [];
+
+    // 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;
   },
 
   getValidator() {
     return new BookmarkValidator();
   }
 };
 
 function BookmarksStore(name, engine) {
--- a/services/sync/tests/unit/head_helpers.js
+++ b/services/sync/tests/unit/head_helpers.js
@@ -11,16 +11,17 @@
 /* global Service */
 
 Cu.import("resource://services-common/async.js");
 Cu.import("resource://testing-common/services/common/utils.js");
 Cu.import("resource://testing-common/PlacesTestUtils.jsm");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
+Cu.import("resource://gre/modules/ObjectUtils.jsm");
 
 XPCOMUtils.defineLazyGetter(this, "SyncPingSchema", function() {
   let ns = {};
   Cu.import("resource://gre/modules/FileUtils.jsm", ns);
   let stream = Cc["@mozilla.org/network/file-input-stream;1"]
                .createInstance(Ci.nsIFileInputStream);
   let jsonReader = Cc["@mozilla.org/dom/json;1"]
                    .createInstance(Components.interfaces.nsIJSON);
@@ -598,8 +599,40 @@ async function addVisit(suffix, referrer
     visitDate: Date.now() * 1000,
     transition,
     referrer,
   });
   await visitAddedPromise;
 
   return uri;
 }
+
+function bookmarkNodesToInfos(nodes) {
+  return nodes.map(node => {
+    let info = {
+      guid: node.guid,
+      index: node.index,
+    };
+    if (node.children) {
+      info.children = bookmarkNodesToInfos(node.children);
+    }
+    if (node.annos) {
+      let orphanAnno = node.annos.find(anno =>
+        anno.name == "sync/parent"
+      );
+      if (orphanAnno) {
+        info.requestedParent = orphanAnno.value;
+      }
+    }
+    return info;
+  });
+}
+
+async function assertBookmarksTreeMatches(rootGuid, expected, message) {
+  let root = await PlacesUtils.promiseBookmarksTree(rootGuid);
+  let actual = bookmarkNodesToInfos(root.children);
+
+  if (!ObjectUtils.deepEqual(actual, expected)) {
+    _(`Expected structure for ${rootGuid}`, JSON.stringify(expected));
+    _(`Actual structure for ${rootGuid}`, JSON.stringify(actual));
+    throw new Assert.constructor.AssertionError({ actual, expected, message });
+  }
+}
--- a/services/sync/tests/unit/test_bookmark_order.js
+++ b/services/sync/tests/unit/test_bookmark_order.js
@@ -1,51 +1,257 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 _("Making sure after processing incoming bookmarks, they show up in the right order");
+Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/util.js");
+Cu.import("resource://testing-common/services/sync/utils.js");
 
-var check = async function(expected, message) {
-  let root = await PlacesUtils.promiseBookmarksTree();
+function run_test() {
+  Svc.Prefs.set("log.logger.engine.bookmarks", "Trace");
+  initTestLogging("Trace");
+  Log.repository.getLogger("Sqlite").level = Log.Level.Info;
+
+  run_next_test();
+}
+
+function serverForFoo(engine) {
+  generateNewKeys(Service.collectionKeys);
+
+  let clientsEngine = Service.clientsEngine;
+  return serverForUsers({"foo": "password"}, {
+    meta: {
+      global: {
+        syncID: Service.syncID,
+        storageVersion: STORAGE_VERSION,
+        engines: {
+          clients: {
+            version: clientsEngine.version,
+            syncID: clientsEngine.syncID,
+          },
+          [engine.name]: {
+            version: engine.version,
+            syncID: engine.syncID,
+          },
+        },
+      },
+    },
+    crypto: {
+      keys: encryptPayload({
+        id: "keys",
+        // Generate a fake default key bundle to avoid resetting the client
+        // before the first sync.
+        default: [
+          Svc.Crypto.generateRandomKey(),
+          Svc.Crypto.generateRandomKey(),
+        ],
+      }),
+    },
+    [engine.name]: {},
+  });
+}
+
+async function resolveConflict(engine, collection, timestamp, buildTree,
+                               message) {
+  let guids = {
+    // These items don't exist on the server.
+    fx: Utils.makeGUID(),
+    nightly: Utils.makeGUID(),
+    support: Utils.makeGUID(),
+    customize: Utils.makeGUID(),
+
+    // These exist on the server, but in a different order, and `res`
+    // has completely different children.
+    res: Utils.makeGUID(),
+    tb: Utils.makeGUID(),
+
+    // These don't exist locally.
+    bz: Utils.makeGUID(),
+    irc: Utils.makeGUID(),
+    mdn: Utils.makeGUID(),
+  };
+
+  await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.menuGuid,
+    children: [{
+      guid: guids.fx,
+      title: "Get Firefox!",
+      url: "http://getfirefox.com/",
+    }, {
+      guid: guids.res,
+      title: "Resources",
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      children: [{
+        guid: guids.nightly,
+        title: "Nightly",
+        url: "https://nightly.mozilla.org/",
+      }, {
+        guid: guids.support,
+        title: "Support",
+        url: "https://support.mozilla.org/",
+      }, {
+        guid: guids.customize,
+        title: "Customize",
+        url: "https://mozilla.org/firefox/customize/",
+      }],
+    }, {
+      title: "Get Thunderbird!",
+      guid: guids.tb,
+      url: "http://getthunderbird.com/",
+    }],
+  });
 
-  let bookmarks = (function mapTree(children) {
-    return children.map(child => {
-      let result = {
-        guid: child.guid,
-        index: child.index,
-      };
-      if (child.children) {
-        result.children = mapTree(child.children);
-      }
-      if (child.annos) {
-        let orphanAnno = child.annos.find(
-          anno => anno.name == "sync/parent");
-        if (orphanAnno) {
-          result.requestedParent = orphanAnno.value;
-        }
-      }
-      return result;
-    });
-  }(root.children));
+  let serverRecords = [{
+    id: "menu",
+    type: "folder",
+    title: "Bookmarks Menu",
+    parentid: "places",
+    children: [guids.tb, guids.res],
+  }, {
+    id: guids.tb,
+    type: "bookmark",
+    parentid: "menu",
+    bmkUri: "http://getthunderbird.com/",
+    title: "Get Thunderbird!",
+  }, {
+    id: guids.res,
+    type: "folder",
+    parentid: "menu",
+    title: "Resources",
+    children: [guids.irc, guids.bz, guids.mdn],
+  }, {
+    id: guids.bz,
+    type: "bookmark",
+    parentid: guids.res,
+    bmkUri: "https://bugzilla.mozilla.org/",
+    title: "Bugzilla",
+  }, {
+    id: guids.mdn,
+    type: "bookmark",
+    parentid: guids.res,
+    bmkUri: "https://developer.mozilla.org/",
+    title: "MDN",
+  }, {
+    id: guids.irc,
+    type: "bookmark",
+    parentid: guids.res,
+    bmkUri: "ircs://irc.mozilla.org/nightly",
+    title: "IRC",
+  }];
+  for (let record of serverRecords) {
+    collection.insert(record.id, encryptPayload(record), timestamp);
+  }
+
+  await sync_engine_and_validate_telem(engine, false);
+
+  let expectedTree = buildTree(guids);
+  await assertBookmarksTreeMatches(PlacesUtils.bookmarks.menuGuid,
+    expectedTree, message);
+}
+
+add_task(async function test_local_order_newer() {
+  let engine = Service.engineManager.get("bookmarks");
+
+  let server = serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
 
-  _("Checking if the bookmark structure is", JSON.stringify(expected));
-  _("Got bookmarks:", JSON.stringify(bookmarks));
-  deepEqual(bookmarks, expected);
-};
+  try {
+    let collection = server.user("foo").collection("bookmarks");
+    let serverModified = Date.now() / 1000 - 120;
+    await resolveConflict(engine, collection, serverModified, guids => [{
+      guid: guids.fx,
+      index: 0,
+    }, {
+      guid: guids.res,
+      index: 1,
+      children: [{
+        guid: guids.nightly,
+        index: 0,
+      }, {
+        guid: guids.support,
+        index: 1,
+      }, {
+        guid: guids.customize,
+        index: 2,
+      }, {
+        guid: guids.irc,
+        index: 3,
+      }, {
+        guid: guids.bz,
+        index: 4,
+      }, {
+        guid: guids.mdn,
+        index: 5,
+      }],
+    }, {
+      guid: guids.tb,
+      index: 2,
+    }], "Should use local order as base if remote is older");
+  } finally {
+    engine.wipeClient();
+    Service.startOver();
+    await promiseStopServer(server);
+  }
+});
+
+add_task(async function test_remote_order_newer() {
+  let engine = Service.engineManager.get("bookmarks");
+
+  let server = serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  try {
+    let collection = server.user("foo").collection("bookmarks");
+    let serverModified = Date.now() / 1000 + 120;
+    await resolveConflict(engine, collection, serverModified, guids => [{
+      guid: guids.tb,
+      index: 0,
+    }, {
+      guid: guids.res,
+      index: 1,
+      children: [{
+        guid: guids.irc,
+        index: 0,
+      }, {
+        guid: guids.bz,
+        index: 1,
+      }, {
+        guid: guids.mdn,
+        index: 2,
+      }, {
+        guid: guids.nightly,
+        index: 3,
+      }, {
+        guid: guids.support,
+        index: 4,
+      }, {
+        guid: guids.customize,
+        index: 5,
+      }],
+    }, {
+      guid: guids.fx,
+      index: 2,
+    }], "Should use remote order as base if local is older");
+  } finally {
+    engine.wipeClient();
+    Service.startOver();
+    await promiseStopServer(server);
+  }
+});
 
 add_task(async function test_bookmark_order() {
-  let store = new BookmarksEngine(Service)._store;
-  initTestLogging("Trace");
+  let engine = new BookmarksEngine(Service);
+  let store = engine._store;
 
   _("Starting with a clean slate of no bookmarks");
   store.wipe();
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     // Index 2 is the tags root. (Root indices depend on the order of the
     // `CreateRoot` calls in `Database::CreateBookmarkRoots`).
@@ -79,17 +285,17 @@ add_task(async function test_bookmark_or
     store._childrenToOrder = {};
     store.applyIncoming(record);
     store._orderChildren();
     delete store._childrenToOrder;
   }
   let id10 = "10_aaaaaaaaa";
   _("basic add first bookmark");
   apply(bookmark(id10, ""));
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -99,17 +305,17 @@ add_task(async function test_bookmark_or
     }],
   }, {
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "basic add first bookmark");
   let id20 = "20_aaaaaaaaa";
   _("basic append behind 10");
   apply(bookmark(id20, ""));
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -126,17 +332,17 @@ add_task(async function test_bookmark_or
   }], "basic append behind 10");
 
   let id31 = "31_aaaaaaaaa";
   let id30 = "f30_aaaaaaaa";
   _("basic create in folder");
   apply(bookmark(id31, id30));
   let f30 = folder(id30, "", [id31]);
   apply(f30);
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -158,17 +364,17 @@ add_task(async function test_bookmark_or
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "basic create in folder");
 
   let id41 = "41_aaaaaaaaa";
   let id40 = "f40_aaaaaaaa";
   _("insert missing parent -> append to unfiled");
   apply(bookmark(id41, id40));
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -194,17 +400,17 @@ add_task(async function test_bookmark_or
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "insert missing parent -> append to unfiled");
 
   let id42 = "42_aaaaaaaaa";
 
   _("insert another missing parent -> append");
   apply(bookmark(id42, id40));
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -233,17 +439,17 @@ add_task(async function test_bookmark_or
   }, {
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "insert another missing parent -> append");
 
   _("insert folder -> move children and followers");
   let f40 = folder(id40, "", [id41, id42]);
   apply(f40);
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -274,17 +480,17 @@ add_task(async function test_bookmark_or
   }, {
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "insert folder -> move children and followers");
 
   _("Moving 41 behind 42 -> update f40");
   f40.children = [id42, id41];
   apply(f40);
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -315,17 +521,17 @@ add_task(async function test_bookmark_or
   }, {
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "Moving 41 behind 42 -> update f40");
 
   _("Moving 10 back to front -> update 10, 20");
   f40.children = [id41, id42];
   apply(f40);
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -355,17 +561,17 @@ add_task(async function test_bookmark_or
     }],
   }, {
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "Moving 10 back to front -> update 10, 20");
 
   _("Moving 20 behind 42 in f40 -> update 50");
   apply(bookmark(id20, id40));
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -397,17 +603,17 @@ add_task(async function test_bookmark_or
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "Moving 20 behind 42 in f40 -> update 50");
 
   _("Moving 10 in front of 31 in f30 -> update 10, f30");
   apply(bookmark(id10, id30));
   f30.children = [id10, id31];
   apply(f30);
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -439,17 +645,17 @@ add_task(async function test_bookmark_or
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "Moving 10 in front of 31 in f30 -> update 10, f30");
 
   _("Moving 20 from f40 to f30 -> update 20, f30");
   apply(bookmark(id20, id30));
   f30.children = [id10, id20, id31];
   apply(f30);
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -481,17 +687,17 @@ add_task(async function test_bookmark_or
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "Moving 20 from f40 to f30 -> update 20, f30");
 
   _("Move 20 back to front -> update 20, f30");
   apply(bookmark(id20, ""));
   f30.children = [id10, id31];
   apply(f30);
-  await check([{
+  await assertBookmarksTreeMatches("", [{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
@@ -519,9 +725,11 @@ add_task(async function test_bookmark_or
       guid: id20,
       index: 2,
     }],
   }, {
     guid: PlacesUtils.bookmarks.mobileGuid,
     index: 4,
   }], "Move 20 back to front -> update 20, f30");
 
+  engine.resetClient();
+  await engine.finalize();
 });