Last Comment Bug 355063 - Password manager does not work on script-generated forms
: Password manager does not work on script-generated forms
Status: VERIFIED FIXED
[Advo]
: regression
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: unspecified
: All All
: -- normal with 37 votes (vote)
: mozilla26
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
http://www.huffingtonpost.com/
: 58724 518516 528749 547035 619715 631417 646013 707438 753685 763391 (view as bug list)
Depends on: 771331 839961 901409 907594
Blocks: cuts-control 759452 58724 646013 908141 946549 999354
  Show dependency treegraph
 
Reported: 2006-10-01 13:06 PDT by al_9x
Modified: 2014-11-04 02:41 PST (History)
75 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
26+


Attachments
Patch v.1 (WIP) (3.14 KB, patch)
2009-06-16 16:51 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Manual testcase (1.40 KB, text/html)
2012-12-21 00:19 PST, Blutin
no flags Details
Patch v.2 (WIP) (4.06 KB, patch)
2013-03-24 00:52 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.3 (WIP) (6.60 KB, patch)
2013-07-05 17:00 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.4 (9.92 KB, patch)
2013-08-04 21:01 PDT, Justin Dolske [:Dolske]
MattN+bmo: review+
Details | Diff | Review
Patch v.5 (17.23 KB, patch)
2013-08-20 19:38 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.6 (17.26 KB, patch)
2013-08-20 23:51 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch for Android/Metro (4.34 KB, patch)
2013-08-21 19:28 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch for Android/Metro (4.33 KB, patch)
2013-08-21 19:35 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.7 (21.22 KB, patch)
2013-08-22 17:49 PDT, Justin Dolske [:Dolske]
margaret.leibovic: review+
jwilde: review+
Details | Diff | Review

Description al_9x 2006-10-01 13:06:31 PDT
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 Michael Wu [:mwu] 2006-10-01 14:54:47 PDT
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 ***
Comment 2 Michael Wu [:mwu] 2006-10-01 15:01:41 PDT
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.
Comment 3 Michael Wu [:mwu] 2006-10-01 15:37:22 PDT
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.
Comment 4 al_9x 2006-10-01 16:38:46 PDT
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 Michael Wu [:mwu] 2006-10-01 17:28:05 PDT
(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. :)
Comment 6 al_9x 2006-10-02 11:27:30 PDT
> 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 Michael Wu [:mwu] 2006-10-02 11:41:01 PDT
(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.
Comment 8 Worcester12345 2006-10-18 14:09:12 PDT
(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?
Comment 9 al_9x 2006-10-23 09:08:26 PDT
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>
_________________________________________________________________________

Comment 10 Paul Borokhov (lensovet) 2006-12-08 22:15:15 PST
is this firefox-only? it doesn't work on camino either making me think that this is a core bug.
Comment 11 Michael Wu [:mwu] 2006-12-08 22:57:18 PST
(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.
Comment 12 al_9x 2006-12-19 06:19:55 PST
> 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?
Comment 13 al_9x 2006-12-21 05:53:28 PST
Can an option be put it to restore old behavior?  Can someone from the development team comment on this?
Comment 14 db 2007-11-02 01:40:26 PDT
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.
Comment 15 Justin Dolske [:Dolske] 2007-11-02 02:09:28 PDT
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 Brook Harty 2008-01-31 12:30:38 PST
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
Comment 17 Olaf 2008-08-24 17:04:18 PDT
I think I have the same problem with powerlink.emc.com
Comment 18 David G King 2008-08-24 19:38:09 PDT
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
Comment 19 Justin Dolske [:Dolske] 2008-09-02 00:01:34 PDT
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.
Comment 20 Yet Ding 2008-10-03 08:18:23 PDT
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.
Comment 21 Justin Dolske [:Dolske] 2008-10-03 11:29:25 PDT
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).
Comment 22 Justin Dolske [:Dolske] 2009-06-16 12:41:32 PDT
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...
Comment 23 Justin Dolske [:Dolske] 2009-06-16 16:51:56 PDT
Created attachment 383577 [details] [diff] [review]
Patch v.1 (WIP)

Quick and dirty PoC.
Comment 24 Justin Dolske [:Dolske] 2009-09-29 16:36:01 PDT
Not actively working at this at the moment.
Comment 25 Mardeg 2010-05-30 17:00:21 PDT
*** Bug 547035 has been marked as a duplicate of this bug. ***
Comment 26 Jay Goldman 2010-07-21 21:34:43 PDT
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 Jay Goldman 2010-07-21 21:36:19 PDT
(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.
Comment 28 François-Xavier Bois 2010-07-30 07:53:45 PDT
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 ?
Comment 29 Cork 2010-12-16 10:41:42 PST
*** Bug 528749 has been marked as a duplicate of this bug. ***
Comment 30 Cork 2010-12-16 10:42:59 PST
*** Bug 619715 has been marked as a duplicate of this bug. ***
Comment 31 Tom 2011-01-30 22:31:06 PST
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 :)
Comment 32 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-02-03 17:42:34 PST
*** Bug 631417 has been marked as a duplicate of this bug. ***
Comment 33 David E. Ross 2011-02-03 19:09:34 PST
This affects logins to Shlashdot at <http://slashdot.org/> and The Huffington Post at <http://www.huffingtonpost.com/>.
Comment 34 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-02-04 20:26:21 PST
The management control panel on our domain registrar had its login page redesigned recently and now gets nailed by this, too.
Comment 35 LA 2011-03-08 04:04:38 PST
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.
Comment 36 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-11-08 14:02:59 PST
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.
Comment 37 christian 2011-11-17 14:58:21 PST
[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!
Comment 38 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-03 11:29:43 PST
*** Bug 518516 has been marked as a duplicate of this bug. ***
Comment 39 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-03 11:30:22 PST
There are some other examples and some implementation discussion in the comments for bug 518516.
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-03 11:30:40 PST
*** Bug 707438 has been marked as a duplicate of this bug. ***
Comment 41 agamemnus 2011-12-26 12:15:57 PST
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 Dan Jacobson 2011-12-26 13:06:36 PST
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.
Comment 43 Shane Tomlinson [:stomlinson] 2012-02-09 05:56:29 PST
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.
Comment 44 Shane Tomlinson [:stomlinson] 2012-02-09 06:34:28 PST
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 Michael Lefevre 2012-02-09 07:00:54 PST
(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 Dan Jacobson 2012-02-09 07:23:26 PST
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.
Comment 47 Cork 2012-05-10 02:40:03 PDT
*** Bug 753685 has been marked as a duplicate of this bug. ***
Comment 48 Alex Shilov 2012-05-14 15:36:31 PDT
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.
Comment 49 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-05-14 16:54:54 PDT
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.
Comment 50 Mauro Molinari 2012-06-11 22:54:19 PDT
*** Bug 763391 has been marked as a duplicate of this bug. ***
Comment 51 Blutin 2012-12-21 00:19:38 PST
Created attachment 694735 [details]
Manual testcase

Refresh testcase of comment #9
Comment 52 Blutin 2012-12-21 00:41:52 PST
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.
Comment 53 Justin Dolske [:Dolske] 2013-03-24 00:52:29 PDT
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.
Comment 54 Justin Dolske [:Dolske] 2013-07-05 17:00:09 PDT
Created attachment 771714 [details] [diff] [review]
Patch v.3 (WIP)
Comment 55 Malkierian 2013-07-27 11:01:56 PDT
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?
Comment 56 Justin Dolske [:Dolske] 2013-07-31 18:17:36 PDT
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.)
Comment 57 Justin Dolske [:Dolske] 2013-08-04 21:01:07 PDT
Created attachment 785609 [details] [diff] [review]
Patch v.4

Now with test! (See also bug 901409)
Comment 58 Justin Dolske [:Dolske] 2013-08-06 20:31:57 PDT
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.
Comment 59 Matthew N. [:MattN] (PTO Jun. 29-30) 2013-08-11 22:37:51 PDT
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?
Comment 60 Amrith Kumar 2013-08-12 04:20:57 PDT
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
Comment 61 Justin Dolske [:Dolske] 2013-08-20 18:43:09 PDT
(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.
Comment 62 Justin Dolske [:Dolske] 2013-08-20 19:38:28 PDT
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.
Comment 63 Justin Dolske [:Dolske] 2013-08-20 23:51:58 PDT
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.
Comment 64 Justin Dolske [:Dolske] 2013-08-21 17:42:44 PDT
\o/

https://hg.mozilla.org/integration/fx-team/rev/42be7b929812
Comment 65 Justin Dolske [:Dolske] 2013-08-21 19:28:26 PDT
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!
Comment 66 Justin Dolske [:Dolske] 2013-08-21 19:35:40 PDT
Created attachment 793801 [details] [diff] [review]
Patch for Android/Metro

Fix dumb typo.
Comment 67 Justin Dolske [:Dolske] 2013-08-21 20:11:32 PDT
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.
Comment 68 Jonathan Wilde [:jwilde] 2013-08-21 20:12:24 PDT
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 :Margaret Leibovic 2013-08-21 20:25:55 PDT
Works for me on Android as well.
Comment 70 Tim Taubert [:ttaubert] 2013-08-22 00:24:28 PDT
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
Comment 71 :Margaret Leibovic 2013-08-22 09:34:06 PDT
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.
Comment 72 Justin Dolske [:Dolske] 2013-08-22 17:49:55 PDT
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.
Comment 73 Jonathan Wilde [:jwilde] 2013-08-23 18:18:16 PDT
Comment on attachment 794353 [details] [diff] [review]
Patch v.7

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

Looks good, tested and WFM on Metro.
Comment 74 :Margaret Leibovic 2013-08-23 20:09:40 PDT
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.
Comment 75 Justin Dolske [:Dolske] 2013-08-23 20:30:33 PDT
Let's try this again! \o/

https://hg.mozilla.org/integration/fx-team/rev/e7d886615ad8
Comment 76 Tim Taubert [:ttaubert] 2013-08-25 06:13:11 PDT
https://hg.mozilla.org/mozilla-central/rev/e7d886615ad8
Comment 77 Asa Dotzler [:asa] 2013-09-13 12:31:57 PDT
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.
Comment 78 Justin Dolske [:Dolske] 2013-09-16 08:41:16 PDT
Given bugs like bug 906300 I wouldn't mention Persona.
Comment 79 [:Aleksej] 2013-11-07 08:54:40 PST
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
Comment 80 David E. Ross 2013-12-14 11:36:25 PST
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.
Comment 81 Matthew N. [:MattN] (PTO Jun. 29-30) 2013-12-14 11:50:45 PST
(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.
Comment 82 David E. Ross 2013-12-18 10:35:01 PST
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.
Comment 83 Matthew N. [:MattN] (PTO Jun. 29-30) 2014-11-04 01:47:48 PST
*** Bug 646013 has been marked as a duplicate of this bug. ***
Comment 84 Matthew N. [:MattN] (PTO Jun. 29-30) 2014-11-04 02:15:03 PST
*** Bug 58724 has been marked as a duplicate of this bug. ***

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