Closed Bug 771331 Opened 12 years ago Closed 11 years ago

Password manager would really like to know when <input type=password> is added to the DOM

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22
Tracking Flags:
Tracking Status
relnote-firefox --- -

People

(Reporter: Dolske, Unassigned)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 6 obsolete files)

make form elements fire events when <input type=password> elements are added/removed
8.36 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
The Firefox password manager is currently pretty basic -- it watches for documents being loaded (via @mozilla.org/docloaderservice;1), waits for DOMContentLoaded to fire, and then attempts to fill in passwords on the page.

This has a couple of significant problems:

* Bad for performance. The vast majority of pages being loaded don't need the password manager to be doing anything. But it has no way to know that without looking through the document.

* Does not work for pages that add login fields after the page has loaded (ie, JS generated forms). This is increasingly common.

I'd like to change this, by having password manager simply listen for some kind of event/notification that indicates there's an <input type=password> that has been added to the document. I made a crude attempt long ago in bug 355063, but didn't really know the DOM stuff well enough. I should probably have filed this bug then. :)


A few random notes:

* It would be nice to get these notifications prior to DOMContentLoaded, since even that even can fire late enough that you can see and empty form, only to have your login blink in seconds later.

* Password mananger really just wants to know it should look at a specific <form>, it doesn't need to know about each <input> individually. EG, for a form with 3 password fields, we only need/want 1 notification.

* Would we want batching for multiple login forms in a document? Or just fire 1 notification for each? How to limit extreme cases like bug 699707?

* I don't know how complex this is to do wrt the various cases where the DOM is being created/modified... initial page load, .innerHTML modification, using createElement to build up a form with elements (ie, attaching a whole subtree), document.write, etc etc.

If it's not terribly difficult, it might be enough to have one of our DOM Form Wizards (hi mounir!) just help guide what/where an implementation of this needs to deal with, and someone else can glue it all together.
Please no. We don't want to add mutation observers for each page.
input type=password could dispatch a chrome only event?
Chrome-only would be just fine. I don't know of any reason why content would be interested in it.
Attached patch Patch (obsolete) — — Splinter Review 
This patch adds an event called DOMInputAdded and DOMInputRemoved that is fired when each <html:input> is attached and detached from the tree.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #651070 - Flags: review?(khuey)
Comment on attachment 651070 [details] [diff] [review]
Patch

I thought you wanted an event when input type=password is added to DOM.
This patch doesn't do that. You would need to do something when element is bound to document.

Also, please don't add a new interface. You could just 
check that element is html:input and cast. do_QueryInterface is a bit slow.

I know I suggested event, but especially if we're going to fire it once for each input element (not only password), that
could become rather expensive. Some pages have thousands of input elements.

To get the behavior the patch has, JS could just listen for pageshow/pagehide events and do the
getElementsByTagNameNS(htmlns, "input"); itself.



We could count type="password" elements which are in the document and fire
event whenever the counter becomes > 0 and fire some other event when the counter becomes 0.
That way there would be just very few events.
Attachment #651070 - Flags: review?(khuey) → review-
It does dispatch events from BindToTree/UnbindFromTree.  You didn't read the patch carefully enough.
Oh, oops, sorry. But still, dispatching event for all the input elements is quite overkill.
Also, the patch wouldn't handle the (possibly rare) case when the type of some
input element changes to password.
(In reply to Olli Pettay [:smaug] from comment #9)
> Also, the patch wouldn't handle the (possibly rare) case when the type of
> some
> input element changes to password.

You can use nsHTMLInputElement::HandleTypeChange for that.
nsHTMLInputElement::BindToTree and nsHTMLInputElement::UnbindFromTree to know when an element is added/removed from the tree.
Also, your event seems to be visible to the content, isn't it? We want something that is not.
Maybe you could use nsContentUtils::DispatchChromeEvent for that?
Hmm, I wonder about the pageshow/hide. Part of the desire for this is to avoid doing extra work when it's not needed. I don't know how expensive that code is, but a least ideally it wouldn't have to iterate over <input>s upon pageshow/hide... EG, if these inputs could observe pagehide/show themselves and fire their own events, there would be no cost when they are not present. Or, since password fields are generally rare, simply caching a "there be password fields here, matey" hint on the document would make the common-case fast (and the uncommon-case would loop as in this patch).

Or maybe the pageshow/hide stuff is followup material. I think we just want them to reattach autocomplete gunk to the "username" field, so perhaps a specialized event for that would be even better? I'd have to think about it.

Lastly... If one only considers 1 password field per form, the current patch seems fine. But for multiple fields, things are a bit weird.

Consider...

  <form>
    <input id=name>
    <input type=password id=newpw>
    <input type=password id=newpw_verify>
    <input type=password id=oldpw>
  </form>

In this case, password manager doesn't really want 3 events. It only needs to look at the form once. (It can even do the wrong thing if it gets the form when only the first field has been added; but since these events are async that shouldn't happen).

One idea would be to have a (single per-document) runnable that collects the pending DOMInputAdded events, and coalesces events within the same <form>. Hmm, but what about interruptable parsing? That would work for such forms added by JS, though, thanks to run-to-completion.
Another possible issue with this patch is that if a password field is inserted and removed, two events will be fired, when ideally you want none.
It's worth looking if the pageshow/hide part is even needed for this case. Those events are for dealing with the bfcache as far as I know. For the DOMLinkAdded case it is necessary because it changes UI outside of the content. However for pw manager, bringing the page back from the bfcache, the fields are probably still filled and the autocomplete "gunk" might still be attached.

Don't know about the other points from dolske's comments.
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Whiteboard: [mentor=jaws][lang=cpp]
Here's another attempt.  It's Jared's patch, but with the nsIInput stuff
removed and implementing khuey's suggestion that adding/removing an input
shouldn't cause events to be fired.  Dolske's musings in comment 11 remain
unaddressed.  It's not clear to me that those bits are necessarily
appropriate for this bug.
Attachment #708682 - Flags: feedback?(dolske)
Attachment #708682 - Flags: feedback?(bugs)
AFAICS, Mounir's desire for this to be chrome-only (comment 10) is already handled in nsAsyncDOMEvent.  I didn't hook this up to nsHTMLInputElement::HandleTypeChange, but that should only be a couple lines of checking.
(In reply to Nathan Froyd (:froydnj) from comment #15)
> AFAICS, Mounir's desire for this to be chrome-only (comment 10) is already
> handled in nsAsyncDOMEvent.

What do you mean?

Also, could you not inline the class in nsHTMLInputElement.h. No need for that and that would prevent including yet another header in nsHTMLInputElement.h. You could have it as a anonymous namespace in nsHTMLInputElement.cpp.

Also, could you write a test making sure that the event isn't seen by the content?
(In reply to Mounir Lamouri (:mounir) from comment #16)
> (In reply to Nathan Froyd (:froydnj) from comment #15)
> > AFAICS, Mounir's desire for this to be chrome-only (comment 10) is already
> > handled in nsAsyncDOMEvent.
> 
> What do you mean?

Pardon my ignorance, but isn't that what the {a,m}DispatchChromeOnly bits in nsAsyncDOMEvent do?  Judging from nsAsyncDOMEvent::Run, we already use nsContentUtils::DispatchChromeEvent, as you suggested in comment 10.

> Also, could you not inline the class in nsHTMLInputElement.h. No need for
> that and that would prevent including yet another header in
> nsHTMLInputElement.h. You could have it as a anonymous namespace in
> nsHTMLInputElement.cpp.

I can do that, but:

  nsRevocableEventPtr<RevocableInputPasswordNotificationEvent> mInputPasswordAddedEvent;

won't work with just a forward declaration.

> Also, could you write a test making sure that the event isn't seen by the
> content?

That's a good idea, I'll add that.
(In reply to Nathan Froyd (:froydnj) from comment #17)
> (In reply to Mounir Lamouri (:mounir) from comment #16)
> > (In reply to Nathan Froyd (:froydnj) from comment #15)
> > > AFAICS, Mounir's desire for this to be chrome-only (comment 10) is already
> > > handled in nsAsyncDOMEvent.
> > 
> > What do you mean?
> 
> Pardon my ignorance, but isn't that what the {a,m}DispatchChromeOnly bits in
> nsAsyncDOMEvent do?  Judging from nsAsyncDOMEvent::Run, we already use
> nsContentUtils::DispatchChromeEvent, as you suggested in comment 10.

Hadn't notice that. I guess the RevocableInputPasswordNotificationEvent ctor shouldn't even take a aChromeOnly bool.

> > Also, could you not inline the class in nsHTMLInputElement.h. No need for
> > that and that would prevent including yet another header in
> > nsHTMLInputElement.h. You could have it as a anonymous namespace in
> > nsHTMLInputElement.cpp.
> 
> I can do that, but:
> 
>   nsRevocableEventPtr<RevocableInputPasswordNotificationEvent>
> mInputPasswordAddedEvent;

I'm not sure why you need that but I guess smaug knows.
Updated patch, with better C++ interface, handling input type switching, and
test checks that we don't fire these events where content can see them.
Attachment #708682 - Attachment is obsolete: true
Attachment #708682 - Flags: feedback?(dolske)
Attachment #708682 - Flags: feedback?(bugs)
Attachment #710295 - Flags: feedback?(dolske)
Attachment #710295 - Flags: feedback?(bugs)
Attachment #651070 - Attachment is obsolete: true
Comment on attachment 710295 [details] [diff] [review]
make <input type=password> elements fire events when they are added/removed


>+nsHTMLInputElement::PostPasswordEvent(bool aAddedEvent)
>+{
>+  nsIDocument* doc = OwnerDoc();
>+  if (!doc) {
>+    return;
>+  }
OwnerDoc() returns always valid value.

>+    NS_IMETHOD Run() {
{ goes to the next line
Attachment #710295 - Flags: feedback?(bugs) → feedback+
Probably should have just asked for r? in the first place...
Attachment #710295 - Attachment is obsolete: true
Attachment #710295 - Flags: feedback?(dolske)
Attachment #713535 - Flags: review?(bugs)
I'm doing some refactoring over in bug 839961, including a small (and ultra-hacky) patch to use this event in password manager. Let me think through some of the potential issues for actually switching over to using the DOMInputPasswordAdded event...

tl;dr: Need to address #1 and maybe #3. Everything else should be ok for login manager.

1) Multiple events for the same form are undesirable. This happens, for example, in change-password forms -- one field for the old password, two fields for the new password. As noted in comment 0, what password manager really wants is a single event signaling that a <form> containing a password field was added to the page.

Hmm, this should be simple to fix, because when _loading_ a page (ie to fill in logins) we don't care about anything beyond the 1st password input. So login manager should be ok with just a minor tweak here, to only dispatch an event the first time a form gets a password input. EG:

  // pseudo-C++ as JS! Woah! :)
  if (!input.form.mAlreadyFiredPWEvent) {
    // fire DOMInputPasswordAdded
    input.form.mAlreadyFiredPWEvent = true;
  }

I'd suggest renaming it to DOMLoginFormAdded, just to avoid any potential confusion if someone else tries using the event and wonders why it doesn't fire repeatedly. (Bonus points for making it fire upon </form>, but login manager doesn't care right now.)

2) Document-oriented processing lets login manager do some optimizations that are lost when switching to an event-based model... There's an early bailout for we have no stored logins for the site, and we cache found logins to avoid hitting SQLite repeatedly for the same info when there are multiple forms in the page. 

Replacing that would be hard, but maybe we don't need to. We already don't do any caching between document loads, so either way we're already relying on whatever DB caching SQLite is doing. A long-term fix for this would be to dump SQLite, keep everything in memory, and flush to disk (JSON) only when needed. I think Taras has proposed this before; it's doable but is a large change.

3) Related, we currently only process the first and last 40 forms on a page (bug 699707). The original issue in that bug goes away (Reddit's per-comment forms were not login forms), but the pathological case still exists in theory. This should be easily fixable here too -- just add a counter to the document, and stop firing DOMInputPasswordAdded if we've already fired it too much. (And maybe reset it if the last event was > X seconds ago, for long-lived gmail-esque pages?)

4) The Master Password is often a PITA here. (What if a login page loads while a MP prompt is already open, what if you cancel entry and want it to not ask again, etc.) I _think_ this is easily adaptable to event-based processing, but there's might be tricky details.

5) I assume that forms should always be in a sane state when DOMInputPasswordAdded fires, since the DOM code should already be handling markup like:
  <form>
    <input type=password>
    <script>stuff</script>
  ...

6) One general risk of processing immediately after an input is added is that some page may have scripts that clear inputs, which currently run before DOMContentLoaded. And thus they'll clear out whatever we fill in. I don't think this is a likely risk, though, and I can think of ways to work around it.
Whiteboard: [mentor=jaws][lang=cpp]
I don't quite understand (1).
What if there is first a password field, then it is removed from DOM and then a new one is added
back to the form. You don't want to get an event about that?

DOMLoginFormAdded is very vague, well, I don't actually understand what it means.

Does (2) we really access sqlite all the time? I hope no main thread I/O.
Comment on attachment 713535 [details] [diff] [review]
make <input type=password> elements fire events when they are added/removed

But sounds like this need still some thinking.
Attachment #713535 - Flags: review?(bugs)
Flagging dolske to answer comment 23, at least.
Flags: needinfo?(dolske)
Argh. I had a comment that got eaten somewhere.

(In reply to Olli Pettay [:smaug] from comment #23)
> I don't quite understand (1).
> What if there is first a password field, then it is removed from DOM and
> then a new one is added
> back to the form. You don't want to get an event about that?

Hmm. Maybe? I don't know how common that is. The one or two examples of bug 355063  I had looked at, I remember as all adding an entire new <form> via JS, not recycling an existing form.

The main case I'm worried about is loading a page with:

  <form>
    <input id=username>
    <input type="password>
    <input type="password>
    ...
  </form>

or (related)

  <form>
    <input id=username>
    <input type="password>
    ....a long network delay happens here....
    <input type="password>
    ...
  </form>

(And, I suppose, JS adding a <form> like that to the DOM at whatever point.)

Having password manager process the form twice at load time is at best wasteful, and at worst I'd be afraid of it being leading to brokenness. Hence my suggestion in comment 22 ("#1") for to only first 1 event per form, but I'm open to any approach that helps.

A possible refinement:

Have each form track the number of password fields in it, and only file some event when that counter changes from 0 to 1.

[Hmm, does the current patch deal with changing the _type_ of on existing input field to "password"?]

> DOMLoginFormAdded is very vague, well, I don't actually understand what it
> means.

Yeah. :/ It really only makes sense in the context of loading a static page.

> Does (2) we really access sqlite all the time? I hope no main thread I/O.

It sure does. Main thread I/O, and the APIs are all synchronous. Design from the glorious days before we knew better. :( It's actually surprising that it has not popped up as a problem. (I presume because the DB is generally small, and stays hot.) We're on the way to fixing it anyway.
Flags: needinfo?(dolske)
(In reply to Justin Dolske [:Dolske] from comment #26)
> It sure does. Main thread I/O, and the APIs are all synchronous. Design from
> the glorious days before we knew better. :( It's actually surprising that it
> has not popped up as a problem. (I presume because the DB is generally
> small, and stays hot.) We're on the way to fixing it anyway.

Not sure where info that this is not a big problem comes from. According to total number of queries with 100ms+ delay, form database io is our top main thread sql IO offender: https://metrics.mozilla.com/data/content/pentaho-cdf-dd/Render?solution=metrics2&path=%2Ftelemetry&file=slowStartupSQL.wcdf
(In reply to Taras Glek (:taras) from comment #27)
> (In reply to Justin Dolske [:Dolske] from comment #26)
> > It sure does. Main thread I/O, and the APIs are all synchronous. Design from
> > the glorious days before we knew better. :( It's actually surprising that it
> > has not popped up as a problem. (I presume because the DB is generally
> > small, and stays hot.) We're on the way to fixing it anyway.
> 
> Not sure where info that this is not a big problem comes from. According to
> total number of queries with 100ms+ delay, form database io is our top main
> thread sql IO offender:
> https://metrics.mozilla.com/data/content/pentaho-cdf-dd/
> Render?solution=metrics2&path=%2Ftelemetry&file=slowStartupSQL.wcdf

I double-checked with Taras that he was seeing formhistory.sqlite, not signons.sqlite as a top offender so that is a bit unrelated.  Form history will be fixed shortly in bug 566746 and bug 697377.
...I'm still wrong, though, because Matt and I looked at the metrics, and while password manager isn't at the top of the list it's very much present and a problem. I'm either misremembering its severity, or what was being measured.

In any case, we can all agree it sucks, but let's not derail this bug any further. :)
Here's a version that implements what I think dolske is asking for, sans any sort of rate-limiting on how many forms for which we will fire events.  This version fires events whenever the number of password inputs in a form goes from 0 to 1, so it *may* fire multiple events for the same form depending on how it is manipulated.

This version seems somewhat uglier to me.  Questions from talking with smaug on IRC:

- Why should we not care about forms that have a password field added after DOMFormHasPassword has already fired?  If we're doing this so that we can sanely handle some forms generated via JS, we might as well make some effort to handling all such forms, right?
- What about inputs that aren't attached to forms?  (Do we even care about such cases with the current login manager?)

Asking smaug for feedback on general content-ish stuff and to see if he's closer to content with this approach.
Attachment #716128 - Flags: feedback?(dolske)
Attachment #716128 - Flags: feedback?(bugs)
(In reply to Nathan Froyd (:froydnj) from comment #30)

> - Why should we not care about forms that have a password field added after
> DOMFormHasPassword has already fired?  If we're doing this so that we can
> sanely handle some forms generated via JS, we might as well make some effort
> to handling all such forms, right?

In a nutshell, password manager works by looking for the first password field and potentially filling it in. If there's a text input somewhere before the first password field, it will be used as the (optional) username field.

Ergo, it's not interested in password fields (or events for them) beyond the first.

For all the gory details, see _getFormFields():

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js#642

[Note that when _submitting_ a form, we're very much interested in other password fields. But that's driven off a earlyformsubmit observer, so isn't involved with the events we're talking about here.]


> - What about inputs that aren't attached to forms?  (Do we even care about
> such cases with the current login manager?)

Hmmpf. I thought a dummy <form> was created for them. But I just tested and that's not the case... someInput.form === null. I've never seen this filed as a bug, and I'm pretty sure it won't work with the current password manager.

So, I think we can not worry about it.
Attachment #716128 - Flags: feedback?(dolske) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #30)
> - What about inputs that aren't attached to forms?  (Do we even care about
> such cases with the current login manager?)

If a form control isn't part of a form, it will not be submitted. Though, you have to take into account that they can be part of a form without being a form descendant with the 'form' attribute.
We can still imagine situations where a form control not part of a form has its value copied during the submit event handler so its value is somewhat sent but I think doing that for a password field would be simply vicious ;)
(In reply to Mounir Lamouri (:mounir) from comment #32)
> (In reply to Nathan Froyd (:froydnj) from comment #30)
> > - What about inputs that aren't attached to forms?  (Do we even care about
> > such cases with the current login manager?)
> 
> If a form control isn't part of a form, it will not be submitted. Though,
> you have to take into account that they can be part of a form without being
> a form descendant with the 'form' attribute.
> We can still imagine situations where a form control not part of a form has
> its value copied during the submit event handler so its value is somewhat
> sent but I think doing that for a password field would be simply vicious ;)

Also, not sure if I find it worth it but. There is the form attribute for input fields too. Sounds harry in this context, but thought it might be worth mentioning so its not forgotten without a discussion.
Comment on attachment 716128 [details] [diff] [review]
make <form> elements fire events when they have an <input type=password> element added to them

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

It seems that we are doing a lot of work to be able to revoke the event and not sending an event if there is more than one password control. Can't the UI work if we simply notify when a form control is removed/added from the tree by firing a "DOMFormPasswordControlChange". When this event is received (with the form as data), the UI could simply look for the first password control is there. I think that would dramatically reduce the complexity of the code in the backend (basically, we simply fire an event when there is a password being added or removed). On the UI side, that means running the same code everytime such an event is received. It would use more cycles but it is unlikely going to be a a performance issue.

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +458,1 @@
>    return rv;

As long as you are here, could you change that to:
return NS_OK;

@@ +1105,5 @@
> +    return;
> +  }
> +
> +  nsRefPtr<PasswordControlNotification> event
> +    = new PasswordControlNotification(this,

nit:
nsRefPtr<PasswordControlNotification> event =
  new PasswordControlNotifications(this,
                                   NS_LITTERAL_STRING("DOMFormHasPassword"));

@@ +1200,2 @@
>    // If it is a password control, and the password manager has not yet been
> +  // initialized, initialize the password manager.

Please, update the comment.

::: content/html/content/src/nsHTMLFormElement.h
@@ +263,5 @@
> +      }
> +      static_cast<nsHTMLFormElement*>(mEventNode.get())->EventHandled();
> +      return nsAsyncDOMEvent::Run();
> +    }
> +    

nit: trailing whitespaces

@@ +268,5 @@
> +    bool mIsRevoked;
> +  };
> +  void CancelPasswordEvent()
> +  {
> +    if (mPasswordControlAddedEvent.get()) {

nit: if (mPasswordControlAddedEvent) {

@@ +273,5 @@
> +      mPasswordControlAddedEvent.Revoke();
> +      mPasswordControlAddedEvent = nullptr;
> +    }
> +  }
> +  void EventHandled() { mPasswordControlAddedEvent = nullptr; }

What's calling EventHandled()?

@@ +427,5 @@
>  
>    /**
> +   * Number of password controls this form contains.
> +   */
> +  int32_t mNumPasswordControls;

uint32_t?

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +3120,5 @@
> +    if (oldType != NS_FORM_INPUT_PASSWORD && aNewType == NS_FORM_INPUT_PASSWORD) {
> +      mForm->PasswordControlAdded();
> +    } else if (oldType == NS_FORM_INPUT_PASSWORD &&
> +               aNewType != NS_FORM_INPUT_PASSWORD) {
> +      mForm->PasswordControlRemoved();

All of that is not needed. RemoveElement() and AddElement() is already called when you do a type change. See:
nsGenericHTMLFormElement::BeforeSetAttr
nsGenericHTMLFormElement::AfterSetAttr
Thanks for the comments, Mounir.

(In reply to Mounir Lamouri (:mounir) from comment #34)
> It seems that we are doing a lot of work to be able to revoke the event and
> not sending an event if there is more than one password control. Can't the
> UI work if we simply notify when a form control is removed/added from the
> tree by firing a "DOMFormPasswordControlChange". When this event is received
> (with the form as data), the UI could simply look for the first password
> control is there. I think that would dramatically reduce the complexity of
> the code in the backend (basically, we simply fire an event when there is a
> password being added or removed). On the UI side, that means running the
> same code everytime such an event is received. It would use more cycles but
> it is unlikely going to be a a performance issue.

When you say "a form control is removed/added" do you mean every <input> or just <input type=password>?  I am assuming the former, as the latter is already what this patch does, no?  Firing one event per input sounds awfully heavyweight, though I guess we already fire events for links and things...

> @@ +268,5 @@
> > +    bool mIsRevoked;
> > +  };
> > +  void CancelPasswordEvent()
> > +  {
> > +    if (mPasswordControlAddedEvent.get()) {
> 
> nit: if (mPasswordControlAddedEvent) {

I thought so too!  But nsRevocableEventPtr doesn't expose the necessary bits to make it work.  Separate bug, perhaps?

> @@ +273,5 @@
> > +      mPasswordControlAddedEvent.Revoke();
> > +      mPasswordControlAddedEvent = nullptr;
> > +    }
> > +  }
> > +  void EventHandled() { mPasswordControlAddedEvent = nullptr; }
> 
> What's calling EventHandled()?

PasswordControlNotification::Run.

> @@ +427,5 @@
> >  
> >    /**
> > +   * Number of password controls this form contains.
> > +   */
> > +  int32_t mNumPasswordControls;
> 
> uint32_t?

Yeah, this should probably be unsigned.

> ::: content/html/content/src/nsHTMLInputElement.cpp
> @@ +3120,5 @@
> > +    if (oldType != NS_FORM_INPUT_PASSWORD && aNewType == NS_FORM_INPUT_PASSWORD) {
> > +      mForm->PasswordControlAdded();
> > +    } else if (oldType == NS_FORM_INPUT_PASSWORD &&
> > +               aNewType != NS_FORM_INPUT_PASSWORD) {
> > +      mForm->PasswordControlRemoved();
> 
> All of that is not needed. RemoveElement() and AddElement() is already
> called when you do a type change. See:
> nsGenericHTMLFormElement::BeforeSetAttr
> nsGenericHTMLFormElement::AfterSetAttr

Thanks for the pointer.  Makes this significantly less ugly.
(In reply to Nathan Froyd (:froydnj) from comment #35)
> Thanks for the comments, Mounir.
> 
> (In reply to Mounir Lamouri (:mounir) from comment #34)
> > It seems that we are doing a lot of work to be able to revoke the event and
> > not sending an event if there is more than one password control. Can't the
> > UI work if we simply notify when a form control is removed/added from the
> > tree by firing a "DOMFormPasswordControlChange". When this event is received
> > (with the form as data), the UI could simply look for the first password
> > control is there. I think that would dramatically reduce the complexity of
> > the code in the backend (basically, we simply fire an event when there is a
> > password being added or removed). On the UI side, that means running the
> > same code everytime such an event is received. It would use more cycles but
> > it is unlikely going to be a a performance issue.
> 
> When you say "a form control is removed/added" do you mean every <input> or
> just <input type=password>?  I am assuming the former, as the latter is
> already what this patch does, no?  Firing one event per input sounds awfully
> heavyweight, though I guess we already fire events for links and things...

I indeed meant "a password control".

Could that solution be used instead of the more complex solution being currently worked on?
(In reply to Mounir Lamouri (:mounir) from comment #36)
> (In reply to Nathan Froyd (:froydnj) from comment #35)
> > When you say "a form control is removed/added" do you mean every <input> or
> > just <input type=password>?  I am assuming the former, as the latter is
> > already what this patch does, no?  Firing one event per input sounds awfully
> > heavyweight, though I guess we already fire events for links and things...
> 
> I indeed meant "a password control".
> 
> Could that solution be used instead of the more complex solution being
> currently worked on?

We have two opinions on the best way to do it on the backend side.  I don't have a dog in the fight here.  I'd just like to get something committed.

How about this as a plan:

- We land the one event per password control add/remove (attachment 713535 [details] [diff] [review]).
- The password manager is modified to use that.
- In the event that performance problems show up, it seems straightforward to modify the backend to use a more complicated scheme (attachment 716128 [details] [diff] [review] or similar); the frontend should need minimal changes to deal with the improved backend scheme.

Does that sound reasonable to everybody involved?
Comment on attachment 716128 [details] [diff] [review]
make <form> elements fire events when they have an <input type=password> element added to them

Sounds reasonable
Attachment #716128 - Flags: feedback?(bugs)
OK, here's the original DOMInputPassword{Added,Removed} event patch.  The tests
are a bit more complete insofar as they actually cover the type of an input
changing, too.
Attachment #713535 - Attachment is obsolete: true
Attachment #716128 - Attachment is obsolete: true
Attachment #719174 - Flags: review?(bugs)
(In reply to Mounir Lamouri (:mounir) from comment #34)

> It seems that we are doing a lot of work to be able to revoke the event and
> not sending an event if there is more than one password control.

Hmm, is the event revoking just a holdover from the previous patch? Seems like it would only get triggered by a pathological page adding and removing password fields quickly? That seems uncommon, and password manager can easily handle the "whoops" case of getting an event for a form that actually no longer has a password field.

> Can't the
> UI work if we simply notify when a form control is removed/added from the
> tree by firing a "DOMFormPasswordControlChange".

I tried to address this in comment 26 and comment 31. Perf is one reason, but I'm also wary of weird bugs popping up from processing the same form repeatedly in short order (master password interactions, in particular, have been a source of pain).
Comment on attachment 719174 [details] [diff] [review]
make <input type=password> elements fire events when they are added/removed

+
>+function tabLoad() {
>+  let tab = gBrowser.selectedTab;
>+  tab.linkedBrowser.removeEventListener("load", tabLoad, true);
>+
>+  let doc = gBrowser.selectedBrowser.contentDocument;
>+  gBrowser.addEventListener("DOMInputPasswordAdded", inputAdded, false);
>+  doc.addEventListener("DOMInputPasswordAdded", unexpectedContentAdd, false);
>+  doc.addEventListener("DOMInputPasswordRemoved", unexpectedContentRemove, false);
>+  let input = doc.createElementNS(HTML_NS, "input");
>+  input.setAttribute("type", "password");
>+  input.setAttribute("data-test", "unique-attribute");
>+  doc.body.appendChild(input);
>+  info("Done appending the input element");
>+}
Could you add also a test where input isn't the root of the subtree
attached to DOM.





>+  class InputPasswordNotificationEvent : public nsAsyncDOMEvent
>+  {
>+  public:
>+    InputPasswordNotificationEvent(nsINode* aEventNode,
>+                                   const nsAString& aEventType)
Could you make the type of aEventNode nsHTMLInputElement.
Attachment #719174 - Flags: review?(bugs) → review+
Comment on attachment 719174 [details] [diff] [review]
make <input type=password> elements fire events when they are added/removed

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

You are going to miss some edge cases with that, like @form being changed. I think you should have the code like in HTMLFormElement because the form knows when an element is added or removed.

Also, I do not think you need a Added/Removed event. Seems like a simple "DOMInputPasswordChanged" event should be enough. Most forms should only fire this event once, right?

I also feel that using a revocable event is an early optimization.
Attachment #719174 - Flags: review-
(In reply to Justin Dolske [:Dolske] from comment #40)
> > Can't the
> > UI work if we simply notify when a form control is removed/added from the
> > tree by firing a "DOMFormPasswordControlChange".
> 
> I tried to address this in comment 26 and comment 31. Perf is one reason,
> but I'm also wary of weird bugs popping up from processing the same form
> repeatedly in short order (master password interactions, in particular, have
> been a source of pain).

Can't the UI handle that?
I have lost count of what version this is.  This version:

- places the handling in the form element;
- fires DOMFormHasPassword when an <input type=password> is added to the form;
- coalesces multiple add events on a given form;
- does not fire events when inputs are removed (seems unnecessary, and add/remove seems unlikely)
Attachment #719174 - Attachment is obsolete: true
Attachment #726156 - Flags: review?(mounir)
Attachment #726156 - Flags: review?(bugs)
(In reply to Nathan Froyd (:froydnj) from comment #44)
> - does not fire events when inputs are removed (seems unnecessary, and
> add/remove seems unlikely)

I'm just a Firefox user and web developer and I have a dumb question: I have a website written in GWT that shows a "dialog" (that is, a dynamic DOM element is added to the DOM) to let the user log in with username and password. Right now, Firefox password manager does not recognize this dialog to remember the user login credentials (and this is why I'm interested to this bug). Anyway, I would like to point out that when login credentials are validated, this dialog is closed and some other elements in the DOM are refreshed, however the page is not reloaded. If you don't fire removal events, will Firefox keep on showing the password manager UI elements until I reload the page, just because it can't detect that meanwhile the password input field has been removed from the DOM?
Comment on attachment 726156 [details] [diff] [review]
make form elements fire events when <input type=password> elements are added/removed


>+nsHTMLFormElement::PostPasswordEvent()
>+{
>+  // Don't fire another add event if we have a pending add event.
>+  if (mFormPasswordEvent.get()) {
>+    return;
>+  }
>+
>+  nsRefPtr<FormPasswordEvent> event
>+    = new FormPasswordEvent(this, NS_LITERAL_STRING("DOMFormHasPassword"));
= should be in the previous line

dolske should probably comment on this too, since this is a feature for the UI stuff.
Attachment #726156 - Flags: review?(bugs) → review+
Attachment #726156 - Flags: review?(mounir)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c079f741c3d0

(In reply to Mauro Molinari from comment #45)
> (In reply to Nathan Froyd (:froydnj) from comment #44)
> > - does not fire events when inputs are removed (seems unnecessary, and
> > add/remove seems unlikely)
>
> If you don't fire removal
> events, will Firefox keep on showing the password manager UI elements until
> I reload the page, just because it can't detect that meanwhile the password
> input field has been removed from the DOM?

That's a good question; I don't know.  Justin, do you know?

I realize everybody might not be completely happy with the outcome here.  Please file followup bugs and CC me if there are more features that need to be added and/or tweaked.
Flags: needinfo?(dolske)
https://hg.mozilla.org/mozilla-central/rev/c079f741c3d0
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(In reply to Nathan Froyd (:froydnj) from comment #47)

> > If you don't fire removal
> > events, will Firefox keep on showing the password manager UI elements until
> > I reload the page, just because it can't detect that meanwhile the password
> > input field has been removed from the DOM?
> 
> That's a good question; I don't know.  Justin, do you know?

Password manager's "is there something to save" logic is entirely driven by form submit events. So, yes, we currently can't save logins in pages like comment 47 describes, and this patch won't make that and better (or worse). I suppose we could look at that again at some point, but it isn't at the top of the list right now. I'd want to carefully think through how that should work, so probably best we don't add a removal-event until we know it's what we actually want.
Flags: needinfo?(dolske)
relnote-firefox: --- → ?
This isn't relnote-worthy on its own (bug 355063 might be).
relnote-firefox: ? → -
For anyone interested, I just filed bug 1132211 to add a similar event for pages where the password field is not contained in a <form>.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: