Closed Bug 1287202 Opened 4 years ago Closed 9 months ago

Run login capture code upon page navigation if there are password fields present in a <form>

Categories

(Toolkit :: Password Manager, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox68 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

(Depends on 1 open bug, Blocks 4 open bugs, Regressed 1 open bug)

Details

(Whiteboard: [webcompat] [passwords:capture-UI])

Attachments

(4 files, 2 obsolete files)

Bug 1166947 only handled capture via navigation outside a <form> to avoid the second doorhanger issue mentioned below. This bug will also capture upon navigation when a <form> is used but we will need to keep some state to avoid the second doorhanger.

+++ This bug was initially created as a clone of Bug 1166947 +++

Pages can use JS to handle logins instead of using form submission events and we should try to capture the saved values in those cases.

We can likely use pagehide or pageshow events for this but we should consider some edge cases:
* Do we want to capture if the user closes a tab? What about the page doing window.close (e.g. Persona)? These may fire pagehide?
* Do we want to capture if the users navigates back/forward with typed text? What about the page navigating back/forward?
* We probably don't want to cause a second capture doorhanger to appear if the values were already captured via onbeforesubmit. We probably still want onbeforesubmit around to handle a submission with event.preventDefault() (no pagehide).
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Duplicate of this bug: 1279924
So this has similar tp5o regressions to bug 1293513 (as one would expect): https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=fc69febcbf6c&newProject=try&newRevision=b5073b4f6b98&framework=1&filter=tart&showOnlyImportant=0

I believe the problem is that I'm adding a webProgressListener on the docShell but really only want it to live for the life of the window. i.e. I only want to listen to the progress when the window's document contains password fields. The first time a password field is seen in a document (and the DOMInputPasswordAdded firres), we add the progress listener to the docShell of the window, but then if the top-level frame navigates to a page not containing password fields, we don't need to listen to progress events anymore but we continue to do so and the overhead of simply having a listener that does nothing shows up on talos.

If the page is navigated away from in a session and then the user goes back to the page, we do need to get those progress events again but DOMInputPasswordAdded isn't fired in the bfcache case.

IIRC, tp5o loads every page in one tab so once we encounter a page in the pageset that has a password field, our progress listener is getting called for every location change and state change even though we will eventually return without doing anything significant.

Olli, is there an existing pattern for handling this case? Ideally I would attach progress listeners on the inner window instead of the docShell but catch navigations and STATE_START away from that window. I suspect we don't have that and won't add such a thing so I guess I need to do some accounting myself. At a high level this bug is roughly about asking the user to save their login when navigating away from a page with a password field filled in (to handle pages which don't use <form> or their submit event).

Some options:
1) Keep track of the innerWindowIDs that progress listeners are added for in a frame/outerWindowID and listen for inner-window-destroyed notifications (since this will handle bfcache properly). Upon receiving this notification, remove the innerWindowID from the set for the associated outerWindowID. If the set for the outerWindowID becomes empty, remove the progress listener for that docShell. This means the listener is still attached when it won't do anything for the active document but allows it to clean up when old pages are evicted from bfcache.
2) Modify the DOMInputPasswordAdded event to also fire upon pageshow. Then we can remove the progress listener on every pagehide and add it again when the DOMInputPasswordAdded arrives (as we do today).
3) Like #2 (have pwmgr listen for pagehide events to remove the progress listener [assuming they happen after the relevant progress events]) and then re-add the progress listener upon some state that remember which pages had password fields (instead of relying on DOMInputPasswordAdded during pageshow).

All of the above will be more complicated when handling subframes. Are there better options? What is your preference?

setupProgressListener: https://hg.mozilla.org/try/file/2fe629bba313/toolkit/components/passwordmgr/LoginManagerContent.jsm#l330
onLocationChange: https://hg.mozilla.org/try/file/2fe629bba313/toolkit/components/passwordmgr/LoginManagerContent.jsm#l75
onStateChange: https://hg.mozilla.org/try/file/2fe629bba313/toolkit/components/passwordmgr/LoginManagerContent.jsm#l87

Thanks in advance
Flags: needinfo?(bugs)
Since I haven't heard back yet I'm going to implement option #1 as it seems like it's the most reliable/viable to succeed.
This is the most important bug for password manager as it means we don't work on a large percentage of sites. Unfortunately our password manager hasn't been prioritized since early 2015 so I don't have a chance to finish this. I'm unassigning myself to reflect actual resourcing.
Assignee: MattN+bmo → nobody
Status: ASSIGNED → NEW
Priority: P3 → P1
Hey Joe, can you help this get prioritized? I just ran in to this on a site that I needed to register for, and it broke my ability to use Password Manager with the new registration details since Password Manager wouldn't offer to save the password.

The site was using ASP.net and doing form submissions using JS with no non-JS fallback. Looking at https://trends.builtwith.com/framework/ASP.NET, the current usage rates of ASP.net are:
Quantcast Top 10k
2,238 of 10,000
22.4%

Quantcast Top 100k
21,477 of 100,000
21.5%

BuiltWith Top Million
142,893 of 823,409
17.4%

Entire Internet
46,715,433 of 371,396,617
12.6%
Flags: needinfo?(joe-bugzilla)
Do we have any idea if the possible regression to tp5 ended up being real?
It's hard to say if there would still be a regression this many months later but I also think it's possible to avoid the performance regression (at least on pages without login fields) with some of the options from comment 6 but I was looking for advice from smaug to see which approach he thinks is best. In other words, I think any potential performance issues are solvable if this bug gets resourced.

FYI: this affects/affected facebook.com on Android (bug 1279924)
Here is a test case using history.pushState that works in Safari and kinda works in Chrome (only when you hit the back button after a successful login). 

Other test cases can be found in bug 1166947 just by adding a <form> wrapper around login fields in those tests.
Am interested in seeing this fixed, thanks for the testcase. The current patch might need an update as LoginManagerContent._onNavigation isn't being hit (that is when applying the patch and running the testcase), MattN I'll ping you with questions on IRC.
(In reply to :garvan from comment #13)
> Am interested in seeing this fixed, thanks for the testcase. The current
> patch might need an update as LoginManagerContent._onNavigation isn't being
> hit (that is when applying the patch and running the testcase),

Yeah, I wasn't totally clear in comment 6 about the connection to bug 1293513. You need to revert bug 1293513 in order for the patch on the bug to work (possibly with a perf. regression). If there are perf issues then we may need to adjust how/when progress listeners are added/removed. See comment 6 for some ideas. It would still be useful to get some insight from smaug on this.
I just rebased the patch and included the revert of bug 1293513 so it's easier for someone else to take this.
Work with Maire to prioritize.
Flags: needinfo?(joe-bugzilla) → needinfo?(mreavy)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)

> The site was using ASP.net and doing form submissions using JS with no
> non-JS fallback. Looking at https://trends.builtwith.com/framework/ASP.NET,
> the current usage rates of ASP.net are:

Hey Jared, do you remember which site you saw this on? I'd like to look at their form submission code.
Flags: needinfo?(jaws)
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #20)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> 
> > The site was using ASP.net and doing form submissions using JS with no
> > non-JS fallback. Looking at https://trends.builtwith.com/framework/ASP.NET,
> > the current usage rates of ASP.net are:
> 
> Hey Jared, do you remember which site you saw this on? I'd like to look at
> their form submission code.

It is http://dteenergy.com/ which redirects to https://www.newlook.dteenergy.com/wps/wcm/connect/dte-web/home
Flags: needinfo?(jaws)
Thanks Jared.

So looking at that specific site, they use the following pattern:

1) set an onclick handler signinAndreloadUnifiedSubmit for <button> (nested inside <form>).
2) signinAndreloadUnifiedSubmit will POST to a login endpoint and fetch a security token, set some cookies, etc.
3) for commercial users (ignoring other types, but seems the same), call getUserProfileData
4) inside getUserProfileData, depending on various things, navigate to a new page using window.location.assign, e.g.
> window.location.assign(reloadToPage("RESIDENTIAL", obj.unauthSessionURL, noOfAccounts));

So nothing quite as fancy as history.pushState, but JS is POSTing to some server endpoint then handling page navigation itself via window.location.

ASP.net or not, this is a pretty common pattern for XHR form submission.
Here's a test page that shows Safari and Chrome prompt to save username/pw and we do not (no need for back button):

https://miketaylr.com/bzla/submitxhr.html

Edge does not... but I don't even know if they have a password manager.
Given the likely relative high frequency of this issue, I do think we should prioritize it to be fixed.  Our users should be able to trust that password manager will work the vast majority of the time.
Do we have any data to guide the prioritization of fixing this?

Comment 11 mentions it might have once affected Facebook on Android. (does it still? is it only on Mobile?) Comment 21 mentions a small/local energy provider. It's not clear to me that the stats in comment 9 are helpful -- seems like there's a big leap from "sites that use ASP.net" to "doesn't work with password manager".

This is certainly a bug that should eventually be fixed, and was an important piece of our earlier work to improve password manager. I'm very sympathetic to that.

But I'm trying to understand the distinction between "this is a major problem suddenly hurting us right now" and "this is just one of many known, long-standing limitations in the current password manager". The former would be something that fixing now would be make a significant impact on improving Firefox, the latter would be a small incremental change that doesn't materially improve Firefox. Password manager needs a _lot_ of additional work to reach the level of quality/robustness that users should expect, and this one issue is only a small step in that direction.
Flags: needinfo?(mreavy)
(In reply to Justin Dolske [:Dolske] from comment #25)
> Do we have any data to guide the prioritization of fixing this?
> 
> Comment 11 mentions it might have once affected Facebook on Android. (does
> it still? is it only on Mobile?) Comment 21 mentions a small/local energy
> provider. It's not clear to me that the stats in comment 9 are helpful --
> seems like there's a big leap from "sites that use ASP.net" to "doesn't work
> with password manager".

Yeah, ASP.net stats aren't useful here.

But any site that does username/pw submission using xhr (rather than form.submit()) then manually navigates in its success handler will fail here. 

That's a lot of sites, and a common pattern for client-side apps: <https://github.com/search?l=JavaScript&p=2&q=login+post+success+location&type=Code&utf8=%E2%9C%93> is one possible search for this kind of thing on the web.
(In reply to Justin Dolske [:Dolske] from comment #25)
> Do we have any data to guide the prioritization of fixing this?

Unlike bug 1166947, we don't have in-product data since the effort to gather the data is similar to the effort to fix the problem. In my experience this is the most common bug that gets filed/reported against password manager (not all linked to this bug) though that's not a large number of bugs per year.

Anecdotally I would estimate it affects 1-5% of login form loads which seems high enough to reduce trust in Firefox to save passwords.

> Comment 11 mentions it might have once affected Facebook on Android. (does
> it still? is it only on Mobile?) Comment 21 mentions a small/local energy
> provider. 

> It's not clear to me that the stats in comment 9 are helpful --
> seems like there's a big leap from "sites that use ASP.net" to "doesn't work
> with password manager".

Yeah, I don't know if there is a strong correlation there but it's clear to me that not using a submit event with a <form> isn't just isolated to a few sites.

> This is certainly a bug that should eventually be fixed, and was an
> important piece of our earlier work to improve password manager. I'm very
> sympathetic to that.
> 
> But I'm trying to understand the distinction between "this is a major
> problem suddenly hurting us right now" and "this is just one of many known,
> long-standing limitations in the current password manager". The former would
> be something that fixing now would be make a significant impact on improving
> Firefox, the latter would be a small incremental change that doesn't
> materially improve Firefox. 

I think it probably should have fell into the former category when it started affecting m.facebook.com but password manager fixes haven't been resourced like they were supposed to be by the FxPrivacy team when the passwords team merged into it.

> Password manager needs a _lot_ of additional
> work to reach the level of quality/robustness that users should expect, and
> this one issue is only a small step in that direction.

I think this is one of the most important bugs affecting trust in Firefox's password manager though… it wasn't clear that it was on the radar to get fixed anytime soon though. IMO all the P1 password manager bugs should be fixed as part of an upcoming project and this is a good starting point. i.e. if we don't want to prioritize this bug by itself then we should prioritize resourcing password manager (starting with P1 bugs like this one).
Blocks: 1275876
No longer depends on: 1275876
Whiteboard: [webcompat]
I don't know whether the password fields in https://mail.163.com share the same character. Firefox fails to remember the password.
(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #23)
> Here's a test page that shows Safari and Chrome prompt to save username/pw
> and we do not (no need for back button):
> 
> https://miketaylr.com/bzla/submitxhr.html
> 
> Edge does not... but I don't even know if they have a password manager.

FWIW, this now works for me in Nightly (tested in Windows and Android).
(In reply to Andreas Bovens [:abovens] from comment #29)
> (In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from
> comment #23)
> > Here's a test page that shows Safari and Chrome prompt to save username/pw
> > and we do not (no need for back button):
> > 
> > https://miketaylr.com/bzla/submitxhr.html
> > 
> > Edge does not... but I don't even know if they have a password manager.
> 
> FWIW, this now works for me in Nightly (tested in Windows and Android).

That's just because it's not a valid testcase. We've saved that kind of form for a long long time. The testcase button needs to change to:

> <button type="button">click me</button>

Use the testcase attachment to test instead (attachment 8880688 [details])

I also know this isn't fixed because bugs that rely on this fix are filed regularly (see the "Blocks" field) against various sites.
Flags: needinfo?(miket)
Flags: needinfo?(miket)
Marking as blocking the Savant experiment. One of the probes we are implementing on Savant is "login_form" which identifies when the client loads or submits a login form. This bug makes the form submission only partially knowable per discussion with Bianca.
Blocks: 1457226
Flags: needinfo?(bugs)
Blocks: 1478508
No longer depends on: 1478508
No longer blocks: 442524
Whiteboard: [webcompat] → [webcompat] [passwords:capture-UI]
Attachment #8880708 - Attachment is obsolete: true
Attachment #8792956 - Attachment is obsolete: true
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Attachment #9045973 - Attachment description: Bug 1287202 - Revert bug 1293513 to setup progress listeners for fields in a <form>. → Bug 1287202 - Revert bug 1293513 to setup progress listeners for fields in a <form>. r=jaws
Attachment #9045974 - Attachment description: Bug 1287202 - Run login capture code upon page navigation if there are password fields present in a <form> → Bug 1287202 - Run login capture code upon page navigation if there are password fields present in a <form>. r=jaws
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/d10006d3c64a
Revert bug 1293513 to setup progress listeners for fields in a <form>. r=jaws
https://hg.mozilla.org/integration/autoland/rev/e99bb6304a7a
Run login capture code upon page navigation if there are password fields present in a <form>. r=jaws
https://hg.mozilla.org/integration/autoland/rev/4b4f6ca7d624
Test login capture from a <form> upon page navigation. r=jaws

Backed out 3 changesets (Bug 1287202) for "e10s test-macosx64/debug-mochitest-e10s-5" failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.10%2Cdebug%2Cmochitests%2Cwith%2Ce10s%2Ctest-macosx64%2Fdebug-mochitest-e10s-5%2Cm-e10s%285%29&tochange=dd235c6dd881e45afb01a1320d4c7567a7dd1067&fromchange=85712605b11aa2d392173a603608bf5ce702d450&selectedJob=234666347

Backout link: https://hg.mozilla.org/integration/autoland/rev/9aebabe324dab2e881782658bb75e0c5257eb5cf

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=234666347&repo=autoland&lineNumber=31266

00:28:12 INFO - AddTask.js | Entering test test_4
00:28:12 INFO - checkPromptState: Expected: Please enter your master password.
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking expected message
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking title always visible on OS X
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking textbox visibility
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking passbox visibility
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking checkbox visibility
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking checkbox label
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking checkbox checked
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking expected icon CSS class
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking textbox value
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking passbox value
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | checking button0 default
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | checking button1 default
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | checking button2 default
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | Checking focused element
00:28:12 INFO - Buffered messages finished
00:28:12 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | checking expected empty user - got "user1", expected ""
00:28:12 INFO - SimpleTest.is@https://example.com/tests/SimpleTest/SimpleTest.js:320:16
00:28:12 INFO - test_4@https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/test_master_password.html:198:3
00:28:12 INFO - asyncnextTick/<@https://example.com/tests/SimpleTest/AddTask.js:70:34
00:28:12 INFO - async
nextTick@https://example.com/tests/SimpleTest/AddTask.js:86:11
00:28:12 INFO - setTimeout handlerSimpleTest_setTimeoutShim@https://example.com/tests/SimpleTest/SimpleTest.js:684:43
00:28:12 INFO - add_task@https://example.com/tests/SimpleTest/AddTask.js:30:7
00:28:12 INFO - @https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/test_master_password.html:54:1
00:28:12 INFO - Not taking screenshot here: see the one that was previously logged
00:28:12 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | checking expected empty pass - got "pass1", expected ""
00:28:12 INFO - SimpleTest.is@https://example.com/tests/SimpleTest/SimpleTest.js:320:16
00:28:12 INFO - test_4@https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/test_master_password.html:199:3
00:28:12 INFO - async
nextTick/<@https://example.com/tests/SimpleTest/AddTask.js:70:34
00:28:12 INFO - asyncnextTick@https://example.com/tests/SimpleTest/AddTask.js:86:11
00:28:12 INFO - setTimeout handler
SimpleTest_setTimeoutShim@https://example.com/tests/SimpleTest/SimpleTest.js:684:43
00:28:12 INFO - add_task@https://example.com/tests/SimpleTest/AddTask.js:30:7
00:28:12 INFO - @https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/test_master_password.html:54:1
00:28:12 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_master_password.html | should be logged out

Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/c293763c1be2
Revert bug 1293513 to setup progress listeners for fields in a <form>. r=jaws
https://hg.mozilla.org/integration/autoland/rev/13c5c3a63939
Run login capture code upon page navigation if there are password fields present in a <form>. r=jaws
https://hg.mozilla.org/integration/autoland/rev/7ca1b9b1a8bb
Test login capture from a <form> upon page navigation. r=jaws
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Duplicate of this bug: 1393679

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #12)

Here is a test case using history.pushState that works in Safari and kinda
works in Chrome (only when you hit the back button after a successful
login).

I have reproduced the issue in affected Nightly v68.0a1 from 2019-03-15 and I have verified the fix in Nightly v68.0a1 from 2019-04-23. The password manager pop-up now appears. (The "Show password" string is not displayed near the check-box, but I believe this is unrelated to this issue's verification.)
If this is sufficient to verify this bug, then please set this issue as verified in firefox68.

Other test cases can be found in bug 1166947 just by adding a <form> wrapper
around login fields in those tests.

If it is worth going the extra mile, I would like to verify other cases that might be valid. If you consider this productive, please write me the <form> wrapper and point me to the login fields from bug 1166947 that you mentioned before or any other.

Thanks!

Flags: needinfo?(MattN+bmo)

I resolved most of the bugs that depend on this one, confirming that the issue was fixed, so I don't think it's worth testing testcases I make, real world site are more important. I do know of a handful that aren't fixed still e.g. iCloud (bug 1289947) and Hulu (bug 1119083) but we have bugs blocking bug 1119035 for different approaches to capture as well.

Status: RESOLVED → VERIFIED
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.