Bug 1408551 - Set hasDupe on outgoing bookmarks when using the buffer r=kitcambridge
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 22 Feb 2018 19:38:55 -0500
changeset 461354 cca108d5a695dd00aa0e527c6764ec041f7d69e1
parent 461353 7bc4e7d466a3b7fd258b89b1fcf9f41ebf9ff8f2
child 461355 640e1784f2dbd5a71f08cc32195183bdfdad6f42
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskitcambridge
bugs1408551
milestone60.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 1408551 - Set hasDupe on outgoing bookmarks when using the buffer r=kitcambridge MozReview-Commit-ID: BU5Hbkn5OKH
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_deletion.js
toolkit/components/places/tests/sync/test_bookmark_kinds.js
toolkit/components/places/tests/sync/test_bookmark_value_changes.js
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -733,16 +733,32 @@ BufferedBookmarksEngine.prototype = {
     this._modified.replace(recordsToUpload);
   },
 
   async _reconcile(item) {
     return true;
   },
 
   async _createRecord(id) {
+    let record = await this._doCreateRecord(id);
+    if (!record.deleted) {
+      // Set hasDupe on all (non-deleted) records since we don't use it (e.g.
+      // the buffered engine doesn't), and we want to minimize the risk of older
+      // clients corrupting records. Note that the SyncedBookmarksMirror sets it
+      // for all records that it created, but we would like to ensure that
+      // weakly uploaded records are marked as hasDupe as well.
+      record.hasDupe = true;
+    }
+    return record;
+  },
+
+  async _doCreateRecord(id) {
+    if (this._needWeakUpload.has(id)) {
+      return this._store.createRecord(id, this.name);
+    }
     let change = this._modified.changes[id];
     if (!change) {
       this._log.error("Creating record for item ${id} not in strong " +
                       "changeset", { id });
       throw new TypeError("Can't create record for unchanged item");
     }
     let record = this._recordFromCleartext(id, change.cleartext);
     record.sortindex = await this._store._calculateIndex(record);
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -815,16 +815,69 @@ add_bookmark_test(async function test_sy
 
 
 
   } finally {
     await cleanup(engine, server);
   }
 });
 
+add_task(async function test_buffer_hasDupe() {
+  await Service.recordManager.clearCache();
+  await PlacesSyncUtils.bookmarks.reset();
+  let engine = new BufferedBookmarksEngine(Service);
+  await engine.initialize();
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+  let collection = server.user("foo").collection("bookmarks");
+  engine._tracker.start(); // We skip usual startup...
+  try {
+    let guid1 = Utils.makeGUID();
+    let guid2 = Utils.makeGUID();
+    await PlacesUtils.bookmarks.insert({
+      guid: guid1,
+      parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+      url: "https://www.example.com",
+      title: "example.com",
+    });
+    await PlacesUtils.bookmarks.insert({
+      guid: guid2,
+      parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+      url: "https://www.example.com",
+      title: "example.com",
+    });
+
+    await sync_engine_and_validate_telem(engine, false);
+    // Make sure we set hasDupe on outgoing records
+    Assert.ok(collection.payloads().every(payload => payload.hasDupe));
+
+    await PlacesUtils.bookmarks.remove(guid1);
+
+    // Make sure it works for weakly uploaded records
+    engine.addForWeakUpload(guid2);
+
+    await sync_engine_and_validate_telem(engine, false);
+
+    let tombstone = JSON.parse(JSON.parse(collection.payload(guid1)).ciphertext);
+    // We shouldn't set hasDupe on tombstones.
+    Assert.ok(tombstone.deleted);
+    Assert.ok(!tombstone.hasDupe);
+
+    let record = JSON.parse(JSON.parse(collection.payload(guid2)).ciphertext);
+    // We should set hasDupe on weakly uploaded records.
+    Assert.ok(!record.deleted);
+    Assert.ok(record.hasDupe,
+      "Buffered bookmark engine should set hasDupe for weakly uploaded records.");
+
+    await sync_engine_and_validate_telem(engine, false);
+  } finally {
+    await cleanup(engine, server);
+  }
+});
+
 // Bug 890217.
 add_task(async function test_sync_imap_URLs() {
   await Service.recordManager.clearCache();
   await PlacesSyncUtils.bookmarks.reset();
   let engine = new BookmarksEngine(Service);
   await engine.initialize();
   let server = await serverForFoo(engine);
   await SyncTestingInfrastructure(server);
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -1664,18 +1664,22 @@ class SyncedBookmarksMirror {
             let queryCleartext = {
               id: recordId,
               type: "query",
               // We ignore `parentid` and use the parent's `children`, but older
               // Desktops and Android use `parentid` as the canonical parent.
               // iOS is stricter and requires both `children` and `parentid` to
               // match.
               parentid: parentRecordId,
-              // Older Desktops use `hasDupe` and `parentName` for deduping.
-              hasDupe: false,
+              // Older Desktops use `hasDupe` (along with `parentName` for
+              // deduping), if hasDupe is true, then they won't attempt deduping
+              // (since they believe that a duplicate for this record should
+              // exist). We set it to true to prevent them from applying their
+              // deduping logic.
+              hasDupe: true,
               parentName: row.getResultByName("parentTitle"),
               dateAdded,
               bmkUri: row.getResultByName("url"),
               title: row.getResultByName("title"),
               queryId: row.getResultByName("smartBookmarkName"),
               folderName: row.getResultByName("tagFolderName"),
             };
             let description = row.getResultByName("description");
@@ -1686,17 +1690,17 @@ class SyncedBookmarksMirror {
               syncChangeCounter, queryCleartext);
             continue;
           }
 
           let bookmarkCleartext = {
             id: recordId,
             type: "bookmark",
             parentid: parentRecordId,
-            hasDupe: false,
+            hasDupe: true,
             parentName: row.getResultByName("parentTitle"),
             dateAdded,
             bmkUri: row.getResultByName("url"),
             title: row.getResultByName("title"),
           };
           let description = row.getResultByName("description");
           if (description) {
             bookmarkCleartext.description = description;
@@ -1722,17 +1726,17 @@ class SyncedBookmarksMirror {
           let feedURLHref = row.getResultByName("feedURL");
           if (feedURLHref) {
             // Places stores livemarks as folders with feed and site URL annos.
             // See bug 1072833 for discussion about changing them to queries.
             let livemarkCleartext = {
               id: recordId,
               type: "livemark",
               parentid: parentRecordId,
-              hasDupe: false,
+              hasDupe: true,
               parentName: row.getResultByName("parentTitle"),
               dateAdded,
               title: row.getResultByName("title"),
               feedUri: feedURLHref,
             };
             let description = row.getResultByName("description");
             if (description) {
               livemarkCleartext.description = description;
@@ -1745,17 +1749,17 @@ class SyncedBookmarksMirror {
               syncChangeCounter, livemarkCleartext);
             continue;
           }
 
           let folderCleartext = {
             id: recordId,
             type: "folder",
             parentid: parentRecordId,
-            hasDupe: false,
+            hasDupe: true,
             parentName: row.getResultByName("parentTitle"),
             dateAdded,
             title: row.getResultByName("title"),
           };
           let description = row.getResultByName("description");
           if (description) {
             folderCleartext.description = description;
           }
@@ -1773,17 +1777,17 @@ class SyncedBookmarksMirror {
           continue;
         }
 
         case PlacesUtils.bookmarks.TYPE_SEPARATOR: {
           let separatorCleartext = {
             id: recordId,
             type: "separator",
             parentid: parentRecordId,
-            hasDupe: false,
+            hasDupe: true,
             parentName: row.getResultByName("parentTitle"),
             dateAdded,
             // Older Desktops use `pos` for deduping.
             pos: row.getResultByName("position"),
           };
           changeRecords[recordId] = new BookmarkChangeRecord(
             syncChangeCounter, separatorCleartext);
           continue;
--- a/toolkit/components/places/tests/sync/test_bookmark_deletion.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_deletion.js
@@ -628,17 +628,17 @@ add_task(async function test_nonexistent
     menu: {
       tombstone: false,
       counter: 2,
       synced: false,
       cleartext: {
         id: "menu",
         type: "folder",
         parentid: "places",
-        hasDupe: false,
+        hasDupe: true,
         parentName: "",
         dateAdded: menuDateAdded.getTime(),
         title: BookmarksMenuTitle,
         children: [],
       },
     },
   });
 
--- a/toolkit/components/places/tests/sync/test_bookmark_kinds.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_kinds.js
@@ -92,48 +92,48 @@ add_task(async function test_livemarks()
       livemarkDDDD: {
         tombstone: false,
         counter: 1,
         synced: false,
         cleartext: {
           id: "livemarkDDDD",
           type: "livemark",
           parentid: "menu",
-          hasDupe: false,
+          hasDupe: true,
           parentName: BookmarksMenuTitle,
           dateAdded: PlacesUtils.toDate(livemarkD.dateAdded).getTime(),
           title: "D",
           feedUri: site + "/feed/d",
           siteUri: site + "/site/d",
         },
       },
       menu: {
         tombstone: false,
         counter: 2,
         synced: false,
         cleartext: {
           id: "menu",
           type: "folder",
           parentid: "places",
-          hasDupe: false,
+          hasDupe: true,
           parentName: "",
           dateAdded: menuInfo.dateAdded.getTime(),
           title: menuInfo.title,
           children: ["livemarkAAAA", "livemarkDDDD"],
         },
       },
       toolbar: {
         tombstone: false,
         counter: 1,
         synced: false,
         cleartext: {
           id: "toolbar",
           type: "folder",
           parentid: "places",
-          hasDupe: false,
+          hasDupe: true,
           parentName: "",
           dateAdded: toolbarInfo.dateAdded.getTime(),
           title: toolbarInfo.title,
           children: ["livemarkCCCC", "livemarkB111"],
         },
       },
     }, "Should upload new local livemark A");
 
--- a/toolkit/components/places/tests/sync/test_bookmark_value_changes.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_value_changes.js
@@ -79,33 +79,33 @@ add_task(async function test_value_combo
     bzBmk_______: {
       tombstone: false,
       counter: 3,
       synced: false,
       cleartext: {
         id: "bzBmk_______",
         type: "bookmark",
         parentid: "toolbar",
-        hasDupe: false,
+        hasDupe: true,
         parentName: BookmarksToolbarTitle,
         dateAdded: bzBmk.dateAdded.getTime(),
         bmkUri: "https://bugzilla.mozilla.org/",
         title: "Bugzilla",
         tags: ["new", "tag"],
       },
     },
     toolbar: {
       tombstone: false,
       counter: 2,
       synced: false,
       cleartext: {
         id: "toolbar",
         type: "folder",
         parentid: "places",
-        hasDupe: false,
+        hasDupe: true,
         parentName: "",
         dateAdded: menuInfo.dateAdded.getTime(),
         title: BookmarksToolbarTitle,
         children: ["fxBmk_______", "tFolder_____", "bzBmk_______"],
       },
     },
   }, "Should upload new local bookmarks and parents");