--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -273,23 +273,29 @@ ClientEngine.prototype = {
removeLocalCommand(command) {
// the implementation of this engine is such that adding a command to
// the local client is how commands are deleted! ¯\_(ツ)_/¯
this._addClientCommand(this.localID, command);
},
_addClientCommand(clientId, command) {
- const allCommands = this._readCommands();
- const clientCommands = allCommands[clientId] || [];
+ const localCommands = this._readCommands();
+ const localClientCommands = localCommands[clientId] || [];
+ const remoteClient = this._store._remoteClients[clientId];
+ let remoteClientCommands = []
+ if (remoteClient && remoteClient.commands) {
+ remoteClientCommands = remoteClient.commands;
+ }
+ const clientCommands = localClientCommands.concat(remoteClientCommands);
if (hasDupeCommand(clientCommands, command)) {
return false;
}
- allCommands[clientId] = clientCommands.concat(command);
- this._saveCommands(allCommands);
+ localCommands[clientId] = localClientCommands.concat(command);
+ this._saveCommands(localCommands);
return true;
},
_removeClientCommands(clientId) {
const allCommands = this._readCommands();
delete allCommands[clientId];
this._saveCommands(allCommands);
},
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -1464,20 +1464,20 @@ add_task(async function test_command_syn
add_task(async function ensureSameFlowIDs() {
let events = []
let origRecordTelemetryEvent = Service.recordTelemetryEvent;
Service.recordTelemetryEvent = (object, method, value, extra) => {
events.push({ object, method, value, extra });
}
+ let server = serverForFoo(engine);
try {
// Setup 2 clients, send them a command, and ensure we get to events
// written, both with the same flowID.
- let server = serverForFoo(engine);
await SyncTestingInfrastructure(server);
let remoteId = Utils.makeGUID();
let remoteId2 = Utils.makeGUID();
_("Create remote client record 1");
server.insertWBO("foo", "clients", new ServerWBO(remoteId, encryptPayload({
id: remoteId,
@@ -1499,47 +1499,118 @@ add_task(async function ensureSameFlowID
}), Date.now() / 1000));
engine._sync();
engine.sendCommand("wipeAll", []);
engine._sync();
equal(events.length, 2);
// we don't know what the flowID is, but do know it should be the same.
equal(events[0].extra.flowID, events[1].extra.flowID);
-
+ // Wipe remote clients to ensure deduping doesn't prevent us from adding the command.
+ for (let client of Object.values(engine._store._remoteClients)) {
+ client.commands = [];
+ }
// check it's correctly used when we specify a flow ID
events.length = 0;
let flowID = Utils.makeGUID();
engine.sendCommand("wipeAll", [], null, { flowID });
engine._sync();
equal(events.length, 2);
equal(events[0].extra.flowID, flowID);
equal(events[1].extra.flowID, flowID);
+ // Wipe remote clients to ensure deduping doesn't prevent us from adding the command.
+ for (let client of Object.values(engine._store._remoteClients)) {
+ client.commands = [];
+ }
+
// and that it works when something else is in "extra"
events.length = 0;
engine.sendCommand("wipeAll", [], null, { reason: "testing" });
engine._sync();
equal(events.length, 2);
equal(events[0].extra.flowID, events[1].extra.flowID);
equal(events[0].extra.reason, "testing");
equal(events[1].extra.reason, "testing");
+ // Wipe remote clients to ensure deduping doesn't prevent us from adding the command.
+ for (let client of Object.values(engine._store._remoteClients)) {
+ client.commands = [];
+ }
// and when both are specified.
events.length = 0;
engine.sendCommand("wipeAll", [], null, { reason: "testing", flowID });
engine._sync();
equal(events.length, 2);
equal(events[0].extra.flowID, flowID);
equal(events[1].extra.flowID, flowID);
equal(events[0].extra.reason, "testing");
equal(events[1].extra.reason, "testing");
+ // Wipe remote clients to ensure deduping doesn't prevent us from adding the command.
+ for (let client of Object.values(engine._store._remoteClients)) {
+ client.commands = [];
+ }
} finally {
Service.recordTelemetryEvent = origRecordTelemetryEvent;
+ cleanup();
+ await promiseStopServer(server);
+ }
+});
+
+add_task(async function test_duplicate_commands_telemetry() {
+ let events = []
+ let origRecordTelemetryEvent = Service.recordTelemetryEvent;
+ Service.recordTelemetryEvent = (object, method, value, extra) => {
+ events.push({ object, method, value, extra });
+ }
+
+ let server = serverForFoo(engine);
+ try {
+ await SyncTestingInfrastructure(server);
+
+ let remoteId = Utils.makeGUID();
+ let remoteId2 = Utils.makeGUID();
+
+ _("Create remote client record 1");
+ server.insertWBO("foo", "clients", new ServerWBO(remoteId, encryptPayload({
+ id: remoteId,
+ name: "Remote client",
+ type: "desktop",
+ commands: [],
+ version: "48",
+ protocols: ["1.5"]
+ }), Date.now() / 1000));
+
+ _("Create remote client record 2");
+ server.insertWBO("foo", "clients", new ServerWBO(remoteId2, encryptPayload({
+ id: remoteId2,
+ name: "Remote client 2",
+ type: "mobile",
+ commands: [],
+ version: "48",
+ protocols: ["1.5"]
+ }), Date.now() / 1000));
+
+ engine._sync();
+ // Make sure deduping works before syncing
+ engine.sendURIToClientForDisplay("https://example.com", remoteId, "Example");
+ engine.sendURIToClientForDisplay("https://example.com", remoteId, "Example");
+ equal(events.length, 1);
+ engine._sync();
+ // And after syncing.
+ engine.sendURIToClientForDisplay("https://example.com", remoteId, "Example");
+ equal(events.length, 1);
+ // Ensure we aren't deduping commands to different clients
+ engine.sendURIToClientForDisplay("https://example.com", remoteId2, "Example");
+ equal(events.length, 2);
+ } finally {
+ Service.recordTelemetryEvent = origRecordTelemetryEvent;
+ cleanup();
+ await promiseStopServer(server);
}
});
function run_test() {
initTestLogging("Trace");
Log.repository.getLogger("Sync.Engine.Clients").level = Log.Level.Trace;
run_next_test();
}