From 1215fa061e8059706742a532598038c099698b05 Mon Sep 17 00:00:00 2001
From: Mark Hammond <mhammond@skippinet.com.au>
Date: Fri, 24 Apr 2015 11:49:22 +1000
Subject: [PATCH] Bug 1149729 - ignore more Sync login error states. r=adw

---
 browser/base/content/browser-syncui.js        | 12 ++++-
 .../content/test/general/browser_syncui.js    | 53 +++++++++++++++++++
 services/sync/modules/policies.js             |  8 ++-
 3 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js
index 767c7c4f5b4..0b853ab1411 100644
--- a/browser/base/content/browser-syncui.js
+++ b/browser/base/content/browser-syncui.js
@@ -136,6 +136,8 @@ let gSyncUI = {
   },
 
   _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;
   },
@@ -234,8 +236,14 @@ let gSyncUI = {
       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;
     }
diff --git a/browser/base/content/test/general/browser_syncui.js b/browser/base/content/test/general/browser_syncui.js
index 68f11243260..91cb2971ccf 100644
--- a/browser/base/content/test/general/browser_syncui.js
+++ b/browser/base/content/test/general/browser_syncui.js
@@ -108,6 +108,59 @@ add_task(function* testSyncLoginError() {
   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");
diff --git a/services/sync/modules/policies.js b/services/sync/modules/policies.js
index d799cb2356f..690be98c8a2 100644
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -766,8 +766,12 @@ ErrorHandler.prototype = {
       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() {