[Password manager] Recognise squashed login+sign-up forms

[Password manager] Recognise squashed login+sign-up forms

Some pages contain a login form and a sign-up form squashed in one <form> element. While that is a bad practice, Chrome should be able to distinguish these forms from change-password forms (the difference being that the former will have at least one non-password text field between the login-part password field and the sign-up part password fields). This distinction enables Chrome to work on the login forms.

BUG=449714
Committed: https://crrev.com/55c69e29a5ca49f991046818157a1be253f38ff6
Cr-Commit-Position: refs/heads/master@{#321965}

[vabr (Chromium), 2015-03-17 17:34:09]

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

[vabr (Chromium), 2015-03-17 17:34:10]

(+dvadym@ on his own demand)

Hi Garrett,

Could you please have a look?

This CL focusses on making the login part of the composed form work. It will
still save the password of the sign-up form as well, but with the first text
field (or the first autocomplete=username field) value as the username, i.e.,
mostly with an empty username. We can try to optimise that in a subsequent CL,
but I would like not to cram too much logic into this one.

Cheers,
Vaclav

https://codereview.chromium.org/1014683006/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/1/components/autofill/content...
components/autofill/content/renderer/password_form_conversion_utils.cc:14:
#include "third_party/re2/re2/re2.h"
While R comes before W in the English alphabet, the ASCII values of capital
letters are lower than those of lowercase ones, so W < r. This order of
#includes is blessed by the presubmit check.

[Garrett Casto, 2015-03-17 22:49:37]

https://codereview.chromium.org/1014683006/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1014683006/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1675: //
Password form without username-field.
I think this is leftover from copy and paste.

https://codereview.chromium.org/1014683006/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:30: if
(RE2::FullMatch(layout_sequence, "NPN+PP"))
As written this is over constrained and doesn't catch either godaddy.com or
nordstrom.com (godaddy has only one password field for the signup portion and
nordstrom has a text field after the last password field).

We want this to be as simple as possible and still allow sites like godaddy to
work but not negatively impact more traditional forms. The best regexp that I
can think of that makes both broken sites work is "NPN+P.*". I can't think of
examples right now that this would match that aren't combined forms. I could
imagine a signup form that could match this ("username, password, random
information, security question, security answer (password field)"), but I would
think such a construct would be rare. Even if it were to accidentally get
labeled as LAYOUT_LOGIN_AND_SIGNUP, filling saved credentials on a signup form,
while bad, isn't a disaster.

Does that regexp change seem reasonable to you?

https://codereview.chromium.org/1014683006/diff/20001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1014683006/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form.h:73: // fill them, but it is not
stored.
This comment seems like more information than is necessary. Consumers of this
data don't necessarily need to care about exactly how exactly the form is laid
out for a form to be classified as LAYOUT_LOGIN_AND_SIGNUP, they just need to
know what it means. I would drop the discussion of regular expression and what
sequence needs to be matched and just talk generally about how these forms are
different.

If you want to keep technical discussion about how we determine Layout, keeping
it in PasswordFormConversionUtils seems more reasonable. Some discussion about
how we determined the current regexp seems like it could be useful (e.g. didn't
use ".*PN+P.*" since it would match signup forms which use password fields to
store private but non-password data).

https://codereview.chromium.org/1014683006/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1014683006/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:548: //
working change password functionality). The combined login-sign-up forms
"The" is unnecessary here.

https://codereview.chromium.org/1014683006/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1014683006/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:855:
EXPECT_FALSE(PasswordFormManager::PasswordToSave(
What caused this change?

[vabr (Chromium), 2015-03-18 16:51:03]

Thanks, Garrett!

Comments addressed, please take a look.

Cheers,
Vaclav

https://codereview.chromium.org/1014683006/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1014683006/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1675: //
Password form without username-field.
On 2015/03/17 22:49:37, Garrett Casto wrote:
> I think this is leftover from copy and paste.

Done.

https://codereview.chromium.org/1014683006/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:30: if
(RE2::FullMatch(layout_sequence, "NPN+PP"))
On 2015/03/17 22:49:37, Garrett Casto wrote:
> As written this is over constrained and doesn't catch either
http://godaddy.com or
> http://nordstrom.com (godaddy has only one password field for the signup
portion and
> nordstrom has a text field after the last password field).

Ah, I forgot to get back from my distilled toy examples to the real world.
Thanks for pointing this out!

> 
> We want this to be as simple as possible and still allow sites like godaddy to
> work but not negatively impact more traditional forms. The best regexp that I
> can think of that makes both broken sites work is "NPN+P.*". I can't think of
> examples right now that this would match that aren't combined forms. I could
> imagine a signup form that could match this ("username, password, random
> information, security question, security answer (password field)"), but I
would
> think such a construct would be rare. Even if it were to accidentally get
> labeled as LAYOUT_LOGIN_AND_SIGNUP, filling saved credentials on a signup
form,
> while bad, isn't a disaster.
> 
> Does that regexp change seem reasonable to you?

It does, I agree with your view of the severity of the potential breakage, and
cannot think of a page I know is already broken by that.

https://codereview.chromium.org/1014683006/diff/20001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1014683006/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form.h:73: // fill them, but it is not
stored.
On 2015/03/17 22:49:37, Garrett Casto wrote:
> This comment seems like more information than is necessary. Consumers of this
> data don't necessarily need to care about exactly how exactly the form is laid
> out for a form to be classified as LAYOUT_LOGIN_AND_SIGNUP, they just need to
> know what it means. I would drop the discussion of regular expression and what
> sequence needs to be matched and just talk generally about how these forms are
> different.
> 
> If you want to keep technical discussion about how we determine Layout,
keeping
> it in PasswordFormConversionUtils seems more reasonable. Some discussion about
> how we determined the current regexp seems like it could be useful (e.g.
didn't
> use ".*PN+P.*" since it would match signup forms which use password fields to
> store private but non-password data).

Good point, I tried to rephrase.

https://codereview.chromium.org/1014683006/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1014683006/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:548: //
working change password functionality). The combined login-sign-up forms
On 2015/03/17 22:49:37, Garrett Casto wrote:
> "The" is unnecessary here.

Done.

https://codereview.chromium.org/1014683006/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1014683006/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:855:
EXPECT_FALSE(PasswordFormManager::PasswordToSave(
On 2015/03/17 22:49:37, Garrett Casto wrote:
> What caused this change?

Previously, if there was a "new password" field, it was unconditionally
considered as the only source of the password value. If empty, nothing was
offered for saving.

To support saving the login-part on a login+sign-up form, the code now is
willing to use the old password value, even in the presence of a new password
field, if the new password value is empty.

In this test that meant that there suddenly was a call to
PromptUserToSavePassword. The password to be saved was the old one, not the
(empty) new one (verified on this line, 855).

The bug checked by this test was about saving an empty password, which replaced
the old one. This does not happen after my CL -- instead of replacing the old
password with an empty password, the old one is "updated" with itself (no-op).
The important between-revisions-invariant to preserve is that we never save an
empty password.

(And of course, if there are both a non-empty old password value, and a
non-empty new password value, the non-empty new password value is saved.)

[Garrett Casto, 2015-03-19 08:22:17]

lgtm

[vabr (Chromium), 2015-03-19 09:24:58]

vabr@chromium.org changed reviewers:
+ cpu@chromium.org, isherman@chromium.org

[vabr (Chromium), 2015-03-19 09:24:58]

Thanks, Garrett.

@Ilya: could you please check that adding the dependency on re2 in
components/autofill/content/renderer/DEPS is OK?

@cpu: could you please give a third_party OWNER rubberstamp for adding the re2
dependency to //components/autofil?

Thanks!
Vaclav

[Ilya Sherman, 2015-03-19 21:25:50]

The dependency on re2 not lgtm.

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:14:
#include "third_party/re2/re2/re2.h"
Existing Autofill code uses ICU for regexes.  I don't think we should have the
component depend on multiple regex libraries.

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:31: //
continues with a sign-up section (N+PP.*). The aim is to distinguish such
Is the end of the regex supposed to be "PP.* or "P.*"?  You use both.

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:32: //
forms from change password-forms (N*PPP?) and forms which use password
Is there intentionally not a .* at the end of this regex?

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:39: }
Honestly, I do not think this code is very clear when written using regexes.  I
think it would be much more readable if you passed a vector of form field types,
and had the matching logic written more explicitly.  You could still keep the
high-level motivational comment written in terms of regexes if you want to,
though please keep in mind that it's a lot of effort for most human readers to
unpack a single-line regex into the logical pieces that underly it.

[vabr (Chromium), 2015-03-20 13:03:47]

(Before responding to Ilya -- removing cpu@ from the reviewers' list, because
#including third_party/icu does not seem to need a change of DEPS. At least it
is not done for the current use in autofill/core/browser.)

[vabr (Chromium), 2015-03-20 13:05:49]

vabr@chromium.org changed reviewers:
- cpu@chromium.org

[vabr (Chromium), 2015-03-20 13:07:01]

Thanks, Ilya, for your review!

I addressed part of your comments, but have questions about the last one. Please
have patience with me and take a look again.

Thanks!
Vaclav

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:14:
#include "third_party/re2/re2/re2.h"
On 2015/03/19 21:25:50, Ilya Sherman wrote:
> Existing Autofill code uses ICU for regexes.  I don't think we should have the
> component depend on multiple regex libraries.

Thanks for pointing this out, I did not know about it. Changed to ICU.

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:31: //
continues with a sign-up section (N+PP.*). The aim is to distinguish such
On 2015/03/19 21:25:50, Ilya Sherman wrote:
> Is the end of the regex supposed to be "PP.* or "P.*"?  You use both.

PP was a typo, thanks for pointing it out.

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:32: //
forms from change password-forms (N*PPP?) and forms which use password
On 2015/03/19 21:25:50, Ilya Sherman wrote:
> Is there intentionally not a .* at the end of this regex?

Not intentionally. It was meant as an example. Added .* to be consistent.

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:39: }
On 2015/03/19 21:25:50, Ilya Sherman wrote:
> Honestly, I do not think this code is very clear when written using regexes. 
I
> think it would be much more readable if you passed a vector of form field
types,
> and had the matching logic written more explicitly.

I'm not sure I follow you here. What do you mean by a vector of form field
types? A vector of some kind of enum? Also, what do you mean by a more explicit
matching logic? Something like a finite-state automaton seems to me to be much
harder to read than a single regexp.

>  You could still keep the
> high-level motivational comment written in terms of regexes if you want to,
> though please keep in mind that it's a lot of effort for most human readers to
> unpack a single-line regex into the logical pieces that underly it.

This is too vague for me -- I don't know how most human readers work, and I do
not understand what unpacking a regex into logical pieces mean. I tried to
improve the comment above the pattern definition, please let me know what you
think about it.

[Ilya Sherman, 2015-03-20 22:43:08]

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:39: }
On 2015/03/20 13:07:00, vabr (Chromium) wrote:
> On 2015/03/19 21:25:50, Ilya Sherman wrote:
> > Honestly, I do not think this code is very clear when written using regexes.

> I
> > think it would be much more readable if you passed a vector of form field
> types,
> > and had the matching logic written more explicitly.
> 
> I'm not sure I follow you here. What do you mean by a vector of form field
> types? A vector of some kind of enum? Also, what do you mean by a more
explicit
> matching logic? Something like a finite-state automaton seems to me to be much
> harder to read than a single regexp.

After trying to write this as a C++ method, I withdraw my objection to using
regexes -- it's just too fiddly to get an equivalent C++ implementation written.

> >  You could still keep the
> > high-level motivational comment written in terms of regexes if you want to,
> > though please keep in mind that it's a lot of effort for most human readers
to
> > unpack a single-line regex into the logical pieces that underly it.
> 
> This is too vague for me -- I don't know how most human readers work, and I do
> not understand what unpacking a regex into logical pieces mean. I tried to
> improve the comment above the pattern definition, please let me know what you
> think about it.

What I mean is, it's hard to parse "NPN+P.*" as a regex that consists of three
parts: "NP" (login section), "N+P" (sign-up section), ".*" (other fields that we
don't care about).  And, hence, it's easy to make a typo that people will miss. 
I'm a big fan of Python's verbose regex syntax [1], which allows you to write
regexes split into their logical parts, instantly making the regexes much easier
to read.

[1] https://docs.python.org/2/library/re.html#re.VERBOSE

https://codereview.chromium.org/1014683006/diff/140001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/140001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:80: }
Since you only have a single regex used in this file, I think it's easier to
just define a base::LazyInstance for the compiled ICU regex, rather than using a
cache as we do in the browser code (where we compute lots of different regexes).

https://codereview.chromium.org/1014683006/diff/140001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:104:
return match == TRUE;
Man, I forgot how much uglier ICU's regex code is than RE2... alas.

Historically, we've used ICU rather than RE2 because ICU regexes were faster
(once compiled -- compiling them is slow, hence the cache).  It would be
interesting to see whether this is still the case; and if not, possibly switch
to RE2 throughout the component.

[vabr (Chromium), 2015-03-23 14:23:45]

Thanks Ilya.

PTAL again, I tried to address your comments further.

Cheers,
Vaclav

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:39: }
On 2015/03/20 22:43:07, Ilya Sherman wrote:
> On 2015/03/20 13:07:00, vabr (Chromium) wrote:
> > On 2015/03/19 21:25:50, Ilya Sherman wrote:
> > > Honestly, I do not think this code is very clear when written using
regexes.
> 
> > I
> > > think it would be much more readable if you passed a vector of form field
> > types,
> > > and had the matching logic written more explicitly.
> > 
> > I'm not sure I follow you here. What do you mean by a vector of form field
> > types? A vector of some kind of enum? Also, what do you mean by a more
> explicit
> > matching logic? Something like a finite-state automaton seems to me to be
much
> > harder to read than a single regexp.
> 
> After trying to write this as a C++ method, I withdraw my objection to using
> regexes -- it's just too fiddly to get an equivalent C++ implementation
written.
> 
> > >  You could still keep the
> > > high-level motivational comment written in terms of regexes if you want
to,
> > > though please keep in mind that it's a lot of effort for most human
readers
> to
> > > unpack a single-line regex into the logical pieces that underly it.
> > 
> > This is too vague for me -- I don't know how most human readers work, and I
do
> > not understand what unpacking a regex into logical pieces mean. I tried to
> > improve the comment above the pattern definition, please let me know what
you
> > think about it.
> 
> What I mean is, it's hard to parse "NPN+P.*" as a regex that consists of three
> parts: "NP" (login section), "N+P" (sign-up section), ".*" (other fields that
we
> don't care about).  And, hence, it's easy to make a typo that people will
miss. 
> I'm a big fan of Python's verbose regex syntax [1], which allows you to write
> regexes split into their logical parts, instantly making the regexes much
easier
> to read.
> 
> [1] https://docs.python.org/2/library/re.html#re.VERBOSE
Thanks for explanation. I tried to emulate the verbose mode by using the
implicit string literal concatenation and C++ comments. Please let me know if
you see further room for improvement.

https://codereview.chromium.org/1014683006/diff/140001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/140001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:80: }
On 2015/03/20 22:43:08, Ilya Sherman wrote:
> Since you only have a single regex used in this file, I think it's easier to
> just define a base::LazyInstance for the compiled ICU regex, rather than using
a
> cache as we do in the browser code (where we compute lots of different
regexes).

Good call, I forgot about LazyInstance.
Done.

https://codereview.chromium.org/1014683006/diff/140001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:104:
return match == TRUE;
On 2015/03/20 22:43:08, Ilya Sherman wrote:
> Man, I forgot how much uglier ICU's regex code is than RE2... alas.
> 
> Historically, we've used ICU rather than RE2 because ICU regexes were faster
> (once compiled -- compiling them is slow, hence the cache).  It would be
> interesting to see whether this is still the case; and if not, possibly switch
> to RE2 throughout the component.

It looks like RE2 outperforms ICU significantly: see the benchmark at
https://codereview.chromium.org/1026003002/. Whether we decide to switch the
whole component to RE2 or not, in this CL I suggest sticking to ICU, though
(provided it lands before the switch to RE2). Even with the ICU syntax, the
boilerplate is not too bad for a single matcher.

[Ilya Sherman, 2015-03-24 00:28:02]

Thanks.  LGTM.

https://codereview.chromium.org/1014683006/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1014683006/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:54: ".*";
 // Anything beyond that.
Thanks, this is indeed much nicer to read :)

[vabr (Chromium), 2015-03-24 09:46:11]

Thanks Ilya and Garrett for your review.
Looks like we are all settled now, so I'll push this to the CQ.

Cheers,
Vaclav

[vabr (Chromium), 2015-03-24 09:46:16]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-03-24 09:46:17]

The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/1014683006/#ps180001
(title: "LazyInstance + commented regexp")

[commit-bot: I haz the power, 2015-03-24 09:46:29]

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

[commit-bot: I haz the power, 2015-03-24 10:45:23]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-03-24 10:46:18]

Patchset 10 (id:??) landed as
https://crrev.com/55c69e29a5ca49f991046818157a1be253f38ff6
Cr-Commit-Position: refs/heads/master@{#321965}