Bug 1374722 - Expand the set of ids requested during the initial repair process r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 20 Jun 2017 13:40:17 -0400
changeset 614575 a32e7dc2e29663c09856ef642b3b2eb653a8cfd4
parent 614478 b8d5c1d7bf82ad052da3e846f8882f04b53a40c9
child 638912 07fb06d947c6c7246a680704462e62968fbbb924
push id70063
push userbmo:tchiovoloni@mozilla.com
push dateMon, 24 Jul 2017 20:17:40 +0000
reviewersmarkh
bugs1374722
milestone56.0a1
Bug 1374722 - Expand the set of ids requested during the initial repair process r?markh MozReview-Commit-ID: 92SGMd9fJgX
services/sync/modules/bookmark_repair.js
services/sync/tests/unit/test_bookmark_repair.js
services/sync/tests/unit/test_bookmark_repair_requestor.js
--- a/services/sync/modules/bookmark_repair.js
+++ b/services/sync/modules/bookmark_repair.js
@@ -106,17 +106,20 @@ class BookmarkRepairRequestor extends Co
     // report duplicates -- if the server is missing a record, it is unlikely
     // to cause only a single problem.
     let ids = new Set();
 
     // Note that we allow any of the validation problem fields to be missing so
     // that tests have a slightly easier time, hence the `|| []` in each loop.
 
     // Missing children records when the parent exists but a child doesn't.
-    for (let { child } of validationInfo.problems.missingChildren || []) {
+    for (let { parent, child } of validationInfo.problems.missingChildren || []) {
+      // We can't be sure if the child is missing or our copy of the parent is
+      // wrong, so request both
+      ids.add(parent);
       ids.add(child);
     }
     if (ids.size > MAX_REQUESTED_IDS) {
       return ids; // might as well give up here - we aren't going to repair.
     }
 
     // Orphans are when the child exists but the parent doesn't.
     // This could either be a problem in the child (it's wrong about the node
@@ -125,28 +128,32 @@ class BookmarkRepairRequestor extends Co
       // Request both, to handle both cases
       ids.add(id);
       ids.add(parent);
     }
     if (ids.size > MAX_REQUESTED_IDS) {
       return ids; // might as well give up here - we aren't going to repair.
     }
 
-    // Entries where we have the parent but know for certain that the child was
-    // deleted.
-    for (let { parent } of validationInfo.problems.deletedChildren || []) {
+    // Entries where we have the parent but we have a record from the server that
+    // claims the child was deleted.
+    for (let { parent, child } of validationInfo.problems.deletedChildren || []) {
+      // Request both, since we don't know if it's a botched deletion or revival
       ids.add(parent);
+      ids.add(child);
     }
     if (ids.size > MAX_REQUESTED_IDS) {
       return ids; // might as well give up here - we aren't going to repair.
     }
 
     // Entries where the child references a parent that we don't have, but we
-    // know why: the parent was deleted.
-    for (let { child } of validationInfo.problems.deletedParents || []) {
+    // have a record from the server that claims the parent was deleted.
+    for (let { parent, child } of validationInfo.problems.deletedParents || []) {
+      // Request both, since we don't know if it's a botched deletion or revival
+      ids.add(parent);
       ids.add(child);
     }
     if (ids.size > MAX_REQUESTED_IDS) {
       return ids; // might as well give up here - we aren't going to repair.
     }
 
     // Cases where the parent and child disagree about who the parent is.
     for (let { parent, child } of validationInfo.problems.parentChildMismatches || []) {
@@ -164,17 +171,16 @@ class BookmarkRepairRequestor extends Co
     // child is right.
     for (let { parents, child } of validationInfo.problems.multipleParents || []) {
       for (let parent of parents) {
         ids.add(parent);
       }
       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);
--- a/services/sync/tests/unit/test_bookmark_repair.js
+++ b/services/sync/tests/unit/test_bookmark_repair.js
@@ -158,34 +158,34 @@ add_task(async function test_bookmark_re
     await validationPromise;
     let flowID = Svc.Prefs.get("repairs.bookmarks.flowID");
     checkRecordedEvents([{
       object: "repair",
       method: "started",
       value: undefined,
       extra: {
         flowID,
-        numIDs: "1",
+        numIDs: "2",
       },
     }, {
       object: "sendcommand",
       method: "repairRequest",
       value: undefined,
       extra: {
         flowID,
         deviceID: Service.identity.hashedDeviceID(remoteID),
       },
     }, {
       object: "repair",
       method: "request",
       value: "upload",
       extra: {
         deviceID: Service.identity.hashedDeviceID(remoteID),
         flowID,
-        numIDs: "1",
+        numIDs: "2",
       },
     }], "Should record telemetry events for repair request");
 
     // We should have started a repair with our second client.
     equal((await clientsEngine.getClientCommands(remoteID)).length, 1,
       "Should queue repair request for remote client after repair");
     _("Sync to send outgoing repair request");
     await Service.sync();
@@ -220,33 +220,33 @@ add_task(async function test_bookmark_re
         flowID,
       },
     }, {
       object: "repairResponse",
       method: "uploading",
       value: undefined,
       extra: {
         flowID,
-        numIDs: "1",
+        numIDs: "2",
       },
     }, {
       object: "sendcommand",
       method: "repairResponse",
       value: undefined,
       extra: {
         flowID,
         deviceID: Service.identity.hashedDeviceID(initialID),
       },
     }, {
       object: "repairResponse",
       method: "finished",
       value: undefined,
       extra: {
         flowID,
-        numIDs: "1",
+        numIDs: "2",
       }
     }], "Should record telemetry events for repair response");
 
     // We should queue the repair response for the initial client.
     equal((await remoteClientsEngine.getClientCommands(initialID)).length, 1,
       "Should queue repair response for initial client after repair");
     ok(user.collection("bookmarks").wbo(bookmarkInfo.guid),
       "Should upload missing bookmark");
@@ -285,17 +285,17 @@ add_task(async function test_bookmark_re
       },
     }, {
       object: "repair",
       method: "response",
       value: "upload",
       extra: {
         flowID,
         deviceID: Service.identity.hashedDeviceID(remoteID),
-        numIDs: "1",
+        numIDs: "2",
       },
     }, {
       object: "repair",
       method: "finished",
       value: undefined,
       extra: {
         flowID,
         numIDs: "0",
--- a/services/sync/tests/unit/test_bookmark_repair_requestor.js
+++ b/services/sync/tests/unit/test_bookmark_repair_requestor.js
@@ -100,22 +100,22 @@ add_task(async function test_requestor_n
 
   await requestor.startRepairs(validationInfo, flowID);
   // there are no clients, so we should end up in "finished" (which we need to
   // check via telemetry)
   deepEqual(mockService._recordedEvents, [
     { object: "repair",
       method: "started",
       value: undefined,
-      extra: { flowID, numIDs: 3 },
+      extra: { flowID, numIDs: 4 },
     },
     { object: "repair",
       method: "finished",
       value: undefined,
-      extra: { flowID, numIDs: 3 },
+      extra: { flowID, numIDs: 4 },
     }
   ]);
 });
 
 add_task(async function test_requestor_one_client_no_response() {
   let mockService = new MockService({ "client-a": makeClientRecord("client-a") });
   let requestor = NewBookmarkRepairRequestor(mockService);
   let validationInfo = {
@@ -151,32 +151,32 @@ add_task(async function test_requestor_o
   await requestor.continueRepairs();
   // There are no more clients, so we've given up.
 
   checkRepairFinished();
   deepEqual(mockService._recordedEvents, [
     { object: "repair",
       method: "started",
       value: undefined,
-      extra: { flowID, numIDs: 3 },
+      extra: { flowID, numIDs: 4 },
     },
     { object: "repair",
       method: "request",
       value: "upload",
-      extra: { flowID, numIDs: 3, deviceID: "client-a" },
+      extra: { flowID, numIDs: 4, deviceID: "client-a" },
     },
     { object: "repair",
       method: "request",
       value: "upload",
-      extra: { flowID, numIDs: 3, deviceID: "client-a" },
+      extra: { flowID, numIDs: 4, deviceID: "client-a" },
     },
     { object: "repair",
       method: "finished",
       value: undefined,
-      extra: { flowID, numIDs: 3 },
+      extra: { flowID, numIDs: 4 },
     }
   ]);
 });
 
 add_task(async function test_requestor_one_client_no_sync() {
   let mockService = new MockService({ "client-a": makeClientRecord("client-a") });
   let requestor = NewBookmarkRepairRequestor(mockService);
   let validationInfo = {
@@ -203,32 +203,32 @@ add_task(async function test_requestor_o
   await requestor.continueRepairs();
 
   // We should be finished as we gave up in disgust.
   checkRepairFinished();
   deepEqual(mockService._recordedEvents, [
     { object: "repair",
       method: "started",
       value: undefined,
-      extra: { flowID, numIDs: 3 },
+      extra: { flowID, numIDs: 4 },
     },
     { object: "repair",
       method: "request",
       value: "upload",
-      extra: { flowID, numIDs: 3, deviceID: "client-a" },
+      extra: { flowID, numIDs: 4, deviceID: "client-a" },
     },
     { object: "repair",
       method: "abandon",
       value: "silent",
       extra: { flowID, deviceID: "client-a" },
     },
     { object: "repair",
       method: "finished",
       value: undefined,
-      extra: { flowID, numIDs: 3 },
+      extra: { flowID, numIDs: 4 },
     }
   ]);
 });
 
 add_task(async function test_requestor_latest_client_used() {
   let mockService = new MockService({
     "client-early": makeClientRecord("client-early", { serverLastModified: Date.now() - 10 }),
     "client-late": makeClientRecord("client-late", { serverLastModified: Date.now() }),
@@ -283,47 +283,47 @@ add_task(async function test_requestor_c
   checkOutgoingCommand(mockService, "client-b");
 
   // Now let's pretend client B wrote all missing IDs.
   let response = {
     collection: "bookmarks",
     request: "upload",
     flowID: requestor._flowID,
     clientID: "client-b",
-    ids: ["a", "b", "c"],
+    ids: ["a", "b", "c", "x"],
   }
   await requestor.continueRepairs(response);
 
   // We should be finished as we got all our IDs.
   checkRepairFinished();
   deepEqual(mockService._recordedEvents, [
     { object: "repair",
       method: "started",
       value: undefined,
-      extra: { flowID, numIDs: 3 },
+      extra: { flowID, numIDs: 4 },
     },
     { object: "repair",
       method: "request",
       value: "upload",
-      extra: { flowID, numIDs: 3, deviceID: "client-a" },
+      extra: { flowID, numIDs: 4, deviceID: "client-a" },
     },
     { object: "repair",
       method: "abandon",
       value: "missing",
       extra: { flowID, deviceID: "client-a" },
     },
     { object: "repair",
       method: "request",
       value: "upload",
-      extra: { flowID, numIDs: 3, deviceID: "client-b" },
+      extra: { flowID, numIDs: 4, deviceID: "client-b" },
     },
     { object: "repair",
       method: "response",
       value: "upload",
-      extra: { flowID, deviceID: "client-b", numIDs: 3 },
+      extra: { flowID, deviceID: "client-b", numIDs: 4 },
     },
     { object: "repair",
       method: "finished",
       value: undefined,
       extra: { flowID, numIDs: 0 },
     }
   ]);
 });
@@ -366,47 +366,47 @@ add_task(async function test_requestor_s
   checkOutgoingCommand(mockService, "client-b");
 
   // Now let's pretend client B write the missing ID.
   response = {
     collection: "bookmarks",
     request: "upload",
     clientID: "client-b",
     flowID: requestor._flowID,
-    ids: ["c"],
+    ids: ["c", "x"],
   }
   await requestor.continueRepairs(response);
 
   // We should be finished as we got all our IDs.
   checkRepairFinished();
   deepEqual(mockService._recordedEvents, [
     { object: "repair",
       method: "started",
       value: undefined,
-      extra: { flowID, numIDs: 3 },
+      extra: { flowID, numIDs: 4 },
     },
     { object: "repair",
       method: "request",
       value: "upload",
-      extra: { flowID, numIDs: 3, deviceID: "client-a" },
+      extra: { flowID, numIDs: 4, deviceID: "client-a" },
     },
     { object: "repair",
       method: "response",
       value: "upload",
       extra: { flowID, deviceID: "client-a", numIDs: 2 },
     },
     { object: "repair",
       method: "request",
       value: "upload",
-      extra: { flowID, numIDs: 1, deviceID: "client-b" },
+      extra: { flowID, numIDs: 2, deviceID: "client-b" },
     },
     { object: "repair",
       method: "response",
       value: "upload",
-      extra: { flowID, deviceID: "client-b", numIDs: 1 },
+      extra: { flowID, deviceID: "client-b", numIDs: 2 },
     },
     { object: "repair",
       method: "finished",
       value: undefined,
       extra: { flowID, numIDs: 0 },
     }
   ]);
 });
@@ -491,24 +491,24 @@ add_task(async function test_requestor_a
   await requestor.continueRepairs(response);
 
   // We should have aborted now
   checkRepairFinished();
   const expected = [
     { method: "started",
       object: "repair",
       value: undefined,
-      extra: { flowID, numIDs: "3" },
+      extra: { flowID, numIDs: "4" },
     },
     { method: "request",
       object: "repair",
       value: "upload",
-      extra: { flowID, numIDs: "3", deviceID: "client-a" },
+      extra: { flowID, numIDs: "4", deviceID: "client-a" },
     },
     { method: "aborted",
       object: "repair",
       value: undefined,
-      extra: { flowID, numIDs: "3", reason: "other clients repairing" },
+      extra: { flowID, numIDs: "4", reason: "other clients repairing" },
     }
   ];
 
   deepEqual(mockService._recordedEvents, expected);
 });