Bug 600261 - Provide hooks for ignoring network errors. r=mconnor a=blocking-betaN

services/sync/modules/constants.js

@@ -77,6 +77,9 @@ MULTI_DESKTOP_SYNC:                    60 * 60 * 1000, // 1 hour
 MULTI_MOBILE_SYNC:                     5 * 60 * 1000, // 5 minutes
 PARTIAL_DATA_SYNC:                     60 * 1000, // 1 minute
 
+MAX_ERROR_COUNT_BEFORE_BACKOFF:        3,
+MAX_IGNORE_ERROR_COUNT:                5,
+
 // HMAC event handling timeout.
 // 10 minutes: a compromise between the multi-desktop sync interval
 // and the mobile sync interval.

services/sync/modules/service.js

@@ -398,6 +398,7 @@ WeaveSvc.prototype = {
     Svc.Obs.add("weave:service:setup-complete", this);
     Svc.Obs.add("network:offline-status-changed", this);
     Svc.Obs.add("weave:service:sync:finish", this);
+    Svc.Obs.add("weave:service:login:error", this);
     Svc.Obs.add("weave:service:sync:error", this);
     Svc.Obs.add("weave:service:backoff:interval", this);
     Svc.Obs.add("weave:engine:score:updated", this);
@@ -542,15 +543,28 @@ WeaveSvc.prototype = {
         this._log.trace("Network offline status change: " + data);
         this._checkSyncStatus();
         break;
+      case "weave:service:login:error":
+        if (Status.login == LOGIN_FAILED_NETWORK_ERROR && !Svc.IO.offline) {
+          this._ignorableErrorCount += 1;
+        }
+        break;
       case "weave:service:sync:error":
         this._handleSyncError();
-        if (Status.sync == CREDENTIALS_CHANGED) {
-          this.logout();
+        switch (Status.sync) {
+          case LOGIN_FAILED_NETWORK_ERROR:
+            if (!Svc.IO.offline) {
+              this._ignorableErrorCount += 1;
+            }
+            break;
+          case CREDENTIALS_CHANGED:
+            this.logout();
+            break;
         }
         break;
       case "weave:service:sync:finish":
         this._scheduleNextSync();
         this._syncErrors = 0;
+        this._ignorableErrorCount = 0;
         break;
       case "weave:service:backoff:interval":
         let interval = (data + Math.random() * data * 0.25) * 1000; // required backoff + up to 25%
@@ -1078,8 +1092,10 @@ WeaveSvc.prototype = {
     this._catch(this._lock("service.js: login", 
           this._notify("login", "", function() {
       this._loggedIn = false;
-      if (Svc.IO.offline)
+      if (Svc.IO.offline) {
+        Status.login = LOGIN_FAILED_NETWORK_ERROR;
         throw "Application is offline, login should not be called";
+      }
 
       let initialStatus = this._checkSetup();
       if (username)
@@ -1617,9 +1633,10 @@ WeaveSvc.prototype = {
   _handleSyncError: function WeaveSvc__handleSyncError() {
     this._syncErrors++;
 
-    // do nothing on the first couple of failures, if we're not in backoff due to 5xx errors
+    // Do nothing on the first couple of failures, if we're not in
+    // backoff due to 5xx errors.
     if (!Status.enforceBackoff) {
-      if (this._syncErrors < 3) {
+      if (this._syncErrors < MAX_ERROR_COUNT_BEFORE_BACKOFF) {
         this._scheduleNextSync();
         return;
       }
@@ -1629,6 +1646,12 @@ WeaveSvc.prototype = {
     this._scheduleAtInterval();
   },
 
+  _ignorableErrorCount: 0,
+  shouldIgnoreError: function shouldIgnoreError() {
+    return ([Status.login, Status.sync].indexOf(LOGIN_FAILED_NETWORK_ERROR) != -1
+            && this._ignorableErrorCount < MAX_IGNORE_ERROR_COUNT);
+  },
+
   _skipScheduledRetry: function _skipScheduledRetry() {
     return [LOGIN_FAILED_INVALID_PASSPHRASE,
             LOGIN_FAILED_LOGIN_REJECTED].indexOf(Status.login) == -1;
@@ -1675,6 +1698,9 @@ WeaveSvc.prototype = {
     // Make sure we should sync or record why we shouldn't
     let reason = this._checkSync();
     if (reason) {
+      if (reason == kSyncNetworkOffline) {
+        Status.sync = LOGIN_FAILED_NETWORK_ERROR;
+      }
       // this is a purposeful abort rather than a failure, so don't set
       // any status bits
       reason = "Can't sync: " + reason;

services/sync/tests/unit/test_service_login.js

@@ -24,6 +24,18 @@ function run_test() {
   let logger = Log4Moz.repository.rootLogger;
   Log4Moz.repository.rootLogger.addAppender(new Log4Moz.DumpAppender());
 
+  try {
+    _("The right bits are set when we're offline.");
+    Svc.IO.offline = true;
+    do_check_eq(Service._ignorableErrorCount, 0);
+    do_check_false(!!Service.login());
+    do_check_eq(Status.login, LOGIN_FAILED_NETWORK_ERROR);
+    do_check_eq(Service._ignorableErrorCount, 0);
+    Svc.IO.offline = false;
+  } finally {
+    Svc.Prefs.resetBranch("");
+  }
+ 
   do_test_pending();
   let server = httpd_setup({
     "/1.0/johndoe/info/collections": login_handler,
@@ -111,7 +123,7 @@ function run_test() {
     Service.logout();
     do_check_false(Service.isLoggedIn);
     do_check_false(Svc.Prefs.get("autoconnect"));
-    
+
     /*
      * Testing login-on-sync.
      */

services/sync/tests/unit/test_service_sync_checkServerError.js

@@ -133,9 +133,12 @@ function test_overQuota(next) {
 }
 
 function test_service_networkError(next) {
+  _("Test: Connection refused error from Service.sync() leads to the right status code.");
   setUp();
   // Provoke connection refused.
   Service.clusterURL = "http://localhost:12345/";
+  Service._ignorableErrorCount = 0;
+
   try {
     do_check_eq(Status.sync, SYNC_SUCCEEDED);
 
@@ -143,6 +146,7 @@ function test_service_networkError(next) {
     Service.sync();
 
     do_check_eq(Status.sync, LOGIN_FAILED_NETWORK_ERROR);
+    do_check_eq(Service._ignorableErrorCount, 1);
   } finally {
     Status.resetSync();
     Service.startOver();
@@ -150,10 +154,54 @@ function test_service_networkError(next) {
   next();
 }
 
+function test_service_offline(next) {
+  _("Test: Wanting to sync in offline mode leads to the right status code but does not increment the ignorable error count.");
+  setUp();
+  Svc.IO.offline = true;
+  Service._ignorableErrorCount = 0;
+
+  try {
+    do_check_eq(Status.sync, SYNC_SUCCEEDED);
+
+    Service._loggedIn = true;
+    Service.sync();
+
+    do_check_eq(Status.sync, LOGIN_FAILED_NETWORK_ERROR);
+    do_check_eq(Service._ignorableErrorCount, 0);
+  } finally {
+    Status.resetSync();
+    Service.startOver();
+  }
+  Svc.IO.offline = false;
+  next();
+}
+
+function test_service_reset_ignorableErrorCount(next) {
+  _("Test: Successful sync resets the ignorable error count.");
+  let server = sync_httpd_setup();
+  setUp();
+  Service._ignorableErrorCount = 10;
+
+  try {
+    do_check_eq(Status.sync, SYNC_SUCCEEDED);
+
+    Service.login();
+    Service.sync();
+
+    do_check_eq(Status.sync, SYNC_SUCCEEDED);
+    do_check_eq(Service._ignorableErrorCount, 0);
+  } finally {
+    Status.resetSync();
+    Service.startOver();
+  }
+  server.stop(next);
+}
+
 function test_engine_networkError(next) {
   _("Test: Network related exceptions from engine.sync() lead to the right status code.");
   let server = sync_httpd_setup();
   setUp();
+  Service._ignorableErrorCount = 0;
 
   Engines.register(CatapultEngine);
   let engine = Engines.get("catapult");
@@ -168,6 +216,7 @@ function test_engine_networkError(next) {
     Service.sync();
 
     do_check_eq(Status.sync, LOGIN_FAILED_NETWORK_ERROR);
+    do_check_eq(Service._ignorableErrorCount, 1);
   } finally {
     Engines.unregister("catapult");
     Status.resetSync();
@@ -213,6 +262,8 @@ function run_test() {
                   test_backoff503,
                   test_overQuota,
                   test_service_networkError,
+                  test_service_offline,
+                  test_service_reset_ignorableErrorCount,
                   test_engine_networkError,
                   test_engine_applyFailed,
                   do_test_finished)();