Report the number of empty usernames without a nonempty pair.

Report the number of empty usernames without a nonempty pair.

In the process of investigation of empty username occurences in the password manager, we suspect re-authentication forms to be a frequent cause. This metric will show us in how many cases this is not the cause.

We interpret occurences of an empty and nonempty username pair on the same signon realm as a presence of both a first-time authentication and re-authentication form.

We report the number of password forms with an empty username in the login database, for which there isn't another password form with a nonempty username on the same signon realm.

Out of several possible approaches:
a) SELECT ... WHERE NOT EXISTS
b) SELECT DISTINCT ... FROM ... JOIN ... ON a.signon_realm=b.signon_realm
c) SELECT DISTINCT ... FROM ... WHERE ... AND a.signon_realm=b.signon_realm
d) SELECT, put into std::set<>, SELECT, iterate and search the set

we implemented a), as manual testing on 1000 random samples seemed to be the fastest.

BUG=456728
Committed: https://crrev.com/e8ca3576174bfa8d4c5ee594fd3f1d0dee62d9a3
Cr-Commit-Position: refs/heads/master@{#325451}

[msramek, 2015-04-15 14:49:55]

msramek@chromium.org changed reviewers:
+ engedy@chromium.org, vasilii@chromium.org

[msramek, 2015-04-15 14:49:56]

Hi Vasilii and Balázs,

could you PTAL?

Thanks,
Martin

[msramek, 2015-04-15 14:55:01]

msramek@chromium.org changed reviewers:
+ isherman@chromium.org

[msramek, 2015-04-15 14:55:01]

+ Ilya, as owner, PTAL.

[vasilii, 2015-04-15 15:24:38]

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

[commit-bot: I haz the power, 2015-04-15 15:24:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087233002/1

[engedy, 2015-04-15 15:29:26]

LGTM % minor comments, thanks!

I suggest that we rephrase the description of the CL as well with the
terminology suggested below.

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
components/password_manager/core/browser/login_database_unittest.cc:1063:
EXPECT_EQ(AddChangeForForm(password_form), db().AddLogin(password_form));
Just to be on the safe side, could you please add another one with an empty
username for this domain? To make it clear how/that multiplicities are correctly
handled.

https://codereview.chromium.org/1087233002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1087233002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:25310: +<histogram
name="PasswordManager.EmptyUsernames.NotPairedWithNonempty">
I think the concept of "pairing" is a bit unclear here. How about
.WithoutCorrespondingNonempty?

https://codereview.chromium.org/1087233002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:25315: +    there isn't another password
form from the same realm with a nonempty
nit: s/isn't/is not/

[commit-bot: I haz the power, 2015-04-15 15:34:22]

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

[commit-bot: I haz the power, 2015-04-15 15:34:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vasilii, 2015-04-15 15:51:55]

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
components/password_manager/core/browser/login_database.cc:511: "AND NOT EXISTS
(SELECT * FROM logins b "
I'd reverse the condition as "There is a corresponding credential with non-empty
username". Thus, it's definitely a bug in the password manager. While a
standalone empty username can mean anything.

[Ilya Sherman, 2015-04-15 22:56:05]

LGTM % nit:

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
components/password_manager/core/browser/login_database.cc:517:
"NotPairedWithNonempty",
nit: Rather than splitting this string across multiple lines, please just wrap
the arguments, i.e.

    UMA_HISTOGRAM_COUNTS_100(
      "PasswordManager.EmptyUsernames.NotPairedWithNonempty", num_entries);

[msramek, 2015-04-16 14:34:27]

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
components/password_manager/core/browser/login_database.cc:511: "AND NOT EXISTS
(SELECT * FROM logins b "
On 2015/04/15 15:51:55, vasilii wrote:
> I'd reverse the condition as "There is a corresponding credential with
non-empty
> username". Thus, it's definitely a bug in the password manager. While a
> standalone empty username can mean anything.

Well, that was my original intention. But then I realized that the histograms
CountInDatabase and "PairedWithNonempty" will not be well readable together.
Sure, if "PairedWithNonempty" has almost the same distribution as
CountInDatabase, we know that this is the overwhelming cause. But if it's, for
example,

CountInDatabase PairedWithNonempty
2: 1 user       1: 1 user  
3: 1 user       2: 1 user  

we won't be able to tell if only one or both of the people encountered an empty
password form.

I think this solution gives us a more interesting information, "How many empty
usernames (per user) are there, that we do not know the cause for yet?"

and the goal of this investigation is to define a histogram that has flat
zeroes.

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
components/password_manager/core/browser/login_database.cc:517:
"NotPairedWithNonempty",
On 2015/04/15 22:56:05, Ilya Sherman wrote:
> nit: Rather than splitting this string across multiple lines, please just wrap
> the arguments, i.e.
> 
>     UMA_HISTOGRAM_COUNTS_100(
>       "PasswordManager.EmptyUsernames.NotPairedWithNonempty", num_entries);

Done.

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
components/password_manager/core/browser/login_database_unittest.cc:1063:
EXPECT_EQ(AddChangeForForm(password_form), db().AddLogin(password_form));
On 2015/04/15 15:29:26, engedy wrote:
> Just to be on the safe side, could you please add another one with an empty
> username for this domain? To make it clear how/that multiplicities are
correctly
> handled.

Since CountInDatabase counts +1 for every entry, without looking for
duplicities, so should NotPairedWithNonempty.

However, note that since for this domain there *exists* a nonempty username
entry, adding another empty username entry won't change the count (which is 1,
coming from fourth.example.com.

https://codereview.chromium.org/1087233002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1087233002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:25310: +<histogram
name="PasswordManager.EmptyUsernames.NotPairedWithNonempty">
On 2015/04/15 15:29:26, engedy wrote:
> I think the concept of "pairing" is a bit unclear here. How about
> .WithoutCorrespondingNonempty?

Sure. Naming is always the hardest.

https://codereview.chromium.org/1087233002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:25315: +    there isn't another password
form from the same realm with a nonempty
On 2015/04/15 15:29:26, engedy wrote:
> nit: s/isn't/is not/

Done.

[engedy, 2015-04-16 14:44:32]

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/1087233002/diff/1/components/password_manager...
components/password_manager/core/browser/login_database_unittest.cc:1063:
EXPECT_EQ(AddChangeForForm(password_form), db().AddLogin(password_form));
On 2015/04/16 14:34:26, msramek wrote:
> On 2015/04/15 15:29:26, engedy wrote:
> > Just to be on the safe side, could you please add another one with an empty
> > username for this domain? To make it clear how/that multiplicities are
> correctly
> > handled.
> 
> Since CountInDatabase counts +1 for every entry, without looking for
> duplicities, so should NotPairedWithNonempty.
> 
> However, note that since for this domain there *exists* a nonempty username
> entry, adding another empty username entry won't change the count (which is 1,
> coming from http://fourth.example.com.

Yeah, I wanted to add this comment to "fourth", but on second thought it does
not make much sense. However, if you go with the negated solution as suggested
by Vasilii, please add another nonempty username for "sixth" to make sure the
one empty still only counts as one. (This is mostly blackbox testing. It would
be needed for a JOIN-based solution.)

[vasilii, 2015-04-16 14:57:02]

lgtm

[msramek, 2015-04-16 14:59:58]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-04-16 14:59:59]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1087233002/#ps20001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2015-04-16 15:00:09]

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

[commit-bot: I haz the power, 2015-04-16 16:10:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-16 16:11:50]

Patchset 2 (id:??) landed as
https://crrev.com/e8ca3576174bfa8d4c5ee594fd3f1d0dee62d9a3
Cr-Commit-Position: refs/heads/master@{#325451}