Remove unloaded profile hash store initialization,

Remove unloaded profile hash store initialization. Also:

* Remove all traces of pref hash stores after last store reset.
* Make super MAC optional.

BUG=372547
TBR=asvitkine,jhawkins

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277209

[erikwright (departed), 2014-06-11 18:40:46]

gab: PTAL. I'm kind of shocked by how many files are touched by just this CL.

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

On 2014/06/11 18:40:46, erikwright wrote:
> gab: PTAL. I'm kind of shocked by how many files are touched by just this CL.

Right, this is why I was really insisting on making it its own CL, it was really
polluting the other one.

Looking..

[gab, 2014-06-11 19:25:42]

You'll also need a rubberstamp from a chrome/OWNER for chrome_browser_main.cc

(and if you're not using the Chromite Butler extension already, it's awesome to
see which files current reviewers cover:
https://chrome.google.com/webstore/detail/chromite-butler/bhcnanendmgjjeghama...)

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:500: if
(GetSettingsEnforcementGroup() >= GROUP_ENFORCE_ALWAYS)
Also remove GROUP_ENFORCE_ON_LOAD which was the same as GROUP_ENFORCE_ALWAYS but
would allow this path.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:35: #include
"chrome/browser/profiles/profile_info_cache.h"
rm this include

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:36: #include
"chrome/browser/profiles/profile_manager.h"
and this one

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:235: //
pref_hash_browsertest.cc and extension_startup_browsertest.cc when updating
Remove mention of "pref_hash_browsertest.cc" from this comment.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_browsertest.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:312: }
This test also had the benefit of being the only integration test. Although it
wasn't a generic integration test it at least confirmed hardening could run and
report as expected.

We should try to bring this back in a different form.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pro...
File chrome/browser/prefs/profile_pref_store_manager.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:188:
"Settings.TrackedPreferencesAlternateStoreVersionUpdatedFrom",
Mark this histogram obsolete in histograms.xml.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:341:
UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferencesAlternateStoreVersion",
Mark this histogram obsolete in histograms.xml.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/pref_service_hash_store_contents.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/pref_service_hash_store_contents.cc:88: if
(update->GetDictionary(kStoreVersionsDict, &version_dict))
Remove this code and instead unconditionally clear kStoreVersionsDict in the
constructor (this is typically how deprecated prefs are handled -- auto-cleared
in some init steps for some time with a TODO to remove them from the codebase
altogether later).

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

PTAL.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:500: if
(GetSettingsEnforcementGroup() >= GROUP_ENFORCE_ALWAYS)
On 2014/06/11 19:25:42, gab wrote:
> Also remove GROUP_ENFORCE_ON_LOAD which was the same as GROUP_ENFORCE_ALWAYS
but
> would allow this path.

Done. Are any experiment configuration changes required?

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:35: #include
"chrome/browser/profiles/profile_info_cache.h"
On 2014/06/11 19:25:41, gab wrote:
> rm this include

Done.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:36: #include
"chrome/browser/profiles/profile_manager.h"
On 2014/06/11 19:25:41, gab wrote:
> and this one

Done.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:235: //
pref_hash_browsertest.cc and extension_startup_browsertest.cc when updating
On 2014/06/11 19:25:41, gab wrote:
> Remove mention of "pref_hash_browsertest.cc" from this comment.

Done.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_browsertest.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:312: }
On 2014/06/11 19:25:41, gab wrote:
> This test also had the benefit of being the only integration test. Although it
> wasn't a generic integration test it at least confirmed hardening could run
and
> report as expected.
> 
> We should try to bring this back in a different form.

Do you want that done in this CL or separately?

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pro...
File chrome/browser/prefs/profile_pref_store_manager.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:188:
"Settings.TrackedPreferencesAlternateStoreVersionUpdatedFrom",
On 2014/06/11 19:25:42, gab wrote:
> Mark this histogram obsolete in histograms.xml.

Done.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:341:
UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferencesAlternateStoreVersion",
On 2014/06/11 19:25:42, gab wrote:
> Mark this histogram obsolete in histograms.xml.

Done.

[gab, 2014-06-11 21:39:29]

lgtm w/ nits and replies to your questions

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:500: if
(GetSettingsEnforcementGroup() >= GROUP_ENFORCE_ALWAYS)
On 2014/06/11 21:08:36, erikwright wrote:
> On 2014/06/11 19:25:42, gab wrote:
> > Also remove GROUP_ENFORCE_ON_LOAD which was the same as GROUP_ENFORCE_ALWAYS
> but
> > would allow this path.
> 
> Done. Are any experiment configuration changes required?

Not immediately, we can remove enforce_on_load entries from the experiments, but
all configs give it 0% of users so it doesn't matter if it sits there for some
time.

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_browsertest.cc (left):

https://codereview.chromium.org/329093003/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:312: }
On 2014/06/11 21:08:35, erikwright wrote:
> On 2014/06/11 19:25:41, gab wrote:
> > This test also had the benefit of being the only integration test. Although
it
> > wasn't a generic integration test it at least confirmed hardening could run
> and
> > report as expected.
> > 
> > We should try to bring this back in a different form.
> 
> Do you want that done in this CL or separately?

Separately, as a follow-up to the main CL (please document all required
follow-ups in the main CL's description).

https://codereview.chromium.org/329093003/diff/40001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/329093003/diff/40001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:186: // Also disallow
seeding of unloaded profiles.
Replace with:
// Enforce protected settings on profile loads.

https://codereview.chromium.org/329093003/diff/40001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/pref_service_hash_store_contents.cc (right):

https://codereview.chromium.org/329093003/diff/40001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/pref_service_hash_store_contents.cc:63:
DictionaryPrefUpdate update(pref_service_, prefs::kProfilePreferenceHashes);
Add:
// TODO(erikwright): Remove kStoreVersionsDict in M30+.

[gab, 2014-06-12 22:09:00]

Just found another piece of dead code, we can expand the CL description and
remove it in this CL IMO.

https://codereview.chromium.org/329093003/diff/40001/chrome/browser/prefs/pro...
File chrome/browser/prefs/profile_pref_store_manager.cc (right):

https://codereview.chromium.org/329093003/diff/40001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:28: class
DictionaryHashStoreContents : public HashStoreContents {
This can be removed altogether as well; not sure when it stopped being used, but
it appears to no longer be used :-)!

I assume this will also allow removal of some headers above :-)!

[erikwright (departed), 2014-06-13 17:19:37]

jhawkins: for chrome_browser_main.cc.
asvitkine: for histograms.xml

This CL removes support for initialization of MAC-based protection of unloaded
profiles (i.e., those besides the default profile).

[erikwright (departed), 2014-06-13 17:38:00]

Combine three CLs.

[erikwright (departed), 2014-06-13 17:41:28]

On 2014/06/13 17:38:00, erikwright wrote:
> Combine three CLs.

I combined two other closely related CLs that only touch the same files as this
one, in order to pass quicker through the CQ:

https://codereview.chromium.org/332473002/
https://codereview.chromium.org/326403003/

[gab, 2014-06-13 18:04:10]

still lgtm w/ one extra test request

https://codereview.chromium.org/329093003/diff/100001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right):

https://codereview.chromium.org/329093003/diff/100001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl_unittest.cc:152:
ASSERT_FALSE(CreateHashStoreContents()->GetSuperMac().empty());
This doesn't really test StampSuperMAC() itself since we also store hashes
above.

It would be good to add a test for StampSuperMAC() specifically.

[erikwright (departed), 2014-06-13 18:08:04]

On 2014/06/13 18:04:10, gab wrote:
> still lgtm w/ one extra test request
> 
>
https://codereview.chromium.org/329093003/diff/100001/chrome/browser/prefs/pr...
> File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/329093003/diff/100001/chrome/browser/prefs/pr...
> chrome/browser/prefs/pref_hash_store_impl_unittest.cc:152:
> ASSERT_FALSE(CreateHashStoreContents()->GetSuperMac().empty());
> This doesn't really test StampSuperMAC() itself since we also store hashes
> above.
> 
> It would be good to add a test for StampSuperMAC() specifically.

StampSuperMAC is not included in this CL, so I feel it would be premature to add
tests for it.

I mostly added this earlier check as a counterpoint to the check for empty() in
the new test. To show that it _wouldn't_ be empty if use_super_mac == true.

[gab, 2014-06-13 20:33:29]

On 2014/06/13 18:08:04, erikwright wrote:
> On 2014/06/13 18:04:10, gab wrote:
> > still lgtm w/ one extra test request
> > 
> >
>
https://codereview.chromium.org/329093003/diff/100001/chrome/browser/prefs/pr...
> > File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/329093003/diff/100001/chrome/browser/prefs/pr...
> > chrome/browser/prefs/pref_hash_store_impl_unittest.cc:152:
> > ASSERT_FALSE(CreateHashStoreContents()->GetSuperMac().empty());
> > This doesn't really test StampSuperMAC() itself since we also store hashes
> > above.
> > 
> > It would be good to add a test for StampSuperMAC() specifically.
> 
> StampSuperMAC is not included in this CL, so I feel it would be premature to
add
> tests for it.
> 
> I mostly added this earlier check as a counterpoint to the check for empty()
in
> the new test. To show that it _wouldn't_ be empty if use_super_mac == true.

Duh of course, I somehow assumed more had been merged into this CL than was
actually the case :-)!

lgtm, thanks!
CQ away!

[erikwright (departed), 2014-06-13 20:37:27]

TBR=asvitkine,jhawkins for trivial changes to chrome_browser_main and
histograms.xml

[erikwright (departed), 2014-06-13 20:37:34]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-06-13 20:39:25]

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

[commit-bot: I haz the power, 2014-06-13 22:15:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_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 22:18:54]

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

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

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[gab, 2014-06-13 22:21:00]

The CQ bit was checked by gab@chromium.org

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

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

[Alexei Svitkine (slow), 2014-06-14 01:36:11]

lgtm

[commit-bot: I haz the power, 2014-06-14 08:47:44]

Change committed as 277209

[James Hawkins, 2014-06-16 20:31:40]

lgtm