Send a TrackedPreference incident when registry pref validation fails.

Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858
Committed: https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Committed: https://crrev.com/c34dbf9b2c3434449a30384ce215a40c45fdae05
Cr-Original-Commit-Position: refs/heads/master@{#422545}
Cr-Commit-Position: refs/heads/master@{#423285}

[proberge, 2016-10-03 18:11:45]

Description was changed from

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858
==========

to

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858
==========

[proberge, 2016-10-03 18:11:46]

proberge@chromium.org changed reviewers:
+ caitkp@chromium.org, gab@chromium.org, jialiul@chromium.org

[Jialiu Lin, 2016-10-03 18:13:32]

lgtm

[gab, 2016-10-03 18:31:29]

lgtm

[Cait (Slow), 2016-10-03 19:26:00]

lgtm

[proberge, 2016-10-03 19:33:48]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2016-10-03 19:34:20]

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, 2016-10-03 19:43:16]

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

[commit-bot: I haz the power, 2016-10-03 19:43:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[proberge, 2016-10-03 20:10:10]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-10-03 20:10:10]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
caitkp@chromium.org, jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2384213002/#ps20001
(title: "Handle UNSUPPORTED enum value in switch")

[commit-bot: I haz the power, 2016-10-03 20:10:35]

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, 2016-10-03 20:21:28]

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

[commit-bot: I haz the power, 2016-10-03 20:21:30]

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_...)

[proberge, 2016-10-03 20:42:14]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-10-03 20:42:15]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
caitkp@chromium.org, jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2384213002/#ps40001
(title: "Handle UNSUPPORTED in another switch")

[commit-bot: I haz the power, 2016-10-03 20:43:00]

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

[gab, 2016-10-03 21:03:58]

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_helper.cc (right):

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_helper.cc:88: return;
This should be NOTREACHED() as well

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_helper.cc:89: default:
And while you're here if you don't mind: remove default (i.e. move "NOTREACHED()
+ return" outside of switch).

Preferred style for switches on enums is no default since clang now detects
unhandled enum values :-).

[commit-bot: I haz the power, 2016-10-03 21:43:51]

Description was changed from

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858
==========

to

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858
==========

[commit-bot: I haz the power, 2016-10-03 21:43:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-03 21:46:59]

Description was changed from

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858
==========

to

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858

Committed: https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Cr-Commit-Position: refs/heads/master@{#422545}
==========

[commit-bot: I haz the power, 2016-10-03 21:47:00]

Patchset 3 (id:??) landed as
https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Cr-Commit-Position: refs/heads/master@{#422545}

[Guido Urdaneta, 2016-10-04 08:48:42]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2390213002/ by guidou@chromium.org.

The reason for reverting is: Parent CL
https://codereview.chromium.org/2204943002 is suspect of breaking WebRTC Win
Tester bots.

See
https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/28938.

[proberge, 2016-10-05 15:36:41]

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_helper.cc (right):

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_helper.cc:88: return;
On 2016/10/03 21:03:58, gab wrote:
> This should be NOTREACHED() as well

Done.

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_helper.cc:89: default:
On 2016/10/03 21:03:58, gab wrote:
> And while you're here if you don't mind: remove default (i.e. move
"NOTREACHED()
> + return" outside of switch).
> 
> Preferred style for switches on enums is no default since clang now detects
> unhandled enum values :-).

I think default:foo is slightly cleaner than adding if(!histogram_name) { foo }
after the switch. Leaving as-is.

[proberge, 2016-10-05 15:36:56]

Description was changed from

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858

Committed: https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Cr-Commit-Position: refs/heads/master@{#422545}
==========

to

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858

Committed: https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Cr-Commit-Position: refs/heads/master@{#422545}
==========

[proberge, 2016-10-05 15:37:31]

Re-opening issue to rollforwards. @gab PTAL I added another parameter for the
Split pref delegate to pass invalid keys.

[gab, 2016-10-05 17:23:22]

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_helper.cc (right):

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_helper.cc:89: default:
On 2016/10/05 15:36:40, proberge wrote:
> On 2016/10/03 21:03:58, gab wrote:
> > And while you're here if you don't mind: remove default (i.e. move
> "NOTREACHED()
> > + return" outside of switch).
> > 
> > Preferred style for switches on enums is no default since clang now detects
> > unhandled enum values :-).
> 
> I think default:foo is slightly cleaner than adding if(!histogram_name) { foo
}
> after the switch. Leaving as-is.

No, the presence of the default prevents clang from generating a compile error
if a new enum value is added. This is bad and you should avoid default cases
when handling enums as much as possible.

You shouldn't handle not having |histogram_name| as it should never happen.
Instead you should document that as

DCHECK(!histogram_name.empty());

after the switch.

https://codereview.chromium.org/2384213002/diff/60001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/2384213002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc:308:
ExpectKeysEquate(external_validation_invalid_keys_,
This is unrelated to the other condition IMO, i.e. instead of being an else
block it should be its own if (external_validation_value_state_ == ...) block.

[proberge, 2016-10-05 17:53:27]

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_helper.cc (right):

https://codereview.chromium.org/2384213002/diff/40001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_helper.cc:89: default:
On 2016/10/05 17:23:21, gab wrote:
> On 2016/10/05 15:36:40, proberge wrote:
> > On 2016/10/03 21:03:58, gab wrote:
> > > And while you're here if you don't mind: remove default (i.e. move
> > "NOTREACHED()
> > > + return" outside of switch).
> > > 
> > > Preferred style for switches on enums is no default since clang now
detects
> > > unhandled enum values :-).
> > 
> > I think default:foo is slightly cleaner than adding if(!histogram_name) {
foo
> }
> > after the switch. Leaving as-is.
> 
> No, the presence of the default prevents clang from generating a compile error
> if a new enum value is added. This is bad and you should avoid default cases
> when handling enums as much as possible.
> 
> You shouldn't handle not having |histogram_name| as it should never happen.
> Instead you should document that as
> 
> DCHECK(!histogram_name.empty());
> 
> after the switch.

Done, but can't use empty() on a char*. Initialized it to nullptr and
DCHECK(histogram_name) instead.

https://codereview.chromium.org/2384213002/diff/60001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/2384213002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc:308:
ExpectKeysEquate(external_validation_invalid_keys_,
On 2016/10/05 17:23:21, gab wrote:
> This is unrelated to the other condition IMO, i.e. instead of being an else
> block it should be its own if (external_validation_value_state_ == ...) block.

Not quite unrelated. Added a test which fails with your suggestion but which
passes with the current patch set.

[gab, 2016-10-05 19:22:26]

lgtm % question

https://codereview.chromium.org/2384213002/diff/60001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/2384213002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc:308:
ExpectKeysEquate(external_validation_invalid_keys_,
On 2016/10/05 17:53:27, proberge wrote:
> On 2016/10/05 17:23:21, gab wrote:
> > This is unrelated to the other condition IMO, i.e. instead of being an else
> > block it should be its own if (external_validation_value_state_ == ...)
block.
> 
> Not quite unrelated. Added a test which fails with your suggestion but which
> passes with the current patch set. 

I see, we only report bypass incident if there is no real incident?

https://codereview.chromium.org/2384213002/diff/80001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/2384213002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc:312:
EXPECT_EQ(0, tp_incident.split_key_size());
Just like this verifies size is zero when not reporting it, should the above
if/else do that too for the expected-not-to-be-reported counterpart?

[proberge, 2016-10-05 19:54:05]

https://codereview.chromium.org/2384213002/diff/60001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/2384213002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc:308:
ExpectKeysEquate(external_validation_invalid_keys_,
On 2016/10/05 19:22:26, gab wrote:
> On 2016/10/05 17:53:27, proberge wrote:
> > On 2016/10/05 17:23:21, gab wrote:
> > > This is unrelated to the other condition IMO, i.e. instead of being an
else
> > > block it should be its own if (external_validation_value_state_ == ...)
> block.
> > 
> > Not quite unrelated. Added a test which fails with your suggestion but which
> > passes with the current patch set. 
> 
> I see, we only report bypass incident if there is no real incident?

That's correct. See preference_validation_delegate.cc:40

https://codereview.chromium.org/2384213002/diff/80001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/2384213002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc:312:
EXPECT_EQ(0, tp_incident.split_key_size());
On 2016/10/05 19:22:26, gab wrote:
> Just like this verifies size is zero when not reporting it, should the above
> if/else do that too for the expected-not-to-be-reported counterpart?

I think it already does so implicitly; tp_incident.split_key() contains a list
of incidents which is compared against either invalid_keys_ or
external_validation_invalid_keys. Both expectations would fail if we
accidentally added both normal and external invalid keys to the incident.

[proberge, 2016-10-05 19:56:59]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-10-05 19:57:00]

The patchset sent to the CQ was uploaded after l-g-t-m from caitkp@chromium.org,
jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2384213002/#ps80001
(title: "Address comments on patch set 4")

[commit-bot: I haz the power, 2016-10-05 19:57:33]

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

[gab, 2016-10-05 20:08:16]

https://codereview.chromium.org/2384213002/diff/80001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc
(right):

https://codereview.chromium.org/2384213002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc:312:
EXPECT_EQ(0, tp_incident.split_key_size());
On 2016/10/05 19:54:05, proberge wrote:
> On 2016/10/05 19:22:26, gab wrote:
> > Just like this verifies size is zero when not reporting it, should the above
> > if/else do that too for the expected-not-to-be-reported counterpart?
> 
> I think it already does so implicitly; tp_incident.split_key() contains a list
> of incidents which is compared against either invalid_keys_ or
> external_validation_invalid_keys. Both expectations would fail if we
> accidentally added both normal and external invalid keys to the incident.

Ah, indeed :-), lgtm++

[commit-bot: I haz the power, 2016-10-05 20:51:59]

Description was changed from

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858

Committed: https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Cr-Commit-Position: refs/heads/master@{#422545}
==========

to

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858

Committed: https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Cr-Commit-Position: refs/heads/master@{#422545}
==========

[commit-bot: I haz the power, 2016-10-05 20:52:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-05 20:53:33]

Description was changed from

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858

Committed: https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Cr-Commit-Position: refs/heads/master@{#422545}
==========

to

==========
Send a TrackedPreference incident when registry pref validation fails.

Note that the new incident types (BYPASS_CLEARED and BYPASS_CHANGED) are only
used when external (registry) validation fails and existing
(secure_preferences-based) validation succeeds.

BUG=624858

Committed: https://crrev.com/1ba5bcd1a74380bb398eeab3787ede5620b5130c
Committed: https://crrev.com/c34dbf9b2c3434449a30384ce215a40c45fdae05
Cr-Original-Commit-Position: refs/heads/master@{#422545}
Cr-Commit-Position: refs/heads/master@{#423285}
==========

[commit-bot: I haz the power, 2016-10-05 20:53:34]

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