Bug 1368393 - Handle already synced commands in clients collection du…

…plicate checking r=eoger,markh

MozReview-Commit-ID: Hs0Jh8mMlOp

services/sync/modules/engines/clients.js

@@ -278,13 +278,19 @@ ClientEngine.prototype = {
   },
 
   _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;
   },
 

services/sync/tests/unit/test_clients_engine.js

@@ -1471,10 +1471,10 @@ add_task(async function ensureSameFlowIDs() {
     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();
@@ -1506,7 +1506,10 @@ add_task(async function ensureSameFlowIDs() {
     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();
@@ -1516,6 +1519,11 @@ add_task(async function ensureSameFlowIDs() {
     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" });
@@ -1524,6 +1532,10 @@ add_task(async function ensureSameFlowIDs() {
     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;
@@ -1534,9 +1546,68 @@ add_task(async function ensureSameFlowIDs() {
     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);
   }
 });