Vaclav, could you please triage this and check what causes this? Thanks.

This is also reproducible on 71.0.3552.0, and also with "--disable-features=new-password-form-parsing". The problem is not with the parser (the old and the new one work the same here), but somewhere with the logic triggering autofilling. I am still looking into that.


More details:

chrome://password-manager-internals report that Chrome finds a username field with id="userID" and a password field with id="password" and autofills them.

When I go to the page after it loads and look with DevTools at all input fields, those two are no longer there (there is one more password, id="my-account-password" field which is filled with the saved password, but the field is not visible to the user). Likely the page removed them during loading.

When I then click on "Sign In or Join", the page creates another pair of input fields: a username field with id="user-id" (but name="userID") and a password field with id="password".

The fill-on-account-select fallback works on the password field, but not the username one, which indicates that the renderer still expects to find the deleted form which was autofilled.

An interesting fact is that the page has two sign-in forms (with the same purpose and action: https://www.marriott.com/aries-auth/loginWithCredentials.comp).

One is accessible through clicking the top-right "Sign In" button, the other through the bottom-centre "Sign In" button. The latter is filled all right, the former is not.

Still investigating.

The cause is now clear.

The form which is not filled is not parsed successfully. The parser (both new and old) fails to see any password fields in it.

This is because, as battre@ correctly noticed, the password field is initially readonly. Both parsers ignore readonly password fields, because they hint at the use of virtual keyboards, in the presence of which filling the value is likely useless.

This does not take into account changes to that status over time, though.

My proposal is to ignore "readonly" during parsing, and only check it during filling: Chrome would not fill if the field is readonly at the moment of filling.

Any objections to this proposal?

I forgot to mention that the password field becomes writeable by the time the user has a chance to type into it.

I'm not sure yet whether it becomes writeable before Chrome attempts filling.

Chrome might need to observe changes to readonly status to handle this properly.

Or, we might give up and rely on the fill-on-account-select workaround.

I think the idea sounds promising but it would be useful to get an estimate of the positive and negative impact of the change. I don't fully grasp the idea and consequences of ignoring "readonly" during parsing and checking during filling.

We should make that by fixing this, we are not leaving bigger problems unfixed and that we are not breaking correctly supported sites.

Just brainstorming: As a variation of the proposal could be to ignore "readonly" only in case the form does not have writeable password fields.

I should have been clearer that I'm afraid that #5 points out a serious downside of the proposal from #4 -- we have no guarantee that filling comes after the page removes the "readonly" attribute. So I withdraw that proposal.

I don't know about ignoring "readonly" if there are no writeable fields -- I suspect the cases where we do want to prevent filling (virtual keyboards) have a good chance of not having a writeable password field.

From what I know so far, the only viable fix would be to observe the removal of the "readonly" attribute and respond to it by filling (the same response we do on dynamic changes to password forms). Not sure if observing the removal of "readonly" is feasible, and if we'd create too much complexity for too little gain. OTOH, I don't think there is space for regressions if we do that.

One thing I can do is use Maxim's crawler to check how frequent readonly password fields are and inspect a few sites to understand what happens there.

I don't think the frequency of this issue is easily measurable with the crawler.

On the Marriott website, for example, the readonly field is not seen until the user clicks the higher of the two "sign-in" buttons. The crawler would need to mimic that and we would not know how often it just did not click the correct thing.

We could measure this via UMA (or UKM if we want a sample of how the cases with readonly really look like).

Dominic, do you think we should invest into measuring the frequency of this through user metrics?

Let's discuss it in the next feature sync.

Summary of the discussion:
* We need to manually review the root causes of our top issues to understand how important this is to fix. This review will happen soon but has not happened yet.
* Independently of that, we intend to put in UKM to signal sites where Chrome won't fill a password form because the password field is readonly.

Oh, I just realised that the readonly fields are only excluded from parsing when using local heuristics. As long as the server hints or autocomplete attributes specify a password field, it is used and filled regardless of being readonly.

Should we add an override for marriott? The signatures for the form and fields are listed in this snippet:

Server predictions: {
Signature of form: 17380206710487356981
Origin: https://www.marriott.com/
Action: https://www.marriott.com/
Form fields: 
userID: 1050503534, type=text, renderer_id = 553, autocomplete=off, SERVER_PREDICTION: EMAIL_ADDRESS
password: 2051817934, type=password, renderer_id = 554, autocomplete=off
rememberMe: 2146615972, type=checkbox, renderer_id = 557
}

We would ideally label field 1050503534 as USERNAME_AND_EMAIL_ADDRESS and field 2051817934 as PASSWORD.

This won't fix the general problem, but marriott.com as a special case.

+cfroussios will your classifier fix this?

The FOAS classifier makes the correct predictions about this form.
https://plx.corp.google.com/scripts2/script_5b._7dbe84_0000_2a34_b20c_883d24fe24b8?p=FORM_SIGNATURE%3A17380206710487356981

Exciting! Then let's not override manually as the new classifier should launch very soon.

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

commit b365ac6502ca3836e404fb30c40d5842a9dfcc0b
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Sep 20 15:32:42 2018

Remove useless null check in AssemblePasswordForm

AssemblePasswordForm inside
components/password_manager/core/browser/form_parsing/form_parser.cc
currently gets a pointer to SignificantFields and checks it for being
null. However, it is never called with the pointer being null.

To clarify the code, this CL changes the AssemblePasswordForm
signature to expect a reference instead.

Bug: 883633
Change-Id: I2a81350836074ca60e925a2a0bd28b5fa0169281
Reviewed-on: https://chromium-review.googlesource.com/1235582
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592807}
[modify] https://crrev.com/b365ac6502ca3836e404fb30c40d5842a9dfcc0b/components/password_manager/core/browser/form_parsing/form_parser.cc

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

commit 2ff70d4f4799e9520959cdb1abb58cde384032e8
Author: Vaclav Brozek <vabr@chromium.org>
Date: Mon Sep 24 07:10:00 2018

Fix typos in PasswordForm parsing code

This CL collects typos encountered during some work on the new
PasswordForm parser.

Bug: 883633
Change-Id: I7243aca6afa66d65039d1f049a16e6ee04cc4f53
Reviewed-on: https://chromium-review.googlesource.com/1236335
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593483}
[modify] https://crrev.com/2ff70d4f4799e9520959cdb1abb58cde384032e8/components/password_manager/core/browser/form_parsing/form_parser.cc
[modify] https://crrev.com/2ff70d4f4799e9520959cdb1abb58cde384032e8/components/password_manager/core/browser/new_password_form_manager.cc

I have been chatting with dvadym@ about ignoring readonly fields, and neither of us could actually find a compelling reason to prevent filling into readonly password fields. The only known use-case, sites with virtual keyboards, is unlikely to work with password manager at all, so there will be either nothing saved to be filled, or the filled value will be overwritten by the site's scripts.

So we both think that the simplest next step locally is just to remove the code for ignoring readonly password fields. In that case I would not even land the UKM for readonly, which also seems less useful given that the FOAS classifier might produce useful server data for readonly-affected pages.

If somebody objects to Chrome filling also readonly password fields, please let me know.

Let's put this into a feature sync meeting. I am not particularly thrilled about "let's change without measuring".

Update after the last feature sync:
* Marriott and similar cases continue to be likely to be fixed by server-side improvements.
* I will prepare a UKM for detecting situations when Chrome ignores password fields because they are readonly.
* With the help of the data collected through UKM, we assess later whether we should just simplify the parser to not care about readonly at all.

Some more details about the last item:
For forms which only have readonly passwords, there is not so much concern, because either it's temporary and then eventually manual usage of those fields will create enough server-side information to fill automatically, or it's some kind of permanent issue (e.g., virtual keyboards) in which case the users are unlikely to have anything saved in PasswordStore in the first place.
The main concern is about forms which have a mix of readonly and non-readonly password fields. There a change in treating readonly might result in different parsing. Therefore we want to learn through UKM how frequent such cases are, and get a a few sample sites where this happens.

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

commit f3ab4805a15bb163d758efba2b6996f51df14b22
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Oct 04 15:38:19 2018

Turn PasswordForm parser into a class

The FormData -> PasswordForm parser has been just a function so far.
However, an increasing amount of metadata is being handled
(predictions, soon also statistics about the parsing such as whether
readonly fields were observed).

Therefore this CL encapsulates such optional data into a parser class.
Notable changes:
  * Predictions are set separately and can be reused for subsequent
    parsing runs. This also avoids passing nullptr for missing
    predictions.
  * Some names enclosed in the class have been shortened to avoid
    redundancy when presented with the class prefix.

Bug: 883633

Change-Id: Ib36d37e134d1abff644de2f167e891bd88735f37
Reviewed-on: https://chromium-review.googlesource.com/c/1236315
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596683}
[modify] https://crrev.com/f3ab4805a15bb163d758efba2b6996f51df14b22/components/password_manager/core/browser/form_parsing/form_parser.cc
[modify] https://crrev.com/f3ab4805a15bb163d758efba2b6996f51df14b22/components/password_manager/core/browser/form_parsing/form_parser.h
[modify] https://crrev.com/f3ab4805a15bb163d758efba2b6996f51df14b22/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
[modify] https://crrev.com/f3ab4805a15bb163d758efba2b6996f51df14b22/components/password_manager/core/browser/form_parsing/fuzzer/form_parser_generic_fuzzer.cc
[modify] https://crrev.com/f3ab4805a15bb163d758efba2b6996f51df14b22/components/password_manager/core/browser/form_parsing/fuzzer/form_parser_proto_generic_fuzzer.cc
[modify] https://crrev.com/f3ab4805a15bb163d758efba2b6996f51df14b22/components/password_manager/core/browser/new_password_form_manager.cc
[modify] https://crrev.com/f3ab4805a15bb163d758efba2b6996f51df14b22/components/password_manager/core/browser/new_password_form_manager.h
[modify] https://crrev.com/f3ab4805a15bb163d758efba2b6996f51df14b22/components/password_manager/core/browser/new_password_form_manager_unittest.cc

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

commit 845ecd2b79f187f227f79ced20885f3f1340334e
Author: Vaclav Brozek <vabr@chromium.org>
Date: Mon Oct 08 14:51:07 2018

Report readonly password fields with UKM

When Chrome parses FormData of password forms into PasswordForm
structures, it omits, under certain conditions, password fields marked
as readonly. To understand the potential impact of removing this
special handling, this CL adds a UKM metric to report when this
occurs. This will provide both the frequency and some sample sites to
understand the usage of readonly password fields.

Privacy review of this metric:
The whole UKM event "PasswordForm" has been reviewed in crbug.com/728707
and go/gkmnc, where the privacy TL agreed that adding new metrics for
this event is OK under certain conditions (see the linked doc). The
new twin metrics for the readonly attribute satisfy such conditions, in
the opinion of the CL author, and hence are covered by that review. The
new metrics have been added to the requested spreadsheet listing all
passwords-related metrics.

Choice of metrics:
The core of the metrics is the ReadonlyPasswordFields enum, which
describes different situations which can happen wrt. readonly fields
during parsing. Those are in the UKM combined with the bit indicating
whether the parsing overall was successful, because this is also
relevant to the issue.
Further, the metrics can be collected at two stages: when a form is
parsed to be filled, and when it is parsed to be saved. Here there
was a choice to either record this as an additional bit in one metric,
or create two "twin" metrics. The advantage of the former is the
ability to study how these two events correlate. The advantage of the
latter is easier reading of the particular metrics and a lower range
of values per metric. Because the correlation is unlikely to be of
significant use, the latter approach was chosen.

Bug: 883633
Change-Id: Ib63e52908ae3a257546646c1b255a6cd8fd48b6b
Reviewed-on: https://chromium-review.googlesource.com/c/1261984
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597561}
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/DEPS
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/form_parsing/form_parser.cc
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/form_parsing/form_parser.h
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/new_password_form_manager.cc
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/new_password_form_manager.h
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/new_password_form_manager_unittest.cc
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/password_form_metrics_recorder.cc
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/components/password_manager/core/browser/password_form_metrics_recorder.h
[modify] https://crrev.com/845ecd2b79f187f227f79ced20885f3f1340334e/tools/metrics/ukm/ukm.xml

With r597561 landing in 71, let's set an alarm for 7 November to see what it reports in beta. Following #19 above, the things to check for are:

* Frequency and examples of kSomeIgnored reported -- does Chrome misclassify password fields because of readonly?
* Frequency and examples of kSomeIgnored | kAllIgnored when parsing did not succeed -- are those the cases like marriott.com which work on saving and will get improved through server hints? Or are there new types of issues?

In the meantime I'm marking this as available and renaming the issue to reflect its more general status now. I'm happy to pick this up again at the NextAction date if nobody else actively works on it at that time.

The NextAction date has arrived: 2018-11-07

Hi Dominic,
Do you think you could help me obtain the results of the UKM:
  PasswordForm.ReadonlyWhenFilling
and
  PasswordForm.ReadonlyWhenSaving ?

I'd like to check what is outlined in #22.
Thanks!
Vaclav

Done: go/lvnov

I forgot that after the override mentioned in #11, handling readonly no longer influences marriott.com (because server hints are applied regardless of readonly).

The interesting bit is that fill on load still does not work, and fill on demand only shows up on the password field first (after shown once, it also starts showing on the username field).

This is not a regression, I observe the same with the old parser. But I don't understand the root cause yet. Will have a look.

In the meantime, I'll re-focus this bug on marriott.com and file a new one for readonly.

The bug on readonly is issue 905635.

Now I understand the issue with marriott.com:

The problem really is the readonly attribute of the password field, however, not during parsing but during filling. What happens is this:

(1) Chrome parses the form. Both the parsers arrive at the correct result (the new one because of the server override). Chrome knows which fields to fill.
(2) PasswordAutofillAgent tries to fill the form, but gives up, because the password field is readonly [1].

This code in PasswordAutofillAgent is used independently of the version of the parser -- that's why this is not a regression.

The current state means that fill on load will never work on readonly fields (manual fallback, filling on account select, did and will work).
If we consider this a bug, we need to stop restricting filling to fields which are editable.



[1] https://cs.chromium.org/chromium/src/components/autofill/content/renderer/password_autofill_agent.cc?l=350&rcl=e676b7b4c45de16791c3148da4214c7e88a1a4bd

The analysis is done.

Because this is not a regression, and my time on the team is ticking out, I am moving my focus elsewhere.

The next step is to make a product decision (see "If we consider this a bug" in #29 above), and depending on it either close as WontFix, or implement the suggested change.

One question that I asked myself a couple of times is whether it would make sense to fill more human-like. I.e. fill the username field character by character, and once the password field becomes visible / editable / available, fill the password field. It might even be a special case of username first flows.

Let's still discuss in a feature sync whether we see further cases where a change would be an improvement or regression.

vabr going hobby only -> reducing involvement.
Please contact me directly in urgent matters.

https://www.register.com/myaccount/productdisplay.rcmx is another example caused by field being read-only on pageload.