Reland: Cache protected password entry and password on focus ping separately.

Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942
>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed: >https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#479221}
>Committed: >https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e1376>0ed9d9542ccfea3c361

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#480695}
Committed: https://chromium.googlesource.com/chromium/src/+/f1b5b08b61b3f3f23d2f6bd82339b20a7141603a

[Jialiu Lin, 2017-05-31 01:14:17]

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

[commit-bot: I haz the power, 2017-05-31 01:15:18]

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-31 02:45:28]

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

[commit-bot: I haz the power, 2017-05-31 02:45:29]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Jialiu Lin, 2017-05-31 18:49:52]

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

[commit-bot: I haz the power, 2017-05-31 18:50:56]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-05-31 18:55:06]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

BUG=727942
==========

to

==========
Cache protected password entry and password on focus ping separately.

This means splitting CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION into 2 settings,
CONTENT_SETTINGS_TYPE_PROTECTED_PASSWORD_ENTRY and
CONTENT_SETTINGS_TYPE_PASSWORD_ON_FOCUS.

CONTENT_SETTINGS_TYPE_PROTECTED_PASSWORD_ENTRY type will inherit the
"password-protection" string in registry.

BUG=727942
==========

[Jialiu Lin, 2017-05-31 18:55:44]

jialiul@chromium.org changed reviewers:
+ raymes@chromium.org

[Jialiu Lin, 2017-05-31 18:55:45]

+raymes@, could you review all the content_settings related change? Thanks!

[commit-bot: I haz the power, 2017-05-31 21:29:39]

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

[commit-bot: I haz the power, 2017-05-31 21:29:40]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-05-31 23:16:48]

Thanks Jialiu

https://codereview.chromium.org/2911293003/diff/20001/components/content_sett...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/2911293003/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings_types.h:68:
CONTENT_SETTINGS_TYPE_PROTECTED_PASSWORD_ENTRY,
This is a dictionary - would it be possible to use 2 entries in the same
dictionary rather than 2 content settings? We try to do this whenever possible
to avoid the overhead of extra content settings.

[Jialiu Lin, 2017-06-02 01:54:42]

https://codereview.chromium.org/2911293003/diff/20001/components/content_sett...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/2911293003/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings_types.h:68:
CONTENT_SETTINGS_TYPE_PROTECTED_PASSWORD_ENTRY,
On 2017/05/31 23:16:48, raymes wrote:
> This is a dictionary - would it be possible to use 2 entries in the same
> dictionary rather than 2 content settings? We try to do this whenever possible
> to avoid the overhead of extra content settings.
Hi raymes, 
I tried to use secondary_url to distinguish these two while keeping a single
content setting type.
e.g.
For a given origin foo.com, I cached the protected password entry verdicts using
"foo.com" as primary url, and GURL() as secondary url; and for password on focus
verdicts I cached them by using "foo.com" as primary and "foo.com" as secondary.

But it seems they are still cached together. I might have some misunderstanding
of how to use secondary url.

[raymes, 2017-06-02 02:02:26]

On 2017/06/02 01:54:42, Jialiu Lin wrote:
>
https://codereview.chromium.org/2911293003/diff/20001/components/content_sett...
> File components/content_settings/core/common/content_settings_types.h (right):
> 
>
https://codereview.chromium.org/2911293003/diff/20001/components/content_sett...
> components/content_settings/core/common/content_settings_types.h:68:
> CONTENT_SETTINGS_TYPE_PROTECTED_PASSWORD_ENTRY,
> On 2017/05/31 23:16:48, raymes wrote:
> > This is a dictionary - would it be possible to use 2 entries in the same
> > dictionary rather than 2 content settings? We try to do this whenever
possible
> > to avoid the overhead of extra content settings.
> Hi raymes, 
> I tried to use secondary_url to distinguish these two while keeping a single
> content setting type.
> e.g.
> For a given origin http://foo.com, I cached the protected password entry
verdicts using
> "foo.com" as primary url, and GURL() as secondary url; and for password on
focus
> verdicts I cached them by using "foo.com" as primary and "foo.com" as
secondary.
> 
> But it seems they are still cached together. I might have some
misunderstanding
> of how to use secondary url.

Hey Jialiu! Sorry if I wasn't clear, I actually wasn't suggesting using the
secondary URL. Instead I was suggesting possibly having two entries in the
base::DictionaryValue. Content Settings stores base::DictionaryValues under the
hood (this is what is passed to SetWebsiteSetting). These can have multiple
key/value pairs. So could you have 2 key/value pairs - one for each setting?
Each sub-value can itself be a DictionaryValue, so you might be able to store
whatever is needed in there. Sometimes a small amount of migration code is
needed to update existing data.

[Jialiu Lin, 2017-06-05 23:57:46]

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

[commit-bot: I haz the power, 2017-06-05 23:58:31]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-06-06 00:59:55]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

This means splitting CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION into 2 settings,
CONTENT_SETTINGS_TYPE_PROTECTED_PASSWORD_ENTRY and
CONTENT_SETTINGS_TYPE_PASSWORD_ON_FOCUS.

CONTENT_SETTINGS_TYPE_PROTECTED_PASSWORD_ENTRY type will inherit the
"password-protection" string in registry.

BUG=727942
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type to cache
verdict. Since password on focus verdicts (a.k.a UNFAMILIAR_LOGIN_PAGE ping
verdict) only used by extended reporting users, we group them under
kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT verdict for
all users) remain the same.
In this way we don't need extra migration code to handle existing verdicts.

BUG=727942
==========

[Jialiu Lin, 2017-06-06 01:05:45]

Hi raymes@,
Thanks for your feedback. I have changed my implementation to only using a
single content settings type. CL description updated accordingly too. 

Please take a look at the small change in
components/content_settings/core/common/content_settings.cc paired with
tools/metrics/histograms/enums.xml

Thank,

[raymes, 2017-06-06 01:51:40]

Thanks Jialiu!

content_settings lgtm. I haven't looked at any of the code for reading
to/writing from the content setting though and making sure that old settings
continue to work properly.

Also, just to make sure, do we clear these settings with Clear Browsing Data (or
at some other point?). If not that might be something to check with privacy
about.

[Jialiu Lin, 2017-06-06 02:21:23]

On 2017/06/06 01:51:40, raymes wrote:
> Thanks Jialiu!
> 
> content_settings lgtm. I haven't looked at any of the code for reading
> to/writing from the content setting though and making sure that old settings
> continue to work properly.
> 
> Also, just to make sure, do we clear these settings with Clear Browsing Data
(or
> at some other point?). If not that might be something to check with privacy
> about.

Thanks Raymes!
Yes, settings are cleared when user clear browsing history.

[Jialiu Lin, 2017-06-06 02:22:42]

jialiul@chromium.org changed reviewers:
+ isherman@chromium.org, nparker@chromium.org, vakh@chromium.org

[Jialiu Lin, 2017-06-06 02:22:42]

+vakh@ and nparker@ for safe_browsing related change 
+isherman@ for histograms.xml

[Ilya Sherman, 2017-06-06 05:41:44]

Metrics LGTM

[Jialiu Lin, 2017-06-07 21:16:09]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type to cache
verdict. Since password on focus verdicts (a.k.a UNFAMILIAR_LOGIN_PAGE ping
verdict) only used by extended reporting users, we group them under
kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT verdict for
all users) remain the same.
In this way we don't need extra migration code to handle existing verdicts.

BUG=727942
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a UNFAMILIAR_LOGIN_PAGE
ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942
==========

[Jialiu Lin, 2017-06-07 21:19:16]

jialiul@chromium.org changed reviewers:
+ lpz@chromium.org
- nparker@chromium.org

[Jialiu Lin, 2017-06-07 21:19:17]

-nparker@  +lpz@ to balance the review load.

[lpz, 2017-06-08 15:18:34]

Looks good aside from some nits and some clarifying questions. Thanks!

https://codereview.chromium.org/2911293003/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2911293003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:78:
LoginReputationClientRequest::TriggerType type,
nit: verdict_type or trigger_type?

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.cc
(right):

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:86:
main_frame_url_, request_type(), cached_response.get());
nit: request_type_ for consistency with the rest of the code?

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
meta comment: this might be just my ignorance, but I'm still unclear about how
the two trigger types are stored. Is one of them a "child" of the other, or are
they contained together in a flat list but the "on focus" verdicts are just
tagged with the new key?

A comment with an example somewhere in here could help. Thanks!

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:434:
std::unique_ptr<base::DictionaryValue> verdict_dictionary =
should this be "cache_dictionary" for consistency with other code? If so, maybe
check the rest of this code for that distinction?

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:438:
if (verdict_dictionary.get() && !verdict_dictionary->empty()) {
I think this block could use a couple comments (ie: the outer dict size contains
both types of verdicts and we want to split them apart...is that right?)

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:582:
return false;
nit: +1 newline after

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:588:
continue;
nit: +1 newline after

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.h
(right):

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.h:112:
TriggerType type);
nit: type -> trigger_type or verdict_type or request_type everywhere in this CL

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.h:156:
// Gets the total number of verdicts of the specified |type| (no matter
nit: i find the wording a bit awkward, consider:

Gets the total number of verdicts of the specified |type| we cached for the
current active profile. This counts both expired and active verdicts.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.h:262:
int stored_verdict_count_password_entry_;
do you foresee having other verdict types that will need to be counted? If so,
perhaps store these in a map keyed by TriggerType?

[Jialiu Lin, 2017-06-08 20:29:04]

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

[commit-bot: I haz the power, 2017-06-08 20:29:33]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-06-08 20:47:31]

Thanks lpz!

https://codereview.chromium.org/2911293003/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2911293003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:78:
LoginReputationClientRequest::TriggerType type,
On 2017/06/08 15:18:33, lpz wrote:
> nit: verdict_type or trigger_type?

Changed to trigger_type.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.cc
(right):

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:86:
main_frame_url_, request_type(), cached_response.get());
On 2017/06/08 15:18:33, lpz wrote:
> nit: request_type_ for consistency with the rest of the code?

Done.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/06/08 15:18:33, lpz wrote:
> meta comment: this might be just my ignorance, but I'm still unclear about how
> the two trigger types are stored. Is one of them a "child" of the other, or
are
> they contained together in a flat list but the "on focus" verdicts are just
> tagged with the new key?
> 
> A comment with an example somewhere in here could help. Thanks!

Yes, UMFAMILIAR_LOGIN_PAGE(a.k.a password on focus ping) verdict is cached as a
child of PASSWORD_REUSE_EVENT(a.k.a protected password entry ping). I'll add
some more comments and an example.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:434:
std::unique_ptr<base::DictionaryValue> verdict_dictionary =
On 2017/06/08 15:18:33, lpz wrote:
> should this be "cache_dictionary" for consistency with other code? If so,
maybe
> check the rest of this code for that distinction?

Done.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:438:
if (verdict_dictionary.get() && !verdict_dictionary->empty()) {
On 2017/06/08 15:18:33, lpz wrote:
> I think this block could use a couple comments (ie: the outer dict size
contains
> both types of verdicts and we want to split them apart...is that right?)

The outer dict size is # of PASSWORD_REUSE_EVENT verdict +1. The "+1" maps to
the lower layer dictionary (keyed on |kPasswordOnFocusCacheKey|) holds all
UMFAMILIAR_LOGIN_PAGE verdicts. 

Added one comment to make it clearer.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:582:
return false;
On 2017/06/08 15:18:33, lpz wrote:
> nit: +1 newline after

Done.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:588:
continue;
On 2017/06/08 15:18:33, lpz wrote:
> nit: +1 newline after

Done.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.h
(right):

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.h:112:
TriggerType type);
On 2017/06/08 15:18:33, lpz wrote:
> nit: type -> trigger_type or verdict_type or request_type everywhere in this
CL

Done.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.h:156:
// Gets the total number of verdicts of the specified |type| (no matter
On 2017/06/08 15:18:33, lpz wrote:
> nit: i find the wording a bit awkward, consider:
> 
> Gets the total number of verdicts of the specified |type| we cached for the
> current active profile. This counts both expired and active verdicts.

Done.

https://codereview.chromium.org/2911293003/diff/60001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.h:262:
int stored_verdict_count_password_entry_;
On 2017/06/08 15:18:33, lpz wrote:
> do you foresee having other verdict types that will need to be counted? If so,
> perhaps store these in a map keyed by TriggerType?

Acknowledged.
No, these are the only two pings.

[Jialiu Lin, 2017-06-08 20:49:17]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a UNFAMILIAR_LOGIN_PAGE
ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a UNFAMILIAR_LOGIN_PAGE
ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

Also change kPasswordFieldOnFocusPinging and kProtectedPasswordEntryPining to
FEATURE_ENABLED_BY_DEFAULT given
the recent launch approval.

BUG=727942
==========

[Jialiu Lin, 2017-06-08 20:49:52]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a UNFAMILIAR_LOGIN_PAGE
ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

Also change kPasswordFieldOnFocusPinging and kProtectedPasswordEntryPining to
FEATURE_ENABLED_BY_DEFAULT given
the recent launch approval.

BUG=727942
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

Also change kPasswordFieldOnFocusPinging and 
kProtectedPasswordEntryPining to FEATURE_ENABLED_BY_DEFAULT given
the recent launch approval.

BUG=727942
==========

[Jialiu Lin, 2017-06-08 21:18:03]

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

[commit-bot: I haz the power, 2017-06-08 21:18:38]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-06-08 21:33:19]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

Also change kPasswordFieldOnFocusPinging and 
kProtectedPasswordEntryPining to FEATURE_ENABLED_BY_DEFAULT given
the recent launch approval.

BUG=727942
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942
==========

[Jialiu Lin, 2017-06-08 21:33:35]

Patchset #7 (id:120001) has been deleted

[Jialiu Lin, 2017-06-08 21:35:28]

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

[Jialiu Lin, 2017-06-08 21:35:30]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-06-08 21:35:58]

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-06-08 23:15:11]

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

[commit-bot: I haz the power, 2017-06-08 23:15:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[lpz, 2017-06-09 14:43:21]

lgtm

[Jialiu Lin, 2017-06-09 16:28:29]

Thanks lpz!

vakh@, could you take a look? Thanks!

[Jialiu Lin, 2017-06-09 22:04:05]

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

[commit-bot: I haz the power, 2017-06-09 22:04:23]

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-06-09 22:07:01]

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

[commit-bot: I haz the power, 2017-06-09 22:07:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jialiu Lin, 2017-06-09 22:37:43]

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

[commit-bot: I haz the power, 2017-06-09 22:38:07]

Dry run: CQ is trying da patch.

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

[vakh (use Gerrit instead), 2017-06-09 23:28:09]

lgtm

https://codereview.chromium.org/2911293003/diff/160001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2911293003/diff/160001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.cc:142:
DCHECK(content_settings_);
This DCHECK is not useful since the de-reference below would fail just as
loudly.

[Jialiu Lin, 2017-06-09 23:33:41]

Thanks vakh@!

https://codereview.chromium.org/2911293003/diff/160001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2911293003/diff/160001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.cc:142:
DCHECK(content_settings_);
On 2017/06/09 23:28:09, vakh (Varun Khaneja) wrote:
> This DCHECK is not useful since the de-reference below would fail just as
> loudly.

Good catch. Removed this dcheck.

[Jialiu Lin, 2017-06-09 23:33:47]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-06-09 23:33:48]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
isherman@chromium.org, lpz@chromium.org, vakh@chromium.org
Link to the patchset: https://codereview.chromium.org/2911293003/#ps180001
(title: "address vakh's comment")

[commit-bot: I haz the power, 2017-06-09 23:34:14]

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-06-10 00:54:34]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1497051227461890,
"parent_rev": "377f71877f1da432e31fa19fbc9f464ae5d5c285", "commit_rev":
"999a40e458bbcfbfad9bb4498c0e2799261a9ee3"}

[commit-bot: I haz the power, 2017-06-10 00:54:47]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#478482}
Committed:
https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb4498c0e...
==========

[commit-bot: I haz the power, 2017-06-10 00:54:49]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb4498c0e...

[findit-for-me, 2017-06-10 02:00:22]

Findit (https://goo.gl/kROfz5) identified this CL at revision 478482 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Jialiu Lin, 2017-06-10 03:26:37]

On 2017/06/10 02:00:22, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 478482 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

Revert cl is in CQ now
 https://chromium-review.googlesource.com/c/530487/

[Jialiu Lin, 2017-06-12 17:40:20]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#478482}
Committed:
https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb4498c0e...
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#478482}
Committed:
https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb4498c0e...
==========

[Jialiu Lin, 2017-06-12 17:44:06]

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

[commit-bot: I haz the power, 2017-06-12 17:44:41]

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-06-12 19:31:41]

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

[commit-bot: I haz the power, 2017-06-12 19:31:42]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-06-12 19:41:13]

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

[commit-bot: I haz the power, 2017-06-12 19:41:44]

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-06-12 21:14:39]

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

[commit-bot: I haz the power, 2017-06-12 21:14:46]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-06-13 01:33:10]

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

[commit-bot: I haz the power, 2017-06-13 01:33:29]

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-06-13 02:13:05]

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

[commit-bot: I haz the power, 2017-06-13 02:13:06]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Jialiu Lin, 2017-06-13 02:33:30]

Patchset #10 (id:220001) has been deleted

[Jialiu Lin, 2017-06-13 20:42:07]

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

[commit-bot: I haz the power, 2017-06-13 20:42:47]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-06-13 20:59:39]

Patchset #9 (id:200001) has been deleted

[Jialiu Lin, 2017-06-13 21:17:30]

Last Saturday after this CL hot committed (with happy try bots) there was a
strange unit tests failure in build cycle
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

It might be caused by other changes, but I reverted this CL  anyway just in
case. 

However, I couldn't reproduce this issue on linux or Win, though it looks like
something related to string operation.
My last patch (#9) only contains rebase-update and wrapping base::StringPiece
around const char[]. No logic or behavior changes.

I'll try to land this code again today. If it is still failing the build cycle,
I'll dig more.

[Jialiu Lin, 2017-06-13 21:31:24]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#478482}
Committed:
https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb4498c0e...
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3
==========

[Jialiu Lin, 2017-06-13 21:31:46]

The CQ bit was unchecked by jialiul@chromium.org

[Jialiu Lin, 2017-06-13 21:31:54]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-06-13 21:31:54]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
lpz@chromium.org, vakh@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2911293003/#ps240001
(title: "Rebase and wrap base::StringPiece arround const char[]")

[commit-bot: I haz the power, 2017-06-13 21:32:52]

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-06-14 00:16:01]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497389514379650,
"parent_rev": "d0b70c4e593e51e13aa4222cbd683c643587017d", "commit_rev":
"dd1e509ada57cbc5e13760ed9d9542ccfea3c361"}

[commit-bot: I haz the power, 2017-06-14 00:16:18]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#479221}
Committed:
https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e13760ed9d95...
==========

[commit-bot: I haz the power, 2017-06-14 00:16:21]

Committed patchset #9 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e13760ed9d95...

[ortuno, 2017-06-14 02:32:24]

ortuno@chromium.org changed reviewers:
+ ortuno@chromium.org

[ortuno, 2017-06-14 02:32:25]

I think this just broke browser_side_navigation_components_unittests:

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests/...

[findit-for-me, 2017-06-14 06:21:49]

Findit (https://goo.gl/kROfz5) identified this CL at revision 479221 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Jialiu Lin, 2017-06-17 20:22:36]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#479221}
Committed:
https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e13760ed9d95...
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#479221}
Committed:
https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e13760ed9d95...
==========

[Jialiu Lin, 2017-06-17 20:23:04]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#479221}
Committed:
https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e13760ed9d95...
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#479221}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e1376>0ed9d9542ccfea3c361
==========

[Jialiu Lin, 2017-06-17 20:23:14]

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

[commit-bot: I haz the power, 2017-06-17 20:23:22]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-06-17 20:34:28]

jialiul@chromium.org changed reviewers:
- ortuno@chromium.org

[Jialiu Lin, 2017-06-17 20:34:30]

Finally, I was able to reproduce crashes on official build (is_official_build =
true). 
Though I'm still not 100% sure what is the root cause of these crashes, it seems
things will go wrong if the following sequence is called:
base::DictionaryValue* value=nullptr;
if (!dict->HasKey(key)) {
  dict->SetDictionaryWithoutPathExpansion(key, base::MakeUnique<..>());
}
 cache_dictionary->GetDictionaryWithoutPathExpansion(key, &value);

Instead, the "standard" way should be:
base::DictionaryValue* value=nullptr;
if (!dict->GetDictionaryWithoutPathExpansion(key, &value)) {
  value = dict->SetDictionaryWithoutPathExpansion(
      key, base::MakeUnique<..>());
}

I thought these two are logically equivalent, but apparently not (maybe a hidden
bug in DictionaryValue class?)

Anyway, after the change in patch #10, no more crash on official build (Tested
on both Linux and Win).

[commit-bot: I haz the power, 2017-06-17 21:38:37]

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

[commit-bot: I haz the power, 2017-06-17 21:38:38]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jialiu Lin, 2017-06-17 21:56:39]

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

[commit-bot: I haz the power, 2017-06-17 21:56:51]

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-06-17 22:23:16]

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

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

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jialiu Lin, 2017-06-18 02:32:12]

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

[commit-bot: I haz the power, 2017-06-18 02:32:16]

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-06-18 03:50:00]

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

[commit-bot: I haz the power, 2017-06-18 03:50:02]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-06-18 23:21:30]

Was the problem that you had a DCHECK around:
 DCHECK(cache_dictionary->GetDictionaryWithoutPathExpansion(	
         kPasswordOnFocusCacheKey, &verdict_dictionary));

? 

That will cause that line not to be compiled into release builds, which would
explain the problem. I've done this before and spent a lot of time trying to
work out why it was failing!

[Jialiu Lin, 2017-06-19 03:44:12]

On 2017/06/18 23:21:30, raymes wrote:
> Was the problem that you had a DCHECK around:
>  DCHECK(cache_dictionary->GetDictionaryWithoutPathExpansion(	
>          kPasswordOnFocusCacheKey, &verdict_dictionary));
> 
> ? 
> 
> That will cause that line not to be compiled into release builds, which would
> explain the problem. I've done this before and spent a lot of time trying to
> work out why it was failing!

Ah, you're right. DCHECK is definitely the reason. Thanks Raymes!

[Jialiu Lin, 2017-06-20 00:38:24]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-06-20 00:38:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, lpz@chromium.org, vakh@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2911293003/#ps260001
(title: "Fix Crashes by Using GetDictionaryWithoutPathExpansion")

[commit-bot: I haz the power, 2017-06-20 00:38:57]

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-06-20 02:36:43]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1497919104328170,
"parent_rev": "d924f0d0d5a28a13bb868820c2797d1368e95411", "commit_rev":
"d9f1b53ca4d3457d11fd01c077bf303d459a3c76"}

[commit-bot: I haz the power, 2017-06-20 02:36:58]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1497919104328170,
"parent_rev": "dae1fce2853ac230302ea2a799f75e63e7e24b08", "commit_rev":
"f1b5b08b61b3f3f23d2f6bd82339b20a7141603a"}

[commit-bot: I haz the power, 2017-06-20 02:37:15]

Description was changed from

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#479221}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e1376>0ed9d9542ccfea3c361
==========

to

==========
Cache protected password entry and password on focus ping separately.

PasswordProtectionService still uses a single content setting type
to cache verdict. Since password on focus verdicts (a.k.a
UNFAMILIAR_LOGIN_PAGE ping verdict) only used by extended reporting
users, we group them under kPasswordOnFocusCacheKey.
And protected password entry verdicts (a.k.a PASSSWORD_REUSE_EVENT 
verdict for all users) remain the same.
In this way we don't need extra migration code to handle existing
verdicts.

BUG=727942

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#478482}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/999a40e458bbcfbfad9bb>4498c0e2799261a9ee3

>Review-Url: https://codereview.chromium.org/2911293003
>Cr-Commit-Position: refs/heads/master@{#479221}
>Committed:
>https://chromium.googlesource.com/chromium/src/+/dd1e509ada57cbc5e1376>0ed9d9542ccfea3c361

Review-Url: https://codereview.chromium.org/2911293003
Cr-Commit-Position: refs/heads/master@{#480695}
Committed:
https://chromium.googlesource.com/chromium/src/+/f1b5b08b61b3f3f23d2f6bd82339...
==========

[commit-bot: I haz the power, 2017-06-20 02:37:17]

Committed patchset #10 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/f1b5b08b61b3f3f23d2f6bd82339...