481733 - Weave incorrectly reports connectivity issues as "Invalid username and/or password."

Status: VERIFIED FIXED

(Cloud Services :: General, defect, P1)

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot of login dialog w/ error message

STEPS TO REPRODUCE:
1. Start Firefox.  Log out of Weave, if it auto-logs in.
2. Unplug / disable all network links.
3. Attempt to log in to Weave.

ACTUAL RESULTS:
 - Login dialog says "Invalid username and/or password."
 - Weave notification appears in status bar, saying:
    ***
    Error While Signing In
    Weave encountered an error while signing you in:
    % Please try again
    ***

EXPECTED RESULTS:
 - Weave should report that there's a *connectivity* issue, rather than a username/password issue.
 - The status-bar-notification shouldn't contain a mystical "%" character :)

NOTE: This is probably the same underlying issue as bug 450480, but I'm filing it separately because the conditions & notification-text are slightly different.  (also, _this_ version of the bug has easy client-side steps-to-reproduce)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: log file

Here's Weave's log file during the failed login.

Comment 3

[Daniel Holbert [:dholbert]]

Un-duping, as bug 471192 is specifically about HTTP error responses, and I don't think we'd receive one of those in this bug's situation.

This bug here is about network connectivity issues -- i.e. situations where we can't reach the weave server _at all_.  AFAICT, this is a different sort of problem from bug 471192.

Comment 4

[Mike Connor [:mconnor]]

We should probably look at the network status via IOservice before we try to connect to anything, and otherwise handle network errors silently.

Comment 5

[Mike Connor [:mconnor]]

Attachment: mitigate this a bunch

This adds retry-on-fail for autoconnect, and disables sync/sign-in UI (needs to be done for Fennec, will file a followup).  Also changes the "Sign In" statusbar text to "Not Signed In" since the string kept confusing me in testing (since the menu is sign in, plus other options, and I kept expecting clicking to start sign-in).

Comment 6

[Mike Connor [:mconnor]]

Attachment: actually fix this bug where it occurs

Still need a followup for Fennec.

Comment 7

[Mike Connor [:mconnor]]

Attachment: make the interval fuzzy and increasing

This generally handles the various errors sanely and doesn't prevent us from retrying sync/login again, provides real user feedback on login failures.

This mostly makes bug 479570 not a P1, though it's hard to say what will fully fix that without knowing why the channel is failing.

Comment 8

[Ed Lee :Mardak]

Comment on attachment 395157 [details] [diff] [review]
make the interval fuzzy and increasing

>+++ b/source/chrome/content/login.js
>-    this._loginStatus.value = this._stringBundle.getString("loginError.label");
This makes the loginError.label unused, so remove that property.
>+    this._loginStatus.value = 
>+      this._stringBundle.getFormattedString("error.login.description", [Weave.Service.detailedStatus.sync]);
1) detailedStatus.sync is whatever string we put in there from service.js, so we'll want to make it localizable. The logic we want is what you added in sync.js where you map the error "code"/string to a string in sync.properties. Unless we expect constants.js.in to be localized, and we can get rid of the mapping and just use it directly.

2) we probably should be setting detailedStatus.login instead of sync

>+++ b/source/chrome/content/preferences.js
>@@ -49,35 +50,41 @@ WeavePrefs.prototype = {
>+    var isOffline = Weave.Svc.IO.offline;
var -> let. In some other file you alias this to just "offline" instead of "isOffline". Just "offline" should be fine.

>+++ b/source/chrome/content/sync.js
>@@ -134,19 +135,36 @@ WeaveWindow.prototype = {
>   onLoginError: function WeaveWin_onLoginError() {
>     this._log.info("Login Error");
>     this._setStatus("offline");
>+    
>+    Cu.import("resource://weave/constants.js");
No need to import on every error, and this is available on the Weave object anyway... (below)

>+    let title = this._stringBundle.getString("error.login.title");
>+    var reasonString;
nit: let
>+    switch (Weave.Service.detailedStatus.sync) {
>+      case LOGIN_FAILED_NETWORK_ERROR:
case Weave.LOGIN_FAILED_NETWORK_ERROR:

>+      case LOGIN_FAILED_NETWORK_ERROR:
>+        reasonString = "error.login.reason.passphrase";
LOGIN_FAILED_INVALID_PASSPHRASE

>+    let reason = this._stringBundle.getString(reasonString);
>     let description =
>       this._stringBundle.getFormattedString("error.login.description", [reason]);
Yeah.. so you'll need to share this logic for both login.js and sync.js, and switching on Weave.Service.detailedStatus.login to map a string message to a string message seems wasteful.

It does make sense if we were logging the constants string in the activity log for us to debug though.

>@@ -325,16 +343,23 @@ WeaveWindow.prototype = {
>+  onNetworkStatusChanged: function WeaveWin_onNetworkStatusChanged() {
>+    var offline = Weave.Svc.IO.offline;
nit: let
>+    document.getElementById("sync-loginitem").setAttribute("disabled", offline);
>+    var disableSync = !(!offline && Weave.Service.isLoggedIn);
disableSync = offline || !loggedIn ? easier to parse for me.. :p

>+++ b/source/modules/service.js
>@@ -471,25 +467,27 @@ WeaveSvc.prototype = {
>     let res = new Resource(this.baseURL + "api/register/chknode/" + username);
>     try {
>       res.get();
>     }
>     catch(ex) { /* we check status below */ }
Shouldn't we be able to merge this try with the below added try? It should only throw if necko failed, and if it did fail, res.lastChannel.responseStatus would throw again anyway.
> 
>-    if (res.lastChannel.responseStatus == 404) {
>-      this._log.debug("Using serverURL as data cluster (multi-cluster support disabled)");
>-      return Svc.Prefs.get("serverURL");
>-    }
>+    try {
>+      if (res.lastChannel.responseStatus == 404) {
Might as well switch(res.lastChannel.responseStatus) { case 404: case 200: }

>@@ -514,22 +512,43 @@ WeaveSvc.prototype = {
>+          if (passphrase && !this.verifyPassphrase(username, password, passphrase)) {
>+            this._setSyncFailure(LOGIN_FAILED_INVALID_PASSPHRASE);
Either create a _setLoginFailure or just use this.detailedStatus.login = blah;
(Not sure why we have so many wrappers going on.. _setSyncFailure uses _detailedStatus.setSyncStatus.. I suppose wrappers make sense for addServerStatus and setEngineStatus though.)

>@@ -606,20 +625,56 @@ WeaveSvc.prototype = {
>+  _autoConnectAttempts: 0,
>+  _autoConnect: function WeaveSvc__attemptAutoConnect() {
>+    try {
>+      if (!this.username || !this.password || !this.passphrase)
>+        return;
>+
>+      if (Svc.IO.offline)
>+        throw "Application is offline";
:( throwing from here to jump down to schedule another autoconnect? How about setting a failure variable and returning early if login succeeds.

>+      if (this.login())
>+        this.syncOnIdle();
return early here
>+      else
>+        throw this.detailedStatus.sync;
save this status
>+    }
>+    catch (ex) {
and this too. And continue the rest of the code outside the catch block:
>+      this._log.debug("Autoconnect failed: " + ex);
>+      let listener = new Utils.EventListener(Utils.bind2(this,
>+        function WeaveSvc__autoConnectCallback(timer) {
>+          this._autoConnectTimer = null;
>+          this._autoConnect();
>+        }));
>+      this._autoConnectTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+      
>+      // back off slowly, with some random fuzz to avoid repeated collisions
>+      var interval = Math.floor(Math.random() * SCHEDULED_SYNC_INTERVAL + 
>+                                SCHEDULED_SYNC_INTERVAL * this._autoConnectAttempts);
What values do you expect this to be? On first failure, it'll be a random wait time from 0 to 5 minutes.

>@@ -630,23 +685,27 @@ WeaveSvc.prototype = {
>       // Try starting the sync timer now that we're logged in
>       this._loggedIn = true;
>       this._checkSyncStatus();
// Clear pending autoconnect if the user manually logged in
>+      if (this._autoConnectTimer) {
>+        this._autoConnectTimer.cancel();
>+        this._autoConnectTimer = null;
>+      }

Comment 9

[Ed Lee :Mardak]

Attachment: error messages in ui

So right now the login screen shows the constants.js error message wrapped with "Error: <error>." (double periods)

If we did switch to the notification box error message of "Network error" the login screen would look like "Error: Network error."

Comment 10

[Mike Connor [:mconnor]]

Attachment: with comments addressed

(In reply to comment #8)
> (From update of attachment 395157 [details] [diff] [review])
> >+++ b/source/chrome/content/login.js
> >-    this._loginStatus.value = this._stringBundle.getString("loginError.label");
> This makes the loginError.label unused, so remove that property.
> >+    this._loginStatus.value = 
> >+      this._stringBundle.getFormattedString("error.login.description", [Weave.Service.detailedStatus.sync]);
> 1) detailedStatus.sync is whatever string we put in there from service.js, so
> we'll want to make it localizable. The logic we want is what you added in
> sync.js where you map the error "code"/string to a string in sync.properties.
> Unless we expect constants.js.in to be localized, and we can get rid of the
> mapping and just use it directly.

We do this in status.js as well, filed as a followup to sort this (and the switching on the string consts to get a string bit): Bug 511548

> 2) we probably should be setting detailedStatus.login instead of sync

Yeah, makes sense, followup: bug 511549

> >@@ -325,16 +343,23 @@ WeaveWindow.prototype = {
> >+  onNetworkStatusChanged: function WeaveWin_onNetworkStatusChanged() {
> >+    var offline = Weave.Svc.IO.offline;
> nit: let
> >+    document.getElementById("sync-loginitem").setAttribute("disabled", offline);
> >+    var disableSync = !(!offline && Weave.Service.isLoggedIn);
> disableSync = offline || !loggedIn ? easier to parse for me.. :p

Yes, quite.  For some reason I only remember that syntax when reviewing other people's code... never when writing it.

> >+      // back off slowly, with some random fuzz to avoid repeated collisions
> >+      var interval = Math.floor(Math.random() * SCHEDULED_SYNC_INTERVAL + 
> >+                                SCHEDULED_SYNC_INTERVAL * this._autoConnectAttempts);
> What values do you expect this to be? On first failure, it'll be a random wait
> time from 0 to 5 minutes.

That's what I expected.  Do you think we should have a minimum threshold for the first retry?  Given that we already check for offline, I don't think the floor really will make much of a difference, though it's possible we'll get something incredibly low.

The rest is all addressed

Comment 11

[Mike Connor [:mconnor]]

Attachment: correct patch

Comment 12

[Ed Lee :Mardak]

Comment on attachment 395471 [details] [diff] [review]
correct patch

r=Mardak with nits:

>+    var offline = Weave.Svc.IO.offline;
let

>+    if (!shouldBackoff) {
>+      try {
>+        shouldBackoff = Utils.checkStatus(Records.lastResource.lastChannel.responseStatus, null, [500,[502,504]]))
wrap and semicolon

Comment 13

[Mike Connor [:mconnor]]

Attachment: work around bitrot (checked in)

Comment 14

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/3eb68a7b4be8

Comment 15

[Tracy Walker [:tracy]]

added additional steps to https://litmus.mozilla.org/show_test.cgi?id=11778