Bug 1149729 - Ignore more Sync login error states. r=adw, a=sledru
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 24 Apr 2015 11:49:22 +1000
changeset 267242 fd5e167468d2a96922f1e1f96bd662d4204b6553
parent 267241 8c45feceeee4775812c1011582b14f3bda64a68e
child 267243 a752cbe8e5b1ebac30a27ab4881111e9b417422e
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, sledru
bugs1149729
milestone39.0a2
Bug 1149729 - Ignore more Sync login error states. r=adw, a=sledru
browser/base/content/browser-syncui.js
browser/base/content/test/general/browser_syncui.js
services/sync/modules/policies.js
--- a/browser/base/content/browser-syncui.js
+++ b/browser/base/content/browser-syncui.js
@@ -131,16 +131,18 @@ let gSyncUI = {
       firstSync = Services.prefs.getCharPref("services.sync.firstSync");
     } catch (e) { }
 
     return Weave.Status.checkSetup() == Weave.CLIENT_NOT_CONFIGURED ||
            firstSync == "notReady";
   },
 
   _loginFailed: function () {
+    this.log.debug("_loginFailed has sync state=${sync}, readinglist state=${rl}",
+                   { sync: Weave.Status.login, rl: ReadingListScheduler.state});
     return Weave.Status.login == Weave.LOGIN_FAILED_LOGIN_REJECTED ||
            ReadingListScheduler.state == ReadingListScheduler.STATE_ERROR_AUTHENTICATION;
   },
 
   updateUI: function SUI_updateUI() {
     let needsSetup = this._needsSetup();
     let loginFailed = this._loginFailed();
 
@@ -229,18 +231,24 @@ let gSyncUI = {
     // if login fails, any other notifications are essentially moot
     Weave.Notifications.removeAll();
 
     // if we haven't set up the client, don't show errors
     if (this._needsSetup()) {
       this.updateUI();
       return;
     }
-    // if we are still waiting for the identity manager to initialize, don't show errors
-    if (Weave.Status.login == Weave.LOGIN_FAILED_NOT_READY) {
+    // if we are still waiting for the identity manager to initialize, or it's
+    // a network/server error, don't show errors.  If it weren't for the legacy
+    // provider we could just check LOGIN_FAILED_LOGIN_REJECTED, but the legacy
+    // provider has states like LOGIN_FAILED_INVALID_PASSPHRASE which we
+    // probably do want to surface.
+    if (Weave.Status.login == Weave.LOGIN_FAILED_NOT_READY ||
+        Weave.Status.login == Weave.LOGIN_FAILED_NETWORK_ERROR ||
+        Weave.Status.login == Weave.LOGIN_FAILED_SERVER_ERROR) {
       this.updateUI();
       return;
     }
     this.showLoginError();
     this.updateUI();
   },
 
   showLoginError() {
--- a/browser/base/content/test/general/browser_syncui.js
+++ b/browser/base/content/test/general/browser_syncui.js
@@ -103,16 +103,69 @@ add_task(function* testSyncLoginError() 
   Weave.Status.login = Weave.LOGIN_SUCCEEDED;
   let promiseNotificationRemoved = promiseObserver("weave:notification:removed");
   Services.obs.notifyObservers(null, "weave:service:login:start", null);
   Services.obs.notifyObservers(null, "weave:service:login:finish", null);
   yield promiseNotificationRemoved;
   Assert.equal(Notifications.notifications.length, 0, "no notifications left");
 });
 
+add_task(function* testSyncLoginNetworkError() {
+  Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
+
+  // This test relies on the fact that observers are synchronous, and that error
+  // notifications synchronously create the error notification, which itself
+  // fires an observer notification.
+  // ie, we should see the error notification *during* the notifyObservers call.
+
+  // To prove that, we cause a notification that *does* show an error and make
+  // sure we see the error notification during that call. We then cause a
+  // notification that *should not* show an error, and the lack of the
+  // notification during the call implies the error was ignored.
+
+  // IOW, if this first part of the test fails in the future, it means the
+  // above is no longer true and we need a different strategy to check for
+  // ignored errors.
+
+  let sawNotificationAdded = false;
+  let obs = (subject, topic, data) => {
+    sawNotificationAdded = true;
+  }
+  Services.obs.addObserver(obs, "weave:notification:added", false);
+  try {
+    // notify of a display-able error - we should synchronously see our flag set.
+    Weave.Status.sync = Weave.LOGIN_FAILED;
+    Weave.Status.login = Weave.LOGIN_FAILED_LOGIN_REJECTED;
+    Services.obs.notifyObservers(null, "weave:ui:login:error", null);
+    Assert.ok(sawNotificationAdded);
+
+    // clear the notification.
+    let promiseNotificationRemoved = promiseObserver("weave:notification:removed");
+    Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+    Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
+    yield promiseNotificationRemoved;
+
+    // cool - so reset the flag and test what should *not* show an error.
+    sawNotificationAdded = false;
+    Weave.Status.sync = Weave.LOGIN_FAILED;
+    Weave.Status.login = Weave.LOGIN_FAILED_NETWORK_ERROR;
+    Services.obs.notifyObservers(null, "weave:ui:login:error", null);
+    Assert.ok(!sawNotificationAdded);
+
+    // ditto for LOGIN_FAILED_SERVER_ERROR
+    Weave.Status.sync = Weave.LOGIN_FAILED;
+    Weave.Status.login = Weave.LOGIN_FAILED_SERVER_ERROR;
+    Services.obs.notifyObservers(null, "weave:ui:login:error", null);
+    Assert.ok(!sawNotificationAdded);
+    // we are done.
+  } finally {
+    Services.obs.removeObserver(obs, "weave:notification:added");
+  }
+});
+
 add_task(function* testRLLoginError() {
   let promiseNotificationAdded = promiseObserver("weave:notification:added");
   Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
 
   // Pretend RL is in an auth error state
   getInternalScheduler().state = ReadingListScheduler.STATE_ERROR_AUTHENTICATION;
   Services.obs.notifyObservers(null, "readinglist:sync:start", null);
   Services.obs.notifyObservers(null, "readinglist:sync:error", null);
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -761,18 +761,22 @@ ErrorHandler.prototype = {
     // an error. This assumes that we'll get a 401 again on a login fetch in
     // order to report the error.
     if (!this.service.clusterURL) {
       this._log.trace("shouldReportError: false (no cluster URL; " +
                       "possible node reassignment).");
       return false;
     }
 
-    return ([Status.login, Status.sync].indexOf(SERVER_MAINTENANCE) == -1 &&
-            [Status.login, Status.sync].indexOf(LOGIN_FAILED_NETWORK_ERROR) == -1);
+
+    let result = ([Status.login, Status.sync].indexOf(SERVER_MAINTENANCE) == -1 &&
+                  [Status.login, Status.sync].indexOf(LOGIN_FAILED_NETWORK_ERROR) == -1);
+    this._log.trace("shouldReportError: ${result} due to login=${login}, sync=${sync}",
+                    {result, login: Status.login, sync: Status.sync});
+    return result;
   },
 
   get currentAlertMode() {
     return Svc.Prefs.get("errorhandler.alert.mode");
   },
 
   set currentAlertMode(str) {
     return Svc.Prefs.set("errorhandler.alert.mode", str);