Bug 1343103 - (3/3) WIP DONOTLAND Implement tombstone syncing for mirror engine draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 19 Apr 2018 15:00:43 -0700
changeset 785894 31215f44a726746938988b52410ebcb2033deb35
parent 785893 0457437ed18c8ed7ad49cfdb07fc633dd747a363
push id107354
push userbmo:tchiovoloni@mozilla.com
push dateFri, 20 Apr 2018 20:14:41 +0000
bugs1343103
milestone61.0a1
Bug 1343103 - (3/3) WIP DONOTLAND Implement tombstone syncing for mirror engine MozReview-Commit-ID: 5XHjdQrKTam
services/sync/tests/unit/test_bookmark_tombstones.js
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_deletion.js
toolkit/components/places/tests/sync/test_sync_utils.js
--- a/services/sync/tests/unit/test_bookmark_tombstones.js
+++ b/services/sync/tests/unit/test_bookmark_tombstones.js
@@ -68,16 +68,17 @@ add_task(async function test_general() {
     let {id: id0} = collection.insertRecord(makeTombstone());
     let {id: id1} = collection.insertRecord(makeTombstone());
     let {id: id2} = collection.insertRecord(makeTombstone());
 
     await sync_engine_and_validate_telem(engine, false);
 
     _("Make sure they're synced and not removed from the server");
 
+
     let tombstones = await PlacesSyncUtils.bookmarks.promiseAllTombstones();
 
     deepEqual([id0, id1, id2].sort(), tombstones.sort());
     ok(collection.cleartext(id0).deleted);
     ok(collection.cleartext(id1).deleted);
     ok(collection.cleartext(id2).deleted);
 
     _("Clobber one of the tombstones");
@@ -94,18 +95,25 @@ add_task(async function test_general() {
       record.children.push(id3);
     }, Date.now() / 1000);
     engine.lastSync = Date.now() / 1000 - 30;
     // Reset the local sync ID, forcing us to prefer deletions over local records.
     await engine.resetLocalSyncID();
 
     await sync_engine_and_validate_telem(engine, true);
 
-    ok(!collection.cleartext("toolbar").children.includes(id0),
-       "Parent shouldn't have the zombie remotely");
+    // XXX It's not clear to me if we should or shouldn't reupload tombstones...
+
+    // ok(collection.cleartext(id0).deleted, "Should reupload tombstone");
+
+    ok(!(await PlacesSyncUtils.bookmarks.fetchChildRecordIds("toolbar")).includes(id0),
+       "Parent shouldn't have the zombie locally");
+
+    // ok(!collection.cleartext("toolbar").children.includes(id0),
+    //    "Parent shouldn't have the zombie remotely");
 
     ok((await PlacesSyncUtils.bookmarks.fetchChildRecordIds("menu")).includes(id3),
        "Should the apply updated parent without issue");
 
     equal((await PlacesUtils.bookmarks.fetch(id3)).title, "69105",
           "Should have successfully applied the remote recod");
 
     ok(collection.cleartext("menu").children.includes(id3),
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -893,18 +893,18 @@ var Bookmarks = Object.freeze({
    *
    * Note that roots are preserved, only their children will be removed.
    *
    * @param {Object} [options={}]
    *        Additional options. Currently supports the following properties:
    *         - source: The change source, forwarded to all bookmark observers.
    *           Defaults to nsINavBookmarksService::SOURCE_DEFAULT.
    *         - removeTombstones: Force removal (or preservation) of tombstones,
-   *           regardless of source (by default we remove tombstones iff
-   *           the source is SOURCES.SYNC).
+   *           regardless of source. By default we remove tombstones iff
+   *           the source is SOURCES.SYNC.
    *
    * @return {Promise} resolved when the removal is complete.
    * @resolves once the removal is complete.
    */
   eraseEverything(options = {}) {
     if (!options.source) {
       options.source = Bookmarks.SOURCES.DEFAULT;
     }
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -1123,17 +1123,19 @@ class SyncedBookmarksMirror {
                     /* Livemarks are folders with a feed URL annotation. */
                     SELECT 1 FROM moz_items_annos a
                     JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id
                     WHERE a.item_id = b.id AND
                           n.name = :feedURLAnno
                   ) THEN :livemarkKind
                   ELSE :folderKind END)
                 ELSE :separatorKind END) AS kind,
-             b.lastModified / 1000 AS localModified, b.syncChangeCounter
+             b.lastModified / 1000 AS localModified,
+             b.syncChangeCounter,
+             b.syncStatus
       FROM moz_bookmarks b
       JOIN moz_bookmarks p ON p.id = b.parent
       JOIN syncedItems s ON s.id = b.id
       ORDER BY s.level, b.parent, b.position`,
       { bookmarkType: PlacesUtils.bookmarks.TYPE_BOOKMARK,
         queryKind: SyncedBookmarksMirror.KIND.QUERY,
         bookmarkKind: SyncedBookmarksMirror.KIND.BOOKMARK,
         folderType: PlacesUtils.bookmarks.TYPE_FOLDER,
@@ -1145,21 +1147,24 @@ class SyncedBookmarksMirror {
     for await (let row of yieldingIterator(itemRows)) {
       let parentGuid = row.getResultByName("parentGuid");
       let node = BookmarkNode.fromLocalRow(row, localTimeSeconds);
       localTree.insert(parentGuid, node);
     }
 
     // Note tombstones for locally deleted items.
     let tombstoneRows = await this.db.execute(`
-      SELECT guid FROM moz_bookmarks_deleted`);
+      SELECT guid, syncStatus
+      FROM moz_bookmarks_deleted
+      WHERE syncStatus != ${PlacesUtils.bookmarks.SYNC_STATUS.NORMAL}`);
 
     for await (let row of yieldingIterator(tombstoneRows)) {
       let guid = row.getResultByName("guid");
-      localTree.noteDeleted(guid);
+      let status = row.getResultByName("syncStatus");
+      localTree.noteDeleted(guid, status);
     }
 
     let elapsedTime = Cu.now() - startTime;
     let totalRows = itemRows.length + tombstoneRows.length;
     this.recordTelemetryEvent("mirror", "fetch", "localTree",
                               { time: String(elapsedTime),
                                 count: String(totalRows) });
 
@@ -1313,20 +1318,32 @@ class SyncedBookmarksMirror {
                      WHERE NOT isUntagging)`);
 
       // Remove annos for the deleted items.
       await this.db.execute(`
         DELETE FROM moz_items_annos
         WHERE item_id IN (SELECT itemId FROM itemsRemoved
                           WHERE NOT isUntagging)`);
 
-      // Remove any local tombstones for deleted items.
+      dump("%% Insert or ignore into deleted from removed\n");
+      // XXX Is there a way to do the next two statements at once?
       await this.db.execute(`
-        DELETE FROM moz_bookmarks_deleted
-        WHERE guid IN (SELECT guid FROM itemsRemoved)`);
+        INSERT OR IGNORE INTO moz_bookmarks_deleted(guid, dateRemoved, syncStatus)
+        SELECT guid,
+               STRFTIME('%s', 'now', 'localtime', 'utc') * 1000000 as dateRemoved,
+               :syncStatus as syncStatus
+        FROM itemsRemoved`,
+        { syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL });
+      dump("%% Update deleted from removed\n");
+      // Mark any local tombstones for deleted items as synced.
+      await this.db.execute(`
+        UPDATE moz_bookmarks_deleted
+        SET syncStatus = :syncStatus
+        WHERE guid IN (SELECT guid FROM itemsRemoved)`,
+        { syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL });
 
       await this.db.execute(`
         DELETE FROM moz_bookmarks
         WHERE id IN (SELECT itemId FROM itemsRemoved
                      WHERE NOT isUntagging)`);
 
       // Flag locally deleted items as merged.
       await this.db.execute(`
@@ -1335,24 +1352,51 @@ class SyncedBookmarksMirror {
         WHERE needsMerge AND
               guid IN (SELECT guid FROM itemsRemoved
                        WHERE NOT isUntagging)`);
     }
 
     MirrorLog.debug("Flagging remotely deleted items as merged");
     for (let chunk of PlacesSyncUtils.chunkArray(remoteDeletions,
       SQLITE_MAX_VARIABLE_NUMBER)) {
+      if (chunk.length) {
+        dump("%% insert or ignore into deleted from newTombstones\n");
+
+        let newTombstones = Array.from(chunk, () =>
+          `(?, STRFTIME('%s', 'now', 'localtime', 'utc') * 1000000, ${
+            PlacesUtils.bookmarks.SYNC_STATUS.NORMAL})`).join(",");
+        await this.db.execute(`
+          INSERT OR IGNORE INTO moz_bookmarks_deleted(guid, dateRemoved, syncStatus)
+          VALUES ${newTombstones}`, chunk);
+      }
 
       await this.db.execute(`
         UPDATE items SET
           needsMerge = 0
         WHERE needsMerge AND
               guid IN (${new Array(chunk.length).fill("?").join(",")})`,
         chunk);
     }
+
+    dump("%% insert or ignore into deleted from items where isDeleted\n");
+    // Insert remote tombstones.
+    // XXX: Not sure if there's a way to do this as part of the above?
+    await this.db.execute(`
+      INSERT OR IGNORE INTO moz_bookmarks_deleted (guid, dateRemoved, syncStatus)
+      SELECT guid,
+             STRFTIME('%s', 'now', 'localtime', 'utc') * 1000000 as dateRemoved,
+             :syncStatus as syncStatus
+      FROM items
+      WHERE isDeleted`,
+      { syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL });
+
+    await this.db.execute(`
+      DELETE FROM moz_bookmarks_deleted
+      WHERE guid IN (SELECT guid FROM moz_bookmarks)
+    `);
   }
 
   /**
    * Rewrites tag query URLs in the mirror to point to the tag.
    *
    * For example, an incoming tag query of, say, "place:type=7&folder=3" means
    * it is a query for whatever tag was defined by the local record with id=3
    * (and the incoming record has the actual tag in the folderName field).
@@ -1676,19 +1720,20 @@ class SyncedBookmarksMirror {
     await this.db.execute(`
       INSERT INTO structureToUpload(guid, parentId, position)
       SELECT b.guid, b.parent, b.position FROM moz_bookmarks b
       JOIN itemsToUpload o ON o.id = b.parent`);
 
     // Finally, stage tombstones for deleted items. Ignore conflicts if we have
     // tombstones for undeleted items; Places Maintenance should clean these up.
     await this.db.execute(`
-      INSERT OR IGNORE INTO itemsToUpload(guid, syncChangeCounter, isDeleted,
-                                          dateAdded)
-      SELECT guid, 1, 1, dateRemoved / 1000 FROM moz_bookmarks_deleted`);
+      INSERT OR IGNORE INTO itemsToUpload(guid, syncChangeCounter, isDeleted, dateAdded)
+      SELECT guid, 1, 1, dateRemoved / 1000
+      FROM moz_bookmarks_deleted
+      WHERE syncStatus = ${PlacesUtils.bookmarks.SYNC_STATUS.NEW}`);
   }
 
   /**
    * Inflates Sync records for all staged outgoing items.
    *
    * @return {Object.<String, BookmarkChangeRecord>}
    *         A changeset containing Sync record cleartexts for outgoing items
    *         and tombstones, keyed by their Sync record IDs.
@@ -2891,21 +2936,22 @@ BookmarkMergeState.remote = new Bookmark
   BookmarkMergeState.TYPE.REMOTE);
 
 /**
  * A node in a local or remote bookmark tree. Nodes are lightweight: they carry
  * enough information for the merger to resolve trivial conflicts without
  * querying the mirror or Places for the complete value state.
  */
 class BookmarkNode {
-  constructor(guid, age, kind, needsMerge = false) {
+  constructor(guid, age, kind, needsMerge = false, possiblyStale = false) {
     this.guid = guid;
     this.kind = kind;
     this.age = age;
     this.needsMerge = needsMerge;
+    this.possiblyStale = possiblyStale;
     this.children = [];
   }
 
   // Creates a virtual folder node for the Places root.
   static root() {
     let guid = PlacesUtils.bookmarks.rootGuid;
     return new BookmarkNode(guid, 0, SyncedBookmarksMirror.KIND.FOLDER);
   }
@@ -2929,17 +2975,20 @@ class BookmarkNode {
     let localModified = row.getResultByName("localModified");
     let age = Math.max(localTimeSeconds - localModified / 1000, 0) || 0;
 
     let kind = row.getResultByName("kind");
 
     let syncChangeCounter = row.getResultByName("syncChangeCounter");
     let needsMerge = syncChangeCounter > 0;
 
-    return new BookmarkNode(guid, age, kind, needsMerge);
+    let syncStatus = row.getResultByName("syncStatus");
+    let possiblyStale = syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN;
+
+    return new BookmarkNode(guid, age, kind, needsMerge, possiblyStale);
   }
 
   /**
    * Creates a bookmark node from a mirror row.
    *
    * @param  {mozIStorageRow} row
    *         The mirror row containing the node info.
    * @param  {Number} remoteTimeSeconds
@@ -3076,26 +3125,31 @@ class BookmarkNode {
 
 /**
  * A complete, rooted tree with tombstones.
  */
 class BookmarkTree {
   constructor(root) {
     this.byGuid = new Map();
     this.infosByNode = new WeakMap();
-    this.deletedGuids = new Set();
+    this.deletedGuids = new Map();
 
     this.root = root;
     this.byGuid.set(this.root.guid, this.root);
   }
 
   isDeleted(guid) {
     return this.deletedGuids.has(guid);
   }
 
+  // Note that it's an error to call this on the remote tree.
+  localTombstoneSyncStatus(guid) {
+    return this.deletedGuids.get(guid);
+  }
+
   nodeForGuid(guid) {
     return this.byGuid.get(guid);
   }
 
   parentNodeFor(childNode) {
     let info = this.infosByNode.get(childNode);
     return info ? info.parentNode : null;
   }
@@ -3135,45 +3189,45 @@ class BookmarkTree {
     parentNode.children.push(node);
     this.byGuid.set(node.guid, node);
 
     let parentInfo = this.infosByNode.get(parentNode);
     let level = parentInfo ? parentInfo.level + 1 : 0;
     this.infosByNode.set(node, { parentNode, level });
   }
 
-  noteDeleted(guid) {
-    this.deletedGuids.add(guid);
+  noteDeleted(guid, syncStatus = -1) {
+    this.deletedGuids.set(guid, syncStatus);
   }
 
   * syncableGuids() {
     let nodesToWalk = PlacesUtils.bookmarks.userContentRoots.map(guid => {
       let node = this.byGuid.get(guid);
       return node ? node.children : [];
     });
     while (nodesToWalk.length) {
       let childNodes = nodesToWalk.pop();
       for (let node of childNodes) {
         yield node.guid;
         nodesToWalk.push(node.children);
       }
     }
-    for (let guid of this.deletedGuids) {
+    for (let [guid] of this.deletedGuids) {
       yield guid;
     }
   }
 
   /**
    * Generates an ASCII art representation of the complete tree.
    *
    * @return {String}
    */
   toASCIITreeString() {
     return `${this.root.toASCIITreeString()}\nDeleted: [${
-            Array.from(this.deletedGuids).join(", ")}]`;
+            Array.from(this.deletedGuids, ([guid, status]) => guid).join(", ")}]`;
   }
 }
 
 /**
  * A node in a merged bookmark tree. Holds the local node, remote node,
  * merged children, and a merge state indicating which side to prefer.
  */
 class MergedBookmarkNode {
@@ -3393,22 +3447,22 @@ class BookmarkMerger {
                                                     remoteSyncableRoot);
       mergedRoot.mergedChildren.push(mergedSyncableRoot);
     }
 
     // Any remaining deletions on one side should be deleted on the other side.
     // This happens when the remote tree has tombstones for items that don't
     // exist in Places, or Places has tombstones for items that aren't on the
     // server.
-    for await (let guid of yieldingIterator(this.localTree.deletedGuids)) {
+    for await (let [guid] of yieldingIterator(this.localTree.deletedGuids)) {
       if (!this.mentions(guid)) {
         this.deleteRemotely.add(guid);
       }
     }
-    for await (let guid of yieldingIterator(this.remoteTree.deletedGuids)) {
+    for await (let [guid] of yieldingIterator(this.remoteTree.deletedGuids)) {
       if (!this.mentions(guid)) {
         this.deleteLocally.add(guid);
       }
     }
     return mergedRoot;
   }
 
   async subsumes(tree) {
@@ -4044,18 +4098,27 @@ class BookmarkMerger {
           "Can't check for structure changes without local parent");
       }
       if (localParentNode.guid != remoteParentNode.guid) {
         return BookmarkMerger.STRUCTURE.MOVED;
       }
       return BookmarkMerger.STRUCTURE.UNCHANGED;
     }
 
+    let localSyncStatus = this.localTree.localTombstoneSyncStatus(remoteNode.guid);
     if (remoteNode.needsMerge) {
       if (!remoteNode.isFolder()) {
+        if (localSyncStatus == PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN) {
+          MirrorLog.trace("Remote non-folder ${remoteNode} deleted locally " +
+                          "with unknown status. taking local change",
+                          { remoteNode });
+          this.structureCounts.localItemDel++;
+          this.deleteRemotely.add(remoteNode.guid);
+          return BookmarkMerger.STRUCTURE.DELETED;
+        }
         // If a non-folder child is deleted locally and changed remotely, we
         // ignore the local deletion and take the remote child.
         MirrorLog.trace("Remote non-folder ${remoteNode} deleted locally " +
                         "and changed remotely; taking remote change",
                         { remoteNode });
         this.structureCounts.remoteRevives++;
         return BookmarkMerger.STRUCTURE.UNCHANGED;
       }
@@ -4117,16 +4180,24 @@ class BookmarkMerger {
       }
       if (remoteParentNode.guid != localParentNode.guid) {
         return BookmarkMerger.STRUCTURE.MOVED;
       }
       return BookmarkMerger.STRUCTURE.UNCHANGED;
     }
 
     if (localNode.needsMerge) {
+      if (localNode.possiblyStale) {
+        MirrorLog.trace("Local non-folder ${localNode} deleted remotely and " +
+                        "changed locally, but local sync status is unknown, " +
+                        "deleting locally.", { localNode });
+        this.structureCounts.localItemDel++;
+        this.deleteLocally.add(localNode.guid);
+        return BookmarkMerger.STRUCTURE.DELETED;
+      }
       if (!localNode.isFolder()) {
         MirrorLog.trace("Local non-folder ${localNode} deleted remotely and " +
                         "changed locally; taking local change", { localNode });
         this.structureCounts.localRevives++;
         return BookmarkMerger.STRUCTURE.UNCHANGED;
       }
       MirrorLog.trace("Local folder ${localNode} deleted remotely and " +
                       "changed locally; taking remote deletion", { localNode });
--- a/toolkit/components/places/tests/sync/test_bookmark_deletion.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_deletion.js
@@ -181,18 +181,17 @@ add_task(async function test_complex_orp
       guid: PlacesUtils.bookmarks.mobileGuid,
       type: PlacesUtils.bookmarks.TYPE_FOLDER,
       index: 4,
       title: MobileBookmarksTitle,
     }],
   }, "Should move orphans to closest surviving parent");
 
   await buf.finalize();
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_locally_modified_remotely_deleted() {
   let mergeTelemetryEvents = [];
   let buf = await openMirror("locally_modified_remotely_deleted", {
     recordTelemetryEvent(object, method, value, extra) {
       equal(object, "mirror", "Wrong object for telemetry event");
       if (method == "merge") {
@@ -253,16 +252,17 @@ add_task(async function test_locally_mod
     title: "D",
     children: ["bookmarkEEEE"],
   }, {
     id: "bookmarkEEEE",
     type: "bookmark",
     title: "E",
     bmkUri: "http://example.com/e",
   }], { needsMerge: false });
+  info("Mark as synced");
   await PlacesTestUtils.markBookmarksAsSynced();
 
   info("Make local changes: change A; B > ((D > F) G)");
   await PlacesUtils.bookmarks.update({
     guid: "bookmarkAAAA",
     title: "A (local)",
     url: "http://example.com/a-local",
   });
@@ -307,17 +307,17 @@ add_task(async function test_locally_mod
   deepEqual(mergeTelemetryEvents, [{
     value: "structure",
     extra: { new: "1", localRevives: "1", remoteDeletes: "2" },
   }], "Should record telemetry for local item and remote folder deletions");
 
   let idsToUpload = inspectChangeRecords(changesToUpload);
   deepEqual(idsToUpload, {
     updated: ["bookmarkAAAA", "bookmarkFFFF", "bookmarkGGGG", "menu"],
-    deleted: [],
+    deleted: ["bookmarkCCCC", "bookmarkEEEE", "folderBBBBBB", "folderDDDDDD"],
   }, "Should upload A, relocated local orphans, and menu");
 
   await assertLocalTree(PlacesUtils.bookmarks.menuGuid, {
     guid: PlacesUtils.bookmarks.menuGuid,
     type: PlacesUtils.bookmarks.TYPE_FOLDER,
     index: 0,
     title: BookmarksMenuTitle,
     children: [{
@@ -337,18 +337,17 @@ add_task(async function test_locally_mod
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
       index: 2,
       title: "G (local)",
       url: "http://example.com/g-local",
     }],
   }, "Should restore A and relocate (F G) to menu");
 
   await buf.finalize();
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_locally_deleted_remotely_modified() {
   let mergeTelemetryEvents = [];
   let buf = await openMirror("locally_deleted_remotely_modified", {
     recordTelemetryEvent(object, method, value, extra) {
       equal(object, "mirror", "Wrong object for telemetry event");
       if (method == "merge") {
@@ -484,18 +483,17 @@ add_task(async function test_locally_del
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
       index: 2,
       title: "G (remote)",
       url: "http://example.com/g-remote",
     }],
   }, "Should restore A and relocate (F G) to menu");
 
   await buf.finalize();
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_move_to_new_then_delete() {
   let buf = await openMirror("move_to_new_then_delete");
 
   info("Set up mirror: A > B > (C D)");
   await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.menuGuid,
@@ -610,18 +608,17 @@ add_task(async function test_move_to_new
       guid: PlacesUtils.bookmarks.mobileGuid,
       type: PlacesUtils.bookmarks.TYPE_FOLDER,
       index: 4,
       title: MobileBookmarksTitle,
     }],
   }, "Should move C to closest surviving parent");
 
   await buf.finalize();
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_nonexistent_on_one_side() {
   let buf = await openMirror("nonexistent_on_one_side");
 
   info("Set up empty mirror");
   await PlacesTestUtils.markBookmarksAsSynced();
 
@@ -633,18 +630,17 @@ add_task(async function test_nonexistent
     title: "A",
     url: "http://example.com/a",
     // Pretend a bookmark restore added A, so that we'll write a tombstone when
     // we remove it.
     source: PlacesUtils.bookmarks.SOURCES.RESTORE,
   });
   await PlacesUtils.bookmarks.remove("bookmarkAAAA");
 
-  // B doesn't exist in Places, and we don't currently persist tombstones (bug
-  // 1343103), so we should ignore it.
+  // B doesn't exist in Places, but we should store it anyway.
   info("Create remote tombstone for nonexistent local item B");
   await storeRecords(buf, [{
     id: "bookmarkBBBB",
     deleted: true
   }]);
 
   info("Apply remote");
   let changesToUpload = await buf.apply();
@@ -670,18 +666,17 @@ add_task(async function test_nonexistent
         dateAdded: menuInfo.dateAdded.getTime(),
         title: BookmarksMenuTitle,
         children: [],
       },
     },
   });
 
   await buf.finalize();
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_clear_folder_then_delete() {
   let buf = await openMirror("clear_folder_then_delete");
 
   info("Set up mirror");
   await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.menuGuid,
@@ -838,18 +833,17 @@ add_task(async function test_clear_folde
         index: 0,
         title: "F",
         url: "http://example.com/f",
       }],
     }],
   }, "Should not orphan moved children of a deleted folder");
 
   await buf.finalize();
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_newer_move_to_deleted() {
   let buf = await openMirror("test_newer_move_to_deleted");
 
   info("Set up mirror");
   await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.menuGuid,
@@ -1010,11 +1004,10 @@ add_task(async function test_newer_move_
       guid: PlacesUtils.bookmarks.mobileGuid,
       type: PlacesUtils.bookmarks.TYPE_FOLDER,
       index: 4,
       title: MobileBookmarksTitle,
     }],
   }, "Should not decide to keep newly moved items in deleted parents");
 
   await buf.finalize();
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
--- a/toolkit/components/places/tests/sync/test_sync_utils.js
+++ b/toolkit/components/places/tests/sync/test_sync_utils.js
@@ -2153,20 +2153,16 @@ add_task(async function test_pullChanges
 
     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
     ), "Roots and NEW items restored from JSON backup should be marked as NORMAL");
-
-    strictEqual(changes[syncedFolder.guid].tombstone, true,
-      `Should include tombstone for overwritten synced bookmark ${
-      syncedFolder.guid}`);
   }
 
   await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_pullChanges_tombstones() {
   await ignoreChangedRoots();
 
@@ -2390,18 +2386,17 @@ add_task(async function test_changes_bet
   // there should be no record for the item we deleted.
   Assert.strictEqual(await PlacesUtils.bookmarks.fetch(guids.bmk), null);
 
   // and re-fetching changes should list it as a tombstone.
   changes = await PlacesSyncUtils.bookmarks.pullChanges();
   Assert.equal(changes[guids.bmk].counter, 1);
   Assert.equal(changes[guids.bmk].tombstone, true);
 
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_touch() {
   await ignoreChangedRoots();
 
   strictEqual(await PlacesSyncUtils.bookmarks.touch(makeGuid()), null,
     "Should not revive nonexistent items");
 
@@ -2927,34 +2922,29 @@ add_task(async function test_bookmarks_r
   let syncFields = await PlacesTestUtils.fetchBookmarkSyncFields(
     PlacesUtils.bookmarks.menuGuid, PlacesUtils.bookmarks.toolbarGuid,
     PlacesUtils.bookmarks.unfiledGuid, "bookmarkAAAA", "bookmarkCCCC",
     "bookmarkDDDD");
   ok(syncFields.every(field =>
     field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NEW
   ), "Should change all sync statuses to NEW after resetting bookmarks sync ID");
 
-  let tombstones = await PlacesTestUtils.fetchSyncTombstones();
-  deepEqual(tombstones, [],
-    "Should remove all tombstones after resetting bookmarks sync ID");
-
   info("Set bookmarks last sync time");
   let lastSync = Date.now() / 1000;
   await PlacesSyncUtils.bookmarks.setLastSync(lastSync);
   equal(await PlacesSyncUtils.bookmarks.getLastSync(), lastSync,
     "Should record bookmarks last sync time");
 
   newSyncId = await PlacesSyncUtils.bookmarks.resetSyncId();
   notEqual(newSyncId, syncId,
     "Should set new bookmarks sync ID if one already exists");
   strictEqual(await PlacesSyncUtils.bookmarks.getLastSync(), 0,
     "Should reset bookmarks last sync time after resetting sync ID");
 
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_bookmarks_wipe() {
   info("Add Sync metadata before wipe");
   let newSyncId = await PlacesSyncUtils.bookmarks.resetSyncId();
   await PlacesSyncUtils.bookmarks.setLastSync(Date.now() / 1000);
 
   let existingSyncId = await PlacesSyncUtils.bookmarks.getSyncId();
@@ -3011,18 +3001,17 @@ add_task(async function test_bookmarks_w
 
   let rootSyncFields = await PlacesTestUtils.fetchBookmarkSyncFields(
     PlacesUtils.bookmarks.menuGuid, PlacesUtils.bookmarks.toolbarGuid,
     PlacesUtils.bookmarks.unfiledGuid);
   ok(rootSyncFields.every(field =>
     field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NEW
   ), "Should reset all sync statuses to NEW after wipe");
 
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_bookmarks_meta_eraseEverything() {
   info("Add Sync metadata before erase");
   let newSyncId = await PlacesSyncUtils.bookmarks.resetSyncId();
   let lastSync = Date.now() / 1000;
   await PlacesSyncUtils.bookmarks.setLastSync(lastSync);
 
@@ -3073,18 +3062,17 @@ add_task(async function test_bookmarks_m
 
   let rootSyncFields = await PlacesTestUtils.fetchBookmarkSyncFields(
     PlacesUtils.bookmarks.menuGuid, PlacesUtils.bookmarks.toolbarGuid,
     PlacesUtils.bookmarks.unfiledGuid, PlacesUtils.bookmarks.mobileGuid);
   ok(rootSyncFields.every(field =>
     field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NORMAL
   ), "Should not reset sync statuses after erasing everything");
 
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_bookmarks_reset() {
   info("Add Sync metadata before reset");
   await PlacesSyncUtils.bookmarks.resetSyncId();
   await PlacesSyncUtils.bookmarks.setLastSync(Date.now() / 1000);
 
   info("Set up local tree before reset");
@@ -3122,27 +3110,23 @@ add_task(async function test_bookmarks_r
 
   strictEqual(await PlacesSyncUtils.bookmarks.getSyncId(), "",
     "Should reset bookmarks sync ID after reset");
   strictEqual(await PlacesSyncUtils.bookmarks.getLastSync(), 0,
     "Should reset bookmarks last sync after reset");
   ok(!(await PlacesSyncUtils.bookmarks.shouldWipeRemote()),
     "Resetting Sync metadata should not wipe server");
 
-  deepEqual(await PlacesTestUtils.fetchSyncTombstones(), [],
-    "Should drop tombstones after reset");
-
   let itemSyncFields = await PlacesTestUtils.fetchBookmarkSyncFields(
     "bookmarkAAAA", "bookmarkCCCC");
   ok(itemSyncFields.every(field =>
     field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NEW
   ), "Should reset sync statuses for existing items to NEW after reset");
 
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_bookmarks_meta_restore() {
   info("Add Sync metadata before manual restore");
   await PlacesSyncUtils.bookmarks.resetSyncId();
   await PlacesSyncUtils.bookmarks.setLastSync(Date.now() / 1000);
 
   ok(!(await PlacesSyncUtils.bookmarks.shouldWipeRemote()),
@@ -3161,18 +3145,17 @@ add_task(async function test_bookmarks_m
 
   let syncFields = await PlacesTestUtils.fetchBookmarkSyncFields(
     PlacesUtils.bookmarks.menuGuid, "NnvGl3CRA4hC",
     PlacesUtils.bookmarks.toolbarGuid, "APzP8MupzA8l");
   ok(syncFields.every(field =>
     field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NEW
   ), "Should reset all sync stauses to NEW after manual restore");
 
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_bookmarks_meta_restore_on_startup() {
   info("Add Sync metadata before simulated automatic restore");
   await PlacesSyncUtils.bookmarks.resetSyncId();
   await PlacesSyncUtils.bookmarks.setLastSync(Date.now() / 1000);
 
   ok(!(await PlacesSyncUtils.bookmarks.shouldWipeRemote()),
@@ -3194,18 +3177,17 @@ add_task(async function test_bookmarks_m
 
   let syncFields = await PlacesTestUtils.fetchBookmarkSyncFields(
     PlacesUtils.bookmarks.menuGuid, "NnvGl3CRA4hC",
     PlacesUtils.bookmarks.toolbarGuid, "APzP8MupzA8l");
   ok(syncFields.every(field =>
     field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN
   ), "Should reset all sync stauses to UNKNOWN after automatic restore");
 
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_bookmarks_ensureCurrentSyncId() {
   info("Set up local tree");
   await ignoreChangedRoots();
   await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.menuGuid,
     source: PlacesUtils.bookmarks.SOURCES.SYNC,
@@ -3296,31 +3278,26 @@ add_task(async function test_bookmarks_e
   {
     await PlacesSyncUtils.bookmarks.ensureCurrentSyncId("syncIdBBBBBB");
 
     equal(await PlacesSyncUtils.bookmarks.getSyncId(), "syncIdBBBBBB",
       "Should replace existing bookmarks sync ID on mismatch");
     strictEqual(await PlacesSyncUtils.bookmarks.getLastSync(), 0,
       "Should reset bookmarks last sync time on sync ID mismatch");
 
-    let tombstones = await PlacesTestUtils.fetchSyncTombstones();
-    deepEqual(tombstones, [],
-      "Should drop tombstones after bookmarks sync ID mismatch");
-
     let syncFields = await PlacesTestUtils.fetchBookmarkSyncFields(
       PlacesUtils.bookmarks.menuGuid, PlacesUtils.bookmarks.toolbarGuid,
       PlacesUtils.bookmarks.unfiledGuid, "bookmarkAAAA", "bookmarkCCCC",
       "bookmarkDDDD");
     ok(syncFields.every(field =>
       field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN
     ), "Should reset all sync statuses to UNKNOWN after bookmarks sync ID mismatch");
   }
 
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
+  await PlacesSyncUtils.bookmarks.wipe();
 });
 
 add_task(async function test_history_resetSyncId() {
   let syncId = await PlacesSyncUtils.history.getSyncId();
   strictEqual(syncId, "", "Should start with empty history sync ID");
 
   info("Assign new history sync ID for first time");
   let newSyncId = await PlacesSyncUtils.history.resetSyncId();