PasswordManager should trigger autofill for new forms + old PasswordFromManagers

PasswordManager should trigger autofill for new forms + old PasswordFromManagers

It can happen that a website presents multiple forms matching the same PasswordFormManager. Currently, PasswordManager only issues an autofill request for the first such form. This CL makes sure that for all such forms the fill info is sent to the renderer.

BUG=435364
Committed: https://crrev.com/e9a2b337c293a96144dc6836feda925ee8b25d87
Cr-Commit-Position: refs/heads/master@{#307109}

[vabr (Chromium), 2014-12-02 14:54:55]

vabr@chromium.org changed reviewers:
+ gcasto@chromium.org

[vabr (Chromium), 2014-12-02 14:54:56]

Hi Garrett,

PTAL.

Thanks,
Vaclav

[vabr (Chromium), 2014-12-02 15:53:49]

FYI: I'm currently fixing the failing test. If you have not started reviewing,
please hold on until I upload the next patch and update the review log.

[vabr (Chromium), 2014-12-02 16:10:14]

Hi Garrett,

This is ready for review again.

I'm not completely happy with the current state: Now the autofill is triggered
both during "password forms parsed" and "password forms rendered", which means
we will send the message to autofill a form twice for each form in most cases.

An alternative would be to not trigger the autofill during the first, "parsed",
phase. All forms from that phase, which are visible, will get autofilled during
the "rendered" phase. Forms, which are invisible, would not get autofilled,
though, even if they become visible later. Also, I would need to add another
parameter to AutofillHostMsg_PasswordFormsParsed, to distinguish sending this
message on page load from sending it on dynamic forms being parsed.

Another alternative would be to not trigger autofill for already existing
PasswordFormManager during the "rendered" phase. That would get rid of the issue
with forms becoming visible later, and would not need modifications to IPC. On
the other hand, if a new form is being added between the "parsed" and "rendered"
phase, and there is already a matching PasswordFormManager, originally created
for another form, the new form won't get autofilled (and the user cannot get it
filled). In other words, this would limit the fix to http://crbug.com/435364 to
forms created before "parsed" or after "rendered". I'm not sure how much we care
about those, but it could make a nasty intermittent bug in a future.

Happy to hear your thoughts on this.

Cheers,
Vaclav

[Garrett Casto, 2014-12-03 00:17:54]

On 2014/12/02 16:10:14, vabr (Chromium) wrote:
> Hi Garrett,
> 
> This is ready for review again.
> 
> I'm not completely happy with the current state: Now the autofill is triggered
> both during "password forms parsed" and "password forms rendered", which means
> we will send the message to autofill a form twice for each form in most cases.
> 
> An alternative would be to not trigger the autofill during the first,
"parsed",
> phase. All forms from that phase, which are visible, will get autofilled
during
> the "rendered" phase. Forms, which are invisible, would not get autofilled,
> though, even if they become visible later. Also, I would need to add another
> parameter to AutofillHostMsg_PasswordFormsParsed, to distinguish sending this
> message on page load from sending it on dynamic forms being parsed.
> 
> Another alternative would be to not trigger autofill for already existing
> PasswordFormManager during the "rendered" phase. That would get rid of the
issue
> with forms becoming visible later, and would not need modifications to IPC. On
> the other hand, if a new form is being added between the "parsed" and
"rendered"
> phase, and there is already a matching PasswordFormManager, originally created
> for another form, the new form won't get autofilled (and the user cannot get
it
> filled). In other words, this would limit the fix to http://crbug.com/435364
to
> forms created before "parsed" or after "rendered". I'm not sure how much we
care
> about those, but it could make a nasty intermittent bug in a future.
> 
> Happy to hear your thoughts on this.
> 

It would be really nice to simplify this logic. Every time we fix bugs it gets
slightly more complicated. The cleanest solution seems like it would be to just
do all detection when the form is parsed. This is similar to your second
suggestion, though I was thinking that we would do nothing during
DidFinishDocumentLoad, and instead send two vectors (on of visible and one of
invisible) of forms at DidFinishLoad(). I think it might be possible to not add
a parameter for dynamic form parsing because it shouldn't be possible to for
this function to be called between the submission of one page and the loading of
the next, but it would make it clearer. The drawback is that filling is going to
be slower by whatever the difference between DidFinishLoad and
DidFinishDocumentLoad is. I'm not sure if this is a large enough time for it to
matter, maybe there are some UMA stats we can look at to see?

Second simplest would be to just send the Autofill messages twice (looks like
there are three at the moment). Not sure how much overhead that is in comparison
to other IPCs. I'm guessing not a lot, but I don't know how to reasonably tell.

Last solution seems to least tasteful. It's complicated and can possibly still
fail. The failure case would require a pretty strange page (dynamically
generated iframe which itself dynamically creates content in a very special
way), but still.

So if we can verify that the latency hit would be small, I would choose the
first solution here. Otherwise I would just go with double sending for now.
Thought?


> Cheers,
> Vaclav

[vabr (Chromium), 2014-12-03 10:40:57]

On 2014/12/03 00:17:54, Garrett Casto wrote:
> On 2014/12/02 16:10:14, vabr (Chromium) wrote:
> > Hi Garrett,
> > 
> > This is ready for review again.
> > 
> > I'm not completely happy with the current state: Now the autofill is
triggered
> > both during "password forms parsed" and "password forms rendered", which
means
> > we will send the message to autofill a form twice for each form in most
cases.
> > 
> > An alternative would be to not trigger the autofill during the first,
> "parsed",
> > phase. All forms from that phase, which are visible, will get autofilled
> during
> > the "rendered" phase. Forms, which are invisible, would not get autofilled,
> > though, even if they become visible later. Also, I would need to add another
> > parameter to AutofillHostMsg_PasswordFormsParsed, to distinguish sending
this
> > message on page load from sending it on dynamic forms being parsed.
> > 
> > Another alternative would be to not trigger autofill for already existing
> > PasswordFormManager during the "rendered" phase. That would get rid of the
> issue
> > with forms becoming visible later, and would not need modifications to IPC.
On
> > the other hand, if a new form is being added between the "parsed" and
> "rendered"
> > phase, and there is already a matching PasswordFormManager, originally
created
> > for another form, the new form won't get autofilled (and the user cannot get
> it
> > filled). In other words, this would limit the fix to http://crbug.com/435364
> to
> > forms created before "parsed" or after "rendered". I'm not sure how much we
> care
> > about those, but it could make a nasty intermittent bug in a future.
> > 
> > Happy to hear your thoughts on this.
> > 
> 
> It would be really nice to simplify this logic. Every time we fix bugs it gets
> slightly more complicated. The cleanest solution seems like it would be to
just
> do all detection when the form is parsed. This is similar to your second
> suggestion, though I was thinking that we would do nothing during
> DidFinishDocumentLoad, and instead send two vectors (on of visible and one of
> invisible) of forms at DidFinishLoad(). I think it might be possible to not
add
> a parameter for dynamic form parsing because it shouldn't be possible to for
> this function to be called between the submission of one page and the loading
of
> the next, but it would make it clearer. The drawback is that filling is going
to
> be slower by whatever the difference between DidFinishLoad and
> DidFinishDocumentLoad is. I'm not sure if this is a large enough time for it
to
> matter, maybe there are some UMA stats we can look at to see?
> 
> Second simplest would be to just send the Autofill messages twice (looks like
> there are three at the moment). Not sure how much overhead that is in
comparison
> to other IPCs. I'm guessing not a lot, but I don't know how to reasonably
tell.
> 
> Last solution seems to least tasteful. It's complicated and can possibly still
> fail. The failure case would require a pretty strange page (dynamically
> generated iframe which itself dynamically creates content in a very special
> way), but still.
> 
> So if we can verify that the latency hit would be small, I would choose the
> first solution here. Otherwise I would just go with double sending for now.
> Thought?
> 
> 
> > Cheers,
> > Vaclav

Thanks, Garrett,
For your response.

I had a look at PLT.FinishDocToFinish
(https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=12-02-2014...).
It looks like for 50% cases this is below 0.7s, but for 90% we are just below
6s. That seems a lot, so we probably should not give up autofilling on
DidFinishLoad.

I'm not sure I quite understood your second suggestion about sending Autofill
messages twice. Did you mean something like what is in the current patch set
(2)?

Cheers,
Vaclav

[Garrett Casto, 2014-12-03 22:00:05]

On 2014/12/03 10:40:57, vabr (Chromium) wrote:
> On 2014/12/03 00:17:54, Garrett Casto wrote:
> > On 2014/12/02 16:10:14, vabr (Chromium) wrote:
> > > Hi Garrett,
> > > 
> > > This is ready for review again.
> > > 
> > > I'm not completely happy with the current state: Now the autofill is
> triggered
> > > both during "password forms parsed" and "password forms rendered", which
> means
> > > we will send the message to autofill a form twice for each form in most
> cases.
> > > 
> > > An alternative would be to not trigger the autofill during the first,
> > "parsed",
> > > phase. All forms from that phase, which are visible, will get autofilled
> > during
> > > the "rendered" phase. Forms, which are invisible, would not get
autofilled,
> > > though, even if they become visible later. Also, I would need to add
another
> > > parameter to AutofillHostMsg_PasswordFormsParsed, to distinguish sending
> this
> > > message on page load from sending it on dynamic forms being parsed.
> > > 
> > > Another alternative would be to not trigger autofill for already existing
> > > PasswordFormManager during the "rendered" phase. That would get rid of the
> > issue
> > > with forms becoming visible later, and would not need modifications to
IPC.
> On
> > > the other hand, if a new form is being added between the "parsed" and
> > "rendered"
> > > phase, and there is already a matching PasswordFormManager, originally
> created
> > > for another form, the new form won't get autofilled (and the user cannot
get
> > it
> > > filled). In other words, this would limit the fix to
http://crbug.com/435364
> > to
> > > forms created before "parsed" or after "rendered". I'm not sure how much
we
> > care
> > > about those, but it could make a nasty intermittent bug in a future.
> > > 
> > > Happy to hear your thoughts on this.
> > > 
> > 
> > It would be really nice to simplify this logic. Every time we fix bugs it
gets
> > slightly more complicated. The cleanest solution seems like it would be to
> just
> > do all detection when the form is parsed. This is similar to your second
> > suggestion, though I was thinking that we would do nothing during
> > DidFinishDocumentLoad, and instead send two vectors (on of visible and one
of
> > invisible) of forms at DidFinishLoad(). I think it might be possible to not
> add
> > a parameter for dynamic form parsing because it shouldn't be possible to for
> > this function to be called between the submission of one page and the
loading
> of
> > the next, but it would make it clearer. The drawback is that filling is
going
> to
> > be slower by whatever the difference between DidFinishLoad and
> > DidFinishDocumentLoad is. I'm not sure if this is a large enough time for it
> to
> > matter, maybe there are some UMA stats we can look at to see?
> > 
> > Second simplest would be to just send the Autofill messages twice (looks
like
> > there are three at the moment). Not sure how much overhead that is in
> comparison
> > to other IPCs. I'm guessing not a lot, but I don't know how to reasonably
> tell.
> > 
> > Last solution seems to least tasteful. It's complicated and can possibly
still
> > fail. The failure case would require a pretty strange page (dynamically
> > generated iframe which itself dynamically creates content in a very special
> > way), but still.
> > 
> > So if we can verify that the latency hit would be small, I would choose the
> > first solution here. Otherwise I would just go with double sending for now.
> > Thought?
> > 
> > 
> > > Cheers,
> > > Vaclav
> 
> Thanks, Garrett,
> For your response.
> 
> I had a look at PLT.FinishDocToFinish
>
(https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=12-02-2014...).
> It looks like for 50% cases this is below 0.7s, but for 90% we are just below
> 6s. That seems a lot, so we probably should not give up autofilling on
> DidFinishLoad.
> 

Hmmm, that is a long time :(. I guess we should avoid that for now.

> I'm not sure I quite understood your second suggestion about sending Autofill
> messages twice. Did you mean something like what is in the current patch set
> (2)?
> 

I was suggesting what you have here. Of the three messages we can now send
twice, the password generation message and the blacklisted bubble message seem
like there are mostly harmless. I am a little more worried about the actual
autofilling. Since we don't overwrite the username it's mostly safe, but we will
overwrite the password if it's changed. I'm also not sure how it would interact
if the autofill bubble is up. For safety I would move the continue line of
OnFillPasswordForm() up before the call to FillFormOnPasswordReceived(). I think
that's guaranteed to be safe then. Otherwise LGTM.

> Cheers,
> Vaclav

[vabr (Chromium), 2014-12-04 10:12:28]

Hi Garrett,

Thanks for your helpful answer.

Patch set 4 (= p.s. 3 + rebasing) is what you just approved of.
Patch set 5 avoids duplicated sending of the blacklisting and generation IPC
messages.

Please let me know, which one you prefer.

Cheers,
Vaclav

[vabr (Chromium), 2014-12-05 15:59:47]

Hi Garrett,

I just wanted to emphasise that despite your previous approval I need you to
have a look and decide if patch set 5 or 4 is what you'd like me to land.

Thanks!
Vaclav

On 2014/12/04 10:12:28, vabr (Chromium) wrote:
> Hi Garrett,
> 
> Thanks for your helpful answer.
> 
> Patch set 4 (= p.s. 3 + rebasing) is what you just approved of.
> Patch set 5 avoids duplicated sending of the blacklisting and generation IPC
> messages.
> 
> Please let me know, which one you prefer.
> 
> Cheers,
> Vaclav

[Garrett Casto, 2014-12-05 18:29:52]

LGTM for patch set 5. Both generation and key icon shouldn't need to be updated
since the only clear after navigation not on frame destruction.

[vabr (Chromium), 2014-12-05 21:41:54]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-12-05 21:42:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773823002/80001

[commit-bot: I haz the power, 2014-12-05 23:31:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-05 23:32:28]

Patchset 5 (id:??) landed as
https://crrev.com/e9a2b337c293a96144dc6836feda925ee8b25d87
Cr-Commit-Position: refs/heads/master@{#307109}