Bug 1303679 - Remove Places roots, reading list items, and pinned sites from the Sync server. r=markh

MozReview-Commit-ID: AhOBOnXsTYi

--HG--
extra : rebase_source : 1f39b406b1a9215ca04ff4602cc8fac54163916f

services/sync/modules/engines.js

@@ -1123,6 +1123,12 @@ SyncEngine.prototype = {
         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);
@@ -1260,6 +1266,13 @@ SyncEngine.prototype = {
     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...
   },

services/sync/modules/engines/bookmarks.js

@@ -46,6 +46,17 @@ 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
@@ -706,6 +717,14 @@ BookmarksEngine.prototype = {
     // 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();
   }

services/sync/tests/unit/test_bookmark_engine.js

@@ -23,6 +23,63 @@ function* assertChildGuids(folderGuid, expectedChildGuids, message) {
   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.");