Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds. r?tcsc,eoger draft
authorMark Hammond <mhammond@skippinet.com.au>
Tue, 19 Jul 2016 16:30:01 +1000
changeset 389370 fbd6dedbdcbdaa7e8b62307a6bdf99c2e6dd186b
parent 388933 e7a27a7538b2bee268abdb08f4f3a6e41c5c58c8
child 525735 233376532f658626454061d765007639cc729d79
push id23385
push userbmo:markh@mozilla.com
push dateTue, 19 Jul 2016 06:30:24 +0000
reviewerstcsc, eoger
bugs1287687
milestone50.0a1
Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds. r?tcsc,eoger MozReview-Commit-ID: 2CpUVqwMEbB
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
@@ -71,32 +71,40 @@ ClientEngine.prototype = {
   get lastRecordUpload() {
     return Svc.Prefs.get(this.name + ".lastRecordUpload", 0);
   },
   set lastRecordUpload(value) {
     Svc.Prefs.set(this.name + ".lastRecordUpload", Math.floor(value));
   },
 
   get remoteClients() {
-    return Object.values(this._store._remoteClients);
+    // return all non-stale clients for external consumption.
+    return Object.values(this._store._remoteClients).filter(v => !v.stale);
+  },
+
+  remoteClientExists(id) {
+    let client = this._store._remoteClients[id];
+    return !!(client && !client.stale);
   },
 
   // Aggregate some stats on the composition of clients on this account
   get stats() {
     let stats = {
       hasMobile: this.localType == DEVICE_TYPE_MOBILE,
       names: [this.localName],
       numClients: 1,
     };
 
     for (let id in this._store._remoteClients) {
-      let {name, type} = this._store._remoteClients[id];
-      stats.hasMobile = stats.hasMobile || type == DEVICE_TYPE_MOBILE;
-      stats.names.push(name);
-      stats.numClients++;
+      let {name, type, stale} = this._store._remoteClients[id];
+      if (!stale) {
+        stats.hasMobile = stats.hasMobile || type == DEVICE_TYPE_MOBILE;
+        stats.names.push(name);
+        stats.numClients++;
+      }
     }
 
     return stats;
   },
 
   /**
    * Obtain information about device types.
    *
@@ -104,16 +112,19 @@ ClientEngine.prototype = {
    */
   get deviceTypes() {
     let counts = new Map();
 
     counts.set(this.localType, 1);
 
     for (let id in this._store._remoteClients) {
       let record = this._store._remoteClients[id];
+      if (record.stale) {
+        continue; // pretend "stale" records don't exist.
+      }
       let type = record.type;
       if (!counts.has(type)) {
         counts.set(type, 0);
       }
 
       counts.set(type, counts.get(type) + 1);
     }
 
@@ -152,20 +163,16 @@ ClientEngine.prototype = {
 
   get localType() {
     return Utils.getDeviceType();
   },
   set localType(value) {
     Svc.Prefs.set("client.type", value);
   },
 
-  remoteClientExists(id) {
-    return !!this._store._remoteClients[id];
-  },
-
   getClientName(id) {
     if (id == this.localID) {
       return this.localName;
     }
     let client = this._store._remoteClients[id];
     return client ? client.name : "";
   },
 
@@ -197,30 +204,32 @@ ClientEngine.prototype = {
       // tabs collection, since showing their list of tabs is confusing.
       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
+      // 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)
       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);
+          record.stale = true;
         }
       }
     } finally {
       this._incomingClients = null;
     }
   },
 
   _uploadOutgoing() {
@@ -340,16 +349,19 @@ ClientEngine.prototype = {
    */
   _sendCommandToClient: function sendCommandToClient(command, args, clientId) {
     this._log.trace("Sending " + command + " to " + clientId);
 
     let client = this._store._remoteClients[clientId];
     if (!client) {
       throw new Error("Unknown remote client ID: '" + clientId + "'.");
     }
+    if (client.stale) {
+      throw new Error("Stale remote client ID: '" + clientId + "'.");
+    }
 
     let action = {
       command: command,
       args: args,
     };
 
     if (!client.commands) {
       client.commands = [action];
@@ -449,18 +461,20 @@ ClientEngine.prototype = {
       this._log.error("Expected " + commandData.args + " args for '" +
                       command + "', but got " + args);
       return;
     }
 
     if (clientId) {
       this._sendCommandToClient(command, args, clientId);
     } else {
-      for (let id in this._store._remoteClients) {
-        this._sendCommandToClient(command, args, id);
+      for (let [id, record] in Iterator(this._store._remoteClients)) {
+        if (!record.stale) {
+          this._sendCommandToClient(command, args, id);
+        }
       }
     }
   },
 
   /**
    * Send a URI to another client for display.
    *
    * A side effect is the score is increased dramatically to incur an
@@ -604,16 +618,22 @@ ClientStore.prototype = {
       record.appPackage = Services.appinfo.ID;
       record.application = this.engine.brandName   // "Nightly"
 
       // We can't compute these yet.
       // record.device = "";            // Bug 1100723
       // record.formfactor = "";        // Bug 1100722
     } else {
       record.cleartext = this._remoteClients[id];
+      if (record.cleartext.stale) {
+        // It's almost certainly a logic error for us to upload a record we
+        // consider stale, so make log noise, but still remove the flag.
+        this._log.warn(`Preparing to upload record ${id} that we consider stale`);
+        delete record.cleartext.stale;
+      }
     }
 
     return record;
   },
 
   itemExists(id) {
     return id in this.getAllIDs();
   },
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -563,19 +563,51 @@ add_test(function test_filter_duplicate_
 
     _("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");
+              [recentID, dupeID, oldID, engine.localID].sort(),
+              "Duplicate ID should remain in getAllIDs");
+    ok(engine._store.itemExists(dupeID), "Dupe ID should be considered as existing for Sync methods.");
+    ok(!engine.remoteClientExists(dupeID), "Dupe ID should not be considered as existing for external methods.");
+
+    // dupe desktop should not appear in .deviceTypes.
+    equal(engine.deviceTypes.get("desktop"), 2);
+    equal(engine.deviceTypes.get("mobile"), 1);
+
+    // dupe desktop should not appear in stats
+    deepEqual(engine.stats, {
+      hasMobile: 1,
+      names: [engine.localName, "My Phone", "My old desktop"],
+      numClients: 3,
+    });
+
+    ok(engine.remoteClientExists(oldID), "non-dupe ID should exist.");
+    ok(!engine.remoteClientExists(dupeID), "dupe ID should not exist");
+    equal(engine.remoteClients.length, 2, "dupe should not be in remoteClients");
+
+    // Check that a subsequent Sync doesn't report anything as being processed.
+    let counts;
+    Svc.Obs.add("weave:engine:sync:applied", function observe(subject, data) {
+      Svc.Obs.remove("weave:engine:sync:applied", observe);
+      counts = subject;
+    });
+
+    engine._sync();
+    equal(counts.applied, 0); // We didn't report applying any records.
+    equal(counts.reconciled, 4); // We reported reconcilliation for all records
+    equal(counts.succeeded, 0);
+    equal(counts.failed, 0);
+    equal(counts.newFailed, 0);
 
     _("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: [] }],
@@ -600,16 +632,32 @@ add_test(function test_filter_duplicate_
     }), 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");
+
+    ok(engine.remoteClientExists(dupeID), "Dupe ID should appear as it synced.");
+
+    // Recently synced dupe desktop should appear in .deviceTypes.
+    equal(engine.deviceTypes.get("desktop"), 3);
+
+    // Recently synced dupe desktop should now appear in stats
+    deepEqual(engine.stats, {
+      hasMobile: 1,
+      names: [engine.localName, "My Phone", engine.localName, "My old desktop"],
+      numClients: 4,
+    });
+
+    ok(engine.remoteClientExists(dupeID), "recently synced dupe ID should now exist");
+    equal(engine.remoteClients.length, 3, "recently synced dupe should now be in remoteClients");
+
   } finally {
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
 
     try {
       server.deleteCollections("foo");
     } finally {
       server.stop(run_next_test);