Bug 1416807 - Don't schedule syncs while we're behind to a captive portal. r?markh draft
authorEdouard Oger <eoger@fastmail.com>
Wed, 15 Nov 2017 16:01:48 -0500
changeset 698524 4904ea35aaf6a00c0896702b851d18c81f822b45
parent 698261 45715ece25fcb064eee4f977ebd842d44a87f22b
child 740409 733bd7140bc792aaf4f9d6cce8b472dfda2d9f9f
push id89323
push userbmo:eoger@fastmail.com
push dateWed, 15 Nov 2017 22:55:10 +0000
reviewersmarkh
bugs1416807
milestone59.0a1
Bug 1416807 - Don't schedule syncs while we're behind to a captive portal. r?markh MozReview-Commit-ID: 2CrO3CfKgC3
services/sync/modules/policies.js
services/sync/modules/service.js
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -20,16 +20,22 @@ Cu.import("resource://services-common/ut
 
 XPCOMUtils.defineLazyModuleGetter(this, "Status",
                                   "resource://services-sync/status.js");
 XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
                                   "resource://gre/modules/AddonManager.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "IdleService",
                                    "@mozilla.org/widget/idleservice;1",
                                    "nsIIdleService");
+XPCOMUtils.defineLazyServiceGetter(this, "NetworkLinkService",
+                                   "@mozilla.org/network/network-link-service;1",
+                                   "nsINetworkLinkService");
+XPCOMUtils.defineLazyServiceGetter(this, "CaptivePortalService",
+                                   "@mozilla.org/network/captive-portal-service;1",
+                                   "nsICaptivePortalService");
 
 // Get the value for an interval that's stored in preferences. To save users
 // from themselves (and us from them!) the minimum time they can specify
 // is 60s.
 function getThrottledIntervalPreference(prefName) {
   return Math.max(Svc.Prefs.get(prefName), 60) * 1000;
 }
 
@@ -108,21 +114,53 @@ SyncScheduler.prototype = {
     return Svc.Prefs.get("clients.devices.desktop", 0) +
            Svc.Prefs.get("clients.devices.mobile", 0);
 
   },
   set numClients(value) {
     throw new Error("Don't set numClients - the clients engine manages it.");
   },
 
+  get offline() {
+    // Services.io.offline has slowly become fairly useless over the years - it
+    // no longer attempts to track the actual network state by default, but one
+    // thing stays true: if it says we're offline then we are definitely not online.
+    //
+    // There is also a network link service that tracks if there is any network
+    // adaptor connected. Sadly this too is unreliable - eg, an adaptor on a
+    // local private network (eg, a VMWare local network) may be technically
+    // connected but still unable to hit the public network - but it should
+    // only be unreliable in terms of indicating we online when we aren't but
+    // not indicate we are offline when we are online.
+    //
+    // Finally, if both of these services don't tell us we're offline for sure,
+    // we'll ask the captive portal service if we are behind a locked captive
+    // portal.
+    //
+    // With these 3 services combined, we believe we can avoid all false positives
+    // and make a good guess on whether an user is online or not in most cases.
+    try {
+      if (Services.io.offline ||
+          !NetworkLinkService.isLinkUp ||
+          CaptivePortalService.state == CaptivePortalService.LOCKED_PORTAL) {
+        return true;
+      }
+    } catch (ex) {
+      this._log.warn("Could not determine network status.", ex);
+    }
+    return false;
+  },
+
   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("network:link-status-changed", this);
+    Svc.Obs.add("captive-portal-detected", this);
     Svc.Obs.add("weave:service:sync:start", this);
     Svc.Obs.add("weave:service:sync:finish", this);
     Svc.Obs.add("weave:engine:sync:finish", this);
     Svc.Obs.add("weave:engine:sync:error", this);
     Svc.Obs.add("weave:service:login:error", this);
     Svc.Obs.add("weave:service:logout:finish", this);
     Svc.Obs.add("weave:service:sync:error", this);
     Svc.Obs.add("weave:service:backoff:interval", this);
@@ -145,16 +183,18 @@ 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:offline-status-changed":
+      case "network:link-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
         this.clearSyncTriggers();
 
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -829,17 +829,17 @@ Sync11Service.prototype = {
       // reflected in the UI.
       Svc.Obs.notify("weave:service:start-over:finish");
     }
   },
 
   async login() {
     async function onNotify() {
       this._loggedIn = false;
-      if (Services.io.offline) {
+      if (this.scheduler.offline) {
         this.status.login = LOGIN_FAILED_NETWORK_ERROR;
         throw new Error("Application is offline, login should not be called");
       }
 
       this._log.info("Logging in the user.");
       // Just let any errors bubble up - they've more context than we do!
       try {
         await this.identity.ensureLoggedIn();
@@ -1039,17 +1039,17 @@ Sync11Service.prototype = {
    * Return whether we should attempt login at the start of a sync.
    *
    * Note that this function has strong ties to _checkSync: callers
    * of this function should typically use _checkSync to verify that
    * any necessary login took place.
    */
   _shouldLogin: function _shouldLogin() {
     return this.enabled &&
-           !Services.io.offline &&
+           !this.scheduler.offline &&
            !this.isLoggedIn &&
            Async.isAppReady();
   },
 
   /**
    * Determine if a sync should run.
    *
    * @param ignore [optional]
@@ -1059,17 +1059,17 @@ Sync11Service.prototype = {
    */
   _checkSync: function _checkSync(ignore) {
     let reason = "";
     // Ideally we'd call _checkSetup() here but that has too many side-effects.
     if (Status.service == CLIENT_NOT_CONFIGURED)
       reason = kSyncNotConfigured;
     else if (Status.service == STATUS_DISABLED || !this.enabled)
       reason = kSyncWeaveDisabled;
-    else if (Services.io.offline)
+    else if (this.scheduler.offline)
       reason = kSyncNetworkOffline;
     else if (this.status.minimumNextSync > Date.now())
       reason = kSyncBackoffNotMet;
     else if ((this.status.login == MASTER_PASSWORD_LOCKED) &&
              Utils.mpLocked())
       reason = kSyncMasterPasswordLocked;
     else if (Svc.Prefs.get("firstSync") == "notReady")
       reason = kFirstSyncChoiceNotMade;