Also happens on tumblr.

The cause: NewPasswordFormManager and the new form parser rely on unique renderer ids instead of field names for filling, so they don't have the "anonymous_password" replacement name for fields without names.

However, the saving logic does not seem to reflect that, so when the parser says that the password field to be saved has an empty name, the value is no longer found. This should be easy to fix.

I stand corrected, there is apparently an empty password field value coming out of the parser already. Looking further...

For prioritisation reasons, fixing this bug is suspended until  bug 895781  is fixed (or blocked). In the meantime, new-password-form-parsing-for-saving should get disabled on all channels.

When I wrote "should get disabled on all channels" I actually meant: "I uploaded an internal CL to disable it on all channels".

Also, could not resist -- the issue seems to be the PasswordToSave function inside NewPasswordFormManager. It returns the "current password" if, in particular, the new password element has an empty name. In the old parser (which created dummy "anonymous*" names for unnamed fields, the new password element name being empty implied that the found password was the current one (so PasswordFormManager::PasswordToSave is correct doing the same).

However, with the new password parser, it might happen that the new password element name is empty, but the only non-empty password value is the new password value.

At this point it is a matter of hours to write a test and a patch, and I think I cannot just sit on that until  bug 895781  is fixed.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b4a42fa7d579c631d0b4d2d3b0a3f1aa3ed9dc7

commit 4b4a42fa7d579c631d0b4d2d3b0a3f1aa3ed9dc7
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Oct 30 15:20:57 2018

Password form parser unittest: separate roles

The structure used to record expectations in the unittest for FormData
-> PasswordForm parser allows to specify the "role" of a field --
whether that field should end up marked as the username or one of the
passwords.

It currently allows marking a field with a role specific for the
"filling" or the "saving" mode of the parser. But it does not allow
one field to have different roles in different modes.

This has not been an issue, but will be in the coming CLs, where some
fields will be "current password" in one mode and "new password" in
the other.

Therefore, this CL adds separate roles in the expectation struct to
capture potentially different roles in different modes. The CL keeps
the original role data member, overriding the two specialised new
ones, to keep this change and the tests themselves less verbose.

As a side effect, the role-describing enum is now shrinked to about a
third of its original size (due to the previous need to have explicit
values saying "role X for filling", "role X for saving", "role X for
both").

Bug:  899687 
Change-Id: Ia8c3bf433a0fc1b003b6b38a11e43ddaffb3387e
Reviewed-on: https://chromium-review.googlesource.com/c/1307493
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603901}
[modify] https://crrev.com/4b4a42fa7d579c631d0b4d2d3b0a3f1aa3ed9dc7/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/54b371f6aa922048dc67ab66cdd2a0501132923c

commit 54b371f6aa922048dc67ab66cdd2a0501132923c
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Oct 30 15:25:52 2018

Improve logging in password form parser unittest

The unittest for the FormData -> PasswordForm parser takes a
description of a form and of the parsing result, synthesises the form,
parses it and compares to the expected result.

To keep the descriptions short, many unimportant things are generated,
such as unique values of fields.

Then, when a test fails, the error messages refer to the generated
values, saying things like: "expected the current password to be
html_304 but it is actually html_511". This makes little sense when
one cannot see the form synthesised from the description.

So this CL adds logging to describe the form when test failures are
encoutered.

Bug:  899687 
Change-Id: Ic0ffdc348a490c8f1fcb3e2f193ac10a348f2494
Reviewed-on: https://chromium-review.googlesource.com/c/1307398
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603903}
[modify] https://crrev.com/54b371f6aa922048dc67ab66cdd2a0501132923c/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/055a2dfcf498d5e2763a40e47d1784bbfb0fc1eb

commit 055a2dfcf498d5e2763a40e47d1784bbfb0fc1eb
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Oct 30 15:31:14 2018

Ignore empty fields in password form parser

The FormData -> PasswordForm parser has a special "saving" mode, in
which it optimizes for saving filled-in forms. This means that the
parser ignores fields with empty values, because from those nothing
can be saved.

That has been true for using structural heuristics. But when the form
contained autocomplete attributes or there were server hints for it,
the parser would trust those even if they hinted at fields with empty
values.

There seems to be no reason to try to save empty-valued fields, under
any circumstances, so this CL filters fields with empty values out
before the parser does any kind of analysis.

Bug:  899687 
Change-Id: Ia5e2ff885c9f15917765c5cb70ab5501ecc40118
Reviewed-on: https://chromium-review.googlesource.com/c/1307401
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603904}
[modify] https://crrev.com/055a2dfcf498d5e2763a40e47d1784bbfb0fc1eb/components/password_manager/core/browser/form_parsing/form_parser.cc
[modify] https://crrev.com/055a2dfcf498d5e2763a40e47d1784bbfb0fc1eb/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/329814a76d615aae4996a54b816363c4869af339

commit 329814a76d615aae4996a54b816363c4869af339
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Oct 30 15:35:48 2018

PasswordToSave in NewPasswordFormManager accepts empty field name

The PasswordToSave function inside NewPasswordFormManager is used to
extract the password to be saved from a filled password form. This can
either be the form's new-password or current-password (the latter only
if there is no new-password).

Currently, the PasswordToSave function returns the current-password
if, in particular, the new-password field has an empty name. This
breaks for sites which choose to assign no name to the new-password
field. Therefore this CL changes that behaviour to only check for
field values and ignore their names.

Note: The old parser created dummy "anonymous*" names for unnamed
fields, so while the old PasswordFormManager::PasswordToSave function
has the same property as the new one, it was not an issue there.

Bug:  899687 
Change-Id: I31a67533f470ba62b7a3a835d1dd3d9e9ce5bab4
Reviewed-on: https://chromium-review.googlesource.com/c/1307438
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603909}
[modify] https://crrev.com/329814a76d615aae4996a54b816363c4869af339/components/password_manager/core/browser/new_password_form_manager.cc
[modify] https://crrev.com/329814a76d615aae4996a54b816363c4869af339/components/password_manager/core/browser/new_password_form_manager_unittest.cc