694311 - Unable to open Sync connection dialog prompt after providing faulty credentials - no access to Sync

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Carla Nadastean]

Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111013 Firefox/10.0a1 Fennec/10.0a1
Device: Motorola Droid 2
OS: Android 2.3

Steps to reproduce:
1. Open Fennec app.
2. Go to Tools -> Preferences tab.
3. Under Sync area tap on Connect button.
4. Select "I'm not near my computer".
5. Enter some incorrect account name, password or recovery key.
6. Tap on Connect button.
7. Wait for the "Incorrect account name or password" message to be displayed and tap again on Connect button.

Expected Result:
User should be able to change his account name, password or recovery key.

Actual Result:
When Connect button is selected, "Connecting..." text is displayed in Sync area and again "Incorrect account name or password" message is displayed.
User is not able to change his incorrect credentials.

Comment 1

[Matt Brubeck (:mbrubeck)]

In Firefox 10 this looks like a regression from bug 688574 - the login fails, but the services.sync.username pref is set, so the Fennec sync UI acts like the user is connected successfully.

I haven't looked at what is happening in the Firefox 9 code yet.

Comment 2

[Matt Brubeck (:mbrubeck)]

When I back out bug 688574 then I get the behavior described in comment 0.  The problem there is in WeaveGlue.tryConnect, where Weave.Status.checkSetup() returns "success.status_ok" even though the login failed.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: patch

prefHasValue("services.sync.username") does not mean that sync is set up successfully.  If there is a username but the login fails because of bad credentials, we should still treat sync as not connected.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch for aurora

This Aurora 9 patch is simpler because it only needs to fix one problem (regression from bug 669199), and not the additional regression from bug 688574 that is only in Firefox 10.

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 566972 [details] [diff] [review]
patch

>   tryConnect: function login() {
>     // If Sync is not configured, simply show the setup dialog
>-    if (!Services.prefs.prefHasUserValue("services.sync.username")) {
>+    if (!Services.prefs.prefHasUserValue("services.sync.username")
>+        || this._loginError == Weave.LOGIN_FAILED_INVALID_PASSPHRASE
>+        || this._loginError == Weave.LOGIN_FAILED_NO_USERNAME
>+        || this._loginError == Weave.LOGIN_FAILED_NO_PASSWORD
>+        || this._loginError == Weave.LOGIN_FAILED_LOGIN_REJECTED) {
>       this.open();
>       return;
>     }
> 
>     // No setup data, do nothing
>     if (!this.setupData)
>       return;
> 
>@@ -392,22 +397,40 @@ let WeaveGlue = {
>     // Make some aliases
>     let connect = this._elements.connect;
>     let connected = this._elements.connected;
>     let details = this._elements.details;
>     let device = this._elements.device;
>     let disconnect = this._elements.disconnect;
>     let sync = this._elements.sync;
> 
>+    // Show what went wrong with login if necessary
>+    if (aTopic == "weave:service:login:error") {

Since bug 659067 we have an ErrorHandler object that determines whether errors need to be bubbled up to the UI or not. If they do, it relays them as "weave:ui:login:error" and "weave:ui:sync:error" notifications. I would recommend just subscribing to those (or perhaps only the login:error one). This would also save you having to whitelist values for Status.login above.

>+      this._loginError = Weave.Status.login;

Why not use Weave.Status.login directly? Saves you from having to reset the value on login:finish below.

>+      if (this._loginError == Weave.MASTER_PASSWORD_LOCKED)
>+        Weave.Service.logout();

Can you explain why you're doing this?

>+      else
>+        connect.setAttribute("desc", Weave.Utils.getErrorString(Weave.Status.login));
>+    } else {
>+      connect.removeAttribute("desc");
>+    }
>+
>+    // Init the setup data if we just logged in
>+    if (!this.setupData && aTopic == "weave:service:login:finish") {
>+      this.loadSetupData();
>+      this._loginError = null;
>+    }

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 566976 [details] [diff] [review]
patch for aurora

Same questions/objections/recommendations for this patch as for the other one.

Also, this patch seems to be missing the reset for this._loginError on a successful login. I would just recommend using Weave.Status.login, unless I'm overlooking something.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

This backs out part of bug 688574.  It turns out that the "username" pref is not a good replacement for Weave.Status.checkSetup, because if you enter incorrect credentials then you will have a username pref but still will not be properly set up.

The perf impact should be minimal.  For users without a "username" pref, the checkSetup code will bail out early and still not load any other modules.  For users with a username, checkSetup will load some (but not all) Weave modules, but this should not happen until after our delayed init (after the first page loads), and it would happen anyway when the Weave service is initialized 10 seconds after startup.

The "username" pref is still used on about:home, because I decided that speed during initial page load was more important than correct handling of this edge case.

> >+      if (this._loginError == Weave.MASTER_PASSWORD_LOCKED)
> >+        Weave.Service.logout();
> 
> Can you explain why you're doing this?

This code was added because of bug 624552.  In the old patch, I was just moving it.  In the new patch I have removed it.  This changes the behavior slightly.  Now if login fails because the master password is locked (or other non-fatal reasons like network errors), we show sync as "connected" anyway, and users can "sync now" button to try again.

> >+      this._loginError = Weave.Status.login;
> 
> Why not use Weave.Status.login directly? Saves you from having to reset the
> value on login:finish below.

Rather than check Weave.Status.login directly, the new patch sets _loginError to "true" on weave:ui:login:error events, letting ErrorHandler decide which errors to expose in the UI.

We still listen to weave:service:login:error, because there's some code we want to run every time login finishes, whether it's an "exposed" error or not. (Specifically, the code that checks Weave.Service.locked and enables the "Sync Now" button.)

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

Removed a stray "debugger" statement.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 567161 [details] [diff] [review]
patch v3

These changes seem OK to me and I know you have a few test cases to check against now.

I guess we'll need to push off trying to minimize our dependence on checkSetup() for now. *shakes fist at sky*

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: patch for aurora v2

A simpler patch for Aurora 9.  This only changes the connect button (tryConnect) so it will open the setup UI again if there was a "fatal" login error (weave:ui:login:error), instead of just re-using the old setup data.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 567187 [details] [diff] [review]
patch for aurora v2

Looks like this should handle all the ways to try to connect to Sync, awesomebar and preferences.

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 567161 [details] [diff] [review]
patch v3

>   _addListeners: function _addListeners() {
>     let topics = ["weave:service:setup-complete",
>       "weave:service:sync:start", "weave:service:sync:finish",
>       "weave:service:sync:error", "weave:service:login:start",
>       "weave:service:login:finish", "weave:service:login:error",
>+      "weave:ui:login:error",

You shouldn't need to subscribe to "weave:service:login:error" anymore, I think. Rest looks good!

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 567187 [details] [diff] [review]
patch for aurora v2

>   _addListeners: function _addListeners() {
>     let topics = ["weave:service:setup-complete",
>       "weave:service:sync:start", "weave:service:sync:finish",
>       "weave:service:sync:error", "weave:service:login:start",
>       "weave:service:login:finish", "weave:service:login:error",
>+      "weave:ui:login:error",

Same thing here: "weave:service:login:error" shouldn't be necessary anymore.

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 567187 [details] [diff] [review]
patch for aurora v2

Requesting approval for Aurora 9.  This fixes a regression in 9.0 that can cause sync to get into an unrecoverable state after login fails.  This state is fairly rare (it happens only in the secondary non-JPAKE setup flow), but the impact on affected users is bad.

The fix is mobile-only, and I wrote this Aurora-only fix to minimize risk.  Unlike the mozilla-central version, this patch does not change any previous behavior, but just ensures we allow the user to re-start the setup process if sync sends us a login error.  It's not zero-risk, but even if I made a bad mistake in the logic then the worst-case consequence should be that users who manually re-try a failed sync login might see the setup UI when they shouldn't.

Comment 16

[Matt Brubeck (:mbrubeck)]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> You shouldn't need to subscribe to "weave:service:login:error" anymore, I
> think. Rest looks good!

Comment 11 explains why the code still listens to weave:service:login:error.  As I discussed with Philipp on IRC, we can fix this using the new ui:clear-error message, which I will do as part of bug 691705.

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e88066a7078

Comment 17

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0e88066a7078

Comment 18

[Andreea Pod]

Verified fixed following the steps from description.

Build: Mozilla /5.0 (Android;Linux armv7l;rv:10.0a1) Gecko/20111019 Firefox/10.0a1 Fennec/10.0a1
Device: LG Optimus 2X (Android 2.2)

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ddc1d746bca

Comment 20

[Catalin Suciu [:csuciu]]

This is Verified fixed on Aurora.
While testing this I noticed that the error messages for incorrect account name, password or sync key are not displayed anymore. For this issue, bug #618945 was logged.

Build: Mozilla /5.0 (Android;Linux armv7l;rv:9.0a2) Gecko/20111030 Firefox/9.0a2 Fennec/9.0a2
Device: HTC Desire
OS: Android 2.2

Comment 21

[Catalin Suciu [:csuciu]]

Sorry, the bug logged for the issue mentioned above is #698356.