Bug 1472963 - Add a `totalSyncChanges` counter and use it in the bookmarks mirror. r=mak

This patch:

* Exposes a global Sync change counter on `nsINavBookmarksService`.
  This is similar to SQLite's `total_changes()`, but just for changes
  to bookmarks that affect Sync, and accounts for changes from multiple
  threads and connections.
* Adds a SQL function to bump the counter, and extends the
  `moz_bookmarks` triggers to call it.
* Moves merging outside the transaction in the bookmarks mirror, and
  checks that the counters match before applying.

Differential Revision: https://phabricator.services.mozilla.com/D2004

--HG--
extra : moz-landing-system : lando

services/sync/modules/engines/bookmarks.js

@@ -401,8 +401,10 @@ BaseBookmarksEngine.prototype = {
           "bookmarks");
       }
     } catch (ex) {
-      if (Async.isShutdownException(ex) || ex.status > 0) {
-        // Don't run maintenance on shutdown or HTTP errors.
+      if (Async.isShutdownException(ex) || ex.status > 0 ||
+          ex.name == "MergeConflictError") {
+        // Don't run maintenance on shutdown or HTTP errors, or if we aborted
+        // the sync because the user changed their bookmarks during merging.
         throw ex;
       }
       // Run Places maintenance periodically to try to recover from corruption

services/sync/tests/unit/test_bookmark_tracker.js

@@ -284,17 +284,20 @@ add_task(async function test_onItemAdded() {
     await startTracking();
 
     _("Insert a folder using the sync API");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     let syncFolderID = PlacesUtils.bookmarks.createFolder(
       PlacesUtils.bookmarks.bookmarksMenuFolder, "Sync Folder",
       PlacesUtils.bookmarks.DEFAULT_INDEX);
     let syncFolderGUID = await PlacesUtils.promiseItemGuid(syncFolderID);
     await verifyTrackedItems(["menu", syncFolderGUID]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
 
     await resetTracker();
     await startTracking();
 
     _("Insert a bookmark using the sync API");
+    totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     let syncBmkID = PlacesUtils.bookmarks.insertBookmark(syncFolderID,
       CommonUtils.makeURI("https://example.org/sync"),
       PlacesUtils.bookmarks.DEFAULT_INDEX,
@@ -302,6 +305,7 @@ add_task(async function test_onItemAdded() {
     let syncBmkGUID = await PlacesUtils.promiseItemGuid(syncBmkID);
     await verifyTrackedItems([syncFolderGUID, syncBmkGUID]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -315,6 +319,7 @@ add_task(async function test_async_onItemAdded() {
     await startTracking();
 
     _("Insert a folder using the async API");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     let asyncFolder = await PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_FOLDER,
       parentGuid: PlacesUtils.bookmarks.menuGuid,
@@ -322,11 +327,13 @@ add_task(async function test_async_onItemAdded() {
     });
     await verifyTrackedItems(["menu", asyncFolder.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
 
     await resetTracker();
     await startTracking();
 
     _("Insert a bookmark using the async API");
+    totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     let asyncBmk = await PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
       parentGuid: asyncFolder.guid,
@@ -335,11 +342,13 @@ add_task(async function test_async_onItemAdded() {
     });
     await verifyTrackedItems([asyncFolder.guid, asyncBmk.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
 
     await resetTracker();
     await startTracking();
 
     _("Insert a separator using the async API");
+    totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     let asyncSep = await PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_SEPARATOR,
       parentGuid: PlacesUtils.bookmarks.menuGuid,
@@ -347,6 +356,7 @@ add_task(async function test_async_onItemAdded() {
     });
     await verifyTrackedItems(["menu", asyncSep.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -371,6 +381,7 @@ add_task(async function test_async_onItemChanged() {
     await startTracking();
 
     _("Update the bookmark using the async API");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.update({
       guid: fxBmk.guid,
       title: "Download Firefox",
@@ -382,6 +393,7 @@ add_task(async function test_async_onItemChanged() {
 
     await verifyTrackedItems([fxBmk.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 1);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -405,6 +417,7 @@ add_task(async function test_onItemChanged_itemDates() {
     await startTracking();
 
     _("Reset the bookmark's added date, should not be tracked");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     let dateAdded = new Date(Date.now() - DAY_IN_MS);
     await PlacesUtils.bookmarks.update({
       guid: fx_bm.guid,
@@ -412,10 +425,12 @@ add_task(async function test_onItemChanged_itemDates() {
     });
     await verifyTrackedCount(0);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges);
 
     await resetTracker();
 
     _("Reset the bookmark's added date and another property, should be tracked");
+    totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     dateAdded = new Date();
     await PlacesUtils.bookmarks.update({
       guid: fx_bm.guid,
@@ -424,15 +439,18 @@ add_task(async function test_onItemChanged_itemDates() {
     });
     await verifyTrackedItems([fx_bm.guid]);
     Assert.equal(tracker.score, 2 * SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 1);
 
     await resetTracker();
 
     _("Set the bookmark's last modified date");
+    totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     let fx_id = await PlacesUtils.promiseItemId(fx_bm.guid);
     let dateModified = Date.now() * 1000;
     PlacesUtils.bookmarks.setItemLastModified(fx_id, dateModified);
     await verifyTrackedItems([fx_bm.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 1);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -465,11 +483,13 @@ add_task(async function test_onItemTagged() {
     await startTracking();
 
     _("Tag the item");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     PlacesUtils.tagging.tagURI(uri, ["foo"]);
 
     // bookmark should be tracked, folder should not be.
     await verifyTrackedItems([bGUID]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 6);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -498,10 +518,12 @@ add_task(async function test_onItemUntagged() {
     await startTracking();
 
     _("Remove the tag");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     PlacesUtils.tagging.untagURI(uri, ["foo"]);
 
     await verifyTrackedItems([fx1GUID, fx2GUID]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 4);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 5);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -541,10 +563,12 @@ add_task(async function test_async_onItemUntagged() {
     await startTracking();
 
     _("Remove the tag using the async bookmarks API");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.remove(fxTag.guid);
 
     await verifyTrackedItems([fxBmk1.guid, fxBmk2.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 4);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 5);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -595,6 +619,7 @@ add_task(async function test_async_onItemTagged() {
     });
 
     _("Tag an item using the async bookmarks API");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
       parentGuid: tag.guid,
@@ -603,6 +628,7 @@ add_task(async function test_async_onItemTagged() {
 
     await verifyTrackedItems([fxBmk1.guid, fxBmk2.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 4);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 5);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -632,6 +658,7 @@ add_task(async function test_async_onItemKeywordChanged() {
     await startTracking();
 
     _("Add a keyword for both items");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.keywords.insert({
       keyword: "the_keyword",
       url: "http://getfirefox.com",
@@ -640,6 +667,7 @@ add_task(async function test_async_onItemKeywordChanged() {
 
     await verifyTrackedItems([fxBmk1.guid, fxBmk2.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 2);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -673,10 +701,12 @@ add_task(async function test_async_onItemKeywordDeleted() {
     await startTracking();
 
     _("Remove the keyword");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.keywords.remove("the_keyword");
 
     await verifyTrackedItems([fxBmk1.guid, fxBmk2.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 2);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -831,6 +861,7 @@ add_task(async function test_onLivemarkAdded() {
     await startTracking();
 
     _("Insert a livemark");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     let livemark = await PlacesUtils.livemarks.addLivemark({
       parentGuid: PlacesUtils.bookmarks.menuGuid,
       // Use a local address just in case, to avoid potential aborts for
@@ -844,6 +875,7 @@ add_task(async function test_onLivemarkAdded() {
     // Two observer notifications: one for creating the livemark folder, and
     // one for setting the "livemark/feedURI" anno on the folder.
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 2);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -866,12 +898,14 @@ add_task(async function test_onLivemarkDeleted() {
     await startTracking();
 
     _("Remove a livemark");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.livemarks.removeLivemark({
       guid: livemark.guid,
     });
 
     await verifyTrackedItems(["menu", livemark.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -966,6 +1000,7 @@ add_task(async function test_async_onItemMoved_update() {
     await startTracking();
 
     _("Repositioning a bookmark should track the folder");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.update({
       guid: tbBmk.guid,
       parentGuid: PlacesUtils.bookmarks.menuGuid,
@@ -973,9 +1008,11 @@ add_task(async function test_async_onItemMoved_update() {
     });
     await verifyTrackedItems(["menu"]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 1);
     await resetTracker();
 
     _("Reparenting a bookmark should track both folders and the bookmark");
+    totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.update({
       guid: tbBmk.guid,
       parentGuid: PlacesUtils.bookmarks.toolbarGuid,
@@ -983,6 +1020,7 @@ add_task(async function test_async_onItemMoved_update() {
     });
     await verifyTrackedItems(["menu", "toolbar", tbBmk.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 3);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -1023,6 +1061,7 @@ add_task(async function test_async_onItemMoved_reorder() {
     await startTracking();
 
     _("Reorder bookmarks");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.reorder(PlacesUtils.bookmarks.menuGuid,
       [mozBmk.guid, fxBmk.guid, tbBmk.guid]);
 
@@ -1030,6 +1069,7 @@ add_task(async function test_async_onItemMoved_reorder() {
     // bump the score for every changed item.
     await verifyTrackedItems(["menu"]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 1);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -1126,6 +1166,7 @@ add_task(async function test_treeMoved() {
     await startTracking();
 
     // Move folder 2 to be a sibling of folder1.
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.update({
       guid: folder2.guid,
       parentGuid: PlacesUtils.bookmarks.menuGuid,
@@ -1135,6 +1176,7 @@ add_task(async function test_treeMoved() {
     // the menu and both folders should be tracked, the children should not be.
     await verifyTrackedItems(["menu", folder1.guid, folder2.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 3);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -1160,10 +1202,12 @@ add_task(async function test_onItemDeleted() {
     await startTracking();
 
     // Delete the last item - the item and parent should be tracked.
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     PlacesUtils.bookmarks.removeItem(tb_id);
 
     await verifyTrackedItems(["menu", tb_guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -1192,10 +1236,12 @@ add_task(async function test_async_onItemDeleted() {
     await startTracking();
 
     _("Delete the first item");
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.remove(fxBmk.guid);
 
     await verifyTrackedItems(["menu", fxBmk.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -1270,6 +1316,7 @@ add_task(async function test_async_onItemDeleted_eraseEverything() {
       guid: bugsChildFolder.guid,
       syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NEW,
     });
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     await PlacesUtils.bookmarks.eraseEverything();
 
     // bugsChildFolder's sync status is still "NEW", so it shouldn't be
@@ -1280,6 +1327,7 @@ add_task(async function test_async_onItemDeleted_eraseEverything() {
                               tbBmk.guid, "unfiled", bzBmk.guid,
                               bugsGrandChildBmk.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 8);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 11);
   } finally {
     _("Clean up.");
     await cleanup();
@@ -1321,10 +1369,12 @@ add_task(async function test_onItemDeleted_tree() {
     await startTracking();
 
     // Delete folder2 - everything we created should be tracked.
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
     PlacesUtils.bookmarks.removeItem(folder2_id);
 
     await verifyTrackedItems([fx_guid, tb_guid, folder1_guid, folder2_guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3);
+    Assert.equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 5);
   } finally {
     _("Clean up.");
     await cleanup();

toolkit/components/places/Database.cpp

@@ -1672,6 +1672,8 @@ Database::InitFunctions()
   NS_ENSURE_SUCCESS(rv, rv);
   rv = SqrtFunction::create(mMainConn);
   NS_ENSURE_SUCCESS(rv, rv);
+  rv = NoteSyncChangeFunction::create(mMainConn);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
@@ -1719,6 +1721,8 @@ Database::InitTempEntities()
   NS_ENSURE_SUCCESS(rv, rv);
   rv = mMainConn->ExecuteSimpleSQL(CREATE_KEYWORDS_FOREIGNCOUNT_AFTERUPDATE_TRIGGER);
   NS_ENSURE_SUCCESS(rv, rv);
+  rv = mMainConn->ExecuteSimpleSQL(CREATE_BOOKMARKS_DELETED_AFTERINSERT_TRIGGER);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }

toolkit/components/places/SQLFunctions.cpp

@@ -1422,5 +1422,36 @@ namespace places {
     return NS_OK;
   }
 
+////////////////////////////////////////////////////////////////////////////////
+//// Note Sync Change Function
+
+  /* static */
+  nsresult
+  NoteSyncChangeFunction::create(mozIStorageConnection *aDBConn)
+  {
+    RefPtr<NoteSyncChangeFunction> function =
+      new NoteSyncChangeFunction();
+    nsresult rv = aDBConn->CreateFunction(
+      NS_LITERAL_CSTRING("note_sync_change"), 0, function
+    );
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    return NS_OK;
+  }
+
+  NS_IMPL_ISUPPORTS(
+    NoteSyncChangeFunction,
+    mozIStorageFunction
+  )
+
+  NS_IMETHODIMP
+  NoteSyncChangeFunction::OnFunctionCall(mozIStorageValueArray *aArgs,
+                                         nsIVariant **_result)
+  {
+    nsNavBookmarks::NoteSyncChange();
+    *_result = nullptr;
+    return NS_OK;
+  }
+
 } // namespace places
 } // namespace mozilla

toolkit/components/places/SQLFunctions.h

@@ -590,6 +590,32 @@ private:
   ~SqrtFunction() {}
 };
 
+
+////////////////////////////////////////////////////////////////////////////////
+//// Note Sync Change Function
+
+/**
+ * Bumps the global Sync change counter. See the comment above
+ * `totalSyncChanges` in `nsINavBookmarksService` for a more detailed
+ * explanation.
+ */
+class NoteSyncChangeFunction final : public mozIStorageFunction
+{
+public:
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_MOZISTORAGEFUNCTION
+
+  /**
+   * Registers the function with the specified database connection.
+   *
+   * @param aDBConn
+   *        The database connection to register with.
+   */
+  static nsresult create(mozIStorageConnection *aDBConn);
+private:
+  ~NoteSyncChangeFunction() {}
+};
+
 } // namespace places
 } // namespace mozilla
 

toolkit/components/places/SyncedBookmarksMirror.jsm

@@ -447,6 +447,33 @@ class SyncedBookmarksMirror {
     // The flow ID is used to correlate telemetry events for each sync.
     let flowID = PlacesUtils.history.makeGuid();
 
+    let changeRecords;
+    try {
+      changeRecords = await this.tryApply(flowID, localTimeSeconds,
+                                          remoteTimeSeconds,
+                                          observersToNotify,
+                                          weakUpload);
+    } catch (ex) {
+      // Include the error message in the event payload, since we can't
+      // easily correlate event telemetry to engine errors in the Sync ping.
+      let why = (typeof ex.message == "string" ? ex.message :
+                 String(ex)).slice(0, 85);
+      this.recordTelemetryEvent("mirror", "apply", "error", { flowID, why });
+      throw ex;
+    }
+
+    MirrorLog.debug("Replaying recorded observer notifications");
+    try {
+      await observersToNotify.notifyAll();
+    } catch (ex) {
+      MirrorLog.warn("Error notifying Places observers", ex);
+    }
+
+    return changeRecords;
+  }
+
+  async tryApply(flowID, localTimeSeconds, remoteTimeSeconds, observersToNotify,
+                 weakUpload) {
     let { missingParents, missingChildren, parentsWithGaps } =
       await this.fetchRemoteOrphans();
     if (missingParents.length) {
@@ -501,149 +528,145 @@ class SyncedBookmarksMirror {
                       remoteTree.toASCIITreeString());
     }
 
-    let changeRecords;
-    try {
-      changeRecords = await this.db.executeTransaction(async () => {
-        let localTree = await withTiming(
-          "Building local tree from Places",
-          () => this.fetchLocalTree(localTimeSeconds),
-          (time, tree) => this.recordTelemetryEvent("mirror", "apply",
-            "fetchLocalTree", { flowID, time, deletions: tree.deletedGuids.size,
-                                nodes: tree.byGuid.size })
-        );
-        if (MirrorLog.level <= Log.Level.Debug) {
-          MirrorLog.debug("Built local tree from Places\n" +
-                          localTree.toASCIITreeString());
-        }
+    // We don't want to keep a transaction open while we're merging, since this
+    // can take some time for large trees, and the transaction will block the
+    // main connection from writing to Places. However, if the database changes
+    // as we're merging, the merged tree will no longer be valid, and we'll
+    // corrupt Places if we try to apply it. To work around this, we store the
+    // total Sync change count before accessing Places, and compare the current
+    // and stored counts after opening our transaction. If they match, we can
+    // safely apply the tree. Otherwise, we bail and try merging again on the
+    // next sync.
+    let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
 
-        let newRemoteContents = await withTiming(
-          "Fetching content info for new mirror items",
-          () => this.fetchNewRemoteContents(),
-          (time, contents) => this.recordTelemetryEvent("mirror", "apply",
-            "fetchNewRemoteContents", { flowID, time, count: contents.size })
-        );
+    let localTree = await withTiming(
+      "Building local tree from Places",
+      () => this.fetchLocalTree(localTimeSeconds),
+      (time, tree) => this.recordTelemetryEvent("mirror", "apply",
+        "fetchLocalTree", { flowID, time, deletions: tree.deletedGuids.size,
+                            nodes: tree.byGuid.size })
+    );
+    if (MirrorLog.level <= Log.Level.Debug) {
+      MirrorLog.debug("Built local tree from Places\n" +
+                      localTree.toASCIITreeString());
+    }
 
-        let newLocalContents = await withTiming(
-          "Fetching content info for new Places items",
-          () => this.fetchNewLocalContents(),
-          (time, contents) => this.recordTelemetryEvent("mirror", "apply",
-            "fetchNewLocalContents", { flowID, time, count: contents.size })
-        );
+    let newRemoteContents = await withTiming(
+      "Fetching content info for new mirror items",
+      () => this.fetchNewRemoteContents(),
+      (time, contents) => this.recordTelemetryEvent("mirror", "apply",
+        "fetchNewRemoteContents", { flowID, time, count: contents.size })
+    );
 
-        let merger = new BookmarkMerger(localTree, newLocalContents,
-                                        remoteTree, newRemoteContents);
-        let mergedRoot = await withTiming(
-          "Building complete merged tree",
-          () => merger.merge(),
-          time => {
-            this.recordTelemetryEvent("mirror", "apply", "merge",
-              { flowID, time, nodes: merger.mergedGuids.size,
-                localDeletions: merger.deleteLocally.size,
-                remoteDeletions: merger.deleteRemotely.size,
-                dupes: merger.dupeCount });
+    let newLocalContents = await withTiming(
+      "Fetching content info for new Places items",
+      () => this.fetchNewLocalContents(),
+      (time, contents) => this.recordTelemetryEvent("mirror", "apply",
+        "fetchNewLocalContents", { flowID, time, count: contents.size })
+    );
 
-            this.recordTelemetryEvent("mirror", "merge", "structure",
-              merger.structureCounts);
+    let merger = new BookmarkMerger(localTree, newLocalContents,
+                                    remoteTree, newRemoteContents);
+    let mergedRoot = await withTiming(
+      "Building complete merged tree",
+      () => merger.merge(),
+      time => {
+        this.recordTelemetryEvent("mirror", "apply", "merge",
+          { flowID, time, nodes: merger.mergedGuids.size,
+            localDeletions: merger.deleteLocally.size,
+            remoteDeletions: merger.deleteRemotely.size,
+            dupes: merger.dupeCount });
+
+        this.recordTelemetryEvent("mirror", "merge", "structure",
+          merger.structureCounts);
+      }
+    );
+    if (MirrorLog.level <= Log.Level.Debug) {
+      MirrorLog.debug([
+        "Built new merged tree",
+        mergedRoot.toASCIITreeString(),
+        ...merger.deletionsToStrings(),
+      ].join("\n"));
+    }
+
+    // The merged tree should know about all items mentioned in the local
+    // and remote trees. Otherwise, it's incomplete, and we'll corrupt
+    // Places or lose data on the server if we try to apply it.
+    if (!await merger.subsumes(localTree)) {
+      throw new SyncedBookmarksMirror.ConsistencyError(
+        "Merged tree doesn't mention all items from local tree");
+    }
+    if (!await merger.subsumes(remoteTree)) {
+      throw new SyncedBookmarksMirror.ConsistencyError(
+        "Merged tree doesn't mention all items from remote tree");
+    }
+
+    return this.db.executeTransaction(async () => {
+      if (totalSyncChanges != PlacesUtils.bookmarks.totalSyncChanges) {
+        throw new SyncedBookmarksMirror.MergeConflictError(
+          "Local tree changed during merge");
+      }
+
+      await withTiming(
+        "Applying merged tree",
+        async () => {
+          let deletions = [];
+          for await (let deletion of yieldingIterator(merger.deletions())) {
+            deletions.push(deletion);
           }
-        );
+          await this.updateLocalItemsInPlaces(mergedRoot, deletions);
+        },
+        time => this.recordTelemetryEvent("mirror", "apply",
+          "updateLocalItemsInPlaces", { flowID, time })
+      );
 
-        if (MirrorLog.level <= Log.Level.Debug) {
-          MirrorLog.debug([
-            "Built new merged tree",
-            mergedRoot.toASCIITreeString(),
-            ...merger.deletionsToStrings(),
-          ].join("\n"));
-        }
+      // At this point, the database is consistent, and we can fetch info to
+      // pass to observers. Note that we can't fetch observer info in the
+      // triggers above, because the structure might not be complete yet. An
+      // incomplete structure might cause us to miss or record wrong parents and
+      // positions.
 
-        // The merged tree should know about all items mentioned in the local
-        // and remote trees. Otherwise, it's incomplete, and we'll corrupt
-        // Places or lose data on the server if we try to apply it.
-        if (!await merger.subsumes(localTree)) {
-          throw new SyncedBookmarksMirror.ConsistencyError(
-            "Merged tree doesn't mention all items from local tree");
-        }
-        if (!await merger.subsumes(remoteTree)) {
-          throw new SyncedBookmarksMirror.ConsistencyError(
-            "Merged tree doesn't mention all items from remote tree");
-        }
+      await withTiming(
+        "Recording observer notifications",
+        () => this.noteObserverChanges(observersToNotify),
+        time => this.recordTelemetryEvent("mirror", "apply",
+          "noteObserverChanges", { flowID, time })
+      );
 
-        await withTiming(
-          "Applying merged tree",
-          async () => {
-            let deletions = [];
-            for await (let deletion of yieldingIterator(merger.deletions())) {
-              deletions.push(deletion);
-            }
-            await this.updateLocalItemsInPlaces(mergedRoot, deletions);
-          },
-          time => this.recordTelemetryEvent("mirror", "apply",
-            "updateLocalItemsInPlaces", { flowID, time })
-        );
+      await withTiming(
+        "Staging locally changed items for upload",
+        () => this.stageItemsToUpload(weakUpload),
+        time => this.recordTelemetryEvent("mirror", "apply",
+          "stageItemsToUpload", { flowID, time })
+      );
 
-        // At this point, the database is consistent, and we can fetch info to
-        // pass to observers. Note that we can't fetch observer info in the
-        // triggers above, because the structure might not be complete yet. An
-        // incomplete structure might cause us to miss or record wrong parents and
-        // positions.
+      let changeRecords = await withTiming(
+        "Fetching records for local items to upload",
+        () => this.fetchLocalChangeRecords(),
+        (time, records) => this.recordTelemetryEvent("mirror", "apply",
+          "fetchLocalChangeRecords", { flowID,
+            count: Object.keys(records).length })
+      );
 
-        await withTiming(
-          "Recording observer notifications",
-          () => this.noteObserverChanges(observersToNotify),
-          time => this.recordTelemetryEvent("mirror", "apply",
-            "noteObserverChanges", { flowID, time })
-        );
+      await withTiming(
+        "Cleaning up merge tables",
+        async () => {
+          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 itemsToUpload`);
+        },
+        time => this.recordTelemetryEvent("mirror", "apply", "cleanup",
+          { flowID, time })
+      );
 
-        await withTiming(
-          "Staging locally changed items for upload",
-          () => this.stageItemsToUpload(weakUpload),
-          time => this.recordTelemetryEvent("mirror", "apply",
-            "stageItemsToUpload", { flowID, time })
-        );
-
-        let changeRecords = await withTiming(
-          "Fetching records for local items to upload",
-          () => this.fetchLocalChangeRecords(),
-          (time, records) => this.recordTelemetryEvent("mirror", "apply",
-            "fetchLocalChangeRecords", { flowID,
-              count: Object.keys(records).length })
-        );
-
-        await withTiming(
-          "Cleaning up merge tables",
-          async () => {
-            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 itemsToUpload`);
-          },
-          time => this.recordTelemetryEvent("mirror", "apply", "cleanup",
-            { flowID, time })
-        );
-
-        return changeRecords;
-      });
-    } catch (ex) {
-      // Include the error message in the event payload, since we can't
-      // easily correlate event telemetry to engine errors in the Sync ping.
-      let why = (typeof ex.message == "string" ? ex.message :
-                 String(ex)).slice(0, 85);
-      this.recordTelemetryEvent("mirror", "apply", "error", { flowID, why });
-      throw ex;
-    }
-
-    MirrorLog.debug("Replaying recorded observer notifications");
-    try {
-      await observersToNotify.notifyAll();
-    } catch (ex) {
-      MirrorLog.warn("Error notifying Places observers", ex);
-    }
-
-    return changeRecords;
+      return changeRecords;
+    });
   }
 
   /**
@@ -1875,6 +1898,18 @@ class ConsistencyError extends Error {
 }
 SyncedBookmarksMirror.ConsistencyError = ConsistencyError;
 
+/**
+ * An error thrown when the merge can't proceed because the local tree
+ * changed during the merge.
+ */
+class MergeConflictError extends Error {
+  constructor(message) {
+    super(message);
+    this.name = "MergeConflictError";
+  }
+}
+SyncedBookmarksMirror.MergeConflictError = MergeConflictError;
+
 /**
  * An error thrown when the mirror database is corrupt, or can't be migrated to
  * the latest schema version, and must be replaced.

toolkit/components/places/nsINavBookmarksService.idl

@@ -301,6 +301,26 @@ interface nsINavBookmarksService : nsISupports
    */
   readonly attribute long long mobileFolder;
 
+  /**
+   * The total number of Sync changes (inserts, updates, deletes, merges, and
+   * uploads) recorded since Places startup for all bookmarks.
+   *
+   * Note that this is *not* the number of bookmark syncs. It's a monotonically
+   * increasing counter incremented for every change that affects a bookmark's
+   * `syncChangeCounter`.
+   *
+   * The counter can be used to avoid keeping an exclusive transaction open for
+   * time-consuming work. One way to do that is to store the current value of
+   * the counter, do the work, start a transaction, check the current value
+   * again, and compare it to the stored value to determine if the database
+   * changed during the work.
+   *
+   * The bookmarks mirror does this to check for changes between building and
+   * applying a merged tree. This avoids blocking the main Places connection
+   * during the merge, and ensures that the new tree still applies cleanly.
+   */
+  readonly attribute long long totalSyncChanges;
+
   /**
    * This value should be used for APIs that allow passing in an index
    * where an index is not known, or not required to be specified.

toolkit/components/places/nsNavBookmarks.cpp

@@ -195,6 +195,16 @@ nsNavBookmarks::StoreLastInsertedId(const nsACString& aTable,
 }
 
 
+Atomic<int64_t> nsNavBookmarks::sTotalSyncChanges(0);
+
+
+void // static
+nsNavBookmarks::NoteSyncChange()
+{
+  sTotalSyncChanges++;
+}
+
+
 nsresult
 nsNavBookmarks::Init()
 {
@@ -350,6 +360,14 @@ nsNavBookmarks::GetMobileFolder(int64_t* aRoot)
 }
 
 
+NS_IMETHODIMP
+nsNavBookmarks::GetTotalSyncChanges(int64_t* aTotalSyncChanges)
+{
+  *aTotalSyncChanges = sTotalSyncChanges;
+  return NS_OK;
+}
+
+
 nsresult
 nsNavBookmarks::InsertBookmarkInDB(int64_t aPlaceId,
                                    enum ItemType aItemType,

toolkit/components/places/nsNavBookmarks.h

@@ -228,6 +228,9 @@ public:
   static void StoreLastInsertedId(const nsACString& aTable,
                                   const int64_t aLastInsertedId);
 
+  static mozilla::Atomic<int64_t> sTotalSyncChanges;
+  static void NoteSyncChange();
+
 private:
   static nsNavBookmarks* gBookmarksService;
 

toolkit/components/places/nsPlacesTriggers.h

@@ -228,6 +228,7 @@
   "AFTER INSERT ON moz_bookmarks FOR EACH ROW " \
   "BEGIN " \
     "SELECT store_last_inserted_id('moz_bookmarks', NEW.id); " \
+    "SELECT note_sync_change() WHERE NEW.syncChangeCounter > 0; " \
     "UPDATE moz_places " \
     "SET foreign_count = foreign_count + 1 " \
     "WHERE id = NEW.fk;" \
@@ -236,14 +237,16 @@
 
 #define CREATE_BOOKMARKS_FOREIGNCOUNT_AFTERUPDATE_TRIGGER NS_LITERAL_CSTRING( \
   "CREATE TEMP TRIGGER moz_bookmarks_foreign_count_afterupdate_trigger " \
-  "AFTER UPDATE OF fk ON moz_bookmarks FOR EACH ROW " \
+  "AFTER UPDATE OF fk, syncChangeCounter ON moz_bookmarks FOR EACH ROW " \
   "BEGIN " \
+    "SELECT note_sync_change() " \
+    "WHERE NEW.syncChangeCounter <> OLD.syncChangeCounter; " \
     "UPDATE moz_places " \
     "SET foreign_count = foreign_count + 1 " \
-    "WHERE id = NEW.fk;" \
+    "WHERE OLD.fk <> NEW.fk AND id = NEW.fk;" \
     "UPDATE moz_places " \
     "SET foreign_count = foreign_count - 1 " \
-    "WHERE id = OLD.fk;" \
+    "WHERE OLD.fk <> NEW.fk AND id = OLD.fk;" \
   "END" \
 )
 
@@ -288,4 +291,12 @@
   "END" \
 )
 
+#define CREATE_BOOKMARKS_DELETED_AFTERINSERT_TRIGGER NS_LITERAL_CSTRING( \
+  "CREATE TEMP TRIGGER moz_bookmarks_deleted_afterinsert_v1_trigger " \
+  "AFTER INSERT ON moz_bookmarks_deleted FOR EACH ROW " \
+  "BEGIN " \
+    "SELECT note_sync_change(); " \
+  "END" \
+)
+
 #endif // __nsPlacesTriggers_h__

toolkit/components/places/tests/sync/test_bookmark_merge_conflicts.js

@@ -0,0 +1,91 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+ChromeUtils.import("resource://gre/modules/Timer.jsm");
+
+// This is tricky to test, because we need to induce a race between a write
+// on the main connection, and the merge transaction on the mirror connection.
+// We do this by starting a transaction on the main connection, then calling
+// `apply`. Both the main and mirror connections use WAL mode, so reading
+// from the mirror will succeed, and the mirror transaction will wait on
+// the main transaction to finish. We can then change bookmarks, commit the main
+// transaction, and check that we abort the merge.
+add_task(async function test_bookmark_change_during_sync() {
+  /* eslint-disable mozilla/no-arbitrary-setTimeout */
+  let buf = await openMirror("bookmark_change_during_sync");
+
+  info("Set up empty mirror");
+  await PlacesTestUtils.markBookmarksAsSynced();
+
+  info("Make remote changes");
+  await storeRecords(buf, [{
+    id: "menu",
+    type: "folder",
+    children: ["bookmarkAAAA"],
+  }, {
+    id: "bookmarkAAAA",
+    type: "bookmark",
+    title: "A",
+    bmkUri: "http://example.com/a",
+  }]);
+
+  await PlacesUtils.withConnectionWrapper("test_bookmark_change_during_sync", async function(db) {
+    info("Open main transaction");
+    await db.execute(`BEGIN EXCLUSIVE`);
+
+    // TODO(lina): We should have the mirror emit an event instead of using
+    // a timeout.
+    info("Wait for mirror to merge");
+    let applyPromise = buf.apply();
+    await new Promise(resolve => setTimeout(resolve, 5000));
+
+    info("Change local bookmark");
+    await db.execute(`
+      UPDATE moz_bookmarks SET
+        syncChangeCounter = syncChangeCounter + 1
+      WHERE guid = :toolbarGuid`,
+      { toolbarGuid: PlacesUtils.bookmarks.toolbarGuid });
+
+    await db.execute(`COMMIT`);
+
+    await Assert.rejects(applyPromise, /Local tree changed during merge/,
+      "Should fail merge if local tree changes before applying");
+  });
+
+  let changesToUpload = await buf.apply();
+  deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+
+  let infoForA = await PlacesUtils.bookmarks.fetch("bookmarkAAAA");
+  let datesAdded = await promiseManyDatesAdded([
+    PlacesUtils.bookmarks.toolbarGuid, "bookmarkAAAA"]);
+  deepEqual(changesToUpload, {
+    toolbar: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "toolbar",
+        type: "folder",
+        parentid: "places",
+        hasDupe: true,
+        parentName: "",
+        dateAdded: datesAdded.get(PlacesUtils.bookmarks.toolbarGuid),
+        title: BookmarksToolbarTitle,
+        children: [],
+      },
+    },
+  }, "Should upload flagged toolbar");
+  deepEqual(infoForA, {
+    guid: "bookmarkAAAA",
+    parentGuid: PlacesUtils.bookmarks.menuGuid,
+    index: 0,
+    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+    title: "A",
+    lastModified: infoForA.lastModified,
+    url: infoForA.url,
+  }, "Should apply A");
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});

toolkit/components/places/tests/sync/test_sync_utils.js

@@ -2142,6 +2142,7 @@ add_task(async function test_pushChanges() {
   }
 
   info("Pull changes");
+  let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
   let changes = await PlacesSyncUtils.bookmarks.pullChanges();
   {
     let actualChanges = Object.entries(changes).map(([recordId, change]) => ({
@@ -2185,6 +2186,7 @@ add_task(async function test_pushChanges() {
   }
 
   await PlacesSyncUtils.bookmarks.pushChanges(changes);
+  equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 3);
 
   {
     let fields = await PlacesTestUtils.fetchBookmarkSyncFields(
@@ -2246,6 +2248,7 @@ add_task(async function test_changes_between_pull_and_push() {
   });
 
   info("Pull changes");
+  let totalSyncChanges = PlacesUtils.bookmarks.totalSyncChanges;
   let changes = await PlacesSyncUtils.bookmarks.pullChanges();
   Assert.equal(changes[guids.bmk].counter, 1);
   Assert.equal(changes[guids.bmk].tombstone, false);
@@ -2255,6 +2258,7 @@ add_task(async function test_changes_between_pull_and_push() {
 
   info("Push changes");
   await PlacesSyncUtils.bookmarks.pushChanges(changes);
+  equal(PlacesUtils.bookmarks.totalSyncChanges, totalSyncChanges + 2);
 
   // we should have a tombstone.
   let ts = await PlacesTestUtils.fetchSyncTombstones();

toolkit/components/places/tests/sync/xpcshell.ini

@@ -13,6 +13,7 @@ support-files =
 [test_bookmark_explicit_weakupload.js]
 [test_bookmark_haschanges.js]
 [test_bookmark_kinds.js]
+[test_bookmark_merge_conflicts.js]
 [test_bookmark_mirror_meta.js]
 [test_bookmark_mirror_migration.js]
 [test_bookmark_observer_recorder.js]