Bug 1317223 (part 5) - a bookmark repair responder. r=kitcambridge
☠☠ backed out by 44f18a46b421 ☠ ☠
authorMark Hammond <mhammond@skippinet.com.au>
Tue, 28 Feb 2017 15:34:37 +1100
changeset 374620 80fce16cfb57ad9fff9f3dc2268dd633dae8e3dc
parent 374619 e91a6d043fc6943c365d56ed34ce07e83f5fc4b9
child 374621 6878846abd906e547c8b05940a0fa66281c33890
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskitcambridge
bugs1317223
milestone54.0a1
Bug 1317223 (part 5) - a bookmark repair responder. r=kitcambridge This is the "repair responder" - it handles a "repairRequest" command sent by another client and attempts to take the list of IDs that client lists as missing and upload whatever records are necessary such that the requesting client would then be likely to find a complete and valid tree on the server. MozReview-Commit-ID: 4xw19nH6EfL
services/sync/modules/bookmark_repair.js
services/sync/modules/collection_repair.js
services/sync/tests/unit/test_bookmark_repair_responder.js
services/sync/tests/unit/xpcshell.ini
toolkit/components/places/PlacesSyncUtils.jsm
tools/lint/eslint/modules.json
--- a/services/sync/modules/bookmark_repair.js
+++ b/services/sync/modules/bookmark_repair.js
@@ -1,21 +1,24 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 const Cu = Components.utils;
 
-this.EXPORTED_SYMBOLS = ["BookmarkRepairRequestor"];
+this.EXPORTED_SYMBOLS = ["BookmarkRepairRequestor", "BookmarkRepairResponder"];
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
+Cu.import("resource://gre/modules/PlacesUtils.jsm");
+Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
+Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/collection_repair.js");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/resource.js");
 Cu.import("resource://services-sync/doctor.js");
 Cu.import("resource://services-common/utils.js");
 
 const log = Log.repository.getLogger("Sync.Engine.Bookmarks.Repair");
 
@@ -526,8 +529,163 @@ class BookmarkRepairRequestor extends Co
 
   /* Used for test mocks.
   */
   _now() {
     // We use the server time, which is SECONDS
     return AsyncResource.serverTime;
   }
 }
+
+/* An object that responds to repair requests initiated by some other device.
+*/
+class BookmarkRepairResponder extends CollectionRepairResponder {
+  async repair(request, rawCommand) {
+    if (request.request != "upload") {
+      this._abortRepair(request, rawCommand,
+                        `Don't understand request type '${request.request}'`);
+      return;
+    }
+
+    // Note that we don't try and guard against multiple repairs being in
+    // progress as we don't do anything too smart that could cause problems,
+    // but just upload items. If we get any smarter we should re-think this
+    // (but when we do, note that checking this._currentState isn't enough as
+    // this responder is not a singleton)
+
+    let engine = this.service.engineManager.get("bookmarks");
+
+    // Some items have been requested, but we need to be careful about how we
+    // handle them:
+    // * The item exists locally, but isn't in the tree of items we sync (eg, it
+    //   might be a left-pane item or similar.) We write a tombstone for these.
+    // * The item exists locally as a folder - and the children of the folder
+    //   also don't exist on the server - just uploading the folder isn't going
+    //   to help. (Note that we assume the parents *do* exist, otherwise the
+    //   device requesting the item be uploaded wouldn't be aware it exists)
+    // Bug 1343101 covers additional issues we might repair in the future.
+
+    let allIDs = new Set(); // all items we discovered inspecting the requested IDs.
+    let maybeToDelete = new Set(); // items we *may* delete.
+    let toUpload = new Set(); // items we will upload.
+    let results = await PlacesSyncUtils.bookmarks.fetchSyncIdsForRepair(request.ids);
+    for (let { syncId: id, syncable } of results) {
+      allIDs.add(id);
+      if (syncable) {
+        toUpload.add(id);
+      } else {
+        log.debug(`repair request to upload item ${id} but it isn't under a syncable root`);
+        maybeToDelete.add(id);
+      }
+    }
+    if (log.level <= Log.Level.Debug) {
+      let missingItems = request.ids.filter(id =>
+        !toUpload.has(id) && !maybeToDelete.has(id)
+      );
+      if (missingItems.length) {
+        log.debug("repair request to upload items that don't exist locally",
+                  missingItems);
+      }
+    }
+    // So we've now got items we know should potentially be uploaded or deleted.
+    // Query the server to find out what it actually has.
+    let existsRemotely = new Set(); // items we determine already exist on the server
+    let itemSource = engine.itemSource();
+    itemSource.ids = Array.from(allIDs);
+    log.trace(`checking the server for items`, itemSource.ids);
+    for (let remoteID of JSON.parse(itemSource.get())) {
+      log.trace(`the server has "${remoteID}"`);
+      existsRemotely.add(remoteID);
+      // This item exists on the server, so remove it from toUpload if it wasn't
+      // explicitly requested (ie, if it's just a child of a requested item and
+      // it exists then there's no need to upload it, but if it was explicitly
+      // requested, that may be due to the requestor believing it is corrupt.
+      if (request.ids.indexOf(remoteID) == -1) {
+        toUpload.delete(remoteID);
+      }
+    }
+    // We only need to flag as deleted items that actually are on the server.
+    let toDelete = CommonUtils.difference(maybeToDelete, existsRemotely);
+    // whew - now add these items to the tracker "weakly" (ie, they will not
+    // persist in the case of a restart, but that's OK - we'll then end up here
+    // again.)
+    log.debug(`repair request will upload ${toUpload.size} items and delete ${toDelete.size} items`);
+    for (let id of toUpload) {
+      engine._modified.setWeak(id, { tombstone: false });
+    }
+    for (let id of toDelete) {
+      engine._modified.setWeak(id, { tombstone: true });
+    }
+
+    // Add an observer for the engine sync being complete.
+    this._currentState = {
+      request,
+      rawCommand,
+      toUpload,
+      toDelete,
+    }
+    if (toUpload.size || toDelete.size) {
+      // We have arranged for stuff to be uploaded, so wait until that's done.
+      Svc.Obs.add("weave:engine:sync:uploaded", this.onUploaded, this);
+      // and record in telemetry that we got this far - just incase we never
+      // end up doing the upload for some obscure reason.
+      let eventExtra = {
+        flowID: request.flowID,
+        numIDs: (toUpload.size + toDelete.size).toString(),
+      };
+      this.service.recordTelemetryEvent("repairResponse", "uploading", undefined, eventExtra);
+    } else {
+      // We were unable to help with the repair, so report that we are done.
+      this._finishRepair();
+    }
+  }
+
+  onUploaded(subject, data) {
+    if (data != "bookmarks") {
+      return;
+    }
+    Svc.Obs.remove("weave:engine:sync:uploaded", this.onUploaded, this);
+    log.debug(`bookmarks engine has uploaded stuff - creating a repair response`);
+    this._finishRepair();
+  }
+
+  _finishRepair() {
+    let clientsEngine = this.service.clientsEngine;
+    let flowID = this._currentState.request.flowID;
+    let response = {
+      request: this._currentState.request.request,
+      collection: "bookmarks",
+      clientID: clientsEngine.localID,
+      flowID,
+      ids: [],
+    }
+    for (let id of this._currentState.toUpload) {
+      response.ids.push(id);
+    }
+    for (let id of this._currentState.toDelete) {
+      response.ids.push(id);
+    }
+    let clientID = this._currentState.request.requestor;
+    clientsEngine.sendCommand("repairResponse", [response], clientID, { flowID });
+    // and nuke the request from our client.
+    clientsEngine.removeLocalCommand(this._currentState.rawCommand);
+    let eventExtra = {
+      flowID,
+      numIDs: response.ids.length.toString(),
+    }
+    this.service.recordTelemetryEvent("repairResponse", "finished", undefined, eventExtra);
+    this._currentState = null;
+  }
+
+  _abortRepair(request, rawCommand, why) {
+    log.warn(`aborting repair request: ${why}`);
+    this.service.clientsEngine.removeLocalCommand(rawCommand);
+    // record telemetry for this.
+    let eventExtra = {
+      flowID: request.flowID,
+      reason: why,
+    };
+    this.service.recordTelemetryEvent("repairResponse", "aborted", undefined, eventExtra);
+    // We could also consider writing a response here so the requestor can take
+    // some immediate action rather than timing out, but we abort only in cases
+    // that should be rare, so let's wait and see what telemetry tells us.
+  }
+}
--- a/services/sync/modules/collection_repair.js
+++ b/services/sync/modules/collection_repair.js
@@ -17,16 +17,17 @@ this.EXPORTED_SYMBOLS = ["getRepairReque
                          "CollectionRepairResponder"];
 
 // The individual requestors/responders, lazily loaded.
 const REQUESTORS = {
   bookmarks: ["bookmark_repair.js", "BookmarkRepairRequestor"],
 }
 
 const RESPONDERS = {
+  bookmarks: ["bookmark_repair.js", "BookmarkRepairResponder"],
 }
 
 // Should we maybe enforce the requestors being a singleton?
 function _getRepairConstructor(which, collection) {
   if (!(collection in which)) {
     return null;
   }
   let [modname, symbolname] = which[collection];
new file mode 100644
--- /dev/null
+++ b/services/sync/tests/unit/test_bookmark_repair_responder.js
@@ -0,0 +1,498 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+Cu.import("resource://gre/modules/PlacesUtils.jsm");
+Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
+Cu.import("resource://testing-common/PlacesTestUtils.jsm");
+Cu.import("resource:///modules/PlacesUIUtils.jsm");
+Cu.import("resource://gre/modules/Log.jsm");
+
+Cu.import("resource://services-sync/engines/bookmarks.js");
+Cu.import("resource://services-sync/service.js");
+Cu.import("resource://services-sync/bookmark_repair.js");
+Cu.import("resource://testing-common/services/sync/utils.js");
+
+initTestLogging("Trace");
+Log.repository.getLogger("Sync.Engine.Bookmarks").level = Log.Level.Trace;
+
+// stub telemetry so we can easily check the right things are recorded.
+var recordedEvents = [];
+Service.recordTelemetryEvent = (object, method, value, extra = undefined) => {
+  recordedEvents.push({ object, method, value, extra });
+}
+
+function checkRecordedEvents(expected) {
+  deepEqual(recordedEvents, expected);
+  // and clear the list so future checks are easier to write.
+  recordedEvents = [];
+}
+
+function getServerBookmarks(server) {
+  return server.user("foo").collection("bookmarks");
+}
+
+async function setup() {
+  let clientsEngine = Service.clientsEngine;
+  let bookmarksEngine = Service.engineManager.get("bookmarks");
+
+  let server = serverForUsers({"foo": "password"}, {
+    meta: {
+      global: {
+        syncID: Service.syncID,
+        storageVersion: STORAGE_VERSION,
+        engines: {
+          clients: {
+            version: clientsEngine.version,
+            syncID: clientsEngine.syncID,
+          },
+          bookmarks: {
+            version: bookmarksEngine.version,
+            syncID: bookmarksEngine.syncID,
+          },
+        },
+      },
+    },
+    crypto: {
+      keys: encryptPayload({
+        id: "keys",
+        // Generate a fake default key bundle to avoid resetting the client
+        // before the first sync.
+        default: [
+          Svc.Crypto.generateRandomKey(),
+          Svc.Crypto.generateRandomKey(),
+        ],
+      }),
+    },
+  });
+
+  await SyncTestingInfrastructure(server);
+
+  // Disable validation so that we don't try to automatically repair the server
+  // when we sync.
+  Svc.Prefs.set("engine.bookmarks.validation.enabled", false);
+
+  return server;
+}
+
+async function cleanup(server) {
+  await promiseStopServer(server);
+  await PlacesSyncUtils.bookmarks.wipe();
+  Svc.Prefs.reset("engine.bookmarks.validation.enabled");
+}
+
+add_task(async function test_responder_no_items() {
+  let server = await setup();
+
+  let request = {
+    request: "upload",
+    ids: [Utils.makeGUID()],
+    flowID: Utils.makeGUID(),
+  }
+  let responder = new BookmarkRepairResponder();
+  await responder.repair(request, null);
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "finished",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "0"},
+    },
+  ]);
+
+  await cleanup(server);
+});
+
+// One item requested and we have it locally, but it's not yet on the server.
+add_task(async function test_responder_upload() {
+  let server = await setup();
+
+  // Pretend we've already synced this bookmark, so that we can ensure it's
+  // uploaded in response to our repair request.
+  let bm = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                                title: "Get Firefox",
+                                                url: "http://getfirefox.com/",
+                                                source: PlacesUtils.bookmarks.SOURCES.SYNC });
+
+  Service.sync();
+  deepEqual(getServerBookmarks(server).keys().sort(), [
+    "menu",
+    "mobile",
+    "toolbar",
+    "unfiled",
+  ], "Should only upload roots on first sync");
+
+  let request = {
+    request: "upload",
+    ids: [bm.guid],
+    flowID: Utils.makeGUID(),
+  }
+  let responder = new BookmarkRepairResponder();
+  await responder.repair(request, null);
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "uploading",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "1"},
+    },
+  ]);
+
+  Service.sync();
+  deepEqual(getServerBookmarks(server).keys().sort(), [
+    "menu",
+    "mobile",
+    "toolbar",
+    "unfiled",
+    bm.guid,
+  ].sort(), "Should upload requested bookmark on second sync");
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "finished",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "1"},
+    },
+  ]);
+
+  await cleanup(server);
+});
+
+// One item requested and we have it locally and it's already on the server.
+// As it was explicitly requested, we should upload it.
+add_task(async function test_responder_item_exists_locally() {
+  let server = await setup();
+
+  let bm = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                                title: "Get Firefox",
+                                                url: "http://getfirefox.com/" });
+  // first sync to get the item on the server.
+  _("Syncing to get item on the server");
+  Service.sync();
+
+  // issue a repair request for it.
+  let request = {
+    request: "upload",
+    ids: [bm.guid],
+    flowID: Utils.makeGUID(),
+  }
+  let responder = new BookmarkRepairResponder();
+  await responder.repair(request, null);
+
+  // We still re-upload the item.
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "uploading",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "1"},
+    },
+  ]);
+
+  _("Syncing to do the upload.");
+  Service.sync();
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "finished",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "1"},
+    },
+  ]);
+  await cleanup(server);
+});
+
+add_task(async function test_responder_tombstone() {
+  let server = await setup();
+
+  // TODO: Request an item for which we have a tombstone locally. Decide if
+  // we want to store tombstones permanently for this. In the integration
+  // test, we can also try requesting a deleted child or ancestor.
+
+  // For now, we'll handle this identically to `test_responder_missing_items`.
+  // Bug 1343103 is a follow-up to better handle this.
+  await cleanup(server);
+});
+
+add_task(async function test_responder_missing_items() {
+  let server = await setup();
+
+  let fxBmk = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    title: "Get Firefox",
+    url: "http://getfirefox.com/",
+  });
+  let tbBmk = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    title: "Get Thunderbird",
+    url: "http://getthunderbird.com/",
+    // Pretend we've already synced Thunderbird.
+    source: PlacesUtils.bookmarks.SOURCES.SYNC,
+  });
+
+  Service.sync();
+  deepEqual(getServerBookmarks(server).keys().sort(), [
+    "menu",
+    "mobile",
+    "toolbar",
+    "unfiled",
+    fxBmk.guid,
+  ].sort(), "Should upload roots and Firefox on first sync");
+
+  _("Request Firefox, Thunderbird, and nonexistent GUID");
+  let request = {
+    request: "upload",
+    ids: [fxBmk.guid, tbBmk.guid, Utils.makeGUID()],
+    flowID: Utils.makeGUID(),
+  }
+  let responder = new BookmarkRepairResponder();
+  await responder.repair(request, null);
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "uploading",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "2"},
+    },
+  ]);
+
+  _("Sync after requesting IDs");
+  Service.sync();
+  deepEqual(getServerBookmarks(server).keys().sort(), [
+    "menu",
+    "mobile",
+    "toolbar",
+    "unfiled",
+    fxBmk.guid,
+    tbBmk.guid,
+  ].sort(), "Second sync should upload Thunderbird; skip nonexistent");
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "finished",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "2"},
+    },
+  ]);
+
+  await cleanup(server);
+});
+
+add_task(async function test_non_syncable() {
+  let server = await setup();
+
+  // Creates the left pane queries as a side effect.
+  let leftPaneId = PlacesUIUtils.leftPaneFolderId;
+  _(`Left pane root ID: ${leftPaneId}`);
+  await PlacesTestUtils.promiseAsyncUpdates();
+
+  // A child folder of the left pane root, containing queries for the menu,
+  // toolbar, and unfiled queries.
+  let allBookmarksId = PlacesUIUtils.leftPaneQueries.AllBookmarks;
+  let allBookmarksGuid = await PlacesUtils.promiseItemGuid(allBookmarksId);
+
+  // Explicitly request the unfiled query; we should also upload tombstones
+  // for the menu and toolbar queries.
+  let unfiledQueryId = PlacesUIUtils.leftPaneQueries.UnfiledBookmarks;
+  let unfiledQueryGuid = await PlacesUtils.promiseItemGuid(unfiledQueryId);
+
+  let request = {
+    request: "upload",
+    ids: [allBookmarksGuid, unfiledQueryGuid],
+    flowID: Utils.makeGUID(),
+  }
+  let responder = new BookmarkRepairResponder();
+  await responder.repair(request, null);
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "uploading",
+      value: undefined,
+      // Tombstones for the folder and its 3 children.
+      extra: {flowID: request.flowID, numIDs: "4"},
+    },
+  ]);
+
+  _("Sync to upload tombstones for items");
+  Service.sync();
+
+  let toolbarQueryId = PlacesUIUtils.leftPaneQueries.BookmarksToolbar;
+  let menuQueryId = PlacesUIUtils.leftPaneQueries.BookmarksMenu;
+  let queryGuids = [
+    allBookmarksGuid,
+    await PlacesUtils.promiseItemGuid(toolbarQueryId),
+    await PlacesUtils.promiseItemGuid(menuQueryId),
+    unfiledQueryGuid,
+  ];
+
+  let collection = getServerBookmarks(server);
+  deepEqual(collection.keys().sort(), [
+    // We always upload roots on the first sync.
+    "menu",
+    "mobile",
+    "toolbar",
+    "unfiled",
+    ...queryGuids,
+  ].sort(), "Should upload roots and queries on first sync");
+
+  for (let guid of queryGuids) {
+    let wbo = collection.wbo(guid);
+    let payload = JSON.parse(JSON.parse(wbo.payload).ciphertext);
+    ok(payload.deleted, `Should upload tombstone for left pane query ${guid}`);
+  }
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "finished",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "4"},
+    },
+  ]);
+
+  await cleanup(server);
+});
+
+add_task(async function test_folder_descendants() {
+  let server = await setup();
+
+  let parentFolder = await PlacesUtils.bookmarks.insert({
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    parentGuid: PlacesUtils.bookmarks.menuGuid,
+    title: "Parent folder",
+  });
+  let childFolder = await PlacesUtils.bookmarks.insert({
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    parentGuid: parentFolder.guid,
+    title: "Child folder",
+  });
+  // This item is in parentFolder and *should not* be uploaded as part of
+  // the repair even though we explicitly request its parent.
+  let existingChildBmk = await PlacesUtils.bookmarks.insert({
+    parentGuid: parentFolder.guid,
+    title: "Get Firefox",
+    url: "http://firefox.com",
+  });
+  // This item is in parentFolder and *should* be uploaded as part of
+  // the repair because we explicitly request its ID.
+  let childSiblingBmk = await PlacesUtils.bookmarks.insert({
+    parentGuid: parentFolder.guid,
+    title: "Get Thunderbird",
+    url: "http://getthunderbird.com",
+  });
+
+  _("Initial sync to upload roots and parent folder");
+  Service.sync();
+
+  let initialSyncIds = [
+    "menu",
+    "mobile",
+    "toolbar",
+    "unfiled",
+    parentFolder.guid,
+    existingChildBmk.guid,
+    childFolder.guid,
+    childSiblingBmk.guid,
+  ].sort();
+  deepEqual(getServerBookmarks(server).keys().sort(), initialSyncIds,
+    "Should upload roots and partial folder contents on first sync");
+
+  _("Insert missing bookmarks locally to request later");
+  // Note that the fact we insert the bookmarks via PlacesSyncUtils.bookmarks.insert
+  // means that we are pretending Sync itself wrote them, hence they aren't
+  // considered "changed" locally so never get uploaded.
+  let childBmk = await PlacesSyncUtils.bookmarks.insert({
+    kind: "bookmark",
+    syncId: Utils.makeGUID(),
+    parentSyncId: parentFolder.guid,
+    title: "Get Firefox",
+    url: "http://getfirefox.com",
+  });
+  let grandChildBmk = await PlacesSyncUtils.bookmarks.insert({
+    kind: "bookmark",
+    syncId: Utils.makeGUID(),
+    parentSyncId: childFolder.guid,
+    title: "Bugzilla",
+    url: "https://bugzilla.mozilla.org",
+  });
+  let grandChildSiblingBmk = await PlacesSyncUtils.bookmarks.insert({
+    kind: "bookmark",
+    syncId: Utils.makeGUID(),
+    parentSyncId: childFolder.guid,
+    title: "Mozilla",
+    url: "https://mozilla.org",
+  });
+
+  _("Sync again; server contents shouldn't change");
+  Service.sync();
+  deepEqual(getServerBookmarks(server).keys().sort(), initialSyncIds,
+    "Second sync should not upload missing bookmarks");
+
+  // This assumes the parent record on the server is correct, and the server
+  // is just missing the children. This isn't a correct assumption if the
+  // parent's `children` array is wrong, or if the parent and children disagree.
+  _("Request missing bookmarks");
+  let request = {
+    request: "upload",
+    ids: [
+      // Already on server (but still uploaded as they are explicitly requested)
+      parentFolder.guid,
+      childSiblingBmk.guid,
+      // Explicitly upload these. We should also upload `grandChildBmk`,
+      // since it's a descendant of `parentFolder` and we requested its
+      // ancestor.
+      childBmk.syncId,
+      grandChildSiblingBmk.syncId],
+    flowID: Utils.makeGUID(),
+  }
+  let responder = new BookmarkRepairResponder();
+  await responder.repair(request, null);
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "uploading",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "5"},
+    },
+  ]);
+
+  _("Sync after requesting repair; should upload missing records");
+  Service.sync();
+  deepEqual(getServerBookmarks(server).keys().sort(), [
+    ...initialSyncIds,
+    childBmk.syncId,
+    grandChildBmk.syncId,
+    grandChildSiblingBmk.syncId,
+  ].sort(), "Third sync should upload requested items");
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "finished",
+      value: undefined,
+      extra: {flowID: request.flowID, numIDs: "5"},
+    },
+  ]);
+
+  await cleanup(server);
+});
+
+// Error handling.
+add_task(async function test_aborts_unknown_request() {
+  let server = await setup();
+
+  let request = {
+    request: "not-upload",
+    ids: [],
+    flowID: Utils.makeGUID(),
+  }
+  let responder = new BookmarkRepairResponder();
+  await responder.repair(request, null);
+
+  checkRecordedEvents([
+    { object: "repairResponse",
+      method: "aborted",
+      value: undefined,
+      extra: { flowID: request.flowID,
+               reason: "Don't understand request type 'not-upload'",
+             },
+    },
+  ]);
+  await cleanup(server);
+});
--- a/services/sync/tests/unit/xpcshell.ini
+++ b/services/sync/tests/unit/xpcshell.ini
@@ -132,16 +132,17 @@ tags = addons
 [test_bookmark_engine.js]
 [test_bookmark_invalid.js]
 [test_bookmark_legacy_microsummaries_support.js]
 [test_bookmark_livemarks.js]
 [test_bookmark_order.js]
 [test_bookmark_places_query_rewriting.js]
 [test_bookmark_record.js]
 [test_bookmark_repair_requestor.js]
+[test_bookmark_repair_responder.js]
 [test_bookmark_smart_bookmarks.js]
 [test_bookmark_store.js]
 [test_bookmark_decline_undecline.js]
 # Too many intermittent "ASSERTION: thread pool wasn't shutdown: '!mPool'" (bug 804479)
 skip-if = debug
 [test_bookmark_tracker.js]
 requesttimeoutfactor = 4
 [test_bookmark_validator.js]
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -122,16 +122,58 @@ const BookmarkSyncUtils = PlacesSyncUtil
     let db = yield PlacesUtils.promiseDBConnection();
     let childGuids = yield fetchChildGuids(db, parentGuid);
     return childGuids.map(guid =>
       BookmarkSyncUtils.guidToSyncId(guid)
     );
   }),
 
   /**
+   * Returns an array of `{ syncId, syncable }` tuples for all items in
+   * `requestedSyncIds`. If any requested ID is a folder, all its descendants
+   * will be included. Ancestors of non-syncable items are not included; if
+   * any are missing on the server, the requesting client will need to make
+   * another repair request.
+   *
+   * Sync calls this method to respond to incoming bookmark repair requests
+   * and upload items that are missing on the server.
+   */
+  fetchSyncIdsForRepair: Task.async(function* (requestedSyncIds) {
+    let requestedGuids = requestedSyncIds.map(BookmarkSyncUtils.syncIdToGuid);
+    let db = yield PlacesUtils.promiseDBConnection();
+    let rows = yield db.executeCached(`
+      WITH RECURSIVE
+      syncedItems(id) AS (
+        SELECT b.id FROM moz_bookmarks b
+        WHERE b.guid IN ('menu________', 'toolbar_____', 'unfiled_____',
+                         'mobile______')
+        UNION ALL
+        SELECT b.id FROM moz_bookmarks b
+        JOIN syncedItems s ON b.parent = s.id
+      ),
+      descendants(id) AS (
+        SELECT b.id FROM moz_bookmarks b
+        WHERE b.guid IN (${requestedGuids.map(guid => JSON.stringify(guid)).join(",")})
+        UNION ALL
+        SELECT b.id FROM moz_bookmarks b
+        JOIN descendants d ON d.id = b.parent
+      )
+      SELECT b.guid, s.id NOT NULL AS syncable
+      FROM descendants d
+      JOIN moz_bookmarks b ON b.id = d.id
+      LEFT JOIN syncedItems s ON s.id = d.id
+      `);
+    return rows.map(row => {
+      let syncId = BookmarkSyncUtils.guidToSyncId(row.getResultByName("guid"));
+      let syncable = !!row.getResultByName("syncable");
+      return { syncId, syncable };
+    });
+  }),
+
+  /**
    * Migrates an array of `{ syncId, modified }` tuples from the old JSON-based
    * tracker to the new sync change counter. `modified` is when the change was
    * added to the old tracker, in milliseconds.
    *
    * Sync calls this method before the first bookmark sync after the Places
    * schema migration.
    */
   migrateOldTrackerEntries(entries) {
--- a/tools/lint/eslint/modules.json
+++ b/tools/lint/eslint/modules.json
@@ -15,17 +15,17 @@
   "assertions.js": ["Assert", "Expect"],
   "async.js": ["Async"],
   "AsyncSpellCheckTestHelper.jsm": ["onSpellCheck"],
   "AutoMigrate.jsm": ["AutoMigrate"],
   "Battery.jsm": ["GetBattery", "Battery"],
   "blocklist-clients.js": ["AddonBlocklistClient", "GfxBlocklistClient", "OneCRLBlocklistClient", "PluginBlocklistClient"],
   "blocklist-updater.js": ["checkVersions", "addTestBlocklistClient"],
   "bogus_element_type.jsm": [],
-  "bookmark_repair.js": ["BookmarkRepairRequestor"],
+  "bookmark_repair.js": ["BookmarkRepairRequestor", "BookmarkRepairResponder"],
   "bookmark_validator.js": ["BookmarkValidator", "BookmarkProblemData"],
   "bookmarks.js": ["BookmarksEngine", "PlacesItem", "Bookmark", "BookmarkFolder", "BookmarkQuery", "Livemark", "BookmarkSeparator"],
   "bookmarks.jsm": ["PlacesItem", "Bookmark", "Separator", "Livemark", "BookmarkFolder", "DumpBookmarks"],
   "BootstrapMonitor.jsm": ["monitor"],
   "browser-loader.js": ["BrowserLoader"],
   "browserid_identity.js": ["BrowserIDManager", "AuthenticationError"],
   "CertUtils.jsm": ["BadCertHandler", "checkCert", "readCertPrefs", "validateCert"],
   "clients.js": ["ClientEngine", "ClientsRec"],