Password manager does not work on script-generated forms

VERIFIED FIXED in mozilla26

Status

()

Toolkit
Password Manager
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: al_9x, Assigned: Dolske)

Tracking

(Blocks: 2 bugs, {regression})

unspecified
mozilla26
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox9-, firefox10-, relnote-firefox 26+)

Details

(Whiteboard: [Advo], URL)

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060918 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060918 Firefox/2.0

By default Firefox does not record login info for windows live mail.  But a bookmarklet (http://www.squarefree.com/bookmarklets/forms.html#remember_password) strips the necessary attributes from the login page and enables Firefox to memorize the login info.  Version 1.5.0.7 then fills out the login form on subsequent visits, but 2.0 rc1 does not, even though the password manager lists the user name and password.

Reproducible: Always

Comment 1

11 years ago
The attribute that turns off the password manager on that login page is autocomplete="off", and that issue is covered in bug 245333.

*** This bug has been marked as a duplicate of 245333 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → DUPLICATE

Comment 2

11 years ago
Hmm, actually, the autocomplete part might not be the real issue. That page may be wiping out the contents of the form after the password manager fills it in. Reopening for now.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---

Comment 3

11 years ago
So it appears that forms that are dynamically generated by javascript do not get the usual password manager event handlers attached. This wasn't a very visible issue before since the password manager hooked everything up after the onload handler was called, but now we hook everything up on DOMContentLoaded, pages which dynamically generate a form onload don't have the password manager hooked up properly.
Status: UNCONFIRMED → NEW
Depends on: 221634
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: firefox 2.0 rc1 does not fill in already recorded login info for windows live mail → Password manager does not work on script generated forms
(Reporter)

Comment 4

11 years ago
This is a regression from 1.5 so hopefully it will be fixed.  The fix from 221634 that caused this assumed a universe of static pages, is that really sound, considering how long dom scripting has been around?

For those who don't use cookies and rely on the password manager this is a major inconvenience.

Perhaps if the first attempt to fill the form early fails then try again after the page is fully loaded?

Comment 5

11 years ago
(In reply to comment #4)
> This is a regression from 1.5 so hopefully it will be fixed.  The fix from
> 221634 that caused this assumed a universe of static pages, is that really
> sound, considering how long dom scripting has been around?
> 
The code always assumed a universe of static pages - 221634 did not change this, but changed a detail which made it work most of the time.

> For those who don't use cookies and rely on the password manager this is a
> major inconvenience.
> 
Unfortunately, this is not exactly a 2.0 blocker. Maybe 2.0.1.

> Perhaps if the first attempt to fill the form early fails then try again after
> the page is fully loaded?
>
No, it's not that easy. :)
(Reporter)

Comment 6

11 years ago
> Unfortunately, this is not exactly a 2.0 blocker. Maybe 2.0.1.

so there is time to dumb down the options ui (cookies, downloads) but not to fix a regression bug?

Comment 7

11 years ago
(In reply to comment #6)
> > Unfortunately, this is not exactly a 2.0 blocker. Maybe 2.0.1.
> 
> so there is time to dumb down the options ui (cookies, downloads) but not to
> fix a regression bug?
> 
Yup. The preferences dialog changes were planned early enough. This regression was found *way* too late. It's all about timing. Beta 2 was the right time to get this bug filed. I can't even get my pet regressions fixed at this point unless it is really bad or really trivial.

Updated

11 years ago
Keywords: regression
Summary: Password manager does not work on script generated forms → Password manager does not work on script-generated forms
Version: unspecified → 2.0 Branch

Comment 8

11 years ago
(In reply to comment #5)
> (In reply to comment #4)
> > This is a regression from 1.5 so hopefully it will be fixed.  The fix from
> > 221634 that caused this assumed a universe of static pages, is that really
> > sound, considering how long dom scripting has been around?
> > 
> The code always assumed a universe of static pages - 221634 did not change
> this, but changed a detail which made it work most of the time.
> 
> > For those who don't use cookies and rely on the password manager this is a
> > major inconvenience.
> > 
> Unfortunately, this is not exactly a 2.0 blocker. Maybe 2.0.1.

How does one mark to block 2.0.1?
(Reporter)

Comment 9

11 years ago
So as not to rely on the Live mail page for reproduction, below is a sample.  Also, the password manager should work in manual mode, when you double click on the field after the page is loaded.  It seems that currently this only invokes the form data manager.  Is 2.0.1 still the projected fix target?
_______________________________________________________________________________

<html>
<head>
<script type="text/javascript">
function GenerateForm()
{
	document.write("<form><input type='text' name='user' /> <input type='password' name='password' /> <input type='submit' /> </form>")
	document.close()
}
</script>
</head>
<body onload="GenerateForm()" />
</html>
_________________________________________________________________________

is this firefox-only? it doesn't work on camino either making me think that this is a core bug.

Comment 11

11 years ago
(In reply to comment #10)
> is this firefox-only? it doesn't work on camino either making me think that
> this is a core bug.
> 
It is toolkit only, and camino doesn't use toolkit as far as I know. If camino has a similar bug, please open another one for camino.
(Reporter)

Comment 12

11 years ago
> Unfortunately, this is not exactly a 2.0 blocker. Maybe 2.0.1.

it seems this wasn't addressed in 2.0.0.1, are there plans to fix it?  When?
(Reporter)

Comment 13

11 years ago
Can an option be put it to restore old behavior?  Can someone from the development team comment on this?

Comment 14

10 years ago
Is this bug what is causing Firefox not to remeber the autofill values at http://www.telenor.se/599.jsp ?

The problem on that page is that if you restart Firefox you must enter the username manually even though you have stored in Password Manager. After entering the username, and then tabbing to the password field, the password is sometimes filled in automatically and sometimes you have to enter it manually. I can't really see a pattern but I guess it has something to do with cookies.

Furthermore, the username, even though it is not autofilled, after entering the first digit (username = phone number) the rest of it is (most of the time) listed in a drop down menu.
(Assignee)

Comment 15

10 years ago
No, it looks like the problem with the telenor.se page is due to them prefilling the field with a value as an instruction (we don't overwrite it if it doesn't match a username, due to other bugs), and it also looks like they're doing some strange style/dom manipulation.

Comment 16

9 years ago
I'm experiencing this also.  Password saves only for login.live.com but not the url passed from trillian (check hotmail). Tried the autosave, wont work on the url , I assume due to it being scripted form. Rather annoying bug, only a few sites Firefox wont save passwords, hotmail has to be a popular one.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Product: Firefox → Toolkit

Comment 17

9 years ago
I think I have the same problem with powerlink.emc.com

Comment 18

9 years ago
The sample in Comment #9 doesn't seem to work for me with

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
(Assignee)

Comment 19

9 years ago
This isn't likely to get fixed anytime soon. AFAIK, the only way to detect new forms being added is with DOM mutation events, and those are very expensive and would cause a perf regression on all pages.
Target Milestone: --- → Future

Comment 20

9 years ago
What about sites that have the username and password fields in the HTML, but not inside a form? mon.itor.us does that. The submit button is inserted by an onload script and triggers a submit script. Should be easier to handle.
(Assignee)

Comment 21

9 years ago
No, that's a completely different problem and is even harder to handle. We don't want to trudge through the entire page looking for <input>s, and we have no way of knowing when you take an action to login (because there's no form submission).
(Assignee)

Comment 22

8 years ago
So, it occurs to me that this might not be too hard to fix...

nsHTMLFormElement.cpp already has code (unused since the pwmgr rewrite) to start up the password manager the first time a password field is created. See:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLFormElement.cpp#1385 / gPasswordManagerInitialized

We could extend this idea to basically fire a notification whenever a document has a password field added. We'd probably want to do some consolidation to suppress multiple events during page load (eg, only send 1 event until DOMContentLoaded has fired). This could even be used to replace pwmgr's current mechanism of waiting for DOMContentLoaded, although we'd still want to run at approximately the same time. [I'm slightly worried about inline scripts modifying the field before that point, which would clobber any pwmgr value.]

Password fields are not terribly common, so this shouldn't add any significant overhead to normal DOM creation. I suppose you could have a page that has/adds a gaillion password fields, but that doesn't seem very likely...
Assignee: nobody → dolske
Target Milestone: Future → mozilla1.9.2
(Assignee)

Comment 23

8 years ago
Created attachment 383577 [details] [diff] [review]
Patch v.1 (WIP)

Quick and dirty PoC.
(Assignee)

Comment 24

8 years ago
Not actively working at this at the moment.
Assignee: dolske → nobody
Blocks: 565512

Updated

7 years ago
Duplicate of this bug: 547035

Comment 26

7 years ago
Looks like this might also affect any apps built in GWT, which uses JS to insert forms. We've had some users report that Firefox is no longer properly restoring their username/password in the Rypple login form:

https://rypple.com/feedback/#login

That might affect you guys a little more close to home, being Rypple users and all :)

Comment 27

7 years ago
(In reply to comment #26)

To clarify, I meant no longer since we made some changes to the form that result in it being inserted rather than because something changed in Firefox.
related page on stackoverflow :

http://stackoverflow.com/questions/2267543/do-browsers-support-autocomplete-for-ajax-loaded-login-forms-at-all

anyone working on this issue ?

Updated

7 years ago
Duplicate of this bug: 528749

Updated

7 years ago
Duplicate of this bug: 619715

Comment 31

6 years ago
Seems to be an issue with www.mail.com as well. Saved passwords for this site are not loading in the autocomplete, no matter how many times they are saved. 

Looking at my password manager, I have 10 entries for the same login details for the same domain (www.mail.com).

A fix would be great :)
Duplicate of this bug: 631417

Comment 33

6 years ago
This affects logins to Shlashdot at <http://slashdot.org/> and The Huffington Post at <http://www.huffingtonpost.com/>.
The management control panel on our domain registrar had its login page redesigned recently and now gets nailed by this, too.

Comment 35

6 years ago
I would also vote for this item to be fixed.
I think there should be an option in the context menu of FF to remember the password on the sites, where FF doesn't suggest to do so.
Setting some flags to get some attention.  This is fast becoming a major usability issue because more and more sites are starting to do this with password forms.
tracking-firefox10: --- → ?
tracking-firefox9: --- → ?

Comment 37

6 years ago
[triage comment]
Denying for tracking. We use the tracking flag to track issues that must be fixed/investigated/resolved for a given release, not to set priorities for engineering.

If you think this bug is not getting the engineering attention it deserves, please bring it up with the module owner, post to a mailing list (dev-platform or dev-planning are likely choices), add information that would make the bug easier to fix, or submit a patch!
tracking-firefox10: ? → -
tracking-firefox9: ? → -
Target Milestone: mozilla1.9.2 → ---
Version: 1.8 Branch → unspecified
Duplicate of this bug: 518516
There are some other examples and some implementation discussion in the comments for bug 518516.
No longer depends on: 221634
Duplicate of this bug: 707438
Blocks: 58724

Comment 41

6 years ago
As a workaround, one can insert an iframe with a pre-built username and password field and then re-attach those fields from the iframe (via .appendChild) to the form in question.

Comment 42

6 years ago
I also notice for e.g., http://en.wikipedia.org/wiki/Discuz!
based sites, Firefox fills in / asks me if I want to update my password,
but all I am doing is looking at different people's various "password protected photo albums"
on the site, not logging into the site, which I have already done.
This issue is affecting BrowserID's dialog as we dynamically show the authentication form depending on the user's state, which is only known on the front end after DOMContentLoaded.

https://github.com/mozilla/browserid/issues/314

Since dynamically loaded content is becoming more and more common, is this going to get any more attention?

I believe I have an interim solution for BrowserID, but I have to test this yet.
To add on to the above comment, I have changed my password field to already be in the DOM on page load, but still no luck.  Does the password manager depend on a form submission or a page refresh?  We do neither within the dialog but depend on XHR requests to submit the authentication info.

Comment 45

5 years ago
(In reply to Shane Tomlinson [:stomlinson] from comment #44)
> To add on to the above comment, I have changed my password field to already
> be in the DOM on page load, but still no luck.  Does the password manager
> depend on a form submission or a page refresh?  We do neither within the
> dialog but depend on XHR requests to submit the authentication info.

I don't know from the code side, but having done this on a site, I think what you need is for the field to be in a form, and then the password is remembered when the form is submitted (see also comment 20 and comment 21 ).

(I'd mention that requesting attention in bug comments is against bugzilla etiquette, but I guess your @mozilla.com address means you don't have to follow the rules...)

Comment 46

5 years ago
All I know is you guys need another level of indirection.
Currently Firefox can't tell the difference if a page is asking me for the site password, or for the password to a
protected photo album on the site. As I mentioned earlier. Also of course those situations where we are being asked
for a password on forms to which Firefox is totally oblivious.

Updated

5 years ago
Duplicate of this bug: 753685

Comment 48

5 years ago
As a UX-developer i want this feature, this make simple scripts more simpler.

BUT...

As a JS-developer, i strongly suggest you to think twice, before implementing this feature. Because it can lead to huge performance regression. 
As i can see on regular non generated pages, password input fields filled after dom completely loads.
Today Firefox fill them with two conditions, first - you must have login (text or hidden) input and password input, second - they must be inside a <form> element.
So I assume, that there will be DOM change event listener (which cant fire before dom exists), like in Skype addon (which drastically slows down all DOM manipulations, and deprecated by now), which fire on any DOM change. You loaded the page - check it for inputs, you generate some DOM elements - check it for inputs, load some XHR - check it for inputs. Its just a my guess.
But, if my guess is right, there will be two variants:
1) There must be some deep optimization. But anyway i cant imagine forcing PM work on script-generated forms without performance cost. And other browsers do not have this feature (only Opera do).
2) You can provide the method for web-developers, to fire the PM event, like window.document.godpleasefillmypasswords(true); And this option can be adopted by other browser vendors, and this take control to web-developers, when they generate the forms. 

Please tell me that I am wrong, and there will be no performance issues.
Sorry if my english is bad.
I would be happy with a menu item to pick which does a "please re-scan the current page for passwords to autofill".  Heck, I'd be happy with a bookmarklet to do it if there was something exposed for the bookmarklet to trigger.  Web developers who want to be user-friendly could trigger this once they draw their form.
Blocks: 646013

Updated

5 years ago
Duplicate of this bug: 763391
(Assignee)

Updated

5 years ago
Depends on: 771331
Blocks: 759452

Comment 51

5 years ago
Created attachment 694735 [details]
Manual testcase

Refresh testcase of comment #9

Comment 52

5 years ago
Testcase from comment #9 works with firefox 17.0.1 :
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20100101 Firefox/17.0

With new testcase attached [https://bugzilla.mozilla.org/attachment.cgi?id=694735], the password can be stored but is never recalled.

So, it is impossible to use the stored password with Microsoft mail (https://outlook.com) and maybe many other sites.
To note also that, as stated by comment #0, user has to use a bookmarklet which set autocomplete=off [http://www.squarefree.com/bookmarklets/forms.html#remember_password] to be able to store the password of Microsoft mail.
(Assignee)

Updated

4 years ago
Depends on: 839741
(Assignee)

Updated

4 years ago
No longer depends on: 839741
(Assignee)

Updated

4 years ago
Depends on: 839961

Updated

4 years ago
Whiteboard: [Advo]
(Assignee)

Comment 53

4 years ago
Created attachment 728702 [details] [diff] [review]
Patch v.2 (WIP)

This builds upon the work in bug 839961 and of course bug 771331.

Needs tests, and probably updates to existing tests, and probably another pass to think about if I've missed any interesting angles to this.
Assignee: nobody → dolske
Attachment #383577 - Attachment is obsolete: true
Attachment #694735 - Attachment is obsolete: true
(Assignee)

Comment 54

4 years ago
Created attachment 771714 [details] [diff] [review]
Patch v.3 (WIP)
Attachment #728702 - Attachment is obsolete: true

Comment 55

4 years ago
Is it possible to apply this WIP patch to release version of Firefox (one that I'm running), or do I have to somehow build the repo myself with this patch added in?
(Assignee)

Comment 56

4 years ago
Comment on attachment 771714 [details] [diff] [review]
Patch v.3 (WIP)

Actually I guess this is done enough to start a review. (Should just need tests.)
Attachment #771714 - Flags: review?(mnoorenberghe+bmo)
(Assignee)

Updated

4 years ago
Depends on: 901409
(Assignee)

Comment 57

4 years ago
Created attachment 785609 [details] [diff] [review]
Patch v.4

Now with test! (See also bug 901409)
Attachment #771714 - Attachment is obsolete: true
Attachment #771714 - Flags: review?(mnoorenberghe+bmo)
Attachment #785609 - Flags: review?(mnoorenberghe+bmo)
(Assignee)

Comment 58

4 years ago
The tests added by bug 699707 (test_maxforms_1.html, test_maxforms_2.html, test_maxforms_3.html) will need to be removed here too.

The root problem that bug was fixing mostly no longer exists, since DOMFormPassword will limit password manager's work to only forms that have potential to be filled in. I suppose it's possible a page could have a huge number of forms with password fields, but that seems uncommon and we can deal with it later if it comes up.
Attachment #694735 - Attachment description: New testcase to reproduce with Firefox 17.0 → Manual testcase
Attachment #694735 - Attachment mime type: text/plain → text/html
Comment on attachment 785609 [details] [diff] [review]
Patch v.4

Review of attachment 785609 [details] [diff] [review]:
-----------------------------------------------------------------

I'm excited for this to be fixed! We're very close.

(In reply to Justin Dolske [:Dolske] from comment #58)
> The tests added by bug 699707 (test_maxforms_1.html, test_maxforms_2.html,
> test_maxforms_3.html) will need to be removed here too.

If we're going to land with the useDOMFormHasPassword pref then the tests should instead be skipped when the pref is true. I don't think we should remove the tests as long as the code they're testing is in the tree (_fillDocument).

::: browser/base/content/content.js
@@ +38,5 @@
>    addEventListener("DOMContentLoaded", function(event) {
>      LoginManagerContent.onContentLoaded(event);
>    });
> +  addEventListener("DOMFormHasPassword", function(event) {
> +    LoginManagerContent.onFormPassword(event);

Note: This can now be combined with the existing DOMFormHasPassword listener for InsecurePasswordUtils.

::: modules/libpref/src/init/all.js
@@ +3898,5 @@
>  pref("signon.SignonFileName3",              "signons3.txt"); // obsolete
>  pref("signon.autofillForms",                true);
>  pref("signon.autologin.proxy",              false);
>  pref("signon.debug",                        false);
> +pref("signon.useDOMFormHasPassword",        true);

I think the pref is OK as an escape hatch if we discover regressions but it should probably be removed at some point after this bug fix makes it to the release channel.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +99,1 @@
>        if (!event.isTrusted)

Nit: I personally would prefer for the isTrusted check to stay first to avoid potential security issues later on from someone inserting code before it.

I would also just combine the gUseDOMFormHasPassword and gEnabled checks in one if condition.

@@ +116,5 @@
> +      // If we're not using the new DOMFormHasPassword event, only fill at pageload.
> +      if (!gUseDOMFormHasPassword)
> +          return;
> +
> +      if (!event.isTrusted)

Ditto

@@ +164,5 @@
> +        return;
> +      }
> +
> +      let autofillForm = !PrivateBrowsingUtils.isWindowPrivate(doc.defaultView) &&
> +                         Services.prefs.getBoolPref("signon.autofillForms");

Nit: Can we have a global with the pref value like we do for the others? That will avoid some overhead this patch will add when there are multiple forms on one page (eg. registration and login on one page).

::: toolkit/components/passwordmgr/test/test_basic_form_pwevent.html
@@ +3,5 @@
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=355063
> +-->
> +<head>
> +  <title>Test for Bug 355063</title>

Add <meta charset="utf-8"/> above <title> to avoid console spew.

@@ +11,5 @@
> +  <script type="application/javascript">
> +  /** Test for Bug 355063 **/
> +
> +  function startTest() {
> +    ok(true, "startTest");

Replace "ok(true, " with "info(" throughout this file

@@ +15,5 @@
> +    ok(true, "startTest");
> +    // Let's do a short timeout just to make sure we're well separated from
> +    // the load event. Shouldn't matter, just feels a bit more like how an
> +    // actual dynamic web page would work.
> +    setTimeout(addForm, 100);

I would rather we didn't have the setTimeout as we want to make sure this works if done immediately in DOMContentLoaded too. We could test after a delay as well I suppose but I'm more worried about problems with a page doing this earlier than we expect. Shall we test document.write too?

@@ +39,5 @@
> +  // Password Manager's own listener should always have been added first, so
> +  // the test's listener should be called after the pwmgr's listener fills in
> +  // a login.
> +  //
> +  //window.addEventListener("DOMFormHasPassword", checkForm);

Nuke this commented-out line?

@@ +42,5 @@
> +  //
> +  //window.addEventListener("DOMFormHasPassword", checkForm);
> +  var chromeWin = SpecialPowers.wrap(window)
> +                        .QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor)
> +                        .getInterface(SpecialPowers.Ci.nsIWebNavigation)

Nit: fix indentation if we don't even up removing this (see below)

@@ +45,5 @@
> +                        .QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor)
> +                        .getInterface(SpecialPowers.Ci.nsIWebNavigation)
> +                        .QueryInterface(SpecialPowers.Ci.nsIDocShell)
> +                        .chromeEventHandler.ownerDocument.defaultView;
> +  chromeWin.gBrowser.addEventListener("DOMFormHasPassword", checkForm);

This never gets cleaned up. It seems like your proposal in bug 901409 to use SpecialPowers.addChromeEventListener works but I don't know what a TabChildGlobal is. Make sure to removeChromeEventListener too.

@@ +46,5 @@
> +                        .getInterface(SpecialPowers.Ci.nsIWebNavigation)
> +                        .QueryInterface(SpecialPowers.Ci.nsIDocShell)
> +                        .chromeEventHandler.ownerDocument.defaultView;
> +  chromeWin.gBrowser.addEventListener("DOMFormHasPassword", checkForm);
> +  window.addEventListener("load", startTest);

As mentioned above, I think we should test upon DOMContentLoaded as sites may construct a login form that early.

Shall we check the signon.useDOMFormHasPassword pref before doing the test in order to make it easier to flip the pref off later?
Attachment #785609 - Flags: review?(mnoorenberghe+bmo) → review+
Attachment #694735 - Attachment is obsolete: false

Comment 60

4 years ago
Would someone confirm that this is the issue that prevents passwords from being remembered on www.tripadvisor.com?

Go to the URL and click on Sign In and you get a sign-in form.

Thanks,

-amrith
(Assignee)

Comment 61

4 years ago
(In reply to Matthew N. [:MattN] from comment #59)

> > The tests added by bug 699707 (test_maxforms_1.html, test_maxforms_2.html,
> > test_maxforms_3.html) will need to be removed here too.
> 
> If we're going to land with the useDOMFormHasPassword pref then the tests
> should instead be skipped when the pref is true. I don't think we should
> remove the tests as long as the code they're testing is in the tree
> (_fillDocument).

Hmm, yeah. Agreed.


> > +pref("signon.useDOMFormHasPassword",        true);
> 
> I think the pref is OK as an escape hatch if we discover regressions but it
> should probably be removed at some point after this bug fix makes it to the
> release channel.

Yep.


> ::: toolkit/components/passwordmgr/LoginManagerContent.jsm
> @@ +99,1 @@
> >        if (!event.isTrusted)
> 
> Nit: I personally would prefer for the isTrusted check to stay first to
> avoid potential security issues later on from someone inserting code before
> it.

I actually did it this way to make it clear that the pref controls absolutely everything under these functions.


> > +      let autofillForm = !PrivateBrowsingUtils.isWindowPrivate(doc.defaultView) &&
> > +                         Services.prefs.getBoolPref("signon.autofillForms");
> 
> Nit: Can we have a global with the pref value like we do for the others?
> That will avoid some overhead this patch will add when there are multiple
> forms on one page (eg. registration and login on one page).

Last I looked prefs were really fast, but I like the idea anyway. Fixed.


> > +    // Let's do a short timeout just to make sure we're well separated from
> > +    // the load event. Shouldn't matter, just feels a bit more like how an
> > +    // actual dynamic web page would work.
> > +    setTimeout(addForm, 100);
> 
> I would rather we didn't have the setTimeout as we want to make sure this
> works if done immediately in DOMContentLoaded too. We could test after a
> delay as well I suppose but I'm more worried about problems with a page
> doing this earlier than we expect. Shall we test document.write too?

Removed. I don't think the other combinations are useful to test.


> > +  chromeWin.gBrowser.addEventListener("DOMFormHasPassword", checkForm);
> 
> This never gets cleaned up.

Oops. Fixed.


> Shall we check the signon.useDOMFormHasPassword pref before doing the test
> in order to make it easier to flip the pref off later?

Yes.
(Assignee)

Comment 62

4 years ago
Created attachment 793284 [details] [diff] [review]
Patch v.5

For some reason I'm now getting a test failure in test_basic_form_observer_autofillForms.html.

I moved up the TestObserver, because it seems like that obviously should listening before the page's password fields show up. Not sure how it was working with this patch before.

Alas it's still broken. From debugging it seems that LoginManagerContent's pref change observer never fires after the test changes the pref. But, bizarrely, nsLoginManager's observer _does_ fire. So I'm not sure what's going on. Will debug more later.
Attachment #785609 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 907594
(Assignee)

Comment 63

4 years ago
Created attachment 793361 [details] [diff] [review]
Patch v.6

Ok, with the fix from bug 907594 tests pass locally now, for both values of signon.useDOMFormHasPassword.
Attachment #793284 - Attachment is obsolete: true
(Assignee)

Comment 64

4 years ago
\o/

https://hg.mozilla.org/integration/fx-team/rev/42be7b929812
(Assignee)

Comment 65

4 years ago
Created attachment 793797 [details] [diff] [review]
Patch for Android/Metro

Uhm, oops, as with bug 839961 there will need to be a small change to Android and Metro too.

Without this password manager will be broken on current m-c for these platforms: it'll get the new signon.useDOMFormHasPassword pref, and hence ignore the existing LoginManagerContent.onContentLoaded() calls, expecting LoginManagerContent.onFormPassword() to be happening instead. (So this can be verified in that a current m-c build without this patch will never fill passwords on a page for Metro/Android, but with patch it will).

Alternatively we could temporarily set signon.useDOMFormHasPassword == false on Android/Metro, but I think this patch is simple enough to not worry about that.

Let's see if I can find some fast reviewers!
(Assignee)

Comment 66

4 years ago
Created attachment 793801 [details] [diff] [review]
Patch for Android/Metro

Fix dumb typo.
Attachment #793797 - Attachment is obsolete: true
(Assignee)

Comment 67

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/91d27f0ac7dd

Confirmed with the indefatigable jwilde that this fixed Metro, and I'm going take it on faith that Android is good too.
Works for me on Metro + Windows 8.0 tablet. Tested with attached testcase and google.com. Gets filled on load/form generation and on autocomplete.

Comment 69

4 years ago
Works for me on Android as well.
Sorry, backed out for ongoing Android bustage:

https://hg.mozilla.org/integration/fx-team/rev/4e82ce74f7f7
https://hg.mozilla.org/integration/fx-team/rev/d727717cbaf1

Updated

4 years ago
Blocks: 908141

Comment 71

4 years ago
Comment on attachment 793361 [details] [diff] [review]
Patch v.6

Review of attachment 793361 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/test/test_basic_form_pwevent.html
@@ +46,5 @@
> +                                 .getInterface(SpecialPowers.Ci.nsIWebNavigation)
> +                                 .QueryInterface(SpecialPowers.Ci.nsIDocShell)
> +                                 .chromeEventHandler.ownerDocument.defaultView;
> +    chromeWin.gBrowser.addEventListener("DOMFormHasPassword", checkForm);
> +    window.addEventListener("load", startTest);

On Android, we don't have gBrowser, and it looks like that caused the test to fail. We use a <deck> to hold the <browser>s that are used for tabs, which you can get at with BrowserApp.deck, but it seems crappy to have platform-specific code in a toolkit test. Can you just add the listener to chromeWin itself?

I imagine you can also look at other mochitests to see what they do to do similar things and still pass on Android.
(Assignee)

Comment 72

4 years ago
Created attachment 794353 [details] [diff] [review]
Patch v.7

Rolled in Android/Metro patch, and fixed the new test to work on Android:

https://tbpl.mozilla.org/?tree=Try&rev=db6357046b49

I even had "//XXX SpecialPowers.addChromeEventListener instead?" in an old version of this patch, but skipped making the change because it was working fine on desktop. Doh.
Attachment #793361 - Attachment is obsolete: true
Attachment #793801 - Attachment is obsolete: true
Attachment #794353 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

4 years ago
Attachment #794353 - Flags: review?(jwilde)
Comment on attachment 794353 [details] [diff] [review]
Patch v.7

Review of attachment 794353 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, tested and WFM on Metro.
Attachment #794353 - Flags: review?(jwilde) → review+

Comment 74

4 years ago
Comment on attachment 794353 [details] [diff] [review]
Patch v.7

Review of attachment 794353 [details] [diff] [review]:
-----------------------------------------------------------------

r+ to the Android changes. Nice to see the test was green on try.
Attachment #794353 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 75

4 years ago
Let's try this again! \o/

https://hg.mozilla.org/integration/fx-team/rev/e7d886615ad8
https://hg.mozilla.org/mozilla-central/rev/e7d886615ad8
Status: NEW → RESOLVED
Last Resolved: 11 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Comment 77

4 years ago
Release note should be something like "Firefox's password manager now supports Persona and other script-generated password fields."  Feel free to suggestion something better if this isn't ideal wording.
relnote-firefox: --- → +

Updated

4 years ago
relnote-firefox: + → ?
(Assignee)

Comment 78

4 years ago
Given bugs like bug 906300 I wouldn't mention Persona.
relnote-firefox: ? → 26+

Comment 79

4 years ago
Apparently this made Secure Login bookmarks (created and used with Secure Login https://addons.mozilla.org/firefox/addon/secure-login/ ) show two Master password dialogs: https://www.mozdev.org/bugs/show_bug.cgi?id=25628
(Assignee)

Updated

4 years ago
Blocks: 946549

Comment 80

4 years ago
Windows 7 (x64)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 SeaMonkey/2.23

This still does not work at <http://slashdot.org/>.  If I try to login with JavaScript enabled, the Password Manager does not provide either my user ID or password in the script popup.  If I first disable JavaScript, I get a separate login page where Password Manager works.  That login page is at <https://slashdot.org/login.pl?op=userlogin> with the same domain as the home page containing the script login.  Thus, Password Manager should recognize the domain and supply the login credentials to the script popup.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to David E. Ross from comment #80)
> Windows 7 (x64)
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 SeaMonkey/2.23
> 
> This still does not work at <http://slashdot.org/>.  If I try to login with
> JavaScript enabled, the Password Manager does not provide either my user ID
> or password in the script popup.  If I first disable JavaScript, I get a
> separate login page where Password Manager works.  That login page is at
> <https://slashdot.org/login.pl?op=userlogin> with the same domain as the
> home page containing the script login.  Thus, Password Manager should
> recognize the domain and supply the login credentials to the script popup.

Please don't re-open bugs once patches have been checked-in. If you think there is a follow-up issue, file a bug blocking this one. Please include output from the browser console with signon.debug set to true for password manager bugs.

In this case, password manager is working as expected since the first URL is http and the latter is https. You must only have the password saved for https and for security reasons we won't auto-fill that password on the http site. If you save the password on the http site, it will be auto-filled on the floating login box. Slashdot really shouldn't be serving the login box on an HTTP page in the first place as it's vulnerable to MITM attacks.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED

Comment 82

4 years ago
Actually, I already had user IDs and passwords for both <http://slashdot.org/> and <https://slashdot.org/>.  However, I did submit a new bug report:  bug #951769.

Updated

3 years ago
Blocks: 999354
Duplicate of this bug: 646013
Duplicate of this bug: 58724
You need to log in before you can comment on or make changes to this bug.