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
This commit is contained in:
Mark Hammond 2017-02-28 15:34:37 +11:00
Родитель 46e87ef366
Коммит 9b6693b8ee
6 изменённых файлов: 702 добавлений и 2 удалений

Просмотреть файл

@ -6,11 +6,14 @@
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");
@ -531,3 +534,158 @@ class BookmarkRepairRequestor extends CollectionRepairRequestor {
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.
}
}

Просмотреть файл

@ -22,6 +22,7 @@ const REQUESTORS = {
}
const RESPONDERS = {
bookmarks: ["bookmark_repair.js", "BookmarkRepairResponder"],
}
// Should we maybe enforce the requestors being a singleton?

Просмотреть файл

@ -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);
});

Просмотреть файл

@ -137,6 +137,7 @@ tags = addons
[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]

Просмотреть файл

@ -126,6 +126,48 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
);
}),
/**
* 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

Просмотреть файл

@ -20,7 +20,7 @@
"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"],