Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server. r=tcsc

MozReview-Commit-ID: 9Wxq2wfUnkb

--HG--
extra : rebase_source : 133133eb765a403a77ca3bbd0d8302c0a0be49a0

services/sync/modules/bookmark_repair.js

@@ -170,6 +170,27 @@ class BookmarkRepairRequestor extends CollectionRepairRequestor {
     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.

services/sync/modules/collection_repair.js

@@ -68,6 +68,17 @@ class CollectionRepairRequestor {
     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.

services/sync/modules/doctor.js

@@ -201,6 +201,10 @@ this.Doctor = {
     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}`);

services/sync/tests/unit/test_bookmark_repair.js

@@ -318,6 +318,270 @@ add_task(async function test_bookmark_repair_integration() {
     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));
+
+    let bookmarkInfo = await PlacesUtils.bookmarks.insert({
+      parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+      url: "http://getfirefox.com/",
+      title: "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 PlacesUtils.bookmarks.remove(bookmarkInfo.guid, {
+      source: PlacesUtils.bookmarks.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 PlacesUtils.bookmarks.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 PlacesUtils.bookmarks.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));
+
+    let bookmarkInfo = await PlacesUtils.bookmarks.insert({
+      parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+      url: "http://getfirefox.com/",
+      title: "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));
+
+    let bookmarkInfo = await PlacesUtils.bookmarks.insert({
+      parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+      url: "http://getfirefox.com/",
+      title: "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 PlacesUtils.bookmarks.fetch(bookmarkInfo.guid)));
   } finally {
     await cleanup(server);
   }

services/sync/tests/unit/test_doctor.js

@@ -82,6 +82,9 @@ add_task(async function test_repairs_start() {
       equal(validationInfo, problems);
       repairStarted = true;
       return true;
+    },
+    tryServerOnlyRepairs() {
+      return false;
     }
   }
   let doctor = mockDoctor({
@@ -110,6 +113,9 @@ add_task(async function test_repairs_advanced_daily() {
   let requestor = {
     continueRepairs() {
       repairCalls++;
+    },
+    tryServerOnlyRepairs() {
+      return false;
     }
   }
   // start now at just after REPAIR_ADVANCE_PERIOD so we do a a first one.
@@ -161,6 +167,9 @@ add_task(async function test_repairs_skip_if_cant_vaidate() {
   let requestor = {
     startRepairs(validationInfo, flowID) {
       assert.ok(false, "Never should start repairs");
+    },
+    tryServerOnlyRepairs() {
+      return false;
     }
   }
   let doctor = mockDoctor({