Bug 1264498 - Hide duplicate remote Sync clients that haven't synced in a week. r?markh draft
authorKit Cambridge <kcambridge@mozilla.com>
Fri, 15 Apr 2016 09:00:59 -0700
changeset 353788 5bac99e3a8466c5ee5a18219e682627eabcbf637
parent 353712 ae7413abfa4d3954a6a4ce7c1613a7100f367f9a
child 518882 8c0eb5651925b476c972e17f8db6eb994e6c373d
push id15947
push userkcambridge@mozilla.com
push dateTue, 19 Apr 2016 22:52:51 +0000
reviewersmarkh
bugs1264498
milestone48.0a1
Bug 1264498 - Hide duplicate remote Sync clients that haven't synced in a week. r?markh MozReview-Commit-ID: LaVb2pABu0X
services/sync/modules/engines/clients.js
services/sync/tests/unit/test_clients_engine.js
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -9,24 +9,26 @@ this.EXPORTED_SYMBOLS = [
 
 var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://services-common/async.js");
 Cu.import("resource://services-common/stringbundle.js");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/record.js");
+Cu.import("resource://services-sync/resource.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://gre/modules/Services.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts",
   "resource://gre/modules/FxAccounts.jsm");
 
 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 = ["1.1", "1.5"];
 
 function hasDupeCommand(commands, action) {
   if (!commands) {
     return false;
   }
   return commands.some(other => other.command == action.command &&
@@ -168,28 +170,46 @@ ClientEngine.prototype = {
       this.lastRecordUpload = Date.now() / 1000;
     }
     SyncEngine.prototype._syncStartup.call(this);
   },
 
   _processIncoming() {
     // Fetch all records from the server.
     this.lastSync = 0;
-    this._incomingClients = [];
+    this._incomingClients = {};
     try {
       SyncEngine.prototype._processIncoming.call(this);
       // Since clients are synced unconditionally, any records in the local store
       // that don't exist on the server must be for disconnected clients. Remove
       // them, so that we don't upload records with commands for clients that will
       // never see them. We also do this to filter out stale clients from the
       // tabs collection, since showing their list of tabs is confusing.
-      let remoteClientIDs = Object.keys(this._store._remoteClients);
-      let staleIDs = Utils.arraySub(remoteClientIDs, this._incomingClients);
-      for (let staleID of staleIDs) {
-        this._removeRemoteClient(staleID);
+      for (let id in this._store._remoteClients) {
+        if (!this._incomingClients[id]) {
+          this._log.info(`Removing local state for deleted client ${id}`);
+          this._removeRemoteClient(id);
+        }
+      }
+      // Bug 1264498: Mobile clients don't remove themselves from the clients
+      // collection when the user disconnects Sync, so we filter out clients
+      // with the same name that haven't synced in over a week.
+      delete this._incomingClients[this.localID];
+      let names = new Set([this.localName]);
+      for (let id in this._incomingClients) {
+        let record = this._store._remoteClients[id];
+        if (!names.has(record.name)) {
+          names.add(record.name);
+          continue;
+        }
+        let remoteAge = AsyncResource.serverTime - this._incomingClients[id];
+        if (remoteAge > STALE_CLIENT_REMOTE_AGE) {
+          this._log.info(`Hiding stale client ${id} with age ${remoteAge}`);
+          this._removeRemoteClient(id);
+        }
       }
     } finally {
       this._incomingClients = null;
     }
   },
 
   _uploadOutgoing() {
     this._clearedCommands = null;
@@ -214,17 +234,17 @@ ClientEngine.prototype = {
       Services.telemetry.getHistogramById(hid).add(count);
     }
     SyncEngine.prototype._syncFinish.call(this);
   },
 
   _reconcile: function _reconcile(item) {
     // Every incoming record is reconciled, so we use this to track the
     // contents of the collection on the server.
-    this._incomingClients.push(item.id);
+    this._incomingClients[item.id] = item.modified;
 
     if (!this._store.itemExists(item.id)) {
       return true;
     }
     // Clients are synced unconditionally, so we'll always have new records.
     // Unfortunately, this will cause the scheduler to use the immediate sync
     // interval for the multi-device case, instead of the active interval. We
     // work around this by updating the record during reconciliation, and
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -490,25 +490,138 @@ add_test(function test_process_incoming_
   _("Ensures local commands are executed");
 
   engine.localCommands = [{ command: "logout", args: [] }];
 
   let ev = "weave:service:logout:finish";
 
   var handler = function() {
     Svc.Obs.remove(ev, handler);
+
+    Svc.Prefs.resetBranch("");
+    Service.recordManager.clearCache();
+    engine._resetClient();
+
     run_next_test();
   };
 
   Svc.Obs.add(ev, handler);
 
   // logout command causes processIncomingCommands to return explicit false.
   do_check_false(engine.processIncomingCommands());
 });
 
+add_test(function test_filter_duplicate_names() {
+  _("Ensure that we exclude clients with identical names that haven't synced in a week.");
+
+  let now = Date.now() / 1000;
+  let contents = {
+    meta: {global: {engines: {clients: {version: engine.version,
+                                        syncID: engine.syncID}}}},
+    clients: {},
+    crypto: {}
+  };
+  let server = serverForUsers({"foo": "password"}, contents);
+  let user   = server.user("foo");
+
+  new SyncTestingInfrastructure(server.server);
+  generateNewKeys(Service.collectionKeys);
+
+  // Synced recently.
+  let recentID = Utils.makeGUID();
+  server.insertWBO("foo", "clients", new ServerWBO(recentID, encryptPayload({
+    id: recentID,
+    name: "My Phone",
+    type: "mobile",
+    commands: [],
+    version: "48",
+    protocols: ["1.5"],
+  }), now - 10));
+
+  // Dupe of our client, synced more than 1 week ago.
+  let dupeID = Utils.makeGUID();
+  server.insertWBO("foo", "clients", new ServerWBO(dupeID, encryptPayload({
+    id: dupeID,
+    name: engine.localName,
+    type: "desktop",
+    commands: [],
+    version: "48",
+    protocols: ["1.5"],
+  }), now - 604810));
+
+  // Synced more than 1 week ago, but not a dupe.
+  let oldID = Utils.makeGUID();
+  server.insertWBO("foo", "clients", new ServerWBO(oldID, encryptPayload({
+    id: oldID,
+    name: "My old desktop",
+    type: "desktop",
+    commands: [],
+    version: "48",
+    protocols: ["1.5"],
+  }), now - 604820));
+
+  try {
+    let store = engine._store;
+
+    _("First sync");
+    strictEqual(engine.lastRecordUpload, 0);
+    engine._sync();
+    ok(engine.lastRecordUpload > 0);
+    deepEqual(user.collection("clients").keys().sort(),
+              [recentID, dupeID, oldID, engine.localID].sort(),
+              "Our record should be uploaded on first sync");
+    deepEqual(Object.keys(store.getAllIDs()).sort(),
+              [recentID, oldID, engine.localID].sort(),
+              "Fresh clients should be downloaded on first sync");
+
+    _("Broadcast logout to all clients");
+    engine.sendCommand("logout", []);
+    engine._sync();
+
+    let collection = server.getCollection("foo", "clients");
+    let recentPayload = JSON.parse(JSON.parse(collection.payload(recentID)).ciphertext);
+    deepEqual(recentPayload.commands, [{ command: "logout", args: [] }],
+              "Should send commands to the recent client");
+
+    let oldPayload = JSON.parse(JSON.parse(collection.payload(oldID)).ciphertext);
+    deepEqual(oldPayload.commands, [{ command: "logout", args: [] }],
+              "Should send commands to the week-old client");
+
+    let dupePayload = JSON.parse(JSON.parse(collection.payload(dupeID)).ciphertext);
+    deepEqual(dupePayload.commands, [],
+              "Should not send commands to the dupe client");
+
+    _("Update the dupe client's modified time");
+    server.insertWBO("foo", "clients", new ServerWBO(dupeID, encryptPayload({
+      id: dupeID,
+      name: engine.localName,
+      type: "desktop",
+      commands: [],
+      version: "48",
+      protocols: ["1.5"],
+    }), now - 10));
+
+    _("Second sync.");
+    engine._sync();
+
+    deepEqual(Object.keys(store.getAllIDs()).sort(),
+              [recentID, oldID, dupeID, engine.localID].sort(),
+              "Stale client synced, so it should no longer be marked as a dupe");
+  } finally {
+    Svc.Prefs.resetBranch("");
+    Service.recordManager.clearCache();
+
+    try {
+      server.deleteCollections("foo");
+    } finally {
+      server.stop(run_next_test);
+    }
+  }
+});
+
 add_test(function test_command_sync() {
   _("Ensure that commands are synced across clients.");
 
   engine._store.wipe();
   generateNewKeys(Service.collectionKeys);
 
   let contents = {
     meta: {global: {engines: {clients: {version: engine.version,