Bug 1454864 - Upload tombstones for non-syncable items. r=markh
☠☠ backed out by 8e02766aa3bc ☠ ☠
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 17 Apr 2018 18:52:28 -0700
changeset 468336 b497152c37e80d945a5e7de07f2554fd9ef89d09
parent 468335 93eea5f99ab74145526fc8d2eda170105652bde2
child 468337 a08096e187fd47f2f9380c461cc7b7d577960de5
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1454864
milestone61.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 1454864 - Upload tombstones for non-syncable items. r=markh MozReview-Commit-ID: HFk7DDyjIHS
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_corruption.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -614,16 +614,17 @@ class SyncedBookmarksMirror {
       await this.db.execute(`DELETE FROM mergeStates`);
       await this.db.execute(`DELETE FROM itemsAdded`);
       await this.db.execute(`DELETE FROM guidsChanged`);
       await this.db.execute(`DELETE FROM itemsChanged`);
       await this.db.execute(`DELETE FROM itemsRemoved`);
       await this.db.execute(`DELETE FROM itemsMoved`);
       await this.db.execute(`DELETE FROM annosChanged`);
       await this.db.execute(`DELETE FROM idsToWeaklyUpload`);
+      await this.db.execute(`DELETE FROM guidsToDeleteRemotely`);
       await this.db.execute(`DELETE FROM itemsToUpload`);
 
       return changeRecords;
     });
 
     MirrorLog.debug("Replaying recorded observer notifications");
     try {
       await observersToNotify.notifyAll();
@@ -1402,16 +1403,21 @@ class SyncedBookmarksMirror {
                        WHERE NOT isUntagging)`);
     }
 
     MirrorLog.trace("Flagging remotely deleted items as merged");
     for (let chunk of PlacesSyncUtils.chunkArray(remoteDeletions,
       SQLITE_MAX_VARIABLE_NUMBER)) {
 
       await this.db.execute(`
+        INSERT INTO guidsToDeleteRemotely(guid)
+        VALUES ${new Array(chunk.length).fill("(?)").join(",")}`,
+        chunk);
+
+      await this.db.execute(`
         UPDATE items SET
           needsMerge = 0
         WHERE needsMerge AND
               guid IN (${new Array(chunk.length).fill("?").join(",")})`,
         chunk);
     }
   }
 
@@ -1681,16 +1687,22 @@ class SyncedBookmarksMirror {
 
     // Record the child GUIDs of locally changed folders, which we use to
     // populate the `children` array in the record.
     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`);
 
+    // Stage tombstones for items that we explicitly flagged for deletion.
+    await this.db.execute(`
+      REPLACE INTO itemsToUpload(guid, syncChangeCounter, isDeleted)
+      SELECT d.guid, 1, 1 FROM guidsToDeleteRemotely d
+      JOIN items v ON v.guid = d.guid`);
+
     // 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)
       SELECT guid, 1, 1 FROM moz_bookmarks_deleted`);
   }
 
   /**
@@ -2677,16 +2689,21 @@ async function initializeTempMirrorEntit
 
   // Stores local IDs for items to upload even if they're not flagged as changed
   // in Places. These are "weak" because we won't try to reupload the item on
   // the next sync if the upload is interrupted or fails.
   await db.execute(`CREATE TEMP TABLE idsToWeaklyUpload(
     id INTEGER PRIMARY KEY
   )`);
 
+  // Stores GUIDs for items that should be deleted on the server.
+  await db.execute(`CREATE TEMP TABLE guidsToDeleteRemotely(
+    guid TEXT PRIMARY KEY
+  ) WITHOUT ROWID`);
+
   // Stores locally changed items staged for upload. See `stageItemsToUpload`
   // for an explanation of why these tables exists.
   await db.execute(`CREATE TEMP TABLE itemsToUpload(
     id INTEGER PRIMARY KEY,
     guid TEXT UNIQUE NOT NULL,
     syncChangeCounter INTEGER NOT NULL,
     isDeleted BOOLEAN NOT NULL DEFAULT 0,
     parentGuid TEXT,
--- a/toolkit/components/places/tests/sync/test_bookmark_corruption.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_corruption.js
@@ -1122,16 +1122,97 @@ add_task(async function test_non_syncabl
   }]);
 
   let changesToUpload = await buf.apply();
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
 
   let datesAdded = await promiseManyDatesAdded([PlacesUtils.bookmarks.menuGuid,
     PlacesUtils.bookmarks.unfiledGuid, "bookmarkBBBB", "bookmarkJJJJ"]);
   deepEqual(changesToUpload, {
+    folderAAAAAA: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "folderAAAAAA",
+        deleted: true,
+      },
+    },
+    folderDDDDDD: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "folderDDDDDD",
+        deleted: true,
+      },
+    },
+    folderLEFTPQ: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "folderLEFTPQ",
+        deleted: true,
+      },
+    },
+    folderLEFTPC: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "folderLEFTPC",
+        deleted: true,
+      },
+    },
+    folderLEFTPR: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "folderLEFTPR",
+        deleted: true,
+      },
+    },
+    folderLEFTPF: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "folderLEFTPF",
+        deleted: true,
+      },
+    },
+    rootHHHHHHHH: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "rootHHHHHHHH",
+        deleted: true,
+      },
+    },
+    bookmarkFFFF: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "bookmarkFFFF",
+        deleted: true,
+      },
+    },
+    bookmarkIIII: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "bookmarkIIII",
+        deleted: true,
+      },
+    },
     bookmarkBBBB: {
       tombstone: false,
       counter: 1,
       synced: false,
       cleartext: {
         id: "bookmarkBBBB",
         type: "bookmark",
         parentid: "menu",
@@ -1182,17 +1263,17 @@ add_task(async function test_non_syncabl
         parentid: "places",
         hasDupe: true,
         parentName: "",
         dateAdded: datesAdded.get(PlacesUtils.bookmarks.unfiledGuid),
         title: UnfiledBookmarksTitle,
         children: ["bookmarkJJJJ", "bookmarkGGGG"],
       },
     },
-  }, "Should upload new structure excluding non-syncable items");
+  }, "Should upload new structure and tombstones for non-syncable items");
 
   await assertLocalTree(PlacesUtils.bookmarks.rootGuid, {
     guid: PlacesUtils.bookmarks.rootGuid,
     type: PlacesUtils.bookmarks.TYPE_FOLDER,
     index: 0,
     title: "",
     children: [{
       guid: PlacesUtils.bookmarks.menuGuid,