Bug 1453887 - Avoid syncing as the network link changes to down r?eoger draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Fri, 13 Apr 2018 12:20:11 -0700
changeset 781928 d1a499adf40cf1062f43fd704393e87264a5d13c
parent 781042 ee1d1bf1dc8a83eec16967ddb61dd5024c8d6058
push id106443
push userbmo:tchiovoloni@mozilla.com
push dateFri, 13 Apr 2018 19:20:35 +0000
reviewerseoger
bugs1453887
milestone61.0a1
Bug 1453887 - Avoid syncing as the network link changes to down r?eoger MozReview-Commit-ID: 5C2qDX4iITU
services/sync/modules/policies.js
services/sync/tests/unit/test_syncscheduler.js
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -197,21 +197,37 @@ SyncScheduler.prototype = {
     switch (topic) {
       case "weave:engine:score:updated":
         if (Status.login == LOGIN_SUCCEEDED) {
           CommonUtils.namedTimer(this.calculateScore, SCORE_UPDATE_DELAY, this,
                            "_scoreTimer");
         }
         break;
       case "network:link-status-changed":
-        if (!this.offline) {
+        // Note: NetworkLinkService is unreliable, we get false negatives for it
+        // in cases such as VMs (bug 1420802), so we don't want to use it in
+        // `get offline`, but we assume that it's probably reliable if we're
+        // getting status changed events. (We might be wrong about this, but
+        // if that's true, then the only downside is that we won't sync as
+        // promptly).
+        let isOffline = this.offline;
+        this._log.debug(`Network link status changed to "${data}". Offline?`,
+                        isOffline);
+        // Data may be one of `up`, `down`, `change`, or `unknown`. We only want
+        // to sync if it's "up".
+        if (data == "up" && !isOffline) {
           this._log.debug("Network link looks up. Syncing.");
           this.scheduleNextSync(0, {why: topic});
+        } else if (data == "down") {
+          // Unschedule pending syncs if we know we're going down. We don't do
+          // this via `checkSyncStatus`, since link status isn't reflected in
+          // `this.offline`.
+          this.clearSyncTriggers();
         }
-        // Intended fallthrough
+        break;
       case "network:offline-status-changed":
       case "captive-portal-detected":
         // Whether online or offline, we'll reschedule syncs
         this._log.trace("Network offline status change: " + data);
         this.checkSyncStatus();
         break;
       case "weave:service:sync:start":
         // Clear out any potentially pending syncs now that we're syncing
--- a/services/sync/tests/unit/test_syncscheduler.js
+++ b/services/sync/tests/unit/test_syncscheduler.js
@@ -1039,8 +1039,30 @@ add_task(async function test_proper_inte
     reconciled: 0
   });
 
   await Async.promiseYield();
   scheduler.adjustSyncInterval();
   Assert.ok(!scheduler.hasIncomingItems);
   Assert.equal(scheduler.syncInterval, scheduler.singleDeviceInterval);
 });
+
+add_task(async function test_link_status_change() {
+  _("Check that we only attempt to sync when link status is up");
+  try {
+    sinon.spy(scheduler, "scheduleNextSync");
+
+    Svc.Obs.notify("network:link-status-changed", null, "down");
+    equal(scheduler.scheduleNextSync.callCount, 0);
+
+    Svc.Obs.notify("network:link-status-changed", null, "change");
+    equal(scheduler.scheduleNextSync.callCount, 0);
+
+    Svc.Obs.notify("network:link-status-changed", null, "up");
+    equal(scheduler.scheduleNextSync.callCount, 1);
+
+    Svc.Obs.notify("network:link-status-changed", null, "change");
+    equal(scheduler.scheduleNextSync.callCount, 1);
+
+  } finally {
+    scheduler.scheduleNextSync.restore();
+  }
+});