Bug 1433178 p2 - Allow callers to SyncedBookmarksMirror.apply() to pass explicit weak uploads. r=kitcambridge
authorEdouard Oger <eoger@fastmail.com>
Tue, 27 Feb 2018 18:01:19 -0500
changeset 461123 ae108452565924e3160d4bc62b596110dd45dd2c
parent 461122 4ccd9765a81772666816264b94d53d0fc2aeb07e
child 461124 3343d7e36d4ccfd8a49656310f30caa3800b2eb3
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskitcambridge
bugs1433178
milestone60.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 1433178 p2 - Allow callers to SyncedBookmarksMirror.apply() to pass explicit weak uploads. r=kitcambridge MozReview-Commit-ID: EqVmk7AqHWS
services/sync/modules/engines/bookmarks.js
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_explicit_weakupload.js
toolkit/components/places/tests/sync/xpcshell.ini
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -722,28 +722,27 @@ BufferedBookmarksEngine.prototype = {
     return new BufferedBookmarksChangeset();
   },
 
   async _processIncoming(newitems) {
     await super._processIncoming(newitems);
     let buf = await this._store.ensureOpenMirror();
     let recordsToUpload = await buf.apply({
       remoteTimeSeconds: Resource.serverTime,
+      weakUpload: [...this._needWeakUpload.keys()],
     });
+    this._needWeakUpload.clear();
     this._modified.replace(recordsToUpload);
   },
 
   async _reconcile(item) {
     return true;
   },
 
   async _createRecord(id) {
-    if (this._needWeakUpload.has(id)) {
-      return this._store.createRecord(id, this.name);
-    }
     let change = this._modified.changes[id];
     if (!change) {
       this._log.error("Creating record for item ${id} not in strong " +
                       "changeset", { id });
       throw new TypeError("Can't create record for unchanged item");
     }
     let record = this._recordFromCleartext(id, change.cleartext);
     record.sortindex = await this._store._calculateIndex(record);
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -305,24 +305,27 @@ class SyncedBookmarksMirror {
    * database until the transaction commits. Asynchronous consumers will retry
    * on `SQLITE_BUSY`; synchronous consumers will fail after waiting for 100ms.
    * See bug 1305563, comment 122 for details.
    *
    * @param  {Number} [options.localTimeSeconds]
    *         The current local time, in seconds.
    * @param  {Number} [options.remoteTimeSeconds]
    *         The current server time, in seconds.
+   * @param  {String[]} [options.weakUpload]
+   *         GUIDs of bookmarks to weakly upload.
    * @return {Object.<String, BookmarkChangeRecord>}
    *         A changeset containing locally changed and reconciled records to
    *         upload to the server, and to store in the mirror once upload
    *         succeeds.
    */
   async apply({ localTimeSeconds = Date.now() / 1000,
-                remoteTimeSeconds = 0 } = {}) {
-    let hasChanges = await this.hasChanges();
+                remoteTimeSeconds = 0,
+                weakUpload = [] } = {}) {
+    let hasChanges = weakUpload.length > 0 || (await this.hasChanges());
     if (!hasChanges) {
       MirrorLog.debug("No changes detected in both mirror and Places");
       return {};
     }
     // We intentionally don't use `executeBeforeShutdown` in this function,
     // since merging can take a while for large trees, and we don't want to
     // block shutdown. Since all new items are in the mirror, we'll just try
     // to merge again on the next sync.
@@ -420,17 +423,17 @@ class SyncedBookmarksMirror {
       // triggers above, because the structure might not be complete yet. An
       // incomplete structure might cause us to miss or record wrong parents and
       // positions.
 
       MirrorLog.debug("Recording observer notifications");
       await this.noteObserverChanges(observersToNotify);
 
       MirrorLog.debug("Staging locally changed items for upload");
-      await this.stageItemsToUpload();
+      await this.stageItemsToUpload(weakUpload);
 
       MirrorLog.debug("Fetching records for local items to upload");
       let changeRecords = await this.fetchLocalChangeRecords();
 
       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`);
@@ -1467,18 +1470,31 @@ class SyncedBookmarksMirror {
    * We'll still upload the new parent on the next sync, but, in the meantime,
    * we've introduced a parent-child disagreement. This can also happen if the
    * user moves many items between folders.
    *
    * Conceptually, `itemsToUpload` is a transient "view" of locally changed
    * items. The change counter in Places is the persistent record of items that
    * we need to upload, so, if upload is interrupted or fails, we'll stage the
    * items again on the next sync.
+   *
+   * @param  {String[]} weakUpload
+   *         GUIDs of bookmarks to weakly upload.
    */
-  async stageItemsToUpload() {
+  async stageItemsToUpload(weakUpload) {
+    // Stage explicit weak uploads such as repair responses.
+    for (let chunk of PlacesSyncUtils.chunkArray(weakUpload,
+      SQLITE_MAX_VARIABLE_NUMBER)) {
+      await this.db.execute(`
+        INSERT INTO itemsToWeaklyReupload(id)
+        SELECT b.id FROM moz_bookmarks b
+        WHERE b.guid IN (${new Array(chunk.length).fill("?").join(",")})`,
+        chunk);
+    }
+
     // Stage remotely changed items with older local creation dates. These are
     // tracked "weakly": if the upload is interrupted or fails, we won't
     // reupload the record on the next sync.
     await this.db.execute(`
       INSERT INTO itemsToWeaklyReupload(id)
       SELECT b.id FROM moz_bookmarks b
       JOIN mergeStates r ON r.mergedGuid = b.guid
       JOIN items v ON v.guid = r.mergedGuid
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/sync/test_bookmark_explicit_weakupload.js
@@ -0,0 +1,39 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+add_task(async function test_explicit_weakupload() {
+  let buf = await openMirror("weakupload");
+
+  await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.menuGuid,
+    children: [{
+      guid: "mozBmk______",
+      url: "https://mozilla.org",
+      title: "Mozilla",
+      tags: ["moz", "dot", "org"],
+    }],
+  });
+  await buf.store(shuffle([{
+    id: "menu",
+    type: "folder",
+    children: ["mozBmk______"],
+  }, {
+    id: "mozBmk______",
+    type: "bookmark",
+    title: "Mozilla",
+    bmkUri: "https://mozilla.org",
+    tags: ["moz", "dot", "org"],
+  }]), { needsMerge: false });
+  await PlacesTestUtils.markBookmarksAsSynced();
+
+  let changesToUpload = await buf.apply({
+    weakUpload: ["mozBmk______"]
+  });
+
+  ok("mozBmk______" in changesToUpload);
+  equal(changesToUpload.mozBmk______.counter, 0);
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});
--- a/toolkit/components/places/tests/sync/xpcshell.ini
+++ b/toolkit/components/places/tests/sync/xpcshell.ini
@@ -3,13 +3,14 @@ head = head_sync.js
 support-files =
   livemark.xml
   sync_utils_bookmarks.html
   sync_utils_bookmarks.json
 
 [test_bookmark_corruption.js]
 [test_bookmark_deduping.js]
 [test_bookmark_deletion.js]
+[test_bookmark_explicit_weakupload.js]
 [test_bookmark_haschanges.js]
 [test_bookmark_kinds.js]
 [test_bookmark_structure_changes.js]
 [test_bookmark_value_changes.js]
 [test_sync_utils.js]