Bug 1303679 - Remove Places roots, reading list items, and pinned sites from the Sync server. r?markh draft
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 09 Nov 2016 14:23:54 -0800
changeset 436871 b85ec54118ae804226dbdf4c276dc65c2d2596df
parent 436738 336759fad4621dfcd0a3293840edbed67018accd
child 536464 918439e3ed71b6692c94207a9268bd3918752d1f
push id35223
push userkcambridge@mozilla.com
push dateWed, 09 Nov 2016 22:24:41 +0000
reviewersmarkh
bugs1303679
milestone52.0a1
Bug 1303679 - Remove Places roots, reading list items, and pinned sites from the Sync server. r?markh MozReview-Commit-ID: AhOBOnXsTYi
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -1118,16 +1118,22 @@ SyncEngine.prototype = {
           throw ex;
         }
         self._log.warn("Error decrypting record", ex);
         self._noteApplyFailure();
         failed.push(item.id);
         return;
       }
 
+      if (self._shouldDeleteRemotely(item)) {
+        self._log.trace("Deleting item from server without applying", item);
+        self._deleteId(item.id);
+        return;
+      }
+
       let shouldApply;
       try {
         shouldApply = self._reconcile(item);
       } catch (ex) {
         if (ex.code == Engine.prototype.eEngineAbortApplyIncoming) {
           self._log.warn("Reconciliation failed: aborting incoming processing.");
           self._noteApplyFailure();
           failed.push(item.id);
@@ -1255,16 +1261,23 @@ SyncEngine.prototype = {
                     count.applied, "applied,",
                     count.succeeded, "successfully,",
                     count.failed, "failed to apply,",
                     count.newFailed, "newly failed to apply,",
                     count.reconciled, "reconciled."].join(" "));
     Observers.notify("weave:engine:sync:applied", count, this.name);
   },
 
+  // Indicates whether an incoming item should be deleted from the server at
+  // the end of the sync. Engines can override this method to clean up records
+  // that shouldn't be on the server.
+  _shouldDeleteRemotely(remoteItem) {
+    return false;
+  },
+
   _noteApplyFailure: function () {
     // here would be a good place to record telemetry...
   },
 
   _noteApplyNewFailure: function () {
     // here would be a good place to record telemetry...
   },
 
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -41,16 +41,27 @@ const {
 } = Ci.nsINavBookmarksService;
 
 const SQLITE_MAX_VARIABLE_NUMBER = 999;
 
 const ORGANIZERQUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
 const ALLBOOKMARKS_ANNO = "AllBookmarks";
 const MOBILE_ANNO = "MobileBookmarks";
 
+// Roots that should be deleted from the server, instead of applied locally.
+// This matches `AndroidBrowserBookmarksRepositorySession::forbiddenGUID`,
+// but allows tags because we don't want to reparent tag folders or tag items
+// to "unfiled".
+const FORBIDDEN_INCOMING_IDS = ["pinned", "places", "readinglist"];
+
+// Items with these parents should be deleted from the server. We allow
+// children of the Places root, to avoid orphaning left pane queries and other
+// descendants of custom roots.
+const FORBIDDEN_INCOMING_PARENT_IDS = ["pinned", "readinglist"];
+
 // The tracker ignores changes made by bookmark import and restore, and
 // changes made by Sync. We don't need to exclude `SOURCE_IMPORT`, but both
 // import and restore fire `bookmarks-restore-*` observer notifications, and
 // the tracker doesn't currently distinguish between the two.
 const IGNORED_SOURCES = [SOURCE_SYNC, SOURCE_IMPORT, SOURCE_IMPORT_REPLACE];
 
 // Returns the constructor for a bookmark record type.
 function getTypeObject(type) {
@@ -701,16 +712,24 @@ BookmarksEngine.prototype = {
       }
     }
 
     // The local, duplicate ID is always deleted on the server - but for
     // bookmarks it is a logical delete.
     // Simply adding this (now non-existing) ID to the tracker is enough.
     this._modified.set(localDupeGUID, { modified: now, deleted: true });
   },
+
+  // Cleans up the Places root, reading list items (ignored in bug 762118,
+  // removed in bug 1155684), and pinned sites.
+  _shouldDeleteRemotely(incomingItem) {
+    return FORBIDDEN_INCOMING_IDS.includes(incomingItem.id) ||
+           FORBIDDEN_INCOMING_PARENT_IDS.includes(incomingItem.parentid);
+  },
+
   getValidator() {
     return new BookmarkValidator();
   }
 };
 
 function BookmarksStore(name, engine) {
   Store.call(this, name, engine);
   this._foldersToDelete = new Set();
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -18,16 +18,73 @@ initTestLogging("Trace");
 Service.engineManager.register(BookmarksEngine);
 
 function* assertChildGuids(folderGuid, expectedChildGuids, message) {
   let tree = yield PlacesUtils.promiseBookmarksTree(folderGuid);
   let childGuids = tree.children.map(child => child.guid);
   deepEqual(childGuids, expectedChildGuids, message);
 }
 
+add_task(function* test_delete_invalid_roots_from_server() {
+  _("Ensure that we delete the Places and Reading List roots from the server.");
+
+  let engine  = new BookmarksEngine(Service);
+  let store   = engine._store;
+  let tracker = engine._tracker;
+  let server = serverForFoo(engine);
+  new SyncTestingInfrastructure(server.server);
+
+  let collection = server.user("foo").collection("bookmarks");
+
+  Svc.Obs.notify("weave:engine:start-tracking");
+
+  try {
+    collection.insert("places", encryptPayload(store.createRecord("places").cleartext));
+
+    let listBmk = new Bookmark("bookmarks", Utils.makeGUID());
+    listBmk.bmkUri = "https://example.com";
+    listBmk.title = "Example reading list entry";
+    listBmk.parentName = "Reading List";
+    listBmk.parentid = "readinglist";
+    collection.insert(listBmk.id, encryptPayload(listBmk.cleartext));
+
+    let readingList = new BookmarkFolder("bookmarks", "readinglist");
+    readingList.title = "Reading List";
+    readingList.children = [listBmk.id];
+    readingList.parentName = "";
+    readingList.parentid = "places";
+    collection.insert("readinglist", encryptPayload(readingList.cleartext));
+
+    let newBmk = new Bookmark("bookmarks", Utils.makeGUID());
+    newBmk.bmkUri = "http://getfirefox.com";
+    newBmk.title = "Get Firefox!";
+    newBmk.parentName = "Bookmarks Toolbar";
+    newBmk.parentid = "toolbar";
+    collection.insert(newBmk.id, encryptPayload(newBmk.cleartext));
+
+    deepEqual(collection.keys().sort(), ["places", "readinglist", listBmk.id, newBmk.id].sort(),
+      "Should store Places root, reading list items, and new bookmark on server");
+
+    yield sync_engine_and_validate_telem(engine, false);
+
+    ok(!store.itemExists("readinglist"), "Should not apply Reading List root");
+    ok(!store.itemExists(listBmk.id), "Should not apply items in Reading List");
+    ok(store.itemExists(newBmk.id), "Should apply new bookmark");
+
+    deepEqual(collection.keys().sort(), ["menu", "mobile", "toolbar", "unfiled", newBmk.id].sort(),
+      "Should remove Places root and reading list items from server; upload local roots");
+  } finally {
+    store.wipe();
+    Svc.Prefs.resetBranch("");
+    Service.recordManager.clearCache();
+    yield new Promise(resolve => server.stop(resolve));
+    Svc.Obs.notify("weave:engine:stop-tracking");
+  }
+});
+
 add_task(function* test_change_during_sync() {
   _("Ensure that we track changes made during a sync.");
 
   let engine  = new BookmarksEngine(Service);
   let store   = engine._store;
   let tracker = engine._tracker;
   let server = serverForFoo(engine);
   new SyncTestingInfrastructure(server.server);