Separate storage for protected preferences into Protected Preferences file.

Separate storage for protected preferences into Protected Preferences file.

Integrates the SegregatedPrefStore with chrome_pref_service_factory.cc .

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

[erikwright (departed), 2014-03-31 19:02:32]

jochen: PTAL for chrome_constants. You already reviewed this in another CL, but
I split it out.
gab: PTAL. This is already reviewed by bauerb but I split it from the rest of
the CL.

The tests that fail on the try jobs also fail for me and mad@ when run without
the patch. I'm going to figure out who to notify/what to do now, since they
don't seem to be failing on 100% of try jobs.

[gab, 2014-03-31 20:28:13]

On 2014/03/31 19:02:32, erikwright wrote:
> jochen: PTAL for chrome_constants. You already reviewed this in another CL,
but
> I split it out.
> gab: PTAL. This is already reviewed by bauerb but I split it from the rest of
> the CL.
> 
> The tests that fail on the try jobs also fail for me and mad@ when run without
> the patch. I'm going to figure out who to notify/what to do now, since they
> don't seem to be failing on 100% of try jobs.

Will take a look shortly.

Can you CC me on the other CL and link to it from the description of this one?

[gab, 2014-03-31 22:24:51]

On 2014/03/31 20:28:13, gab wrote:
> On 2014/03/31 19:02:32, erikwright wrote:
> > jochen: PTAL for chrome_constants. You already reviewed this in another CL,
> but
> > I split it out.
> > gab: PTAL. This is already reviewed by bauerb but I split it from the rest
of
> > the CL.
> > 
> > The tests that fail on the try jobs also fail for me and mad@ when run
without
> > the patch. I'm going to figure out who to notify/what to do now, since they
> > don't seem to be failing on 100% of try jobs.
> 
> Will take a look shortly.
> 
> Can you CC me on the other CL and link to it from the description of this one?

(nvm found it via bug)

[jochen (gone - plz use gerrit), 2014-04-01 11:31:19]

constants lgtm

[erikwright (departed), 2014-04-01 15:04:13]

gab: PTAL.

[gab, 2014-04-01 20:19:52]

lg, nits and comments about the migration phase.

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

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:98:
DISALLOW_COPY_AND_ASSIGN(SimpleMutableDictionary);
nit: +empty line above

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:288: for (size_t i = 0; i <
tracking_configuration_.size(); ++i) {
Use a std::vector<std::string>::const_iterator instead.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:298:
scoped_ptr<PrefHashFilter> unprotected_pref_hash_filter(
Store these directly in scoped_ptr<PrefFilter>'s since you don't use them as
PrefHashFilter's and it will avoid a PassAs below.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:303: new
PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
Why not start putting hashes in the Protected Preferences file in the same CL?
Seems like otherwise we'll introduce yet another migration case :(.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:314:
protected_pref_hash_filter.PassAs<PrefFilter>()));
So migration is icky the way I see it, since both PrefHashFilter use the same
underlying HashStoreContents (i.e. local_state).

On migration:
1) Unprotected store loads and reports UNCHANGED for its values.
2) Protected store loads and reports CLEARED for its values (as it has no values
in store yet it sees MACs in its hash store) and it sets MACs to match NULL
values.
3) MigrateValues kicks in and would reset every migrated value because the hash
was modified -- except it happens to be using an old copy of the
HashStoreContents built on line 322... so it's doesn't... and it then stores the
correct values which restores the hashes and thus it kinda works...

This would at the very least generate an undesired CLEARED spike for all
migrated values I think.

A better approach IMO would be to:
1) Harden the protected values left in the unprotected store.
2) Migrate those values to the protected store as-is once we know they're clean.

I don't really see how we do this cleanly either though... I think the easiest
thing is really to not have this intermediate state where Protected Preferences
use local_state as a hash store. How much harder is it to go straight there?

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:324: reporting_ids_count_)),
Meta-comment either here or on MigrateValues() about how this dance works.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right):

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:68: const char
kProtectedSplit[] = "protected_split";
So this value is never actually tested?

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:128: bool
ResetRecorded() {
s/ResetRecorded/WasResetRecorded

Otherwise I thought from reading the name of this method that it was itself
performing the action of resetting some recorded state.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:139:
scoped_refptr<PersistentPrefStore> pref_store =
Add a comment stating that this initializes the SegregatedPrefStore. I think it
matters, yet it wasn't obvious to me on first read (I had to come back up with
the question "so where is the segregated store tested from?" to realize it was
here.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:194:
profile_dir_.path(), true, base::FileEnumerator::FILES);
Why recursive?

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:197: while
(!(pref_file_path = file_enumerator.Next()).empty()) {
Use the pattern suggested @
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_en...
to loop over the files in the FileEnumerator.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:377: 99u,
kUnprotectedAtomic, PrefHashFilter::ENFORCE_ON_LOAD,
Why 99u? This is wrong in practice, although it doesn't actually matter in this
test, as you'll report that the reporting_ids_count is configuration.size()
which is 1.

0u works just the same as is more correct IMO.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:440: if (it->name ==
std::string(kProtectedAtomic)) {
I don't think a string compare is required here; a pointer comparison is enough,
the configs also use the pointer to the constant.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/segregated_pref_store.cc:124: if (read_error ==
PersistentPrefStore::PREF_READ_ERROR_NO_FILE)
Add a comment as to why you disregard PREF_READ_ERROR_NO_FILE here.

[erikwright (departed), 2014-04-02 15:03:16]

PTAL. Especially at my suggestion for avoiding the CLEARED spike. I'm going to
work on a version for that now.

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

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:98:
DISALLOW_COPY_AND_ASSIGN(SimpleMutableDictionary);
On 2014/04/01 20:19:52, gab wrote:
> nit: +empty line above

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:288: for (size_t i = 0; i <
tracking_configuration_.size(); ++i) {
On 2014/04/01 20:19:52, gab wrote:
> Use a std::vector<std::string>::const_iterator instead.

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:298:
scoped_ptr<PrefHashFilter> unprotected_pref_hash_filter(
On 2014/04/01 20:19:52, gab wrote:
> Store these directly in scoped_ptr<PrefFilter>'s since you don't use them as
> PrefHashFilter's and it will avoid a PassAs below.

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:303: new
PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
On 2014/04/01 20:19:52, gab wrote:
> Why not start putting hashes in the Protected Preferences file in the same CL?
> Seems like otherwise we'll introduce yet another migration case :(.

You have the burden of migrating hashes from one store to another anyway
(because you still want to migrate hashes for tracked-only preferences, if any,
unless we are absolutely sure to get rid of them).

But the biggest reason is that it's not ready yet, and we can land this and
hopefully get it in M35.

I don't expect the MAC migration to be that big a deal. It basically amounts to
providing an alternative HashStore to the PrefHashFilter, and it won't look
different whether we do it now or later, or whether the values have already
moved or not.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:314:
protected_pref_hash_filter.PassAs<PrefFilter>()));
On 2014/04/01 20:19:52, gab wrote:
> So migration is icky the way I see it, since both PrefHashFilter use the same
> underlying HashStoreContents (i.e. local_state).
> 
> On migration:
> 1) Unprotected store loads and reports UNCHANGED for its values.

Yes, but only for tracking-only values, and this is as expected.

> 2) Protected store loads and reports CLEARED for its values (as it has no
values
> in store yet it sees MACs in its hash store) and it sets MACs to match NULL
> values.

Yes, that's true.

> 3) MigrateValues kicks in and would reset every migrated value because the
hash
> was modified -- except it happens to be using an old copy of the
> HashStoreContents built on line 322... so it's doesn't... and it then stores
the
> correct values which restores the hashes and thus it kinda works...
> 
> This would at the very least generate an undesired CLEARED spike for all
> migrated values I think.
> 
> A better approach IMO would be to:
> 1) Harden the protected values left in the unprotected store.
> 2) Migrate those values to the protected store as-is once we know they're
clean.

This is equally messy, because you have to provide the unprotected store with a
full configuration to protect all of the values, when, post-migration, it's only
responsible for a subset.

> I don't really see how we do this cleanly either though... I think the easiest
> thing is really to not have this intermediate state where Protected
Preferences
> use local_state as a hash store. How much harder is it to go straight there?

I think it's a few days away from a code review.

Here's a solution I just thought of that might solve the problem:

(1) Have the PrefHashFilter store a list of all known protected values in the
pref store.
(2) In FilterOnLoad, skip verification of any value that is configured for
protection but is not in the loaded dictionary. DO NOT touch the stored MAC, if
any.
(3) Of course, in FilterUpdate, we add the updated value into the list of known
protected values (if it is protected) or remove it from the list (if it is
unprotected, i.e., if we are removing a previously protected value to migrate it
back to the main store).

WDYT?

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:324: reporting_ids_count_)),
On 2014/04/01 20:19:52, gab wrote:
> Meta-comment either here or on MigrateValues() about how this dance works.

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right):

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:68: const char
kProtectedSplit[] = "protected_split";
On 2014/04/01 20:19:52, gab wrote:
> So this value is never actually tested?

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:128: bool
ResetRecorded() {
On 2014/04/01 20:19:52, gab wrote:
> s/ResetRecorded/WasResetRecorded
> 
> Otherwise I thought from reading the name of this method that it was itself
> performing the action of resetting some recorded state.

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:139:
scoped_refptr<PersistentPrefStore> pref_store =
On 2014/04/01 20:19:52, gab wrote:
> Add a comment stating that this initializes the SegregatedPrefStore. I think
it
> matters, yet it wasn't obvious to me on first read (I had to come back up with
> the question "so where is the segregated store tested from?" to realize it was
> here.

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:194:
profile_dir_.path(), true, base::FileEnumerator::FILES);
On 2014/04/01 20:19:52, gab wrote:
> Why recursive?

Assuming that the files are stored at the top-level seems an unnecessary
assumption to me.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:197: while
(!(pref_file_path = file_enumerator.Next()).empty()) {
On 2014/04/01 20:19:52, gab wrote:
> Use the pattern suggested @
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_en...
> to loop over the files in the FileEnumerator.

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:377: 99u,
kUnprotectedAtomic, PrefHashFilter::ENFORCE_ON_LOAD,
On 2014/04/01 20:19:52, gab wrote:
> Why 99u? This is wrong in practice, although it doesn't actually matter in
this
> test, as you'll report that the reporting_ids_count is configuration.size()
> which is 1.
> 
> 0u works just the same as is more correct IMO.

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:440: if (it->name ==
std::string(kProtectedAtomic)) {
On 2014/04/01 20:19:52, gab wrote:
> I don't think a string compare is required here; a pointer comparison is
enough,
> the configs also use the pointer to the constant.

Done.

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/segregated_pref_store.cc:124: if (read_error ==
PersistentPrefStore::PREF_READ_ERROR_NO_FILE)
On 2014/04/01 20:19:52, gab wrote:
> Add a comment as to why you disregard PREF_READ_ERROR_NO_FILE here.

Done. See also the class comments in the header.

[gab, 2014-04-02 18:24:37]

lg, just had another another idea which I think solves the migration problem,
PTAL!

Gab

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

https://codereview.chromium.org/218583003/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:314:
protected_pref_hash_filter.PassAs<PrefFilter>()));
On 2014/04/02 15:03:17, erikwright wrote:
> On 2014/04/01 20:19:52, gab wrote:
> > So migration is icky the way I see it, since both PrefHashFilter use the
same
> > underlying HashStoreContents (i.e. local_state).
> > 
> > On migration:
> > 1) Unprotected store loads and reports UNCHANGED for its values.
> 
> Yes, but only for tracking-only values, and this is as expected.
> 
> > 2) Protected store loads and reports CLEARED for its values (as it has no
> values
> > in store yet it sees MACs in its hash store) and it sets MACs to match NULL
> > values.
> 
> Yes, that's true.
> 
> > 3) MigrateValues kicks in and would reset every migrated value because the
> hash
> > was modified -- except it happens to be using an old copy of the
> > HashStoreContents built on line 322... so it's doesn't... and it then stores
> the
> > correct values which restores the hashes and thus it kinda works...
> > 
> > This would at the very least generate an undesired CLEARED spike for all
> > migrated values I think.
> > 
> > A better approach IMO would be to:
> > 1) Harden the protected values left in the unprotected store.
> > 2) Migrate those values to the protected store as-is once we know they're
> clean.
> 
> This is equally messy, because you have to provide the unprotected store with
a
> full configuration to protect all of the values, when, post-migration, it's
only
> responsible for a subset.
> 
> > I don't really see how we do this cleanly either though... I think the
easiest
> > thing is really to not have this intermediate state where Protected
> Preferences
> > use local_state as a hash store. How much harder is it to go straight there?
> 
> I think it's a few days away from a code review.
> 
> Here's a solution I just thought of that might solve the problem:
> 
> (1) Have the PrefHashFilter store a list of all known protected values in the
> pref store.
> (2) In FilterOnLoad, skip verification of any value that is configured for
> protection but is not in the loaded dictionary. DO NOT touch the stored MAC,
if
> any.
> (3) Of course, in FilterUpdate, we add the updated value into the list of
known
> protected values (if it is protected) or remove it from the list (if it is
> unprotected, i.e., if we are removing a previously protected value to migrate
it
> back to the main store).
> 
> WDYT?

Here is another idea, taking advantage of two things:
1) Assume that Preferences will load before Protected Preferences (by virtue of
the order in which the tasks are pushed on the worker threads).
2) Use PrefHashFilter as a true filter.

And do this:
1) Use the same PrefHashFilter for both stores (configured for all
tracked+protected prefs).
2) Augment FilterOnLoad to take a "phase" argument (PHASE_UNPROTECTED,
PHASE_PROTECTED).
3) Have the JsonPrefStores know which phase they are part of so they can pass
this information to PrefHashFilter::FilterOnLoad().
4) In FilterOnLoad(PHASE_UNPROTECTED), retain the value of any protected
preference (and remove it from the underlying store).
5) In FilterOnLoad(PHASE_PROTECTED), skip unprotected values, for any missing
protected value, inject the one that was retained in the UNPROTECTED phase and
harden it from there.

Note that this approach does two things:
1) It takes care of the value migration.
2) It hardens values directly in the protected store as if they'd always been
there. The filter does all the work (as a true filter) before anyone else is
made aware of anything :)!

WDYT?!

[erikwright (departed), 2014-04-03 13:10:07]

I have decided that my proposed solution to avoid the CLEARED spike is too
complicated to merit implementation.

Given Robert's preference to land this even if we can't avoid the CLEARED spike,
please let me know if any other issues are outstanding.

[gab, 2014-04-03 15:38:48]

lgtm w/ nits that can be addressed in a follow-up CL.

Cheers!
Gab

https://codereview.chromium.org/218583003/diff/80001/chrome/browser/prefs/pro...
File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right):

https://codereview.chromium.org/218583003/diff/80001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:65: const char
kUnprotectedAtomic[] = "unprotected_atomic";
nit: s/unprotected_atomic/unprotected_pref

"atomic" doesn't mean anything in the unprotected world.

https://codereview.chromium.org/218583003/diff/80001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:101: it->name, "",
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
nit: s/""/std::string()

[erikwright (departed), 2014-04-03 15:39:50]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-04-03 15:40:33]

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

[commit-bot: I haz the power, 2014-04-03 16:46:55]

Change committed as 261448