Move preference MACs to the protected preference stores.

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 d5a64b05a4b8502064d0d1d536fce315db014b7f..c666bb28c9a6c0f4d7a5b3a23fdf484725aadc10 100644
--- a/chrome/browser/prefs/profile_pref_store_manager.cc
+++ b/chrome/browser/prefs/profile_pref_store_manager.cc
@@ -14,6 +14,7 @@
 #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/pref_store_hash_store_contents.h"
 #include "chrome/browser/prefs/tracked/segregated_pref_store.h"
 #include "chrome/browser/prefs/tracked/tracked_preferences_migration.h"
 #include "chrome/common/chrome_constants.h"
@@ -22,104 +23,50 @@
 
 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 {
+// An in-memory PrefStore backed by an immutable DictionaryValue.
+class DictionaryPrefStore : public WriteablePrefStore {
  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_; }
+  explicit DictionaryPrefStore(base::DictionaryValue* dictionary)
+      : dictionary_(dictionary) {}
 
-  virtual void Reset() OVERRIDE {
-    dictionary_.reset();
-    super_mac_.clear();
-    version_.reset();
-  }
+  // PrefStore implementation
+  virtual bool GetValue(const std::string& key,
+                        const base::Value** result) const OVERRIDE {
+    const base::Value* tmp = NULL;
+    if (!dictionary_->Get(key, &tmp))
+      return false;
 
-  virtual bool IsInitialized() const OVERRIDE {
-    return dictionary_;
+    if (result)
+      *result = tmp;
+    return true;
   }
 
-  virtual const base::DictionaryValue* GetContents() const OVERRIDE{
-    return dictionary_.get();
+  virtual void AddObserver(PrefStore::Observer* observer) OVERRIDE {
+    observers_.AddObserver(observer);
   }
 
-  virtual scoped_ptr<MutableDictionary> GetMutableContents() OVERRIDE {
-    return scoped_ptr<MutableDictionary>(
-        new SimpleMutableDictionary(this));
+  virtual void RemoveObserver(PrefStore::Observer* observer) OVERRIDE {
+    observers_.RemoveObserver(observer);
   }
 
-  virtual std::string GetSuperMac() const OVERRIDE { return super_mac_; }
-
-  virtual void SetSuperMac(const std::string& super_mac) OVERRIDE {
-    super_mac_ = super_mac;
+  virtual bool HasObservers() const OVERRIDE {
+    return observers_.might_have_observers();
   }
 
-  virtual bool GetVersion(int* version) const OVERRIDE {
-    if (!version_)
-      return false;
-    *version = *version_;
-    return true;
+  // WriteablePrefStore implementation
+  virtual void SetValue(const std::string& key, base::Value* value) OVERRIDE {
+    dictionary_->Set(key, value);
+    FOR_EACH_OBSERVER(Observer, observers_, OnPrefValueChanged(key));

[gab, 2014/06/06 21:54:59]

Call ReportValueChanged() here and below instead of making the call explicitly
(and thus expanding the macro in multiple places)?

[erikwright (departed), 2014/06/10 20:27:47]

Done.

   }
 
-  virtual void SetVersion(int version) OVERRIDE {
-    version_.reset(new int(version));
+  virtual void RemoveValue(const std::string& key) OVERRIDE {
+    dictionary_->Remove(key, NULL);
+    FOR_EACH_OBSERVER(Observer, observers_, OnPrefValueChanged(key));
   }
 
-  virtual void CommitPendingWrite() OVERRIDE {}
-
- 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);
-  };
-
-  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:
-  explicit DictionaryPrefStore(const base::DictionaryValue* dictionary)
-      : dictionary_(dictionary) {}
-
-  virtual bool GetValue(const std::string& key,
-                        const base::Value** result) const OVERRIDE {
-    const base::Value* tmp = NULL;
+  virtual bool GetMutableValue(const std::string& key,
+                               base::Value** result) OVERRIDE {
+    base::Value* tmp = NULL;
     if (!dictionary_->Get(key, &tmp))
       return false;
 
@@ -128,69 +75,55 @@ class DictionaryPrefStore : public PrefStore {
     return true;

[gab, 2014/06/06 21:54:59]

Just like JsonPrefStore::GetMutableValue(), I think
return dictionary_->Get(key, result);
works just fine here.

(and I think it also works fine for GetValue()).

[erikwright (departed), 2014/06/10 20:27:47]

Done.

   }
 
+  virtual void ReportValueChanged(const std::string& key) OVERRIDE {
+    FOR_EACH_OBSERVER(Observer, observers_, OnPrefValueChanged(key));
+  }
+
  private:
   virtual ~DictionaryPrefStore() {}
 
-  const base::DictionaryValue* dictionary_;
+  base::DictionaryValue* dictionary_;
+  ObserverList<PrefStore::Observer, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(DictionaryPrefStore);
 };
 
-// Waits for a PrefStore to be initialized and then initializes the
-// corresponding PrefHashStore.
-// The observer deletes itself when its work is completed.
-class InitializeHashStoreObserver : public PrefStore::Observer {
+class MigrationDelegateImpl : public TrackedPreferencesMigrationDelegate {
  public:
-  // Creates an observer that will initialize |pref_hash_store| with the
-  // contents of |pref_store| when the latter is fully loaded.
-  InitializeHashStoreObserver(
-      const std::vector<PrefHashFilter::TrackedPreferenceMetadata>&
-          tracking_configuration,
-      size_t reporting_ids_count,
-      const scoped_refptr<PrefStore>& pref_store,
-      scoped_ptr<PrefHashStoreImpl> pref_hash_store_impl)
-      : tracking_configuration_(tracking_configuration),
-        reporting_ids_count_(reporting_ids_count),
-        pref_store_(pref_store),
-        pref_hash_store_impl_(pref_hash_store_impl.Pass()) {}
-
-  virtual ~InitializeHashStoreObserver();
-
-  // PrefStore::Observer implementation.
-  virtual void OnPrefValueChanged(const std::string& key) OVERRIDE;
-  virtual void OnInitializationCompleted(bool succeeded) OVERRIDE;
+  MigrationDelegateImpl(const base::WeakPtr<JsonPrefStore>& pref_store,

[gab, 2014/06/06 21:54:59]

Although this delegate is potentially a nice clean up (it would need to check
weak pointers properly below though), I don't think it's necessary in order to
get working code so I'd vote against it in this CL since you may want to request
merge.

+                        InterceptablePrefFilter* pref_filter,
+                        PrefHashStore* pref_hash_store)
+      : pref_store_(pref_store),
+        pref_filter_(pref_filter),
+        pref_hash_store_(pref_hash_store) {}
+  virtual void CleanPreference(const std::string& key) OVERRIDE;
+  virtual void NotifyOnSuccessfulWrite(
+      const base::Closure& on_successful_write) OVERRIDE;
+  virtual void InterceptLoadedPreferences(const Intercept& intercept) OVERRIDE;
+  virtual PrefHashStore* GetPrefHashStore() OVERRIDE;
 
  private:
-  const std::vector<PrefHashFilter::TrackedPreferenceMetadata>
-      tracking_configuration_;
-  const size_t reporting_ids_count_;
-  scoped_refptr<PrefStore> pref_store_;
-  scoped_ptr<PrefHashStoreImpl> pref_hash_store_impl_;
-
-  DISALLOW_COPY_AND_ASSIGN(InitializeHashStoreObserver);
+  base::WeakPtr<JsonPrefStore> pref_store_;
+  InterceptablePrefFilter* pref_filter_;
+  PrefHashStore* pref_hash_store_;
 };
 
-InitializeHashStoreObserver::~InitializeHashStoreObserver() {}
+void MigrationDelegateImpl::CleanPreference(const std::string& key) {
+  pref_store_->RemoveValueSilently(key);
+}
 
-void InitializeHashStoreObserver::OnPrefValueChanged(const std::string& key) {}
+void MigrationDelegateImpl::NotifyOnSuccessfulWrite(
+    const base::Closure& on_successful_write) {
+  pref_store_->RegisterOnNextSuccessfulWriteCallback(on_successful_write);
+}
 
-void InitializeHashStoreObserver::OnInitializationCompleted(bool succeeded) {
-  // If we successfully loaded the preferences _and_ the PrefHashStoreImpl
-  // hasn't been initialized by someone else in the meantime, initialize it now.
-  const PrefHashStoreImpl::StoreVersion pre_update_version =
-      pref_hash_store_impl_->GetCurrentVersion();
-  if (succeeded && pre_update_version < PrefHashStoreImpl::VERSION_LATEST) {
-    PrefHashFilter(pref_hash_store_impl_.PassAs<PrefHashStore>(),
-                   tracking_configuration_,
-                   NULL,
-                   reporting_ids_count_).Initialize(*pref_store_);
-    UMA_HISTOGRAM_ENUMERATION(
-        "Settings.TrackedPreferencesAlternateStoreVersionUpdatedFrom",
-        pre_update_version,
-        PrefHashStoreImpl::VERSION_LATEST + 1);
-  }
-  pref_store_->RemoveObserver(this);
-  delete this;
+void MigrationDelegateImpl::InterceptLoadedPreferences(
+    const Intercept& intercept) {
+  pref_filter_->InterceptNextFilterOnLoad(intercept);
+}
+
+PrefHashStore* MigrationDelegateImpl::GetPrefHashStore() {
+  return pref_hash_store_;
 }
 
 }  // namespace
@@ -287,14 +220,22 @@ PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
       unprotected_pref_names.insert(it->name);
     }
   }
+  scoped_ptr<PrefHashStoreImpl> unprotected_pref_hash_store =
+      GetPrefHashStoreImpl();
+  scoped_ptr<PrefHashStoreImpl> protected_pref_hash_store =
+      GetPrefHashStoreImpl();
+  PrefHashStoreImpl* raw_unprotected_pref_hash_store =
+      unprotected_pref_hash_store.get();
+  PrefHashStoreImpl* raw_protected_pref_hash_store =
+      protected_pref_hash_store.get();
 
   scoped_ptr<PrefHashFilter> unprotected_pref_hash_filter(
-      new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+      new PrefHashFilter(unprotected_pref_hash_store.PassAs<PrefHashStore>(),
                          unprotected_configuration,
                          validation_delegate,
                          reporting_ids_count_));
   scoped_ptr<PrefHashFilter> protected_pref_hash_filter(
-      new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+      new PrefHashFilter(protected_pref_hash_store.PassAs<PrefHashStore>(),
                          protected_configuration,
                          validation_delegate,
                          reporting_ids_count_));
@@ -313,50 +254,33 @@ PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
       io_task_runner,
       protected_pref_hash_filter.PassAs<PrefFilter>()));
 
+  scoped_ptr<HashStoreContents> unprotected_hash_store_contents(
+      new PrefStoreHashStoreContents(unprotected_pref_store));
+  scoped_ptr<HashStoreContents> protected_hash_store_contents(
+      new PrefStoreHashStoreContents(protected_pref_store));
+
+  raw_unprotected_pref_hash_store->SetHashStoreContents(
+      unprotected_hash_store_contents.Pass());
+  raw_protected_pref_hash_store->SetHashStoreContents(
+      protected_hash_store_contents.Pass());
+
   SetupTrackedPreferencesMigration(
       unprotected_pref_names,
       protected_pref_names,
-      base::Bind(&JsonPrefStore::RemoveValueSilently,
-                 unprotected_pref_store->AsWeakPtr()),
-      base::Bind(&JsonPrefStore::RemoveValueSilently,
-                 protected_pref_store->AsWeakPtr()),
-      base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteCallback,
-                 unprotected_pref_store->AsWeakPtr()),
-      base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteCallback,
-                 protected_pref_store->AsWeakPtr()),
-      raw_unprotected_pref_hash_filter,
-      raw_protected_pref_hash_filter);
+      scoped_ptr<TrackedPreferencesMigrationDelegate>(
+          new MigrationDelegateImpl(unprotected_pref_store->AsWeakPtr(),
+                                    raw_unprotected_pref_hash_filter,
+                                    raw_unprotected_pref_hash_store)),
+      scoped_ptr<TrackedPreferencesMigrationDelegate>(
+          new MigrationDelegateImpl(protected_pref_store->AsWeakPtr(),
+                                    raw_protected_pref_hash_filter,
+                                    raw_protected_pref_hash_store)),
+      GetLegacyPrefHashStoreImpl().PassAs<PrefHashStore>());
 
   return new SegregatedPrefStore(unprotected_pref_store, protected_pref_store,
                                  protected_pref_names);
 }
 
-void ProfilePrefStoreManager::UpdateProfileHashStoreIfRequired(
-    const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
-  if (!kPlatformSupportsPreferenceTracking)
-    return;
-  scoped_ptr<PrefHashStoreImpl> pref_hash_store_impl(GetPrefHashStoreImpl());
-  const PrefHashStoreImpl::StoreVersion current_version =
-      pref_hash_store_impl->GetCurrentVersion();
-  UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferencesAlternateStoreVersion",
-                            current_version,
-                            PrefHashStoreImpl::VERSION_LATEST + 1);
-
-  // Update the pref hash store if it's not at the latest version.
-  if (current_version != PrefHashStoreImpl::VERSION_LATEST) {
-    scoped_refptr<JsonPrefStore> pref_store =
-        new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
-                          io_task_runner,
-                          scoped_ptr<PrefFilter>());
-    pref_store->AddObserver(
-        new InitializeHashStoreObserver(tracking_configuration_,
-                                        reporting_ids_count_,
-                                        pref_store,
-                                        pref_hash_store_impl.Pass()));
-    pref_store->ReadPrefsAsync(NULL);
-  }
-}
-
 bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
     const base::DictionaryValue& master_prefs) {
   // Create the profile directory if it doesn't exist yet (very possible on
@@ -364,6 +288,21 @@ bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
   if (!base::CreateDirectory(profile_path_))
     return false;
 
+  scoped_ptr<base::DictionaryValue> protected_master_prefs;
+
+  if (kPlatformSupportsPreferenceTracking) {
+    protected_master_prefs.reset(master_prefs.DeepCopy());
+    scoped_refptr<WriteablePrefStore> pref_store(
+        new DictionaryPrefStore(protected_master_prefs.get()));
+    scoped_ptr<PrefHashStoreImpl> pref_hash_store(GetPrefHashStoreImpl());
+    pref_hash_store->SetHashStoreContents(scoped_ptr<HashStoreContents>(
+        new PrefStoreHashStoreContents(pref_store)));
+    PrefHashFilter(pref_hash_store.PassAs<PrefHashStore>(),
+                   tracking_configuration_,
+                   NULL,
+                   reporting_ids_count_).Initialize(*pref_store);
+  }

[gab, 2014/06/06 21:54:59]

In my proposed model this logic can remain as it used to be.

[erikwright (departed), 2014/06/10 20:27:47]

Hmm. I don't see how. Previously we wrote out the values without MACs and then
(in a second step) wrote the MACs to local state (in memory). Now we need to
augment the dictionary with MACs before writing it out.

[gab, 2014/06/11 18:23:55]

This new code is required because of the circular dependency. In my proposal,
where FilterOnLoad temporarily owns the DictionaryValue* and is in charge of
building a DictionaryHashStoreContents to initiate a PrefHashStoreTransaction
from: all of the subtleties happens in FilterOnLoad, the initialization code
here doesn't need to be aware of any of that.

+
   // This will write out to a single combined file which will be immediately
   // migrated to two files on load.
   JSONFileValueSerializer serializer(
@@ -374,16 +313,8 @@ bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
   // complete before Chrome can start (as master preferences seed the Local
   // State and Preferences files). This won't trip ThreadIORestrictions as they
   // won't have kicked in yet on the main thread.
-  bool success = serializer.Serialize(master_prefs);
-
-  if (success && kPlatformSupportsPreferenceTracking) {
-    scoped_refptr<const PrefStore> pref_store(
-        new DictionaryPrefStore(&master_prefs));
-    PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
-                   tracking_configuration_,
-                   NULL,
-                   reporting_ids_count_).Initialize(*pref_store);
-  }
+  bool success = serializer.Serialize(
+      protected_master_prefs ? *protected_master_prefs : master_prefs);
 
   UMA_HISTOGRAM_BOOLEAN("Settings.InitializedFromMasterPrefs", success);
   return success;
@@ -395,7 +326,7 @@ ProfilePrefStoreManager::CreateDeprecatedCombinedProfilePrefStore(
   scoped_ptr<PrefFilter> pref_filter;
   if (kPlatformSupportsPreferenceTracking) {
     pref_filter.reset(
-        new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+        new PrefHashFilter(GetLegacyPrefHashStoreImpl().PassAs<PrefHashStore>(),
                            tracking_configuration_,
                            NULL,
                            reporting_ids_count_));
@@ -408,9 +339,17 @@ ProfilePrefStoreManager::CreateDeprecatedCombinedProfilePrefStore(
 scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl() {
   DCHECK(kPlatformSupportsPreferenceTracking);
 
-  return make_scoped_ptr(new PrefHashStoreImpl(
-      seed_,
-      device_id_,
+  return make_scoped_ptr(new PrefHashStoreImpl(seed_, device_id_));
+}
+
+scoped_ptr<PrefHashStoreImpl>
+ProfilePrefStoreManager::GetLegacyPrefHashStoreImpl() {
+  DCHECK(kPlatformSupportsPreferenceTracking);
+
+  scoped_ptr<PrefHashStoreImpl> instance(
+      new PrefHashStoreImpl(seed_, device_id_));
+  instance->SetHashStoreContents(
       scoped_ptr<HashStoreContents>(new PrefServiceHashStoreContents(
-          profile_path_.AsUTF8Unsafe(), local_state_))));
+          profile_path_.AsUTF8Unsafe(), local_state_)));
+  return instance.Pass();
 }