Make super MAC optional.

Make super MAC optional.

BUG=372547

[erikwright (departed), 2014-06-11 18:39:22]

gab: PTAL. I will add a test prior to commit.

[gab, 2014-06-11 18:57:54]

I just realized that this will dilute (or boost if the default is "false") the
Initialized graphs on channels where hardening is not enabled.

I guess we have to live with that (but something to keep in mind); one way to
avoid having to suppress the super MAC -- and thus keep our stats sane -- would
be to introduce a property unique to each store in the calculation of the super
MAC (which would also avoid the problem of one store being able to generate
super MACs valid in the other store).

@Robert: WDYT? Let's chat with Erik offline.

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_store_impl.cc:70: : false),
What do you think of making the default "true" here so that we get
TrustedInitialized reports instead of Initialized noise for tracked-only prefs?

[erikwright (departed), 2014-06-11 21:15:15]

The stats are reported pref-by-pref, so we _can_ look separately at each one,
but yes it is currently convenient to aggregate them in some cases.

That being said - it might be better all around to be able to look at the graphs
for prefs in Protected Preferences vs. prefs in Preferences. Since we know that
some phenomena affect only one file or the other and it can be complicated to
heuristically determine where a preference was stored in a specific client
report. Maybe we should split the metrics into Settings.* and
Settings.Protected.*?

[gab, 2014-06-11 21:54:13]

On 2014/06/11 21:15:15, erikwright wrote:
> The stats are reported pref-by-pref, so we _can_ look separately at each one,
> but yes it is currently convenient to aggregate them in some cases.

I know that we can look pref-by-pref, but this is not what I meant:

What I meant is that, for a given tracked-only pref, there will no longer be a
notion of TrustedInitialized so the graph for Initialized will go up for these
prefs (or conversely go down if we decide to always TrustedInitialize
tracked-only pref).

But now that I think about it some more, we would still TrustInitialize NULL
values (which is mostly what we care about to avoid having an Initialized graph
polluted with reports from new profile creations).

Therefore I retract my objections -- CL comments to follow.

> 
> That being said - it might be better all around to be able to look at the
graphs
> for prefs in Protected Preferences vs. prefs in Preferences. Since we know
that
> some phenomena affect only one file or the other and it can be complicated to
> heuristically determine where a preference was stored in a specific client
> report. Maybe we should split the metrics into Settings.* and
> Settings.Protected.*?

Oh god no! Not more histograms! We can already view this data split by Finch
setting (i.e. only for users that are disabled or enabled) on the Finch
dashboard.

And even when viewing it all on the same graph I think it's easy enough to know
which ones are in which bucket.

[gab, 2014-06-11 21:54:33]

lgtm w/ 1 request for simplification

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_store_impl.cc:70: : false),
On 2014/06/11 18:57:54, gab wrote:
> What do you think of making the default "true" here so that we get
> TrustedInitialized reports instead of Initialized noise for tracked-only
prefs?

Actually, never mind.

Setting this to false will keep it such that for tracked-only pref we get:
1) Prefs initialized from NULL value => TrustedInitialized
2) Prefs initialized from any other value => Initialized

The only thing we lose is that we will get an Initialization spike rather than a
TrustedInitialized spike if we ever add a non-protected tracked pref -- which I
don't think we care about.

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_store_impl.cc:159: if (!outer_->use_super_mac_ ||
outer_->initial_hashes_dictionary_trusted_)
I don't think this is even required since line 104 already takes care of this.

[erikwright (departed), 2014-06-13 13:25:24]

Added a unit test. Hitting CQ but you can unclick if you disagree with my
response to your nit.

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_store_impl.cc:159: if (!outer_->use_super_mac_ ||
outer_->initial_hashes_dictionary_trusted_)
On 2014/06/11 21:54:32, gab wrote:
> I don't think this is even required since line 104 already takes care of this.

This saves us thinking that we have altered the store. During filter on load
that will save us from telling the JsonPrefStore that it needs to commit to
disk.

[erikwright (departed), 2014-06-13 13:29:33]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-06-13 13:30:18]

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

[gab, 2014-06-13 14:48:58]

test lgtm as well

Thanks!
Gab

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/332473002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_store_impl.cc:159: if (!outer_->use_super_mac_ ||
outer_->initial_hashes_dictionary_trusted_)
On 2014/06/13 13:25:24, erikwright wrote:
> On 2014/06/11 21:54:32, gab wrote:
> > I don't think this is even required since line 104 already takes care of
this.
> 
> This saves us thinking that we have altered the store. During filter on load
> that will save us from telling the JsonPrefStore that it needs to commit to
> disk.

I see, good point :-)!

[commit-bot: I haz the power, 2014-06-13 18:41:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-13 18:51:34]

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

[commit-bot: I haz the power, 2014-06-13 18:51:35]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[gab, 2014-06-13 20:35:27]

Combined in https://codereview.chromium.org/329093003/, closing.