Bug 1299338 - Replace `ignoreAll` with change source checks in the Sync bookmarks engine. r=markh
authorKit Cambridge <kcambridge@mozilla.com>
Tue, 06 Sep 2016 08:36:21 -0700
changeset 313005 db9dfcdbef4aa204aba5d9b3c384f21ee1c66c61
parent 313004 af11902f242f7bdaa7437a497f286e2528af4f31
child 313006 85e02f8c2b692c2f241783d604e67ec15a3a785f
push id30668
push userkwierso@gmail.com
push dateThu, 08 Sep 2016 00:26:51 +0000
treeherdermozilla-central@7c655e03eef7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1299338
milestone51.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 1299338 - Replace `ignoreAll` with change source checks in the Sync bookmarks engine. r=markh MozReview-Commit-ID: DAsTlQbFanK
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
toolkit/components/places/nsNavHistoryResult.cpp
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -22,31 +22,41 @@ Cu.import("resource://services-sync/book
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/PlacesBackups.jsm");
 
 const ANNOS_TO_TRACK = [BookmarkAnnos.DESCRIPTION_ANNO, BookmarkAnnos.SIDEBAR_ANNO,
                         PlacesUtils.LMANNO_FEEDURI, PlacesUtils.LMANNO_SITEURI];
 
 const SERVICE_NOT_SUPPORTED = "Service not supported on this platform";
 const FOLDER_SORTINDEX = 1000000;
-const { SOURCE_SYNC } = Ci.nsINavBookmarksService;
+const {
+  SOURCE_SYNC,
+  SOURCE_IMPORT,
+  SOURCE_IMPORT_REPLACE,
+} = Ci.nsINavBookmarksService;
 
 // Maps Sync record property names to `PlacesSyncUtils` bookmark properties.
 const RECORD_PROPS_TO_BOOKMARK_PROPS = {
   title: "title",
   bmkUri: "url",
   tags: "tags",
   keyword: "keyword",
   description: "description",
   loadInSidebar: "loadInSidebar",
   queryId: "query",
   siteUri: "site",
   feedUri: "feed",
 };
 
+// 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];
+
 this.PlacesItem = function PlacesItem(collection, id, type) {
   CryptoWrapper.call(this, collection, id);
   this.type = type || "item";
 }
 PlacesItem.prototype = {
   decrypt: function PlacesItem_decrypt(keyBundle) {
     // Do the normal CryptoWrapper decrypt, but change types before returning
     let clear = CryptoWrapper.prototype.decrypt.call(this, keyBundle);
@@ -372,19 +382,17 @@ BookmarksEngine.prototype = {
     this._store._childrenToOrder = {};
   },
 
   _processIncoming: function (newitems) {
     try {
       SyncEngine.prototype._processIncoming.call(this, newitems);
     } finally {
       // Reorder children.
-      this._tracker.ignoreAll = true;
       this._store._orderChildren();
-      this._tracker.ignoreAll = false;
       delete this._store._childrenToOrder;
     }
   },
 
   _syncFinish: function _syncFinish() {
     SyncEngine.prototype._syncFinish.call(this);
     this._tracker._ensureMobileQuery();
   },
@@ -900,16 +908,26 @@ function BookmarksTracker(name, engine) 
   this._batchSawScoreIncrement = false;
   Tracker.call(this, name, engine);
 
   Svc.Obs.add("places-shutdown", this);
 }
 BookmarksTracker.prototype = {
   __proto__: Tracker.prototype,
 
+  //`_ignore` checks the change source for each observer notification, so we
+  // don't want to let the engine ignore all changes during a sync.
+  get ignoreAll() {
+    return false;
+  },
+
+  // Define an empty setter so that the engine doesn't throw a `TypeError`
+  // setting a read-only property.
+  set ignoreAll(value) {},
+
   startTracking: function() {
     PlacesUtils.bookmarks.addObserver(this, true);
     Svc.Obs.add("bookmarks-restore-begin", this);
     Svc.Obs.add("bookmarks-restore-success", this);
     Svc.Obs.add("bookmarks-restore-failed", this);
   },
 
   stopTracking: function() {
@@ -920,30 +938,27 @@ BookmarksTracker.prototype = {
   },
 
   observe: function observe(subject, topic, data) {
     Tracker.prototype.observe.call(this, subject, topic, data);
 
     switch (topic) {
       case "bookmarks-restore-begin":
         this._log.debug("Ignoring changes from importing bookmarks.");
-        this.ignoreAll = true;
         break;
       case "bookmarks-restore-success":
         this._log.debug("Tracking all items on successful import.");
-        this.ignoreAll = false;
 
         this._log.debug("Restore succeeded: wiping server and other clients.");
         this.engine.service.resetClient([this.name]);
         this.engine.service.wipeServer([this.name]);
         this.engine.service.clientsEngine.sendCommand("wipeEngine", [this.name]);
         break;
       case "bookmarks-restore-failed":
         this._log.debug("Tracking all items on failed import.");
-        this.ignoreAll = false;
         break;
     }
   },
 
   QueryInterface: XPCOMUtils.generateQI([
     Ci.nsINavBookmarkObserver,
     Ci.nsINavBookmarkObserver_MOZILLA_1_9_1_ADDITIONS,
     Ci.nsISupportsWeakReference
@@ -973,21 +988,25 @@ BookmarksTracker.prototype = {
 
   /**
    * Determine if a change should be ignored.
    *
    * @param itemId
    *        Item under consideration to ignore
    * @param folder (optional)
    *        Folder of the item being changed
+   * @param guid
+   *        Places GUID of the item being changed
+   * @param source
+   *        A change source constant from `nsINavBookmarksService::SOURCE_*`.
    */
-  _ignore: function BMT__ignore(itemId, folder, guid) {
-    // Ignore unconditionally if the engine tells us to.
-    if (this.ignoreAll)
+  _ignore: function BMT__ignore(itemId, folder, guid, source) {
+    if (IGNORED_SOURCES.includes(source)) {
       return true;
+    }
 
     // Get the folder id if we weren't given one.
     if (folder == null) {
       try {
         folder = PlacesUtils.bookmarks.getFolderIdForItem(itemId);
       } catch (ex) {
         this._log.debug("getFolderIdForItem(" + itemId +
                         ") threw; calling _ensureMobileQuery.");
@@ -1013,28 +1032,29 @@ BookmarksTracker.prototype = {
       return true;
     }
 
     return false;
   },
 
   onItemAdded: function BMT_onItemAdded(itemId, folder, index,
                                         itemType, uri, title, dateAdded,
-                                        guid, parentGuid) {
-    if (this._ignore(itemId, folder, guid))
+                                        guid, parentGuid, source) {
+    if (this._ignore(itemId, folder, guid, source)) {
       return;
+    }
 
     this._log.trace("onItemAdded: " + itemId);
     this._add(itemId, guid);
     this._add(folder, parentGuid);
   },
 
   onItemRemoved: function (itemId, parentId, index, type, uri,
-                           guid, parentGuid) {
-    if (this._ignore(itemId, parentId, guid)) {
+                           guid, parentGuid, source) {
+    if (this._ignore(itemId, parentId, guid, source)) {
       return;
     }
 
     this._log.trace("onItemRemoved: " + itemId);
     this._add(itemId, guid);
     this._add(parentId, parentGuid);
   },
 
@@ -1044,19 +1064,16 @@ BookmarksTracker.prototype = {
         id => PlacesUtils.annotations.getItemAnnotation(id, BookmarkAnnos.ORGANIZERQUERY_ANNO) == val
       );
 
     // Don't continue if the Library isn't ready
     let all = find(BookmarkAnnos.ALLBOOKMARKS_ANNO);
     if (all.length == 0)
       return;
 
-    // Disable handling of notifications while changing the mobile query
-    this.ignoreAll = true;
-
     let mobile = find(BookmarkAnnos.MOBILE_ANNO);
     let queryURI = Utils.makeURI("place:folder=" + BookmarkSpecialIds.mobile);
     let title = Str.sync.get("mobile.label");
 
     // Don't add OR remove the mobile bookmarks if there's nothing.
     if (PlacesUtils.bookmarks.getIdForItemAt(BookmarkSpecialIds.mobile, 0) == -1) {
       if (mobile.length != 0)
         PlacesUtils.bookmarks.removeItem(mobile[0], SOURCE_SYNC);
@@ -1068,52 +1085,49 @@ BookmarksTracker.prototype = {
                                   PlacesUtils.annotations.EXPIRE_NEVER, SOURCE_SYNC);
       PlacesUtils.annotations.setItemAnnotation(query, BookmarkAnnos.EXCLUDEBACKUP_ANNO, 1, 0,
                                   PlacesUtils.annotations.EXPIRE_NEVER, SOURCE_SYNC);
     }
     // Make sure the existing title is correct
     else if (PlacesUtils.bookmarks.getItemTitle(mobile[0]) != title) {
       PlacesUtils.bookmarks.setItemTitle(mobile[0], title, SOURCE_SYNC);
     }
-
-    this.ignoreAll = false;
   },
 
   // This method is oddly structured, but the idea is to return as quickly as
   // possible -- this handler gets called *every time* a bookmark changes, for
   // *each change*.
   onItemChanged: function BMT_onItemChanged(itemId, property, isAnno, value,
                                             lastModified, itemType, parentId,
-                                            guid, parentGuid) {
-    // Quicker checks first.
-    if (this.ignoreAll)
-      return;
-
+                                            guid, parentGuid, source) {
     if (isAnno && (ANNOS_TO_TRACK.indexOf(property) == -1))
       // Ignore annotations except for the ones that we sync.
       return;
 
     // Ignore favicon changes to avoid unnecessary churn.
     if (property == "favicon")
       return;
 
-    if (this._ignore(itemId, parentId, guid))
+    if (this._ignore(itemId, parentId, guid, source)) {
       return;
+    }
 
     this._log.trace("onItemChanged: " + itemId +
                     (", " + property + (isAnno? " (anno)" : "")) +
                     (value ? (" = \"" + value + "\"") : ""));
     this._add(itemId, guid);
   },
 
   onItemMoved: function BMT_onItemMoved(itemId, oldParent, oldIndex,
                                         newParent, newIndex, itemType,
-                                        guid, oldParentGuid, newParentGuid) {
-    if (this._ignore(itemId, newParent, guid))
+                                        guid, oldParentGuid, newParentGuid,
+                                        source) {
+    if (this._ignore(itemId, newParent, guid, source)) {
       return;
+    }
 
     this._log.trace("onItemMoved: " + itemId);
     this._add(oldParent, oldParentGuid);
     if (oldParent != newParent) {
       this._add(itemId, guid);
       this._add(newParent, newParentGuid);
     }
 
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -12,16 +12,113 @@ Cu.import("resource://services-sync/serv
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 Cu.import("resource://gre/modules/Promise.jsm");
 
 initTestLogging("Trace");
 
 Service.engineManager.register(BookmarksEngine);
 
+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 server = serverForFoo(engine);
+  new SyncTestingInfrastructure(server.server);
+
+  let collection = server.user("foo").collection("bookmarks");
+
+  Svc.Obs.notify("weave:engine:start-tracking");
+
+  try {
+    let folder1_id = PlacesUtils.bookmarks.createFolder(
+      PlacesUtils.bookmarks.toolbarFolder, "Folder 1", 0);
+    let folder1_guid = store.GUIDForId(folder1_id);
+    _(`Folder GUID: ${folder1_guid}`);
+
+    let bmk1_id = PlacesUtils.bookmarks.insertBookmark(
+      folder1_id, Utils.makeURI("http://getthunderbird.com/"),
+      PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!");
+    let bmk1_guid = store.GUIDForId(bmk1_id);
+    _(`Thunderbird GUID: ${bmk1_guid}`);
+
+    // Sync is synchronous, so, to simulate a bookmark change made during a
+    // sync, we create a server record that adds a bookmark as a side effect.
+    let bmk2_guid = "get-firefox1";
+    let bmk3_id = -1;
+    {
+      let localRecord = new Bookmark("bookmarks", bmk2_guid);
+      localRecord.bmkUri        = "http://getfirefox.com/";
+      localRecord.description   = "Firefox is awesome.";
+      localRecord.title         = "Get Firefox!";
+      localRecord.tags          = ["firefox", "awesome", "browser"];
+      localRecord.keyword       = "awesome";
+      localRecord.loadInSidebar = false;
+      localRecord.parentName    = "Folder 1";
+      localRecord.parentid      = folder1_guid;
+
+      let remoteRecord = collection.insert(bmk2_guid, encryptPayload(localRecord.cleartext));
+      remoteRecord.get = function get() {
+        _("Inserting bookmark into local store");
+        bmk3_id = PlacesUtils.bookmarks.insertBookmark(
+          folder1_id, Utils.makeURI("https://mozilla.org/"),
+          PlacesUtils.bookmarks.DEFAULT_INDEX, "Mozilla");
+
+        return ServerWBO.prototype.get.apply(this, arguments);
+      };
+    }
+
+    {
+      let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid);
+      let childGuids = tree.children.map(child => child.guid);
+      deepEqual(childGuids, [bmk1_guid], "Folder should have 1 child before first sync");
+    }
+
+    _("Perform first sync");
+    yield sync_engine_and_validate_telem(engine, false);
+
+    let bmk2_id = store.idForGUID(bmk2_guid);
+    let bmk3_guid = store.GUIDForId(bmk3_id);
+    _(`Mozilla GUID: ${bmk3_guid}`);
+    {
+      equal(store.GUIDForId(bmk2_id), bmk2_guid,
+        "Remote bookmark should be applied during first sync");
+      ok(bmk3_id > -1,
+        "Bookmark created during first sync should exist locally");
+      ok(!collection.wbo(bmk3_guid),
+        "Bookmark created during first sync shouldn't be uploaded yet");
+
+      let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid);
+      let childGuids = tree.children.map(child => child.guid);
+      deepEqual(childGuids, [bmk1_guid, bmk3_guid, bmk2_guid],
+        "Folder should have 3 children after first sync");
+    }
+
+    _("Perform second sync");
+    yield sync_engine_and_validate_telem(engine, false);
+
+    {
+      ok(collection.wbo(bmk3_guid),
+        "Bookmark created during first sync should be uploaded during second sync");
+
+      let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid);
+      let childGuids = tree.children.map(child => child.guid);
+      deepEqual(childGuids, [bmk1_guid, bmk3_guid, bmk2_guid],
+        "Folder should have same children after second sync");
+    }
+  } 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* bad_record_allIDs() {
   let server = new SyncServer();
   server.start();
   let syncTesting = new SyncTestingInfrastructure(server.server);
 
   _("Ensure that bad Places queries don't cause an error in getAllIDs.");
   let engine = new BookmarksEngine(Service);
   let store = engine._store;
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -3960,22 +3960,22 @@ nsNavHistoryFolderResultNode::OnItemMove
       NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
       nsresult rv = bookmarks->GetBookmarkURI(aItemId, getter_AddRefs(itemURI));
       NS_ENSURE_SUCCESS(rv, rv);
       rv = bookmarks->GetItemTitle(aItemId, itemTitle);
       NS_ENSURE_SUCCESS(rv, rv);
     }
     if (aOldParent == mTargetFolderItemId) {
       OnItemRemoved(aItemId, aOldParent, aOldIndex, aItemType, itemURI,
-                    aGUID, aOldParentGUID, nsINavBookmarksService::SOURCE_DEFAULT);
+                    aGUID, aOldParentGUID, aSource);
     }
     if (aNewParent == mTargetFolderItemId) {
       OnItemAdded(aItemId, aNewParent, aNewIndex, aItemType, itemURI, itemTitle,
                   RoundedPRNow(), // This is a dummy dateAdded, not the real value.
-                  aGUID, aNewParentGUID, nsINavBookmarksService::SOURCE_DEFAULT);
+                  aGUID, aNewParentGUID, aSource);
     }
   }
   return NS_OK;
 }
 
 
 /**
  * Separator nodes do not hold any data.