Track empty username and password in PasswordStore.

Track empty username and password in PasswordStore.

Password Manager shouldn't push empty pair username/password. We should track how it happens and how often.

BUG=404012
Committed: https://crrev.com/70272fbdd018c7cad24ff0f41706166e66f8a285
Cr-Commit-Position: refs/heads/master@{#291716}

[vasilii, 2014-08-21 16:19:59]

Hi Ilya,

we still don't know how we lost usernames and passwords. I don't think it was
the problem with the keychain because in this case we'd at least see the
usernames.
My guess is that it's Password Generation fault. Maybe it's specific to some
revision of Chrome.
If we generate a soft crash, we'll see the source of the bug (Sync or Password
Manager).

[Ilya Sherman, 2014-08-22 03:54:29]

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

https://codereview.chromium.org/493123002/diff/1/components/password_manager/...
components/password_manager/core/browser/login_database.cc:114: void
CheckEmptyUsernameAndPassword(const PasswordForm& form) {
nit: "Check" -> "CheckFor"

https://codereview.chromium.org/493123002/diff/1/components/password_manager/...
components/password_manager/core/browser/login_database.cc:116:
base::debug::DumpWithoutCrashing();
I don't really know how this code works.  According to the comment in the
header, you should first call SetDumpWithoutCrashingFunction().

What happens with the dump?  Does it get uploaded to the crash server?  Or does
it just remain local to the system that generated it?

[vasilii, 2014-08-22 09:32:25]

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

https://codereview.chromium.org/493123002/diff/1/components/password_manager/...
components/password_manager/core/browser/login_database.cc:114: void
CheckEmptyUsernameAndPassword(const PasswordForm& form) {
On 2014/08/22 03:54:29, Ilya Sherman wrote:
> nit: "Check" -> "CheckFor"

Done.

https://codereview.chromium.org/493123002/diff/1/components/password_manager/...
components/password_manager/core/browser/login_database.cc:116:
base::debug::DumpWithoutCrashing();
On 2014/08/22 03:54:29, Ilya Sherman wrote:
> I don't really know how this code works.  According to the comment in the
> header, you should first call SetDumpWithoutCrashingFunction().
> 
> What happens with the dump?  Does it get uploaded to the crash server?  Or
does
> it just remain local to the system that generated it?

This callback is a platform dependent Breakpad method. I copied this function
from browser_sync::ChromeReportUnrecoverableError() in sync. For sync it uploads
the report for sure.

[Ilya Sherman, 2014-08-23 00:08:51]

I'm still not convinced that we're using this API correctly.  The Sync code
looks like it's disabled by default -- the kErrorUploadRatio constant is set to
0.0.

Mark, it looks like you might be familiar with the DumpWithoutCrashing() code. 
Does this look like a correct use to you?

[vasilii, 2014-08-25 08:43:40]

It was changed to 0 recently https://codereview.chromium.org/470653003. And this
is a big and unpleasant surprise for me.

[Mark Seaborn, 2014-08-25 17:11:13]

On 22 August 2014 17:08, <isherman@chromium.org> wrote:

> I'm still not convinced that we're using this API correctly.  The Sync code
> looks like it's disabled by default -- the kErrorUploadRatio constant is
> set to
> 0.0.
>

You're referring
to chrome/browser/sync/glue/chrome_report_unrecoverable_error.cc?  How is
that related?  Are you mentioning it just because it's another caller of
base::debug::DumpWithoutCrashing()?


> Mark, it looks like you might be familiar with the DumpWithoutCrashing()
> code.
> Does this look like a correct use to you?


Yes, it looks OK.  You might want to test it locally by enabling crash
reporting and triggering a crash.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-08-25 17:12:38]

Thanks, Mark.  LGTM.

[vasilii, 2014-08-25 17:14:25]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-25 17:15:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/493123002/20001

[commit-bot: I haz the power, 2014-08-25 18:10:54]

Committed patchset #2 (20001) as 15c9a5fad3a4bf8f167006281d40345cb56b9b9c

[commit-bot: I haz the power, 2014-09-10 02:35:45]

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