Closed
Bug 355063
Opened 18 years ago
Closed 11 years ago
Password manager does not work on script-generated forms
Categories
(Toolkit :: Password Manager, defect)
Toolkit
Password Manager
Tracking
()
VERIFIED
FIXED
mozilla26
People
(Reporter: al_9x, Assigned: Dolske)
References
(Blocks 1 open bug, )
Details
(Keywords: regression, Whiteboard: [Advo])
Attachments
(2 files, 8 obsolete files)
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•18 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
Closed: 18 years ago
Resolution: --- → DUPLICATE
Comment 2•18 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•18 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
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•18 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. :)
> 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•18 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•18 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•18 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?
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•18 years ago
|
||
is this firefox-only? it doesn't work on camino either making me think that this is a core bug.
Comment 11•18 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•18 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•18 years ago
|
||
Can an option be put it to restore old behavior? Can someone from the development team comment on this?
Comment 14•17 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•17 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•17 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
Updated•16 years ago
|
Product: Firefox → Toolkit
Comment 17•16 years ago
|
||
I think I have the same problem with powerlink.emc.com
Comment 18•16 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•16 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•16 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•16 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•15 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•15 years ago
|
||
Quick and dirty PoC.
Assignee | ||
Comment 24•15 years ago
|
||
Not actively working at this at the moment.
Assignee: dolske → nobody
Updated•15 years ago
|
Blocks: cuts-control
Comment 26•14 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•14 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.
Comment 28•14 years ago
|
||
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 31•14 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 :)
Comment 33•14 years ago
|
||
This affects logins to Shlashdot at <http://slashdot.org/> and The Huffington Post at <http://www.huffingtonpost.com/>.
Comment 34•14 years ago
|
||
The management control panel on our domain registrar had its login page redesigned recently and now gets nailed by this, too.
Comment 35•14 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.
Comment 36•13 years ago
|
||
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•13 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!
Updated•13 years ago
|
Target Milestone: mozilla1.9.2 → ---
Version: 1.8 Branch → unspecified
Comment 39•13 years ago
|
||
There are some other examples and some implementation discussion in the comments for bug 518516.
No longer depends on: 221634
Comment 41•13 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•13 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.
Comment 43•13 years ago
|
||
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•13 years ago
|
||
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•13 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•13 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.
Comment 48•13 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.
Comment 49•13 years ago
|
||
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 51•12 years ago
|
||
Refresh testcase of comment #9
Comment 52•12 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.
Updated•12 years ago
|
Whiteboard: [Advo]
Assignee | ||
Comment 53•12 years ago
|
||
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•11 years ago
|
||
Attachment #728702 -
Attachment is obsolete: true
Comment 55•11 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•11 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 | ||
Comment 57•11 years ago
|
||
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•11 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.
Updated•11 years ago
|
Attachment #694735 -
Attachment description: New testcase to reproduce with Firefox 17.0 → Manual testcase
Attachment #694735 -
Attachment mime type: text/plain → text/html
Updated•11 years ago
|
Comment 59•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #694735 -
Attachment is obsolete: false
Comment 60•11 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•11 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•11 years ago
|
||
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 | ||
Comment 63•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 65•11 years ago
|
||
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 67•11 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.
Comment 68•11 years ago
|
||
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•11 years ago
|
||
Works for me on Android as well.
Comment 70•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 years ago
|
Attachment #794353 -
Flags: review?(jwilde)
Comment 73•11 years ago
|
||
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•11 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•11 years ago
|
||
Let's try this again! \o/
https://hg.mozilla.org/integration/fx-team/rev/e7d886615ad8
Comment 76•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 18 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 77•11 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•11 years ago
|
Assignee | ||
Comment 78•11 years ago
|
||
Given bugs like bug 906300 I wouldn't mention Persona.
Updated•11 years ago
|
Comment 79•11 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
Comment 80•11 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 → ---
Comment 81•11 years ago
|
||
(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
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Comment 82•11 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•