New issue
Advanced search Search tips

Issue 840384 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature

Blocking:
issue 847793
issue 838979



Sign in to add a comment

Implement clientside vote collection for password filling

Project Member Reported by cfroussios@chromium.org, May 7 2018

Issue description

Issue 838979 requires additional votes to be collected on login forms.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 16 2018

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

commit 75e1af27a08d78b0c0c36b1d6fed9795461db5f7
Author: Christos Froussios <cfroussios@chromium.org>
Date: Wed May 16 14:56:56 2018

[Password Manager] Refactor PasswordFormManagerTest vote tests

Our current matchers fail the test if they don't match. This is against
the formal definition of matchers, which are supposed to be side-effect-
free. It also prevents us form setting multiple expectations per test,
which will be needed for  crbug.com/840384 .

I've refactored the matchers to not fail the test. I've also split the
matchers and added matching result messages, to keep the explanatory
value of error messages.

Bug:  840384 
Change-Id: Ie96af6230e9c165b2ce21d1d6930f3a0b3f3b68b
Reviewed-on: https://chromium-review.googlesource.com/1055393
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559091}
[modify] https://crrev.com/75e1af27a08d78b0c0c36b1d6fed9795461db5f7/components/password_manager/core/browser/password_form_manager_unittest.cc

Comment 2 by kolos@chromium.org, May 22 2018

Cc: kolos@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, May 23 2018

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

commit 37321b3ac91b580d62d52d8a03d691c1c2561a9c
Author: Christos Froussios <cfroussios@chromium.org>
Date: Wed May 23 14:10:35 2018

[Password Manager] Autofill vote on first use

The first time that a credential is used by the user to login, we will
upload a USERNAME and PASSWORD crowdsourcing vote. The votes will be
marked by VoteType::FIRST_USE. UsernameVoteType was repurposed to
VoteType to allow it to be used for password votes too.

We've also introduced a new flag, which is applied to the new vote. The
FieldPropertiesFlags::KNOWN_VALUE indicates that the value, which was
seen on the field on submission, matches a previously stored credential.

TODOs:
* If the credential does not have a username, repeat the FIRST_USE vote
as soon as the user updates it with a username.
* Metrics

Bug:  840384 
Change-Id: I16a9dc9c3c9bbb0d5420a33d6147cb3ed8071978
Reviewed-on: https://chromium-review.googlesource.com/1047268
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561064}
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/autofill/core/browser/autofill_field.cc
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/autofill/core/browser/autofill_field.h
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/autofill/core/browser/form_structure.cc
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/autofill/core/browser/form_structure_unittest.cc
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/autofill/core/browser/proto/server.proto
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/autofill/core/common/form_field_data.h
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/password_manager/core/browser/browser_save_password_progress_logger.cc
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/password_manager/core/browser/password_form_manager.cc
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/password_manager/core/browser/password_form_manager.h
[modify] https://crrev.com/37321b3ac91b580d62d52d8a03d691c1c2561a9c/components/password_manager/core/browser/password_form_manager_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 23 2018

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

commit 1c156e81bb3b4dad5eeb5f75888f0fffecee48ea
Author: Christos Froussios <cfroussios@chromium.org>
Date: Wed May 23 17:44:51 2018

[Password Manager] Repeat first login vote when a username is added

If a credential does not contain a username, e.g because password manager
cannot save the username in the corresponding signup form, then the
crowdsourcing vote on the first login will not contain info about the
username.

To cover this blindspot, we will repeat the vote if and when the user
updates the credential adding a username.

Bug:  840384 
Change-Id: I9ed6b485d9d2446d8cd291fb2ee16784e432e611
Reviewed-on: https://chromium-review.googlesource.com/1064631
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561147}
[modify] https://crrev.com/1c156e81bb3b4dad5eeb5f75888f0fffecee48ea/components/password_manager/core/browser/password_form_manager.cc
[modify] https://crrev.com/1c156e81bb3b4dad5eeb5f75888f0fffecee48ea/components/password_manager/core/browser/password_form_manager_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, May 25 2018

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

commit 13ececd77199814d8f36149db50f30329f743516
Author: Christos Froussios <cfroussios@chromium.org>
Date: Fri May 25 14:34:42 2018

[Password Manager] Don't upload FIRST_USE vote for new passwords

When a credential is used for the first time, we upload a FIRST_USE
crowdsourcing vote, voting the fields as USERNAME and PASSWORD, even if
the use results in a new password value being saved. This was to be
resilient against cases where the password manager saves the wrong
password in the original form. However, it is semantically incorrect,
both in the above case and also when applied to change-password forms.

We change the vote so that we don't upload a vote for the password
field.

This does not apply to usernames, because a new username is treated as
a new credential. In the case where we add a username to an old
credential, we keep setting the KNOWN_VALUE flag, because this event
only happens as a correction and doesn't say something different about
the field. Also, the fact that the user accepted the prompt is a strong
indication that the field contains a proper username.

Bug:  840384 
Change-Id: Iff20a022abc50a4dab2f32f46e13e33621eb6129
Reviewed-on: https://chromium-review.googlesource.com/1071523
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561870}
[modify] https://crrev.com/13ececd77199814d8f36149db50f30329f743516/components/password_manager/core/browser/password_form_manager.cc
[modify] https://crrev.com/13ececd77199814d8f36149db50f30329f743516/components/password_manager/core/browser/password_form_manager_unittest.cc

Labels: Merge-Request-68
Can we merge r561870 into M68? It matters for the quality of new crowdsourcing votes, which we started collecting in M68. It has been on dev for 3 days.
Project Member

Comment 7 by bugdroid1@chromium.org, May 28 2018

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

commit 9f17f15e43226a2bb339490fbd84416a32537002
Author: Christos Froussios <cfroussios@chromium.org>
Date: Mon May 28 17:25:15 2018

[Password Manager] Remove stale todo

The described task for resolved with
https://chromium-review.googlesource.com/c/chromium/src/+/1064631

TBR=kolos@chromium.org

Bug:  840384 
Change-Id: Ieedcb1568ac556091c5d587ab0c682faac49f4c7
Reviewed-on: https://chromium-review.googlesource.com/1075587
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562283}
[modify] https://crrev.com/9f17f15e43226a2bb339490fbd84416a32537002/components/password_manager/core/browser/password_form_manager.cc

Project Member

Comment 8 by sheriffbot@chromium.org, May 29 2018

Labels: -Merge-Request-68 Hotlist-Merge-Approved Merge-Approved-68
Your change meets the bar and is auto-approved for M68. Please go ahead and merge the CL to branch 3440 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 9 by bugdroid1@chromium.org, May 29 2018

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58e85bfd8ffba913284fd65ed719b3f618461942

commit 58e85bfd8ffba913284fd65ed719b3f618461942
Author: Christos Froussios <cfroussios@chromium.org>
Date: Tue May 29 17:45:54 2018

[Password Manager] Don't upload FIRST_USE vote for new passwords

When a credential is used for the first time, we upload a FIRST_USE
crowdsourcing vote, voting the fields as USERNAME and PASSWORD, even if
the use results in a new password value being saved. This was to be
resilient against cases where the password manager saves the wrong
password in the original form. However, it is semantically incorrect,
both in the above case and also when applied to change-password forms.

We change the vote so that we don't upload a PASSWORD vote for the
new password field.

This does not apply to usernames, because a new username is treated as
a new credential. In the case where we add a username to an old
credential, we keep setting the KNOWN_VALUE flag, because this event
only happens as a correction and doesn't say something different about
the field. Also, the fact that the user accepted the prompt is a strong
indication that the field contains a proper username.

Bug:  840384 
Change-Id: Iff20a022abc50a4dab2f32f46e13e33621eb6129
Reviewed-on: https://chromium-review.googlesource.com/1071523
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#561870}(cherry picked from commit 13ececd77199814d8f36149db50f30329f743516)
Reviewed-on: https://chromium-review.googlesource.com/1076550
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#24}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/58e85bfd8ffba913284fd65ed719b3f618461942/components/password_manager/core/browser/password_form_manager.cc
[modify] https://crrev.com/58e85bfd8ffba913284fd65ed719b3f618461942/components/password_manager/core/browser/password_form_manager_unittest.cc

Blocking: 847793
Status: Fixed (was: Assigned)

Sign in to add a comment