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 313066 db9dfcdbef4aa204aba5d9b3c384f21ee1c66c61
parent 313065 af11902f242f7bdaa7437a497f286e2528af4f31
child 313067 85e02f8c2b692c2f241783d604e67ec15a3a785f
push id20479
push userkwierso@gmail.com
push dateThu, 08 Sep 2016 01:08:46 +0000
treeherderfx-team@fb7c6b034329 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1299338
milestone51.0a1
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.