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() {