Introduce SuppressedHTTPSFormFetcher.

Introduce SuppressedHTTPSFormFetcher.

SuppressedHTTPSFormFetcher is a helper used by FormFetcherImpl to fetch
credentials stored for the HTTPS counterpart of a non-secure (i.e HTTP) origin,
when the FormFetcherImpl itself is created for an HTTP origin. The suppressed
forms are fetched asynchronously, without blocking Consumer::ProcessMatches.

This data will be used to measure how often HTTPS credentials cannot be filled 
on HTTP sites. When no matching HTTP credentials exist for a non-secure origin,
but there are suppressed HTTPS credentials, that could indicate a premature
`move-to-HTTPS` migration, or simply that the site serves its sign-up or some
of its sign-in forms over HTTPS, while others still over HTTP.

BUG=720599
Review-Url: https://codereview.chromium.org/2878463003
Cr-Commit-Position: refs/heads/master@{#473725}
Committed: https://chromium.googlesource.com/chromium/src/+/70cb0b5541ede847097e9b0ce639f9b382e12d1a

[engedy, 2017-05-10 19:43:01]

Description was changed from

==========
Measure how often HTTPS credentials cannot be filled on HTTP sites.

BUG=720599
==========

to

==========
Measure how often HTTPS credentials cannot be filled on HTTP sites.

BUG=720599
==========

[engedy, 2017-05-10 19:43:01]

engedy@chromium.org changed reviewers:
+ dvadym@chromium.org, kolos@chromium.org

[engedy, 2017-05-10 19:50:35]

Maxim, Vadym, please take an initial look at the overall approach.

Recording just impressions seems not all that useful in itself, because without
a submission, we cannot tell if the user could have actually used the suppressed
HTTPS credential, or entered a completely different username/password. I think
it makes sense to record both.

Based on ActionsTakenV3, it seems that there is a non-negligible number of cases
when the password manager fills, but the user still overrides username and/or
password, therefore I think we should record whether there is a suppressed HTTPS
credential even when an HTTP credential exists and gets auto-filled, but then
gets overwritten by the user.

Finally, there are also some cases when the user fills but ultimately ends up
not submitting (or we cannot detect the submission). We should measure this too.

Hence the complexity. Happy to split this into multiple histograms if you feel
that the current one is too hard to digest.

[engedy, 2017-05-10 19:51:27]

> Recording just impressions seems not all that useful in itself, because
without
> a submission, we cannot tell if the user could have actually used the
suppressed
> HTTPS credential, or entered a completely different username/password. I think
> it makes sense to record both.

For clarity, I am talking about login form impressions on HTTP origin where we
do not fill even though we have a login stored for the HTTPS version of the
origin.

[engedy, 2017-05-10 19:52:24]

Also, I'll probably need to do some refactoring to test this properly, so tests
are coming, stay tuned. :

[dvadym, 2017-05-11 11:59:07]

Thanks Balazs, it looks great! Yeah, it's a little bit complicated, but having
more dimensions would allow us to aggregate by some of them and to get more
comprehensive data. So it's up to you to split or not to split into multiple
histograms.

I would prefer to know not only about not filling HTTPS credentials into HTTP
forms, but also vice versa (i.e. HTTP credentials into HTTPS forms). Of course
in that case we would need to make an additional password store request for
https pages as well, that could have performance implications for https pages
(and hopefully most sign-in pages are https). Balazs, Maxim, WDYT?

[engedy, 2017-05-11 12:53:05]

> I would prefer to know not only about not filling HTTPS credentials into HTTP
> forms, but also vice versa (i.e. HTTP credentials into HTTPS forms). Of course
> in that case we would need to make an additional password store request for
> https pages as well, that could have performance implications for https pages
> (and hopefully most sign-in pages are https). Balazs, Maxim, WDYT?

Actually, I looked at the migration code in more details, and it looks like we
automatically copy-migrate HTTP->HTTPS (and even move if the site sets HSTS). So
I don't expect to see any results there.

[kolos1, 2017-05-12 14:08:30]

On 2017/05/11 12:53:05, engedy wrote:
> > I would prefer to know not only about not filling HTTPS credentials into
HTTP
> > forms, but also vice versa (i.e. HTTP credentials into HTTPS forms). Of
course
> > in that case we would need to make an additional password store request for
> > https pages as well, that could have performance implications for https
pages
> > (and hopefully most sign-in pages are https). Balazs, Maxim, WDYT?
> 
> Actually, I looked at the migration code in more details, and it looks like we
> automatically copy-migrate HTTP->HTTPS (and even move if the site sets HSTS).
So
> I don't expect to see any results there.

Thanks Balazs for your systematic approach! The CL goes even beyond what we
planed as P0. I had little time to review it today, continue on Monday.

[kolos1, 2017-05-15 08:27:29]

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

https://codereview.chromium.org/2878463003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:312: if
(https_near_matches_.has_value() && !https_near_matches_->empty())
Have some questions about this block. Let's discuss it in person.

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

https://codereview.chromium.org/2878463003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:335:
kNearMatchingOriginNone,     // No near-matching stored form found.
What does "near" mean here? HTTP-HTTPS substitution or PSL-matching or a sort of
fuzzy matching?

https://codereview.chromium.org/2878463003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:350: //
CredentialEquivalence, and SubmitResult.
CredentialEquivalence => UserEnteredCredentialRelation

[engedy, 2017-05-19 13:20:28]

Patchset #5 (id:80001) has been deleted

[engedy, 2017-05-19 13:20:34]

Patchset #5 (id:100001) has been deleted

[engedy, 2017-05-19 13:22:08]

This CL now distinguishes between generated/manual passwords, and adds a few
more dimensions. PTAL while I finish the unit tests.

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

https://codereview.chromium.org/2878463003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:312: if
(https_near_matches_.has_value() && !https_near_matches_->empty())
On 2017/05/15 08:27:28, kolos1 wrote:
> Have some questions about this block. Let's discuss it in person.

Typo, sorry. :)

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

https://codereview.chromium.org/2878463003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:335:
kNearMatchingOriginNone,     // No near-matching stored form found.
On 2017/05/15 08:27:28, kolos1 wrote:
> What does "near" mean here? HTTP-HTTPS substitution or PSL-matching or a sort
of
> fuzzy matching?

I started using the term `suppressed account` consistently. Hopefully that's
cleaner.

https://codereview.chromium.org/2878463003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:350: //
CredentialEquivalence, and SubmitResult.
On 2017/05/15 08:27:28, kolos1 wrote:
> CredentialEquivalence => UserEnteredCredentialRelation 

N/A, reworked this dimension.

[kolos1, 2017-05-19 14:45:03]

Thanks Balazs. LGTM with few comments. Looking forward to review tests. 

Please also test the new code on real web pages.

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
File components/password_manager/core/browser/form_fetcher.h (right):

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
components/password_manager/core/browser/form_fetcher.h:53: // serves its
sign-up form over HTTPS, but some login forms still over HTTP.
FYI. one sign-in form might be over HTTPS, another - over HTTP. sign-up  =>
sign-in is one of possible cases.

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:279: // Same as
above, without the obsoleted 'Blacklisted' action.
Do we really need the duplicate?

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:344: //
Recorded when there a suppressed credential, but there is no submitted or
"there is"?

[engedy, 2017-05-19 22:39:54]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 22:40:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 23:26:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 23:26:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2017-05-22 08:19:10]

Actually, this has grown significantly thanks to the tests, let me split this
into two.

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
File components/password_manager/core/browser/form_fetcher.h (right):

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
components/password_manager/core/browser/form_fetcher.h:53: // serves its
sign-up form over HTTPS, but some login forms still over HTTP.
On 2017/05/19 14:45:02, kolos1 wrote:
> FYI. one sign-in form might be over HTTPS, another - over HTTP. sign-up  =>
> sign-in is one of possible cases.

Done.

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:279: // Same as
above, without the obsoleted 'Blacklisted' action.
On 2017/05/19 14:45:02, kolos1 wrote:
> Do we really need the duplicate?

Yes, the histogram is already too large. The obsoleted value would inflate it
further by 50%.

https://codereview.chromium.org/2878463003/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:344: //
Recorded when there a suppressed credential, but there is no submitted or
On 2017/05/19 14:45:03, kolos1 wrote:
> "there is"?

Done.

[engedy, 2017-05-22 09:37:10]

Description was changed from

==========
Measure how often HTTPS credentials cannot be filled on HTTP sites.

BUG=720599
==========

to

==========
Introduce SuppressedHTTPSFormFetcher.

SuppressedHTTPSFormFetcher is a helper instantiated for FormFetcherImpl's 
created for non-secure origins (i.e. the scheme of which is HTTP). It is
responsible for querying credentials stored for the HTTPS counterpart of that
origin, asynchronously, without blocking FormFetcher::Consumer::ProcessMatches.

This data will be used to measure how often HTTPS credentials cannot be filled 
on HTTP sites. When there exists no matching HTTP credentials for a non-secure
origin, but there are suppressed HTTPS credentials, that could indicate a 
premature `move-to-HTTPS` migration, or simply that the site serves its sign-up
or some of its sign-in forms over HTTPS, while others still over HTTP.

BUG=720599
==========

[engedy, 2017-05-22 09:37:47]

Maxim, Vadym, please take a final look.

[engedy, 2017-05-22 09:38:06]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 09:38:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-05-22 09:39:56]

Description was changed from

==========
Introduce SuppressedHTTPSFormFetcher.

SuppressedHTTPSFormFetcher is a helper instantiated for FormFetcherImpl's 
created for non-secure origins (i.e. the scheme of which is HTTP). It is
responsible for querying credentials stored for the HTTPS counterpart of that
origin, asynchronously, without blocking FormFetcher::Consumer::ProcessMatches.

This data will be used to measure how often HTTPS credentials cannot be filled 
on HTTP sites. When there exists no matching HTTP credentials for a non-secure
origin, but there are suppressed HTTPS credentials, that could indicate a 
premature `move-to-HTTPS` migration, or simply that the site serves its sign-up
or some of its sign-in forms over HTTPS, while others still over HTTP.

BUG=720599
==========

to

==========
Introduce SuppressedHTTPSFormFetcher.

SuppressedHTTPSFormFetcher is a helper created for FormFetcherImpls that are
themselves created for non-secure origins (i.e. the scheme of which is HTTP).
It is responsible for querying credentials stored for the HTTPS counterpart of
that origin, asynchronously, without blocking Consumer::ProcessMatches.

This data will be used to measure how often HTTPS credentials cannot be filled 
on HTTP sites. When there exists no matching HTTP credentials for a non-secure
origin, but there are suppressed HTTPS credentials, that could indicate a 
premature `move-to-HTTPS` migration, or simply that the site serves its sign-up
or some of its sign-in forms over HTTPS, while others still over HTTP.

BUG=720599
==========

[engedy, 2017-05-22 09:43:57]

Description was changed from

==========
Introduce SuppressedHTTPSFormFetcher.

SuppressedHTTPSFormFetcher is a helper created for FormFetcherImpls that are
themselves created for non-secure origins (i.e. the scheme of which is HTTP).
It is responsible for querying credentials stored for the HTTPS counterpart of
that origin, asynchronously, without blocking Consumer::ProcessMatches.

This data will be used to measure how often HTTPS credentials cannot be filled 
on HTTP sites. When there exists no matching HTTP credentials for a non-secure
origin, but there are suppressed HTTPS credentials, that could indicate a 
premature `move-to-HTTPS` migration, or simply that the site serves its sign-up
or some of its sign-in forms over HTTPS, while others still over HTTP.

BUG=720599
==========

to

==========
Introduce SuppressedHTTPSFormFetcher.

SuppressedHTTPSFormFetcher is a helper used by FormFetcherImpl to fetch
credentials stored for the HTTPS counterpart of a non-secure (i.e HTTP) origin,
when the FormFetcherImpl itself is created for an HTTP origin. The suppressed
forms are fetched asynchronously, without blocking Consumer::ProcessMatches.

This data will be used to measure how often HTTPS credentials cannot be filled 
on HTTP sites. When no matching HTTP credentials exist for a non-secure origin,
but there are suppressed HTTPS credentials, that could indicate a premature
`move-to-HTTPS` migration, or simply that the site serves its sign-up or some
of its sign-in forms over HTTPS, while others still over HTTP.

BUG=720599
==========

[dvadym, 2017-05-22 09:58:50]

LGTM, thanks!

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
File components/password_manager/core/browser/suppressed_https_form_fetcher.cc
(right):

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
components/password_manager/core/browser/suppressed_https_form_fetcher.cc:41:
return form->is_public_suffix_match || form->is_affiliation_based_match;
Just wondering, why do we want do exclude PSL matches?

[engedy, 2017-05-22 10:32:28]

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
File components/password_manager/core/browser/suppressed_https_form_fetcher.cc
(right):

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
components/password_manager/core/browser/suppressed_https_form_fetcher.cc:41:
return form->is_public_suffix_match || form->is_affiliation_based_match;
On 2017/05/22 09:58:50, dvadym wrote:
> Just wondering, why do we want do exclude PSL matches?

PSL matches will be included inherently in the other histogram I'm planning to
add (about same site-name matches), so I thought that I'd exclude them here to
make the histograms disjoint, but I don't feel strongly about this and happy to
include them here as well.

[commit-bot: I haz the power, 2017-05-22 11:20:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 11:20:10]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-05-22 12:35:07]

LGTM

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
File components/password_manager/core/browser/form_fetcher_impl_unittest.cc
(right):

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:599: 
Could we split this test into 3 test?

[dvadym, 2017-05-22 12:42:26]

Still LGTM

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
File components/password_manager/core/browser/suppressed_https_form_fetcher.cc
(right):

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
components/password_manager/core/browser/suppressed_https_form_fetcher.cc:41:
return form->is_public_suffix_match || form->is_affiliation_based_match;
On 2017/05/22 10:32:28, engedy wrote:
> On 2017/05/22 09:58:50, dvadym wrote:
> > Just wondering, why do we want do exclude PSL matches?
> 
> PSL matches will be included inherently in the other histogram I'm planning to
> add (about same site-name matches), so I thought that I'd exclude them here to
> make the histograms disjoint, but I don't feel strongly about this and happy
to
> include them here as well

Acknowledged, I'm ok with separate histograms.

[engedy, 2017-05-22 20:20:19]

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
File components/password_manager/core/browser/form_fetcher_impl_unittest.cc
(right):

https://codereview.chromium.org/2878463003/diff/200001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:599: 
On 2017/05/22 12:35:07, kolos1 wrote:
> Could we split this test into 3 test?

Done. I also introduced two helper methods to cut down the redundancy here.

[engedy, 2017-05-22 20:20:37]

The CQ bit was checked by engedy@chromium.org

[engedy, 2017-05-22 20:20:38]

The patchset sent to the CQ was uploaded after l-g-t-m from kolos@chromium.org,
dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/2878463003/#ps240001
(title: "Polish.")

[commit-bot: I haz the power, 2017-05-22 20:21:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 22:23:22]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495484437627300,
"parent_rev": "56f8007e48b48058ce86712bde36107fc79e0c5d", "commit_rev":
"70cb0b5541ede847097e9b0ce639f9b382e12d1a"}

[commit-bot: I haz the power, 2017-05-22 22:23:32]

Description was changed from

==========
Introduce SuppressedHTTPSFormFetcher.

SuppressedHTTPSFormFetcher is a helper used by FormFetcherImpl to fetch
credentials stored for the HTTPS counterpart of a non-secure (i.e HTTP) origin,
when the FormFetcherImpl itself is created for an HTTP origin. The suppressed
forms are fetched asynchronously, without blocking Consumer::ProcessMatches.

This data will be used to measure how often HTTPS credentials cannot be filled 
on HTTP sites. When no matching HTTP credentials exist for a non-secure origin,
but there are suppressed HTTPS credentials, that could indicate a premature
`move-to-HTTPS` migration, or simply that the site serves its sign-up or some
of its sign-in forms over HTTPS, while others still over HTTP.

BUG=720599
==========

to

==========
Introduce SuppressedHTTPSFormFetcher.

SuppressedHTTPSFormFetcher is a helper used by FormFetcherImpl to fetch
credentials stored for the HTTPS counterpart of a non-secure (i.e HTTP) origin,
when the FormFetcherImpl itself is created for an HTTP origin. The suppressed
forms are fetched asynchronously, without blocking Consumer::ProcessMatches.

This data will be used to measure how often HTTPS credentials cannot be filled 
on HTTP sites. When no matching HTTP credentials exist for a non-secure origin,
but there are suppressed HTTPS credentials, that could indicate a premature
`move-to-HTTPS` migration, or simply that the site serves its sign-up or some
of its sign-in forms over HTTPS, while others still over HTTP.

BUG=720599

Review-Url: https://codereview.chromium.org/2878463003
Cr-Commit-Position: refs/heads/master@{#473725}
Committed:
https://chromium.googlesource.com/chromium/src/+/70cb0b5541ede847097e9b0ce639...
==========

[commit-bot: I haz the power, 2017-05-22 22:23:34]

Committed patchset #11 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/70cb0b5541ede847097e9b0ce639...