Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server. r=tcsc
☠☠ backed out by 52a505ea3c6c ☠ ☠
authorEdouard Oger <eoger@fastmail.com>
Tue, 14 Mar 2017 18:02:20 -0400
changeset 349310 16d5620747919e3c0cab25f901053eb8c8196d60
parent 349309 873ccd759d932a973695329530a3fedf423997ad
child 349311 b31c893d19a012af563970eca9f4eb4694a48ae0
push id31550
push usercbook@mozilla.com
push dateFri, 24 Mar 2017 13:22:27 +0000
treeherdermozilla-central@473e0b201761 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc
bugs1339340
milestone55.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 1339340 - Repair items the bookmark validator reports as missing on the client or server. r=tcsc MozReview-Commit-ID: 9Wxq2wfUnkb
services/sync/modules/bookmark_repair.js
services/sync/modules/collection_repair.js
services/sync/modules/doctor.js
services/sync/tests/unit/test_bookmark_repair.js
--- a/services/sync/modules/bookmark_repair.js
+++ b/services/sync/modules/bookmark_repair.js
@@ -165,16 +165,37 @@ class BookmarkRepairRequestor extends Co
       }
       ids.add(child);
     }
 
     // XXX - any others we should consider?
     return ids;
   }
 
+  _countServerOnlyFixableProblems(validationInfo) {
+    const fixableProblems = ["clientMissing", "serverMissing", "serverDeleted"];
+    return fixableProblems.reduce((numProblems, problemLabel) => {
+      return numProblems + validationInfo.problems[problemLabel].length;
+    }, 0);
+  }
+
+  tryServerOnlyRepairs(validationInfo) {
+    if (this._countServerOnlyFixableProblems(validationInfo) == 0) {
+      return false;
+    }
+    let engine = this.service.engineManager.get("bookmarks");
+    for (let id of validationInfo.problems.serverMissing) {
+      engine._modified.setWeak(id, { tombstone: false });
+    }
+    let toFetch = engine.toFetch.concat(validationInfo.problems.clientMissing,
+                                        validationInfo.problems.serverDeleted);
+    engine.toFetch = Array.from(new Set(toFetch));
+    return true;
+  }
+
   /* See if the repairer is willing and able to begin a repair process given
      the specified validation information.
      Returns true if a repair was started and false otherwise.
   */
   startRepairs(validationInfo, flowID) {
     if (this._currentState != STATE.NOT_REPAIRING) {
       log.info(`Can't start a repair - repair with ID ${this._flowID} is already in progress`);
       return false;
--- a/services/sync/modules/collection_repair.js
+++ b/services/sync/modules/collection_repair.js
@@ -63,16 +63,27 @@ function getRepairResponder(collection) 
 
 // The abstract classes.
 class CollectionRepairRequestor {
   constructor(service = null) {
     // allow service to be mocked in tests.
     this.service = service || Weave.Service;
   }
 
+  /* Try to resolve some issues with the server without involving other clients.
+     Returns true if we repaired some items.
+
+     @param   validationInfo       {Object}
+              The validation info as returned by the collection's validator.
+
+  */
+  tryServerOnlyRepairs(validationInfo) {
+    return false;
+  }
+
   /* See if the repairer is willing and able to begin a repair process given
      the specified validation information.
      Returns true if a repair was started and false otherwise.
 
      @param   validationInfo       {Object}
               The validation info as returned by the collection's validator.
 
      @param   flowID               {String}
--- a/services/sync/modules/doctor.js
+++ b/services/sync/modules/doctor.js
@@ -196,16 +196,20 @@ this.Doctor = {
     if (!this._shouldRepair(engine)) {
       log.info(`Skipping repair of ${engine.name} - disabled via preferences`);
       return;
     }
 
     let requestor = this._getRepairRequestor(engine.name);
     let didStart = false;
     if (requestor) {
+      if (requestor.tryServerOnlyRepairs(validationResults)) {
+        return; // TODO: It would be nice if we could request a validation to be
+                // done on next sync.
+      }
       didStart = requestor.startRepairs(validationResults, flowID);
     }
     log.info(`${didStart ? "did" : "didn't"} start a repair of ${engine.name} with flowID ${flowID}`);
   },
 
   _shouldRepair(engine) {
     return Svc.Prefs.get(`engine.${engine.name}.repair.enabled`, false);
   },
--- a/services/sync/tests/unit/test_bookmark_repair.js
+++ b/services/sync/tests/unit/test_bookmark_repair.js
@@ -315,10 +315,268 @@ add_task(async function test_bookmark_re
         numIDs: "0",
       },
     }]);
     await revalidationPromise;
     ok(!Services.prefs.prefHasUserValue("services.sync.repairs.bookmarks.state"),
       "Should clear repair pref after successfully completing repair");
   } finally {
     await cleanup(server);
+    clientsEngine = Service.clientsEngine = new ClientEngine(Service);
+  }
+});
+
+add_task(async function test_repair_client_missing() {
+  enableValidationPrefs();
+
+  _("Ensure that a record missing from the client only will get re-downloaded from the server");
+
+  let contents = {
+    meta: {
+      global: {
+        engines: {
+          clients: {
+            version: clientsEngine.version,
+            syncID: clientsEngine.syncID,
+          },
+          bookmarks: {
+            version: bookmarksEngine.version,
+            syncID: bookmarksEngine.syncID,
+          },
+        }
+      }
+    },
+    clients: {},
+    bookmarks: {},
+    crypto: {},
+  };
+  let server = serverForUsers({"foo": "password"}, contents);
+  await SyncTestingInfrastructure(server);
+
+  let user = server.user("foo");
+
+  let initialID = Service.clientsEngine.localID;
+  let remoteID = Utils.makeGUID();
+  try {
+
+    _("Syncing to initialize crypto etc.");
+    Service.sync();
+
+    _("Create remote client record");
+    server.insertWBO("foo", "clients", new ServerWBO(remoteID, encryptPayload({
+      id: remoteID,
+      name: "Remote client",
+      type: "desktop",
+      commands: [],
+      version: "54",
+      protocols: ["1.5"],
+    }), Date.now() / 1000));
+
+    // Create a couple of bookmarks.
+    let bookmarkInfo = createBookmark(PlacesUtils.bookmarks.toolbarFolder,
+                                      "http://getfirefox.com/", "Get Firefox!");
+
+    let validationPromise = promiseValidationDone([]);
+    _("Syncing.");
+    Service.sync();
+    // should have 2 clients
+    equal(clientsEngine.stats.numClients, 2)
+    await validationPromise;
+
+    // Delete the bookmark localy, but cheat by telling places that Sync did
+    // it, so Sync still thinks we have it.
+    await bms.remove(bookmarkInfo.guid, {
+      source: bms.SOURCE_SYNC,
+    });
+    // sanity check we aren't going to sync this removal.
+    do_check_empty(bookmarksEngine.pullNewChanges());
+    // sanity check that the bookmark is not there anymore
+    do_check_false(await bms.fetch(bookmarkInfo.guid));
+
+    // sync again - we should have a few problems...
+    _("Syncing again.");
+    validationPromise = promiseValidationDone([
+      {"name":"clientMissing","count":1},
+      {"name":"structuralDifferences","count":1},
+    ]);
+    Service.sync();
+    await validationPromise;
+
+    // We shouldn't have started a repair with our second client.
+    equal(clientsEngine.getClientCommands(remoteID).length, 0);
+
+    // Trigger a sync (will request the missing item)
+    Service.sync();
+
+    // And we got our bookmark back
+    do_check_true(await bms.fetch(bookmarkInfo.guid));
+  } finally {
+    await cleanup(server);
   }
 });
+
+add_task(async function test_repair_server_missing() {
+  enableValidationPrefs();
+
+  _("Ensure that a record missing from the server only will get re-upload from the client");
+
+  let contents = {
+    meta: {
+      global: {
+        engines: {
+          clients: {
+            version: clientsEngine.version,
+            syncID: clientsEngine.syncID,
+          },
+          bookmarks: {
+            version: bookmarksEngine.version,
+            syncID: bookmarksEngine.syncID,
+          },
+        }
+      }
+    },
+    clients: {},
+    bookmarks: {},
+    crypto: {},
+  };
+  let server = serverForUsers({"foo": "password"}, contents);
+  await SyncTestingInfrastructure(server);
+
+  let user = server.user("foo");
+
+  let initialID = Service.clientsEngine.localID;
+  let remoteID = Utils.makeGUID();
+  try {
+
+    _("Syncing to initialize crypto etc.");
+    Service.sync();
+
+    _("Create remote client record");
+    server.insertWBO("foo", "clients", new ServerWBO(remoteID, encryptPayload({
+      id: remoteID,
+      name: "Remote client",
+      type: "desktop",
+      commands: [],
+      version: "54",
+      protocols: ["1.5"],
+    }), Date.now() / 1000));
+
+    // Create a couple of bookmarks.
+    let bookmarkInfo = createBookmark(PlacesUtils.bookmarks.toolbarFolder,
+                                      "http://getfirefox.com/", "Get Firefox!");
+
+    let validationPromise = promiseValidationDone([]);
+    _("Syncing.");
+    Service.sync();
+    // should have 2 clients
+    equal(clientsEngine.stats.numClients, 2)
+    await validationPromise;
+
+    // Now we will reach into the server and hard-delete the bookmark
+    user.collection("bookmarks").wbo(bookmarkInfo.guid).delete();
+
+    // sync again - we should have a few problems...
+    _("Syncing again.");
+    validationPromise = promiseValidationDone([
+      {"name":"serverMissing","count":1},
+      {"name":"missingChildren","count":1},
+    ]);
+    Service.sync();
+    await validationPromise;
+
+    // We shouldn't have started a repair with our second client.
+    equal(clientsEngine.getClientCommands(remoteID).length, 0);
+
+    // Trigger a sync (will upload the missing item)
+    Service.sync();
+
+    // And the server got our bookmark back
+    do_check_true(user.collection("bookmarks").wbo(bookmarkInfo.guid));
+  } finally {
+    await cleanup(server);
+  }
+});
+
+add_task(async function test_repair_server_deleted() {
+  enableValidationPrefs();
+
+  _("Ensure that a record marked as deleted on the server but present on the client will get deleted on the client");
+
+  let contents = {
+    meta: {
+      global: {
+        engines: {
+          clients: {
+            version: clientsEngine.version,
+            syncID: clientsEngine.syncID,
+          },
+          bookmarks: {
+            version: bookmarksEngine.version,
+            syncID: bookmarksEngine.syncID,
+          },
+        }
+      }
+    },
+    clients: {},
+    bookmarks: {},
+    crypto: {},
+  };
+  let server = serverForUsers({"foo": "password"}, contents);
+  await SyncTestingInfrastructure(server);
+
+  let user = server.user("foo");
+
+  let initialID = Service.clientsEngine.localID;
+  let remoteID = Utils.makeGUID();
+  try {
+
+    _("Syncing to initialize crypto etc.");
+    Service.sync();
+
+    _("Create remote client record");
+    server.insertWBO("foo", "clients", new ServerWBO(remoteID, encryptPayload({
+      id: remoteID,
+      name: "Remote client",
+      type: "desktop",
+      commands: [],
+      version: "54",
+      protocols: ["1.5"],
+    }), Date.now() / 1000));
+
+    // Create a couple of bookmarks.
+    let bookmarkInfo = createBookmark(PlacesUtils.bookmarks.toolbarFolder,
+                                      "http://getfirefox.com/", "Get Firefox!");
+
+    let validationPromise = promiseValidationDone([]);
+    _("Syncing.");
+    Service.sync();
+    // should have 2 clients
+    equal(clientsEngine.stats.numClients, 2)
+    await validationPromise;
+
+    // Now we will reach into the server and create a tombstone for that bookmark
+    server.insertWBO("foo", "bookmarks", new ServerWBO(bookmarkInfo.guid, encryptPayload({
+      id: bookmarkInfo.guid,
+      deleted: true,
+    }), Date.now() / 1000));
+
+    // sync again - we should have a few problems...
+    _("Syncing again.");
+    validationPromise = promiseValidationDone([
+      {"name":"serverDeleted","count":1},
+      {"name":"deletedChildren","count":1},
+      {"name":"orphans","count":1}
+    ]);
+    Service.sync();
+    await validationPromise;
+
+    // We shouldn't have started a repair with our second client.
+    equal(clientsEngine.getClientCommands(remoteID).length, 0);
+
+    // Trigger a sync (will upload the missing item)
+    Service.sync();
+
+    // And the client deleted our bookmark
+    do_check_true(!(await bms.fetch(bookmarkInfo.guid)));
+  } finally {
+    await cleanup(server);
+  }
+});