Last Comment Bug 694311 - Unable to open Sync connection dialog prompt after providing faulty credentials - no access to Sync
: Unable to open Sync connection dialog prompt after providing faulty credentia...
Status: VERIFIED FIXED
: regression
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All Android
: -- major (vote)
: Firefox 9
Assigned To: Matt Brubeck (:mbrubeck)
:
:
Mentors:
: 694633 (view as bug list)
Depends on:
Blocks: 669199 688574 694543
  Show dependency treegraph
 
Reported: 2011-10-13 07:07 PDT by Carla Nadastean
Modified: 2013-12-10 10:00 PST (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.44 KB, patch)
2011-10-13 16:48 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch for aurora (2.32 KB, patch)
2011-10-13 17:02 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch v2 (7.80 KB, patch)
2011-10-14 12:41 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch v3 (7.85 KB, patch)
2011-10-14 12:42 PDT, Matt Brubeck (:mbrubeck)
mark.finkle: review+
philipp: review+
Details | Diff | Splinter Review
patch for aurora v2 (2.91 KB, patch)
2011-10-14 14:46 PDT, Matt Brubeck (:mbrubeck)
mark.finkle: review+
philipp: review+
lmandel: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Carla Nadastean 2011-10-13 07:07:08 PDT
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) 2011-10-13 14:04:10 PDT
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) 2011-10-13 14:13:31 PDT
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) 2011-10-13 16:48:54 PDT
Created attachment 566972 [details] [diff] [review]
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) 2011-10-13 17:02:29 PDT
Created attachment 566976 [details] [diff] [review]
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] 2011-10-13 17:52:10 PDT
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] 2011-10-13 17:54:53 PDT
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) 2011-10-14 12:41:03 PDT
Created attachment 567160 [details] [diff] [review]
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) 2011-10-14 12:42:47 PDT
Created attachment 567161 [details] [diff] [review]
patch v3

Removed a stray "debugger" statement.
Comment 9 Matt Brubeck (:mbrubeck) 2011-10-14 12:51:17 PDT
*** Bug 694633 has been marked as a duplicate of this bug. ***
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-14 12:52:12 PDT
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) 2011-10-14 14:46:57 PDT
Created attachment 567187 [details] [diff] [review]
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?) 2011-10-14 18:52:13 PDT
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] 2011-10-15 15:12:51 PDT
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] 2011-10-15 15:14:01 PDT
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) 2011-10-15 15:59:24 PDT
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) 2011-10-15 16:50:04 PDT
(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 18 Andreea Pod 2011-10-20 00:49:42 PDT
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) 2011-10-24 11:23:33 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ddc1d746bca
Comment 20 Catalin Suciu [:csuciu] 2011-10-31 02:58:54 PDT
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] 2011-10-31 03:02:35 PDT
Sorry, the bug logged for the issue mentioned above is #698356.

Note You need to log in before you can comment on or make changes to this bug.