Separate storage for protected preferences into Protected Preferences file.

chrome/browser/prefs/profile_pref_store_manager.cc

Index: chrome/browser/prefs/profile_pref_store_manager.cc
diff --git a/chrome/browser/prefs/profile_pref_store_manager.cc b/chrome/browser/prefs/profile_pref_store_manager.cc
index fc350a04902b467d889d39525de8ece17c21cc98..17dab987bece2f77bbde3f6f107271df01b995e9 100644
--- a/chrome/browser/prefs/profile_pref_store_manager.cc
+++ b/chrome/browser/prefs/profile_pref_store_manager.cc
@@ -13,12 +13,99 @@
 #include "base/prefs/pref_registry_simple.h"
 #include "chrome/browser/prefs/pref_hash_store_impl.h"
 #include "chrome/browser/prefs/tracked/pref_service_hash_store_contents.h"
+#include "chrome/browser/prefs/tracked/segregated_pref_store.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 
 namespace {
 
+// An adaptor that allows a PrefHashStoreImpl to access a preference store
+// directly as a dictionary. Uses an equivalent layout to
+// PrefStoreHashStoreContents.
+class DictionaryHashStoreContents : public HashStoreContents {
+ public:
+  // Instantiates a HashStoreContents that is a copy of |to_copy|. The copy is
+  // mutable but does not affect the original, nor is it persisted to disk in
+  // any other way.
+  explicit DictionaryHashStoreContents(const HashStoreContents& to_copy)
+      : hash_store_id_(to_copy.hash_store_id()),
+        super_mac_(to_copy.GetSuperMac()) {
+    if (to_copy.IsInitialized())
+      dictionary_.reset(to_copy.GetContents()->DeepCopy());
+    int version = 0;
+    if (to_copy.GetVersion(&version))
+      version_.reset(new int(version));
+  }
+
+  // HashStoreContents implementation
+  virtual std::string hash_store_id() const OVERRIDE { return hash_store_id_; }
+
+  virtual void Reset() OVERRIDE {
+    dictionary_.reset();
+    super_mac_.clear();
+    version_.reset();
+  }
+
+  virtual bool IsInitialized() const OVERRIDE {
+    return dictionary_;
+  }
+
+  virtual const base::DictionaryValue* GetContents() const OVERRIDE{
+    return dictionary_.get();
+  }
+
+  virtual scoped_ptr<MutableDictionary> GetMutableContents() OVERRIDE {
+    return scoped_ptr<MutableDictionary>(
+        new SimpleMutableDictionary(this));
+  }
+
+  virtual std::string GetSuperMac() const OVERRIDE { return super_mac_; }
+
+  virtual void SetSuperMac(const std::string& super_mac) OVERRIDE {
+    super_mac_ = super_mac;
+  }
+
+  virtual bool GetVersion(int* version) const OVERRIDE {
+    if (!version_)
+      return false;
+    *version = *version_;
+    return true;
+  }
+
+  virtual void SetVersion(int version) OVERRIDE {
+    version_.reset(new int(version));
+  }
+
+ private:
+  class SimpleMutableDictionary
+      : public HashStoreContents::MutableDictionary {
+   public:
+    explicit SimpleMutableDictionary(DictionaryHashStoreContents* outer)
+        : outer_(outer) {}
+
+    virtual ~SimpleMutableDictionary() {}
+
+    // MutableDictionary implementation
+    virtual base::DictionaryValue* operator->() OVERRIDE {
+      if (!outer_->dictionary_)
+        outer_->dictionary_.reset(new base::DictionaryValue);
+      return outer_->dictionary_.get();
+    }
+
+   private:
+    DictionaryHashStoreContents* outer_;
+    DISALLOW_COPY_AND_ASSIGN(SimpleMutableDictionary);

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

nit: +empty line above

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+  };
+
+  const std::string hash_store_id_;
+  std::string super_mac_;
+  scoped_ptr<int> version_;
+  scoped_ptr<base::DictionaryValue> dictionary_;
+
+  DISALLOW_COPY_AND_ASSIGN(DictionaryHashStoreContents);
+};
+
 // An in-memory PrefStore backed by an immutable DictionaryValue.
 class DictionaryPrefStore : public PrefStore {
  public:
@@ -187,15 +274,56 @@ void ProfilePrefStoreManager::ResetPrefHashStore() {
 PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
     const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
   scoped_ptr<PrefFilter> pref_filter;
-  if (kPlatformSupportsPreferenceTracking) {
-    pref_filter.reset(
-        new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
-                           tracking_configuration_,
-                           reporting_ids_count_));
+  if (!kPlatformSupportsPreferenceTracking) {
+    return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                             io_task_runner,
+                             scoped_ptr<PrefFilter>());
   }
-  return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
-                           io_task_runner,
-                           pref_filter.Pass());
+
+  std::vector<PrefHashFilter::TrackedPreferenceMetadata>
+      unprotected_configuration;
+  std::vector<PrefHashFilter::TrackedPreferenceMetadata>
+      protected_configuration;
+  std::set<std::string> protected_pref_names;
+  for (size_t i = 0; i < tracking_configuration_.size(); ++i) {

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

Use a std::vector<std::string>::const_iterator instead.

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+    if (tracking_configuration_[i].enforcement_level >
+        PrefHashFilter::NO_ENFORCEMENT) {
+      protected_configuration.push_back(tracking_configuration_[i]);
+      protected_pref_names.insert(tracking_configuration_[i].name);
+    } else {
+      unprotected_configuration.push_back(tracking_configuration_[i]);
+    }
+  }
+
+  scoped_ptr<PrefHashFilter> unprotected_pref_hash_filter(

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

Store these directly in scoped_ptr<PrefFilter>'s since you don't use them as
PrefHashFilter's and it will avoid a PassAs below.

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+      new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+                         unprotected_configuration,
+                         reporting_ids_count_));
+  scoped_ptr<PrefHashFilter> protected_pref_hash_filter(
+      new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),

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

Why not start putting hashes in the Protected Preferences file in the same CL?
Seems like otherwise we'll introduce yet another migration case :(.

[erikwright (departed), 2014/04/02 15:03:17]

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.

+                         protected_configuration,
+                         reporting_ids_count_));
+
+  scoped_refptr<PersistentPrefStore> unprotected_pref_store(
+      new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                        io_task_runner,
+                        unprotected_pref_hash_filter.PassAs<PrefFilter>()));
+  scoped_refptr<PersistentPrefStore> protected_pref_store(new JsonPrefStore(
+      profile_path_.Append(chrome::kProtectedPreferencesFilename),
+      io_task_runner,
+      protected_pref_hash_filter.PassAs<PrefFilter>()));

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

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?

[erikwright (departed), 2014/04/02 15:03:17]

Yes, but only for tracking-only values, and this is as expected.
Yes, that's true.
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 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?

[gab, 2014/04/02 18:24:38]

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?!

+
+  return new SegregatedPrefStore(
+      unprotected_pref_store,
+      protected_pref_store,
+      protected_pref_names,
+      base::Bind(&PrefHashFilter::MigrateValues,
+                 base::Owned(new PrefHashFilter(
+                     CopyPrefHashStore(),
+                     protected_configuration,
+                     reporting_ids_count_)),

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

Meta-comment either here or on MigrateValues() about how this dance works.

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+                 unprotected_pref_store,
+                 protected_pref_store));
 }
 
 void ProfilePrefStoreManager::UpdateProfileHashStoreIfRequired(
@@ -231,6 +359,8 @@ bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
   if (!base::CreateDirectory(profile_path_))
     return false;
 
+  // This will write out to a single combined file which will be immediately
+  // migrated to two files on load.
   JSONFileValueSerializer serializer(
       GetPrefFilePathFromProfilePath(profile_path_));
 
@@ -253,6 +383,21 @@ bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
   return success;
 }
 
+PersistentPrefStore*
+ProfilePrefStoreManager::CreateDeprecatedCombinedProfilePrefStore(
+    const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
+  scoped_ptr<PrefFilter> pref_filter;
+  if (kPlatformSupportsPreferenceTracking) {
+    pref_filter.reset(
+        new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+                           tracking_configuration_,
+                           reporting_ids_count_));
+  }
+  return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                           io_task_runner,
+                           pref_filter.Pass());
+}
+
 scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl() {
   DCHECK(kPlatformSupportsPreferenceTracking);
 
@@ -262,3 +407,15 @@ scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl() {
       scoped_ptr<HashStoreContents>(new PrefServiceHashStoreContents(
           profile_path_.AsUTF8Unsafe(), local_state_))));
 }
+
+scoped_ptr<PrefHashStore> ProfilePrefStoreManager::CopyPrefHashStore() {
+  DCHECK(kPlatformSupportsPreferenceTracking);
+
+  PrefServiceHashStoreContents real_contents(profile_path_.AsUTF8Unsafe(),
+                                             local_state_);
+  return scoped_ptr<PrefHashStore>(new PrefHashStoreImpl(
+      seed_,
+      device_id_,
+      scoped_ptr<HashStoreContents>(
+          new DictionaryHashStoreContents(real_contents))));
+}