Bug 1310065 - use more reliable preferences for calculating the client count in the sync scheduler. r=eoger
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 25 Jan 2017 18:11:05 +1100
changeset 341097 6a40c16fd2114e0c50a61e207a9bc3d4a178f147
parent 341096 6ce29dfac81bd89af0d88b17b11a498a2b60337a
child 341098 bd81e72532b3171f4fbde1f0feec4d6bd70c1987
push id86634
push usercbook@mozilla.com
push dateTue, 07 Feb 2017 13:14:58 +0000
treeherdermozilla-inbound@9dbd2d9b334e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerseoger
bugs1310065
milestone54.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 1310065 - use more reliable preferences for calculating the client count in the sync scheduler. r=eoger This patch switches to using preferences written by the clients engine to determine the number of clients so it's correct before the first sync for a browser session. It also fixes another bug I discovered - if a mobile device dropped from the client list, the preference for the number of mobile devices would remain at 1 rather than resetting to zero. MozReview-Commit-ID: IPythSQcorx
services/sync/modules/engines/clients.js
services/sync/modules/policies.js
services/sync/tests/unit/test_interval_triggers.js
services/sync/tests/unit/test_service_startOver.js
services/sync/tests/unit/test_syncscheduler.js
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -123,22 +123,25 @@ ClientEngine.prototype = {
     }
 
     return stats;
   },
 
   /**
    * Obtain information about device types.
    *
-   * Returns a Map of device types to integer counts.
+   * Returns a Map of device types to integer counts. Guaranteed to include
+   * "desktop" (which will have at least 1 - this device) and "mobile" (which
+   * may have zero) counts. It almost certainly will include only these 2.
    */
   get deviceTypes() {
     let counts = new Map();
 
-    counts.set(this.localType, 1);
+    counts.set(this.localType, 1); // currently this must be DEVICE_TYPE_DESKTOP
+    counts.set(DEVICE_TYPE_MOBILE, 0);
 
     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)) {
@@ -380,26 +383,28 @@ ClientEngine.prototype = {
         collections: ["clients"]
       }
     };
     fxAccounts.notifyDevices(ids, message, NOTIFY_TAB_SENT_TTL_SECS);
   },
 
   _syncFinish() {
     // Record histograms for our device types, and also write them to a pref
-    // so non-histogram telemetry (eg, UITelemetry) has easy access to them.
+    // so non-histogram telemetry (eg, UITelemetry) and the sync scheduler
+    // has easy access to them, and so they are accurate even before we've
+    // successfully synced the first time after startup.
     for (let [deviceType, count] of this.deviceTypes) {
       let hid;
       let prefName = this.name + ".devices.";
       switch (deviceType) {
-        case "desktop":
+        case DEVICE_TYPE_DESKTOP:
           hid = "WEAVE_DEVICE_COUNT_DESKTOP";
           prefName += "desktop";
           break;
-        case "mobile":
+        case DEVICE_TYPE_MOBILE:
           hid = "WEAVE_DEVICE_COUNT_MOBILE";
           prefName += "mobile";
           break;
         default:
           this._log.warn(`Unexpected deviceType "${deviceType}" recording device telemetry.`);
           continue;
       }
       Services.telemetry.getHistogramById(hid).add(count);
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -52,16 +52,21 @@ SyncScheduler.prototype = {
     this.activeInterval       = getThrottledIntervalPreference("scheduler.activeInterval");
     this.immediateInterval    = getThrottledIntervalPreference("scheduler.immediateInterval");
     this.eolInterval          = getThrottledIntervalPreference("scheduler.eolInterval");
 
     // A user is non-idle on startup by default.
     this.idle = false;
 
     this.hasIncomingItems = false;
+    // This is the last number of clients we saw when previously updating the
+    // client mode. If this != currentNumClients (obtained from prefs written
+    // by the clients engine) then we need to transition to and from
+    // single and multi-device mode.
+    this.numClientsLastSync = 0;
 
     this.clearSyncTriggers();
   },
 
   // nextSync is in milliseconds, but prefs can't hold that much
   get nextSync() {
     return Svc.Prefs.get("nextSync", 0) * 1000;
   },
@@ -85,21 +90,25 @@ SyncScheduler.prototype = {
 
   get globalScore() {
     return Svc.Prefs.get("globalScore", 0);
   },
   set globalScore(value) {
     Svc.Prefs.set("globalScore", value);
   },
 
+  // The number of clients we have is maintained in preferences via the
+  // clients engine, and only updated after a successsful sync.
   get numClients() {
-    return Svc.Prefs.get("numClients", 0);
+    return Svc.Prefs.get("clients.devices.desktop", 0) +
+           Svc.Prefs.get("clients.devices.mobile", 0);
+
   },
   set numClients(value) {
-    Svc.Prefs.set("numClients", value);
+    throw new Error("Don't set numClients - the clients engine manages it.")
   },
 
   init: function init() {
     this._log.level = Log.Level[Svc.Prefs.get("log.logger.service.main")];
     this.setDefaults();
     Svc.Obs.add("weave:engine:score:updated", this);
     Svc.Obs.add("network:offline-status-changed", this);
     Svc.Obs.add("weave:service:sync:start", this);
@@ -326,26 +335,26 @@ SyncScheduler.prototype = {
       engines[i]._tracker.resetScore();
     }
 
     this._log.trace("Global score updated: " + this.globalScore);
     this.checkSyncStatus();
   },
 
   /**
-   * Process the locally stored clients list to figure out what mode to be in
+   * Query the number of known clients to figure out what mode to be in
    */
   updateClientMode: function updateClientMode() {
     // Nothing to do if it's the same amount
-    let numClients = this.service.clientsEngine.stats.numClients;
-    if (this.numClients == numClients)
+    let numClients = this.numClients;
+    if (numClients == this.numClientsLastSync)
       return;
 
-    this._log.debug("Client count: " + this.numClients + " -> " + numClients);
-    this.numClients = numClients;
+    this._log.debug(`Client count: ${this.numClientsLastSync} -> ${numClients}`);
+    this.numClientsLastSync = numClients;
 
     if (numClients <= 1) {
       this._log.trace("Adjusting syncThreshold to SINGLE_USER_THRESHOLD");
       this.syncThreshold = SINGLE_USER_THRESHOLD;
     } else {
       this._log.trace("Adjusting syncThreshold to MULTI_DEVICE_THRESHOLD");
       this.syncThreshold = MULTI_DEVICE_THRESHOLD;
     }
--- a/services/sync/tests/unit/test_interval_triggers.js
+++ b/services/sync/tests/unit/test_interval_triggers.js
@@ -109,17 +109,17 @@ add_task(async function test_successful_
   do_check_eq(syncSuccesses, 4);
   do_check_true(scheduler.idle);
   do_check_false(scheduler.numClients > 1);
   do_check_true(scheduler.hasIncomingItems);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
 
   _("Test as long as idle && numClients > 1 our sync interval is idleInterval.");
   // idle == true && numClients > 1 && hasIncomingItems == true
-  Service.clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  Service.clientsEngine._store.create({ id: "foo", cleartext: { name: "bar", type: "mobile" } });
   Service.sync();
   do_check_eq(syncSuccesses, 5);
   do_check_true(scheduler.idle);
   do_check_true(scheduler.numClients > 1);
   do_check_true(scheduler.hasIncomingItems);
   do_check_eq(scheduler.syncInterval, scheduler.idleInterval);
 
   // idle == true && numClients > 1 && hasIncomingItems == false
@@ -213,17 +213,18 @@ add_task(async function test_unsuccessfu
   do_check_eq(syncFailures, 4);
   do_check_true(scheduler.idle);
   do_check_false(scheduler.numClients > 1);
   do_check_true(scheduler.hasIncomingItems);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
 
   _("Test as long as idle && numClients > 1 our sync interval is idleInterval.");
   // idle == true && numClients > 1 && hasIncomingItems == true
-  Service.clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  Svc.Prefs.set("clients.devices.mobile", 2);
+  scheduler.updateClientMode();
 
   Service.sync();
   do_check_eq(syncFailures, 5);
   do_check_true(scheduler.idle);
   do_check_true(scheduler.numClients > 1);
   do_check_true(scheduler.hasIncomingItems);
   do_check_eq(scheduler.syncInterval, scheduler.idleInterval);
 
@@ -266,17 +267,17 @@ add_task(async function test_back_trigge
   await setUp(server);
 
   // Single device: no sync triggered.
   scheduler.idle = true;
   scheduler.observe(null, "active", Svc.Prefs.get("scheduler.idleTime"));
   do_check_false(scheduler.idle);
 
   // Multiple devices: sync is triggered.
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  Svc.Prefs.set("clients.devices.mobile", 2);
   scheduler.updateClientMode();
 
   let promiseDone = promiseOneObserver("weave:service:sync:finish");
 
   scheduler.idle = true;
   scheduler.observe(null, "active", Svc.Prefs.get("scheduler.idleTime"));
   do_check_false(scheduler.idle);
   await promiseDone;
@@ -304,17 +305,17 @@ add_task(async function test_adjust_inte
   _("Test unsuccessful sync updates client mode & sync intervals");
   // Force a sync fail.
   Svc.Prefs.set("firstSync", "notReady");
 
   do_check_eq(syncFailures, 0);
   do_check_false(scheduler.numClients > 1);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
 
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  Svc.Prefs.set("clients.devices.mobile", 2);
   Service.sync();
 
   do_check_eq(syncFailures, 1);
   do_check_true(scheduler.numClients > 1);
   do_check_eq(scheduler.syncInterval, scheduler.activeInterval);
 
   Svc.Obs.remove("weave:service:sync:error", onSyncError);
   Service.startOver();
@@ -377,35 +378,36 @@ add_task(async function test_bug671378_s
 
       scheduler.scheduleNextSync();
       do_check_neq(scheduler.nextSync, 0);
       do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
       do_check_eq(scheduler.syncTimer.delay, scheduler.singleDeviceInterval);
     });
   });
 
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  Service.clientsEngine._store.create({ id: "foo", cleartext: { name: "bar", type: "mobile" } });
   Service.sync();
   await promiseDone;
 });
 
 add_test(function test_adjust_timer_larger_syncInterval() {
   _("Test syncInterval > current timout period && nextSync != 0, syncInterval is NOT used.");
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  Svc.Prefs.set("clients.devices.mobile", 2);
   scheduler.updateClientMode();
   do_check_eq(scheduler.syncInterval, scheduler.activeInterval);
 
   scheduler.scheduleNextSync();
 
   // Ensure we have a small interval.
   do_check_neq(scheduler.nextSync, 0);
   do_check_eq(scheduler.syncTimer.delay, scheduler.activeInterval);
 
   // Make interval large again
   clientsEngine._wipeClient();
+  Svc.Prefs.reset("clients.devices.mobile");
   scheduler.updateClientMode();
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
 
   scheduler.scheduleNextSync();
 
   // Ensure timer delay remains as the small interval.
   do_check_neq(scheduler.nextSync, 0);
   do_check_true(scheduler.syncTimer.delay <= scheduler.activeInterval);
@@ -419,17 +421,17 @@ add_test(function test_adjust_timer_smal
   _("Test current timout > syncInterval period && nextSync != 0, syncInterval is used.");
   scheduler.scheduleNextSync();
 
   // Ensure we have a large interval.
   do_check_neq(scheduler.nextSync, 0);
   do_check_eq(scheduler.syncTimer.delay, scheduler.singleDeviceInterval);
 
   // Make interval smaller
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  Svc.Prefs.set("clients.devices.mobile", 2);
   scheduler.updateClientMode();
   do_check_eq(scheduler.syncInterval, scheduler.activeInterval);
 
   scheduler.scheduleNextSync();
 
   // Ensure smaller timer delay is used.
   do_check_neq(scheduler.nextSync, 0);
   do_check_true(scheduler.syncTimer.delay <= scheduler.activeInterval);
--- a/services/sync/tests/unit/test_service_startOver.js
+++ b/services/sync/tests/unit/test_service_startOver.js
@@ -74,17 +74,17 @@ add_test(function test_removeClientData(
 
   run_next_test();
 });
 
 add_test(function test_reset_SyncScheduler() {
   // Some non-default values for SyncScheduler's attributes.
   Service.scheduler.idle = true;
   Service.scheduler.hasIncomingItems = true;
-  Service.scheduler.numClients = 42;
+  Svc.Prefs.set("clients.devices.desktop", 42);
   Service.scheduler.nextSync = Date.now();
   Service.scheduler.syncThreshold = MULTI_DEVICE_THRESHOLD;
   Service.scheduler.syncInterval = Service.scheduler.activeInterval;
 
   Service.startOver();
 
   do_check_false(Service.scheduler.idle);
   do_check_false(Service.scheduler.hasIncomingItems);
--- a/services/sync/tests/unit/test_syncscheduler.js
+++ b/services/sync/tests/unit/test_syncscheduler.js
@@ -162,29 +162,29 @@ add_test(function test_prefAttributes() 
 
 add_task(async function test_updateClientMode() {
   _("Test updateClientMode adjusts scheduling attributes based on # of clients appropriately");
   do_check_eq(scheduler.syncThreshold, SINGLE_USER_THRESHOLD);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
   do_check_false(scheduler.numClients > 1);
   do_check_false(scheduler.idle);
 
-  // Trigger a change in interval & threshold by adding a client.
-  clientsEngine._store.create(
-    { id: "foo", cleartext: { os: "mobile", version: "0.01", type: "desktop" } }
-  );
+  // Trigger a change in interval & threshold by noting there are multiple clients.
+  Svc.Prefs.set("clients.devices.desktop", 1);
+  Svc.Prefs.set("clients.devices.mobile", 1);
   scheduler.updateClientMode();
 
   do_check_eq(scheduler.syncThreshold, MULTI_DEVICE_THRESHOLD);
   do_check_eq(scheduler.syncInterval, scheduler.activeInterval);
   do_check_true(scheduler.numClients > 1);
   do_check_false(scheduler.idle);
 
   // Resets the number of clients to 0.
   clientsEngine.resetClient();
+  Svc.Prefs.reset("clients.devices.mobile");
   scheduler.updateClientMode();
 
   // Goes back to single user if # clients is 1.
   do_check_eq(scheduler.numClients, 1);
   do_check_eq(scheduler.syncThreshold, SINGLE_USER_THRESHOLD);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
   do_check_false(scheduler.numClients > 1);
   do_check_false(scheduler.idle);
@@ -597,34 +597,34 @@ add_task(async function test_idle_adjust
 
   // Single device: nothing changes.
   scheduler.observe(null, "idle", Svc.Prefs.get("scheduler.idleTime"));
   do_check_eq(scheduler.idle, true);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
 
   // Multiple devices: switch to idle interval.
   scheduler.idle = false;
-  clientsEngine._store.create(
-    { id: "foo", cleartext: { os: "mobile", version: "0.01", type: "desktop" } }
-  );
+  Svc.Prefs.set("clients.devices.desktop", 1);
+  Svc.Prefs.set("clients.devices.mobile", 1);
   scheduler.updateClientMode();
   scheduler.observe(null, "idle", Svc.Prefs.get("scheduler.idleTime"));
   do_check_eq(scheduler.idle, true);
   do_check_eq(scheduler.syncInterval, scheduler.idleInterval);
 
   await cleanUpAndGo();
 });
 
 add_task(async function test_back_triggersSync() {
   // Confirm defaults.
   do_check_false(scheduler.idle);
   do_check_eq(Status.backoffInterval, 0);
 
   // Set up: Define 2 clients and put the system in idle.
-  scheduler.numClients = 2;
+  Svc.Prefs.set("clients.devices.desktop", 1);
+  Svc.Prefs.set("clients.devices.mobile", 1);
   scheduler.observe(null, "idle", Svc.Prefs.get("scheduler.idleTime"));
   do_check_true(scheduler.idle);
 
   // We don't actually expect the sync (or the login, for that matter) to
   // succeed. We just want to ensure that it was attempted.
   let promiseObserved = promiseOneObserver("weave:service:login:error");
 
   // Send an 'active' event to trigger sync soonish.
@@ -635,17 +635,18 @@ add_task(async function test_back_trigge
 
 add_task(async function test_active_triggersSync_observesBackoff() {
   // Confirm defaults.
   do_check_false(scheduler.idle);
 
   // Set up: Set backoff, define 2 clients and put the system in idle.
   const BACKOFF = 7337;
   Status.backoffInterval = scheduler.idleInterval + BACKOFF;
-  scheduler.numClients = 2;
+  Svc.Prefs.set("clients.devices.desktop", 1);
+  Svc.Prefs.set("clients.devices.mobile", 1);
   scheduler.observe(null, "idle", Svc.Prefs.get("scheduler.idleTime"));
   do_check_eq(scheduler.idle, true);
 
   function onLoginStart() {
     do_throw("Shouldn't have kicked off a sync!");
   }
   Svc.Obs.add("weave:service:login:start", onLoginStart);
 
@@ -664,17 +665,18 @@ add_task(async function test_active_trig
 
 add_task(async function test_back_debouncing() {
   _("Ensure spurious back-then-idle events, as observed on OS X, don't trigger a sync.");
 
   // Confirm defaults.
   do_check_eq(scheduler.idle, false);
 
   // Set up: Define 2 clients and put the system in idle.
-  scheduler.numClients = 2;
+  Svc.Prefs.set("clients.devices.desktop", 1);
+  Svc.Prefs.set("clients.devices.mobile", 1);
   scheduler.observe(null, "idle", Svc.Prefs.get("scheduler.idleTime"));
   do_check_eq(scheduler.idle, true);
 
   function onLoginStart() {
     do_throw("Shouldn't have kicked off a sync!");
   }
   Svc.Obs.add("weave:service:login:start", onLoginStart);