Bug 1433178 p2 - Allow callers to SyncedBookmarksMirror.apply() to pass explicit weak uploads. r?kitcambridge draft
authorEdouard Oger <eoger@fastmail.com>
Tue, 27 Feb 2018 18:01:19 -0500
changeset 762047 7c14371efc73f51562a847cf5bbe056b4fce462a
parent 762046 99a97938aecb675cfdc3ea944e81d609977f065f
push id101062
push userbmo:eoger@fastmail.com
push dateThu, 01 Mar 2018 18:27:01 +0000
reviewerskitcambridge
bugs1433178
milestone60.0a1
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]