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

VERIFIED FIXED in Firefox 9

Status

Fennec Graveyard
General
--
major
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Carla Nadastean, Assigned: mbrubeck)

Tracking

({regression})

Trunk
Firefox 9
All
Android
regression
Dependency tree / graph

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
status-firefox10: --- → affected
status-firefox9: --- → affected
Keywords: regression
Summary: Incorrect account name or password can't be changed in Preferences->Sync → Unable to open Sync connection dialog prompt after providing faulty credentials - no access to Sync

Updated

6 years ago
Keywords: regressionwindow-wanted
(Assignee)

Updated

6 years ago
tracking-fennec: --- → ?
(Assignee)

Updated

6 years ago
Assignee: nobody → mbrubeck
(Assignee)

Comment 1

6 years ago
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.
Blocks: 688574
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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.
Attachment #566972 - Flags: review?(mark.finkle)
Attachment #566972 - Flags: feedback?(philipp)
(Assignee)

Comment 4

6 years ago
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.
Attachment #566976 - Flags: review?(mark.finkle)
Attachment #566976 - Flags: feedback?(philipp)
(Assignee)

Updated

6 years ago
Blocks: 669199
Keywords: regressionwindow-wanted
(Assignee)

Updated

6 years ago
Whiteboard: [has patch]
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;
>+    }
Attachment #566972 - Flags: feedback?(philipp)
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.
Attachment #566976 - Flags: feedback?(philipp)
(Assignee)

Updated

6 years ago
Attachment #566972 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Attachment #566976 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Blocks: 694543
(Assignee)

Comment 7

6 years ago
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.)
Attachment #566972 - Attachment is obsolete: true
Attachment #566976 - Attachment is obsolete: true
Attachment #567160 - Flags: review?(philipp)
Attachment #567160 - Flags: review?(mark.finkle)
(Assignee)

Comment 8

6 years ago
Created attachment 567161 [details] [diff] [review]
patch v3

Removed a stray "debugger" statement.
Attachment #567160 - Attachment is obsolete: true
Attachment #567160 - Flags: review?(philipp)
Attachment #567160 - Flags: review?(mark.finkle)
Attachment #567161 - Flags: review?(philipp)
Attachment #567161 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 694633
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*
Attachment #567161 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 11

6 years ago
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.
Attachment #567187 - Flags: review?(philipp)
Attachment #567187 - Flags: review?(mark.finkle)
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.
Attachment #567187 - Flags: review?(mark.finkle) → review+
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!
Attachment #567161 - Flags: review?(philipp) → review+
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.
Attachment #567187 - Flags: review?(philipp) → review+
(Assignee)

Comment 15

6 years ago
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.
Attachment #567187 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

6 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/0e88066a7078
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → Firefox 10
(Assignee)

Updated

6 years ago
status-firefox10: affected → fixed

Comment 18

6 years ago
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)
Status: RESOLVED → VERIFIED

Updated

6 years ago
Whiteboard: [QA+]
Attachment #567187 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 19

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ddc1d746bca
status-firefox9: affected → fixed
Target Milestone: Firefox 10 → Firefox 9
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
Whiteboard: [QA+]
Sorry, the bug logged for the issue mentioned above is #698356.
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.