Closed Bug 1121040 Opened 9 years ago Closed 9 years ago

Multiple saved passwords for a website, selecting with cursor keys + ENTER Key doesn't work when website handles enter keypress itself

Categories

(Toolkit :: Form Manager, defect, P1)

35 Branch
defect
Points:
8

Tracking

()

RESOLVED FIXED
mozilla39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox35 --- affected
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox39 --- fixed

People

(Reporter: martinsiller, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Keywords: regression, site-compat, testcase)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150108202552

Steps to reproduce:

I visit a website where i need to login and there is more than 1 set of saved credentials (username + password) in Firefox.
So there is no username and password filled in automatically when visiting the website.

So I press 'Cursor Down' to view the list of available login names, select one by pressing 'curser down' one or more times until the desired username is selected and then press 'ENTER'.



Actual results:

The login fails.


Expected results:

I should have logged in successfully.

Login works, if i select the desired user name with the mouse cursor instead.
I noticed this issue in Firefox 33 and higher.
Version 32 doesn't show this bug
Which website? It's possible that the website is acting on the <enter> keypress before we've filled the form, or something - at least, I've been using 34 and 35 for a while now and have multiple gmail accounts, and have never seen it being an issue there... so the website on which this is happening for you is relevant. :-)
Component: Untriaged → Password Manager
Flags: needinfo?(martinsiller)
Product: Firefox → Toolkit
I can give you an example:

https://myayk.ayksolutions.com

When I select the username with the cursor key and press enter, it gives me the following error:

Login Failed
Username or password missing.

If i try to login with an incorrect password, the error is 

Login Failed
Invalid login or password.

So indeed, the ENTER keypress after selecting the username with the cursor keys seems to start the login before filling the form.

However, with Firefox 32, i can login without issues by using the keyboard only
Flags: needinfo?(martinsiller)
Yes, I can reproduce this. I thought this was a duplicate, but I can't find anything, so I'm confirming for now. I'll set needinfo on myself to investigate exactly when this broke and/or what is breaking it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(gijskruitbosch+bugs)
Summary: Multiple saved passwords for a website, selecting with cursor keys + ENTER Key doesn't work → Multiple saved passwords for a website, selecting with cursor keys + ENTER Key doesn't work when website handles enter keypress itself
Whiteboard: DUPEME
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=464bca437658&tochange=be076357691c

This has:

bug 1020495
bug 995489
bug 993197

which all look like they could have caused this. :-\

Matt, do you know which of these is most likely and/or if we can do much here?

Basically, it seems we fill things in asynchronously now, and because the enter keypress immediately triggers the website submitting the form, we fail.

Part of me thinks the website shouldn't be seeing the <return> keypress at all, but then the form also wouldn't be submitted. We could call the form submit handler ourselves, but that wouldn't work on all sites... thoughts?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(MattN+bmo)
Whiteboard: DUPEME
(In reply to :Gijs Kruitbosch from comment #4)
> Matt, do you know which of these is most likely and/or if we can do much
> here?

I suspect it's one of the e10s-related fixes e.g. the latter two or bug 949617

> Basically, it seems we fill things in asynchronously now, and because the
> enter keypress immediately triggers the website submitting the form, we fail.

Yeah, I'm pretty sure it was bug 949617 now since it's changing tests that caught this regression and made  them async so they passed :( https://hg.mozilla.org/mozilla-central/rev/2f1356e9b56d#l22.119

> Part of me thinks the website shouldn't be seeing the <return> keypress at
> all, but then the form also wouldn't be submitted. We could call the form
> submit handler ourselves, but that wouldn't work on all sites... thoughts?

We can see what other browsers do in this case.

If we don't let the page see the <return>, we don't necessarily have to call the form submit handler ourselves if we're fine with the user needing to hit <return> a second time to submit. I think that change in behaviour would be better than the one reported here although some may still consider it a regression. This seems to be what Safari does for me on 10.9. Chrome is hard to test since they don't prompt to save credentials when they think login fails. I don't have IE accessible at the moment.

I'm not against going back to the older behaviour. We may be able to catch the existing event and then re-dispatch it once we have actually autofilled. It sounds like this may also affect regular form history (e.g. a search form with a field and search button) but I'm not 100% sure without testing it or seeing the exact cause of the problem.
Blocks: 949617
Flags: needinfo?(MattN+bmo) → firefox-backlog+
OS: Windows 7 → All
Hardware: x86_64 → All
Hmm... thinking about it more though, when we've done the async creation of the dropdown, can't the actual filling in be sync? All the data should be readily available, and I'm assuming the dropdown already lives in the content process inasmuch as e10s is involved... :-\
Flags: needinfo?(MattN+bmo)
Yeah, it looks like Enter/Return is handled in the parent currently so if we can handle it in the child then we should be able to use the already-sync selectedIndex getter (see the relevant comment there). https://mxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js?rev=cc368429c22a&mark=538-546#526

I had thought that may it worked like I think form validation popups work: they live in the parent and just position themselves over content.
Flags: needinfo?(MattN+bmo)
Assignee: nobody → ally
Priority: -- → P1
Whiteboard: [cpp]
Whiteboard: [cpp] → [lang=cpp]
Whiteboard: [lang=cpp] → [lang=cpp][wip]
(In reply to Matthew N. [:MattN] from comment #7)
> Yeah, it looks like Enter/Return is handled in the parent currently so if we
> can handle it in the child then we should be able to use the already-sync
> selectedIndex getter (see the relevant comment there).
> https://mxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.
> js?rev=cc368429c22a&mark=538-546#526
> 
> I had thought that may it worked like I think form validation popups work:
> they live in the parent and just position themselves over content.

Blake, since you wrote the e10s code that caused this regression, does MattN's plan make sense from your side of the world? Or would I be trading one regression for others?
Flags: needinfo?(mrbkap)
I think MattN's plan makes sense. I don't see any other regressions that might come from it.

(I also happen to think that hiding the return from the page makes sense, but that's neither here nor there, I suppose.)
Flags: needinfo?(mrbkap)
Status: NEW → ASSIGNED
(Note that it's possible the page has changed since the initial report so I'm attaching a reduced testcase that I think represents the problem.)

What I'm seeing now is that the username does get autocompleted before the submit but the password doesn't so HandleEnter/selectedIndex doesn't seem to be the problem at the moment (it's possible there is still racing going on there, especially with e10s).

I now think the change that caused the problem is: http://hg.mozilla.org/mozilla-central/diff/2f1356e9b56d/toolkit/components/passwordmgr/LoginManagerContent.jsm#l1.273

With non-e10s, I confirmed that onUsernameInput gets called before the form submission but the change in question no longer fills the form synchronously.
Assignee: ally → MattN+bmo
Attached file MozReview Request: bz://1121040/MattN (obsolete) —
/r/4117 - Bug 1121040 - Cache login data in the content JSM to avoid round-trips to the parent and allow synchronous filling when needed. r=dolske

Pull down this commit:

hg pull review -r db9140120eb0e4fae8cbbf33a19366e5717330f0
Comment on attachment 8567325 [details]
MozReview Request: bz://1121040/MattN

This type of cache is also needed for recipes so that we don't have to duplicate the recipes in memory in every process.

I think this makes sense from a performance perspective too. I suspect we'll be fine not worrying about cache invalidation for a while as I think it's rare to need login updates/additions to propagate to other open tabs. Now that I think about it, it would be easy to update the cache in RemoteLogins:loginsAutoCompleted so simply triggering autocomplete updates the saved logins.

I'm going to go through and revert the test changes from bug 949617 that caught the issue but were changed and add a new test specifically for the Enter-to-submit case.
Attachment #8567325 - Flags: feedback?(dolske)
Whiteboard: [lang=cpp][wip]
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify?
Hi Matt, can you provide a point value.
Flags: needinfo?(MattN+bmo)
Points: --- → 5
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(MattN+bmo)
Points: 5 → 8
Comment on attachment 8567325 [details]
MozReview Request: bz://1121040/MattN

/r/4117 - Bug 1121040 - Cache login data in the content JSM to avoid round-trips to the parent and allow synchronous filling when needed. r=dolske

Pull down this commit:

hg pull review -r 2ca0220d445cef484aaa6338e822cef4f19e51e6
Attachment #8567325 - Flags: feedback?(dolske) → review?(dolske)
Comment on attachment 8567325 [details]
MozReview Request: bz://1121040/MattN

I don't think this is the right way to fix this...

Mainly, I think this adds too big of a footgun, because now you should be dealing with cache invalidation. Otherwise autocomplete does the wrong thing if the user changes their password or deletes that login.

I'm also dubious about adding a cache for synchronicity (Police reference goes here). Feels like a bit too much of a hack to work around the actual problem, but perhaps I could be sold on it if that's the least-worst option.

Smaller/fixable issue: we know we want password manager to work on pages without <form>s, so adding further dependencies on having a form (as the map key) isn't great.


Alternative ideas:

Right now autocomplete is rather dumb, because the UserAutoCompleteResult implementation has all the login data, but all it does it plop the username into the field and rely on the usual onUsernameInput processing to fill the form (just like if the user typed it). It would seem quite useful to be somehow keep those logins available through the whole chain of operations.

One thing that might work here, to avoid solving that whole enchilada right now... Can we make getFinalCompleteValueAt() just trigger fillForm() itself?
Attachment #8567325 - Flags: review?(dolske) → review-
Also, I don't understand why we'd need caching for the recipees? Can you say a few words more about that?
(In reply to Justin Dolske [:Dolske] from comment #16)
> Mainly, I think this adds too big of a footgun, because now you should be
> dealing with cache invalidation. Otherwise autocomplete does the wrong thing
> if the user changes their password or deletes that login.

Note that I didn't actually change autocomplete UI results therefore the dropdown itself will still always have the correct live results but the blur/autocomplete auto-fill may be stale. I think cache validation can be solved here though.

> Smaller/fixable issue: we know we want password manager to work on pages
> without <form>s, so adding further dependencies on having a form (as the map
> key) isn't great.

My understanding is that we're still going to have a form-like wrapper to define where a form starts/ends so I don't think this is a big problem and I considered it during implementation.

I'm still considering the rest right now but…

(In reply to Justin Dolske [:Dolske] from comment #17)
> Also, I don't understand why we'd need caching for the recipees? Can you say
> a few words more about that?

It's that we would be reading the recipes once in the parent process and we need to be able to use them from any frame script. We could serialize all recipes and duplicate them in memory for every new frame script but that seems like a waste of memory. I was thinking that we would send only relevant recipes (e.g. for the origin) to the child only when a password field was present (e.g. DOMFormHasPassword) and we would want to use them for auto-fill, autocomplete and capture so having them cached for the form/page makes sense to me.
(In reply to Justin Dolske [:Dolske] from comment #16)
> Alternative ideas:
> 
> Right now autocomplete is rather dumb, because the UserAutoCompleteResult
> implementation has all the login data, but all it does it plop the username
> into the field and rely on the usual onUsernameInput processing to fill the
> form (just like if the user typed it). It would seem quite useful to be
> somehow keep those logins available through the whole chain of operations.
> 
> One thing that might work here, to avoid solving that whole enchilada right
> now... Can we make getFinalCompleteValueAt() just trigger fillForm() itself?

Note that I think this wouldn't fix auto-filling when hitting enter in the username field when the autocomplete popup isn't open. I also think the controller is probably a better place that the result object but that gets trickier since it's not just for login autocompletion.
(In reply to :Gijs Kruitbosch from comment #4)
> Part of me thinks the website shouldn't be seeing the <return> keypress at
> all, but then the form also wouldn't be submitted. We could call the form
> submit handler ourselves, but that wouldn't work on all sites... thoughts?

This was actually our behaviour at one point but it seems to have been changed only for username fields in this regression window: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c3c4c902e9cd&tochange=31879b88cc82

I'm surprised the behaviour of satchel and password manager differ here since they share a lot of the code.

satchel testcase: data:text/html,<form><input name=q><input type=submit></form>
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
(In reply to Matthew N. [:MattN] from comment #18)

> > Mainly, I think this adds too big of a footgun, because now you should be
> > dealing with cache invalidation. Otherwise autocomplete does the wrong thing
> > if the user changes their password or deletes that login.
> 
> Note that I didn't actually change autocomplete UI results therefore the
> dropdown itself will still always have the correct live results but the
> blur/autocomplete auto-fill may be stale.

Maybe I'm not reading the code right, but it seems like this is broken:

1) Load singlepageapp.com
2) User logs in, and logins for that form get cached
3) User changes their password / deletes that login.
...time passes, but same single-page-app still open and login times out or something...
4) User goes to log in again, we're using stale logins from #2.


> (In reply to Justin Dolske [:Dolske] from comment #17)
> > Also, I don't understand why we'd need caching for the recipees? Can you say
> > a few words more about that?
> 
> It's that we would be reading the recipes once in the parent process and we
> need to be able to use them from any frame script. We could serialize all
> recipes and duplicate them in memory for every new frame script but that
> seems like a waste of memory. I was thinking that we would send only
> relevant recipes (e.g. for the origin) to the child only when a password
> field was present (e.g. DOMFormHasPassword) and we would want to use them
> for auto-fill, autocomplete and capture so having them cached for the
> form/page makes sense to me.

I think this is premature optimization. :) For the near future the recipees should be small, so even per-process duplication isn't a big deal. But, sure, store them all in the parent and pass them to the child on demand. I don't think that needs caching in the child, it should be cheap to fetch them again when needed, and avoids the complexity of caching/invalidation entirely.
(In reply to Matthew N. [:MattN] from comment #20)

> This was actually our behaviour at one point but it seems to have been
> changed only for username fields in this regression window:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=c3c4c902e9cd&tochange=31879b88cc82

Huh. Weird, nothing looks obvious.

That satchel works different is an interesting data point too... If the user is selecting their username from the autocomplete dropdown using the normal satchel / form history code (e.g. password manager is entirely disabled), that really really shouldn't be triggering a form submission -- either we or the page are fundamentally broken. Otherwise how is the user expected to get to the password field to enter their password?
(In reply to Justin Dolske [:Dolske] from comment #22)
> That satchel works different is an interesting data point too... If the user
> is selecting their username from the autocomplete dropdown using the normal
> satchel / form history code (e.g. password manager is entirely disabled),
> that really really shouldn't be triggering a form submission -- either we or
> the page are fundamentally broken. Otherwise how is the user expected to get
> to the password field to enter their password?

I'm confused by your reply since it should work fine now when password manager is disabled but not when we will autofill the password.

I'm going to look into going back to the old behaviour because that fixes most of this bug and seems to match IE, Chrome and Safari on the OSs I tested. It also bring back internal consistency in our UI.
(In reply to Matthew N. [:MattN] from comment #20)
> (In reply to :Gijs Kruitbosch from comment #4)
> > Part of me thinks the website shouldn't be seeing the <return> keypress at
> > all, but then the form also wouldn't be submitted. We could call the form
> > submit handler ourselves, but that wouldn't work on all sites... thoughts?
> 
> This was actually our behaviour at one point but it seems to have been
> changed only for username fields in this regression window:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=c3c4c902e9cd&tochange=31879b88cc82
> 
> I'm surprised the behaviour of satchel and password manager differ here
> since they share a lot of the code.
> 
> satchel testcase: data:text/html,<form><input name=q><input
> type=submit></form>

Ugh, ignore the part about this only affecting username fields since my satchel testcase wasn't testing the same thing (with a keypress handler in the page to submit).

I'm pretty sure bug 633058 is what regressed handling of Enter in an autocomplete popup but that doesn't exactly match the regression range (it's off by a few months).
Blocks: 633058
Ugh… preventing the page from seeing the event requires a solution like bug 286933 (which I independently came up with) but which caused regressions last time. Perhaps now that bug 633058 landed this will be less of a problem to fix…
See Also: → 286933
I'm going to make this bug a subset of bug 286933 and only fix the RETURN keypress as it's the most important of the keypresses, it will reduce site-compat risk, and it affects password forms.
Blocks: 286933
Component: Password Manager → Form Manager
See Also: 286933
Comment on attachment 8567325 [details]
MozReview Request: bz://1121040/MattN

/r/4117 - Bug 1121040 - Don't send RETURN keypresses to content while a satchel autocomplete entry is selected. r=Gijs,smaug

Pull down this commit:

hg pull review -r e7899ab86ec5a444b8e99f6f3b8447df4040726e
Attachment #8567325 - Flags: review-
Comment on attachment 8567325 [details]
MozReview Request: bz://1121040/MattN

/r/4117 - Bug 1121040 - Don't send RETURN keypresses to content while a satchel autocomplete entry is selected. r=Gijs,smaug

Pull down this commit:

hg pull review -r 821a037af3118f8a2d94e2dae899bef906067cd2
Attachment #8567325 - Flags: review?(gijskruitbosch+bugs)
Attachment #8567325 - Flags: review?(bugs)
Attachment #8567325 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8567325 [details]
MozReview Request: bz://1121040/MattN

https://reviewboard.mozilla.org/r/4115/#review3595

::: toolkit/components/satchel/nsFormFillController.cpp
(Diff revision 4)
> +      aEvent->StopPropagation();

Shouldn't this be aEvent->StopImmediatePropagation() ?
Comment on attachment 8567325 [details]
MozReview Request: bz://1121040/MattN

StopImmediatePropagation might be a tad better, but doesn't matter much
since the target is in chrome.
Attachment #8567325 - Flags: review?(bugs) → review+
https://reviewboard.mozilla.org/r/4115/#review3627

> Shouldn't this be aEvent->StopImmediatePropagation() ?

Since this is in chrome I decided not to use StopImmediatePropagation in case other Chrome still wants to listen.
See comment 31 and comment 32. Are you fine with dropping this? I don't have a strong opinion but I think the ability in comment 32 is useful and as-is the change is less risky.
Flags: needinfo?(gijskruitbosch+bugs)
Sure, let's land it like this, then.
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/integration/fx-team/rev/99d7d283b44a
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
Keywords: site-compat
https://hg.mozilla.org/mozilla-central/rev/99d7d283b44a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla39
Attachment #8567325 - Attachment is obsolete: true
Attachment #8619122 - Flags: review+
Depends on: 1220618
Blocks: 1388123
You need to log in before you can comment on or make changes to this bug.