Bug 1453887 - Avoid syncing as the network link changes to down r=eoger a=jcristau
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Fri, 13 Apr 2018 12:20:11 -0700
changeset 463378 dad20eed22b948cfc168a6d4dcfe49e446a05c65
parent 463377 c5734a8b1d7a590e4d1c54cdbf90ea0fa348f9d6
child 463379 6dccd26e00afeaa2b62671e87fd8b634ba67ed81
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerseoger, jcristau
bugs1453887
milestone60.0
Bug 1453887 - Avoid syncing as the network link changes to down r=eoger a=jcristau 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
@@ -1038,8 +1038,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();
+  }
+});