Bug 1393904 - Ensure insertTree removes Sync tombstones for restore…

…d bookmarks. r=mak

MozReview-Commit-ID: EbGybRbhWKJ

services/sync/tests/unit/test_telemetry.js

@@ -181,7 +181,6 @@ add_task(async function test_uploading() {
 
     PlacesUtils.bookmarks.setItemTitle(bmk_id, "New Title");
 
-    await store.wipe();
     await engine.resetClient();
 
     ping = await sync_engine_and_validate_telem(engine, false);

toolkit/components/places/Bookmarks.jsm

@@ -95,6 +95,7 @@ async function promiseTagsFolderId() {
 
 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({
   /**
@@ -1438,6 +1439,15 @@ function insertBookmarkTree(items, source, parent, urls, lastAddedForParent) {
          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);
     });
@@ -2384,3 +2394,14 @@ function adjustSeparatorsSyncCounter(db, parentId, startIndex, syncChangeDelta)
       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);
+  }
+}

toolkit/components/places/PlacesSyncUtils.jsm

@@ -1947,6 +1947,9 @@ async function fetchQueryItem(db, bookmarkItem) {
 
 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) {
@@ -1976,7 +1979,7 @@ function addRowToChangeRecords(row, changeRecords) {
 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
@@ -1995,8 +1998,10 @@ var pullSyncChanges = async function(db) {
     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;
 };

toolkit/components/places/tests/unit/test_sync_utils.js

@@ -2230,7 +2230,7 @@ add_task(async function test_pullChanges_restore_json_tracked() {
     ], "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);
@@ -2909,3 +2909,78 @@ add_task(async function test_ensureMobileQuery() {
   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();
+});