Bug 1264498 - Hide duplicate remote Sync clients that haven't synced in a week. r=markh
authorKit Cambridge <kcambridge@mozilla.com>
Fri, 15 Apr 2016 09:00:59 -0700
changeset 294476 fe77c894cb9ab23dc3b232f831261be76e4d6aa9
parent 294475 467bd00c72db8f6f34c4d9740378abb90365269f
child 294477 c7544b24989db41199a53b12982e0a696aea3e27
push id75556
push usercbook@mozilla.com
push dateFri, 22 Apr 2016 13:58:01 +0000
treeherdermozilla-inbound@56d0b315df5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1264498
milestone48.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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,