Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. r?tcsc
MozReview-Commit-ID: DhrZ1oFY2WG
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -45,16 +45,17 @@ XPCOMUtils.defineLazyModuleGetter(this,
XPCOMUtils.defineLazyModuleGetter(this, "getRepairResponder",
"resource://services-sync/collection_repair.js");
const CLIENTS_TTL = 1814400; // 21 days
const CLIENTS_TTL_REFRESH = 604800; // 7 days
const STALE_CLIENT_REMOTE_AGE = 604800; // 7 days
const SUPPORTED_PROTOCOL_VERSIONS = [SYNC_API_VERSION];
+const LAST_MODIFIED_ON_PROCESS_COMMAND_PREF = "services.sync.clients.lastModifiedOnProcessCommands";
function hasDupeCommand(commands, action) {
if (!commands) {
return false;
}
return commands.some(other => other.command == action.command &&
Utils.deepEquals(other.args, action.args));
}
@@ -87,16 +88,27 @@ this.ClientEngine = function ClientEngin
ClientEngine.prototype = {
__proto__: SyncEngine.prototype,
_storeObj: ClientStore,
_recordObj: ClientsRec,
_trackerObj: ClientsTracker,
allowSkippedRecord: false,
_knownStaleFxADeviceIds: null,
+ // These two properties allow us to avoid replaying the same commands
+ // continuously if we cannot manage to upload our own record.
+ _localClientLastModified: 0,
+ get _lastModifiedOnProcessCommands() {
+ return Services.prefs.getIntPref(LAST_MODIFIED_ON_PROCESS_COMMAND_PREF, -1);
+ },
+
+ set _lastModifiedOnProcessCommands(value) {
+ Services.prefs.setIntPref(LAST_MODIFIED_ON_PROCESS_COMMAND_PREF, value);
+ },
+
// Always sync client data as it controls other sync behavior
get enabled() {
return true;
},
get lastRecordUpload() {
return Svc.Prefs.get(this.name + ".lastRecordUpload", 0);
},
@@ -369,16 +381,17 @@ ClientEngine.prototype = {
await this._removeRemoteClient(id);
}
}
// Bug 1264498: Mobile clients don't remove themselves from the clients
// collection when the user disconnects Sync, so we mark as stale clients
// with the same name that haven't synced in over a week.
// (Note we can't simply delete them, or we re-apply them next sync - see
// bug 1287687)
+ this._localClientLastModified = Math.round(this._incomingClients[this.localID]);
delete this._incomingClients[this.localID];
let names = new Set([this.localName]);
for (let [id, serverLastModified] of Object.entries(this._incomingClients)) {
let record = this._store._remoteClients[id];
// stash the server last-modified time on the record.
record.serverLastModified = serverLastModified;
if (record.fxaDeviceId && this._knownStaleFxADeviceIds.includes(record.fxaDeviceId)) {
this._log.info(`Hiding stale client ${id} - in known stale clients list`);
@@ -633,19 +646,22 @@ ClientEngine.prototype = {
/**
* Check if the local client has any remote commands and perform them.
*
* @return false to abort sync
*/
async processIncomingCommands() {
return this._notify("clients:process-commands", "", async function() {
- if (!this.localCommands) {
+ if (!this.localCommands ||
+ (this._lastModifiedOnProcessCommands == this._localClientLastModified
+ && !this.ignoreLastModifiedOnProcessCommands)) {
return true;
}
+ this._lastModifiedOnProcessCommands = this._localClientLastModified;
const clearedCommands = await this._readCommands()[this.localID];
const commands = this.localCommands.filter(command => !hasDupeCommand(clearedCommands, command));
let didRemoveCommand = false;
let URIsToDisplay = [];
// Process each command in order.
for (let rawCommand of commands) {
let shouldRemoveCommand = true; // most commands are auto-removed.
--- a/services/sync/tests/unit/test_bookmark_repair.js
+++ b/services/sync/tests/unit/test_bookmark_repair.js
@@ -33,16 +33,17 @@ const BOOKMARK_REPAIR_STATE_PREFS = [
];
let clientsEngine;
let bookmarksEngine;
var recordedEvents = [];
add_task(async function setup() {
clientsEngine = Service.clientsEngine;
+ clientsEngine.ignoreLastModifiedOnProcessCommands = true;
bookmarksEngine = Service.engineManager.get("bookmarks");
await generateNewKeys(Service.collectionKeys);
Service.recordTelemetryEvent = (object, method, value, extra = undefined) => {
recordedEvents.push({ object, method, value, extra });
};
});
@@ -195,16 +196,17 @@ add_task(async function test_bookmark_re
"Should not record repair telemetry after sending repair request");
_("Back up repair state to restore later");
let restoreInitialRepairState = backupPrefs(BOOKMARK_REPAIR_STATE_PREFS);
// so now let's take over the role of that other client!
_("Create new clients engine pretending to be remote client");
let remoteClientsEngine = Service.clientsEngine = new ClientEngine(Service);
+ remoteClientsEngine.ignoreLastModifiedOnProcessCommands = true;
await remoteClientsEngine.initialize();
remoteClientsEngine.localID = remoteID;
_("Restore missing bookmark");
// Pretend Sync wrote the bookmark, so that we upload it as part of the
// repair instead of the sync.
bookmarkInfo.source = PlacesUtils.bookmarks.SOURCE_SYNC;
await PlacesUtils.bookmarks.insert(bookmarkInfo);
@@ -302,16 +304,17 @@ add_task(async function test_bookmark_re
},
}]);
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);
+ clientsEngine.ignoreLastModifiedOnProcessCommands = true;
clientsEngine.initialize();
}
});
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");
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -1285,17 +1285,17 @@ add_task(async function test_keep_cleare
},
{
command: "displayURI",
args: ["https://deviceclink2.com", deviceCID, "Device C link 2"],
flowID: Utils.makeGUID(),
}],
version: "48",
protocols: ["1.5"],
- }), now - 10));
+ }), now - 5));
// Simulate reboot
SyncEngine.prototype._uploadOutgoing = oldUploadOutgoing;
engine = Service.clientsEngine = new ClientEngine(Service);
await engine.initialize();
commandsProcessed = 0;
engine._handleDisplayURIs = (uris) => { commandsProcessed = uris.length };
@@ -1727,13 +1727,31 @@ add_task(async function process_incoming
await engine._processIncoming();
ok(stubRefresh.notCalled, "Should not refresh the known stale clients since it's already populated");
stubProcessIncoming.restore();
stubRefresh.restore();
});
+add_task(async function process_incoming_refreshes_known_stale_clients() {
+ Services.prefs.clearUserPref("services.sync.clients.lastModifiedOnProcessCommands");
+ engine._localClientLastModified = Math.round(Date.now() / 1000);
+
+ const stubRemoveLocalCommand = sinon.stub(engine, "removeLocalCommand");
+ const tabProcessedSpy = sinon.spy(engine, "_handleDisplayURIs");
+ engine.localCommands = [{ command: "displayURI", args: ["https://foo.bar", "fxaid1", "foo"] }];
+
+ await engine.processIncomingCommands();
+ ok(tabProcessedSpy.calledOnce);
+ // Let's say we failed to upload and we end up calling processIncomingCommands again
+ await engine.processIncomingCommands();
+ ok(tabProcessedSpy.calledOnce);
+
+ tabProcessedSpy.restore();
+ stubRemoveLocalCommand.restore();
+});
+
function run_test() {
initTestLogging("Trace");
Log.repository.getLogger("Sync.Engine.Clients").level = Log.Level.Trace;
run_next_test();
}