Bug 1374722 - Expand the set of ids requested during the initial repair process r=markh
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 20 Jun 2017 13:40:17 -0400
changeset 420804 14ca55749ce4f71c56c9b2214d582cc5b3e16542
parent 420803 213199e73ebfe35fd21f7ae1c1d2404dba48a892
child 420805 2c6f632e13263b78cae4e6822acf4e48e6a054c2
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1374722
milestone56.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 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);
 });