Bug 1393904 - Ensure `insertTree` removes Sync tombstones for restored bookmarks. r=mak draft
authorKit Cambridge <kit@yakshaving.ninja>
Fri, 25 Aug 2017 12:04:22 -0700
changeset 661547 5dcd69f358b153144ebc5b83b8f9f25a8e1fe258
parent 659873 c959327c6b75cd4930a6ea087583c38b805e7524
child 730631 e5b37e72c768eec01ba7cff43681257559298526
push id78825
push userbmo:kit@mozilla.com
push dateFri, 08 Sep 2017 19:02:47 +0000
reviewersmak
bugs1393904
milestone57.0a1
Bug 1393904 - Ensure `insertTree` removes Sync tombstones for restored bookmarks. r=mak MozReview-Commit-ID: EbGybRbhWKJ
services/sync/tests/unit/test_telemetry.js
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/tests/unit/test_sync_utils.js
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -176,17 +176,16 @@ add_task(async function test_uploading()
     equal(ping.engines.length, 1);
     equal(ping.engines[0].name, "bookmarks");
     ok(!!ping.engines[0].outgoing);
     greater(ping.engines[0].outgoing[0].sent, 0)
     ok(!ping.engines[0].incoming);
 
     PlacesUtils.bookmarks.setItemTitle(bmk_id, "New Title");
 
-    await store.wipe();
     await engine.resetClient();
 
     ping = await sync_engine_and_validate_telem(engine, false);
     equal(ping.engines.length, 1);
     equal(ping.engines[0].name, "bookmarks");
     equal(ping.engines[0].outgoing.length, 1);
     ok(!!ping.engines[0].incoming);
 
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -90,16 +90,17 @@ async function promiseTagsFolderId() {
     "SELECT id FROM moz_bookmarks WHERE guid = :guid",
     { guid: Bookmarks.tagsGuid }
   );
   return gTagsFolderId = rows[0].getResultByName("id");
 }
 
 const MATCH_ANYWHERE_UNMODIFIED = Ci.mozIPlacesAutoComplete.MATCH_ANYWHERE_UNMODIFIED;
 const BEHAVIOR_BOOKMARK = Ci.mozIPlacesAutoComplete.BEHAVIOR_BOOKMARK;
+const SQLITE_MAX_VARIABLE_NUMBER = 999;
 
 var Bookmarks = Object.freeze({
   /**
    * Item's type constants.
    * These should stay consistent with nsINavBookmarksService.idl
    */
   TYPE_BOOKMARK: 1,
   TYPE_FOLDER: 2,
@@ -1433,16 +1434,25 @@ function insertBookmarkTree(items, sourc
                                     dateAdded, lastModified, guid,
                                     syncChangeCounter, syncStatus)
          VALUES (CASE WHEN :url ISNULL THEN NULL ELSE (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url) END, :type,
          (SELECT id FROM moz_bookmarks WHERE guid = :parentGuid),
          IFNULL(:index, (SELECT count(*) FROM moz_bookmarks WHERE parent = :rootId)),
          NULLIF(:title, ""), :date_added, :last_modified, :guid,
          :syncChangeCounter, :syncStatus)`, items);
 
+      // Remove stale tombstones for new items.
+      for (let chunk of chunkArray(items, SQLITE_MAX_VARIABLE_NUMBER)) {
+        await db.executeCached(
+          `DELETE FROM moz_bookmarks_deleted WHERE guid IN (${
+            new Array(chunk.length).fill("?").join(",")})`,
+          chunk.map(item => item.guid)
+        );
+      }
+
       await setAncestorsLastModified(db, parent.guid, lastAddedForParent,
                                      syncChangeDelta);
     });
 
     // We don't wait for the frecency calculation.
     updateFrecency(db, urls, true).catch(Cu.reportError);
 
     return items;
@@ -2379,8 +2389,19 @@ function adjustSeparatorsSyncCounter(db,
     `,
     {
       delta: syncChangeDelta,
       parent: parentId,
       start_index: startIndex,
       item_type: Bookmarks.TYPE_SEPARATOR
     });
 }
+
+function* chunkArray(array, chunkLength) {
+  if (array.length <= chunkLength) {
+    yield array;
+    return;
+  }
+  let startIndex = 0;
+  while (startIndex < array.length) {
+    yield array.slice(startIndex, startIndex += chunkLength);
+  }
+}
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -1942,16 +1942,19 @@ async function fetchQueryItem(db, bookma
     item.query = query;
   }
 
   return item;
 }
 
 function addRowToChangeRecords(row, changeRecords) {
   let syncId = BookmarkSyncUtils.guidToSyncId(row.getResultByName("guid"));
+  if (syncId in changeRecords) {
+    throw new Error(`Duplicate entry for ${syncId} in changeset`);
+  }
   let modifiedAsPRTime = row.getResultByName("modified");
   let modified = modifiedAsPRTime / MICROSECONDS_PER_SECOND;
   if (Number.isNaN(modified) || modified <= 0) {
     BookmarkSyncLog.error("addRowToChangeRecords: Invalid modified date for " +
                           syncId, modifiedAsPRTime);
     modified = 0;
   }
   changeRecords[syncId] = {
@@ -1971,17 +1974,17 @@ function addRowToChangeRecords(row, chan
  *        The Sqlite.jsm connection handle.
  * @return {Promise} resolved once all items have been fetched.
  * @resolves to an object containing records for changed bookmarks, keyed by
  *           the sync ID.
  */
 var pullSyncChanges = async function(db) {
   let changeRecords = {};
 
-  await db.executeCached(`
+  let rows = await db.executeCached(`
     WITH RECURSIVE
     syncedItems(id, guid, modified, syncChangeCounter, syncStatus) AS (
       SELECT b.id, b.guid, b.lastModified, b.syncChangeCounter, b.syncStatus
        FROM moz_bookmarks b
        WHERE b.guid IN ('menu________', 'toolbar_____', 'unfiled_____',
                         'mobile______')
       UNION ALL
       SELECT b.id, b.guid, b.lastModified, b.syncChangeCounter, b.syncStatus
@@ -1990,18 +1993,20 @@ var pullSyncChanges = async function(db)
     )
     SELECT guid, modified, syncChangeCounter, syncStatus, 0 AS tombstone
     FROM syncedItems
     WHERE syncChangeCounter >= 1
     UNION ALL
     SELECT guid, dateRemoved AS modified, 1 AS syncChangeCounter,
            :deletedSyncStatus, 1 AS tombstone
     FROM moz_bookmarks_deleted`,
-    { deletedSyncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL },
-    row => addRowToChangeRecords(row, changeRecords));
+    { deletedSyncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL });
+  for (let row of rows) {
+    addRowToChangeRecords(row, changeRecords);
+  }
 
   return changeRecords;
 };
 
 var touchSyncBookmark = async function(db, bookmarkItem) {
   if (BookmarkSyncLog.level <= Log.Level.Trace) {
     BookmarkSyncLog.trace(
       `touch: Reviving item "${bookmarkItem.guid}" and marking parent ` +
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -2225,17 +2225,17 @@ add_task(async function test_pullChanges
       PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
       PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
       PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
       PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN,
       PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN,
     ], "Pulling items restored from JSON backup should not mark them as syncing");
 
     let tombstones = await PlacesTestUtils.fetchSyncTombstones();
-    ok(tombstones.map(({ guid }) => guid), [syncedFolder.guid],
+    deepEqual(tombstones.map(({ guid }) => guid), [syncedFolder.guid],
       "Tombstones should exist after restoring from JSON backup");
 
     await PlacesSyncUtils.bookmarks.markChangesAsSyncing(changes);
     let normalFields = await PlacesTestUtils.fetchBookmarkSyncFields(
       PlacesUtils.bookmarks.menuGuid, PlacesUtils.bookmarks.toolbarGuid,
       PlacesUtils.bookmarks.unfiledGuid, "NnvGl3CRA4hC", "APzP8MupzA8l");
     ok(normalFields.every(field =>
       field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NORMAL
@@ -2904,8 +2904,83 @@ add_task(async function test_ensureMobil
   do_print("We shouldn't track the query or the left pane root");
 
   let changes = await PlacesSyncUtils.bookmarks.pullChanges();
   ok(!(queryGuid in changes), "Should not track mobile query");
 
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
+
+add_task(async function test_remove_stale_tombstones() {
+  do_print("Insert and delete synced bookmark");
+  {
+    await PlacesUtils.bookmarks.insert({
+      guid: "bookmarkAAAA",
+      parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+      url: "http://example.com/a",
+      title: "A",
+      source: PlacesUtils.bookmarks.SOURCES.SYNC,
+    });
+    await PlacesUtils.bookmarks.remove("bookmarkAAAA");
+    let tombstones = await PlacesTestUtils.fetchSyncTombstones();
+    deepEqual(tombstones.map(({ guid }) => guid), ["bookmarkAAAA"],
+      "Should store tombstone for deleted synced bookmark");
+  }
+
+  do_print("Reinsert deleted bookmark");
+  {
+    // Different parent, URL, and title, but same GUID.
+    await PlacesUtils.bookmarks.insert({
+      guid: "bookmarkAAAA",
+      parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+      url: "http://example.com/a-restored",
+      title: "A (Restored)",
+    });
+
+    let tombstones = await PlacesTestUtils.fetchSyncTombstones();
+    deepEqual(tombstones, [],
+      "Should remove tombstone for reinserted bookmark");
+  }
+
+  do_print("Insert tree and erase everything");
+  {
+    await PlacesUtils.bookmarks.insertTree({
+      guid: PlacesUtils.bookmarks.menuGuid,
+      source: PlacesUtils.bookmarks.SOURCES.SYNC,
+      children: [{
+        guid: "bookmarkBBBB",
+        url: "http://example.com/b",
+        title: "B",
+      }, {
+        guid: "bookmarkCCCC",
+        url: "http://example.com/c",
+        title: "C",
+      }],
+    });
+    await PlacesUtils.bookmarks.eraseEverything();
+    let tombstones = await PlacesTestUtils.fetchSyncTombstones();
+    deepEqual(tombstones.map(({ guid }) => guid).sort(), ["bookmarkBBBB",
+      "bookmarkCCCC"], "Should store tombstones after erasing everything");
+  }
+
+  do_print("Reinsert tree");
+  {
+    await PlacesUtils.bookmarks.insertTree({
+      guid: PlacesUtils.bookmarks.mobileGuid,
+      children: [{
+        guid: "bookmarkBBBB",
+        url: "http://example.com/b",
+        title: "B",
+      }, {
+        guid: "bookmarkCCCC",
+        url: "http://example.com/c",
+        title: "C",
+      }],
+    });
+    let tombstones = await PlacesTestUtils.fetchSyncTombstones();
+    deepEqual(tombstones.map(({ guid }) => guid).sort(), [],
+      "Should remove tombstones after reinserting tree");
+  }
+
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});