Move preference MACs to the protected preference stores.

chrome/browser/prefs/tracked/tracked_preferences_migration.cc

Index: chrome/browser/prefs/tracked/tracked_preferences_migration.cc
diff --git a/chrome/browser/prefs/tracked/tracked_preferences_migration.cc b/chrome/browser/prefs/tracked/tracked_preferences_migration.cc
index c358256f06b4f50ff9e88b6045c82634d2b41586..1d74f97558260a90861f500cec01a908aa99a938 100644
--- a/chrome/browser/prefs/tracked/tracked_preferences_migration.cc
+++ b/chrome/browser/prefs/tracked/tracked_preferences_migration.cc
@@ -11,6 +11,8 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/interceptable_pref_filter.h"
+#include "chrome/browser/prefs/pref_hash_store.h"
+#include "chrome/browser/prefs/pref_hash_store_transaction.h"
 
 namespace {
 
@@ -20,16 +22,9 @@ class TrackedPreferencesMigrator
   TrackedPreferencesMigrator(
       const std::set<std::string>& unprotected_pref_names,
       const std::set<std::string>& protected_pref_names,
-      const base::Callback<void(const std::string& key)>&
-          unprotected_store_cleaner,
-      const base::Callback<void(const std::string& key)>&
-          protected_store_cleaner,
-      const base::Callback<void(const base::Closure&)>&
-          register_on_successful_unprotected_store_write_callback,
-      const base::Callback<void(const base::Closure&)>&
-          register_on_successful_protected_store_write_callback,
-      InterceptablePrefFilter* unprotected_pref_filter,
-      InterceptablePrefFilter* protected_pref_filter);
+      scoped_ptr<TrackedPreferencesMigrationDelegate> unprotected_delegate,
+      scoped_ptr<TrackedPreferencesMigrationDelegate> protected_delegate,
+      scoped_ptr<PrefHashStore> legacy_pref_hash_store);
 
  private:
   friend class base::RefCounted<TrackedPreferencesMigrator>;
@@ -47,7 +42,17 @@ class TrackedPreferencesMigrator
       PrefFilterID id,
       const InterceptablePrefFilter::FinalizeFilterOnLoadCallback&
           finalize_filter_on_load,
-      scoped_ptr<base::DictionaryValue> prefs);
+      base::DictionaryValue* prefs);
+
+  void CleanupPrefStore(
+      TrackedPreferencesMigrationDelegate* delegate,
+      const std::set<std::string>& keys_to_clean);
+
+  void ScheduleSourcePrefStoreCleanup(
+      TrackedPreferencesMigrationDelegate* source_delegate,
+      TrackedPreferencesMigrationDelegate* destination_delegate,
+      const std::set<std::string>& keys_to_clean,
+      bool wait_for_commit_to_destination_store);
 
   // Proceeds with migration if both |unprotected_prefs_| and |protected_prefs_|
   // have been set.
@@ -56,31 +61,36 @@ class TrackedPreferencesMigrator
   const std::set<std::string> unprotected_pref_names_;
   const std::set<std::string> protected_pref_names_;
 
-  const base::Callback<void(const std::string& key)> unprotected_store_cleaner_;
-  const base::Callback<void(const std::string& key)> protected_store_cleaner_;
-  const base::Callback<void(const base::Closure&)>
-      register_on_successful_unprotected_store_write_callback_;
-  const base::Callback<void(const base::Closure&)>
-      register_on_successful_protected_store_write_callback_;
+  scoped_ptr<TrackedPreferencesMigrationDelegate> unprotected_delegate_;
+  scoped_ptr<TrackedPreferencesMigrationDelegate> protected_delegate_;
+  scoped_ptr<PrefHashStore> legacy_pref_hash_store_;
+  TrackedPreferencesMigrationDelegate::InterceptCompletionCallback
+      unprotected_intercept_completion_callback_;
+  TrackedPreferencesMigrationDelegate::InterceptCompletionCallback
+      protected_intercept_completion_callback_;
 
-  InterceptablePrefFilter::FinalizeFilterOnLoadCallback
-      finalize_unprotected_filter_on_load_;
-  InterceptablePrefFilter::FinalizeFilterOnLoadCallback
-      finalize_protected_filter_on_load_;
-
-  scoped_ptr<base::DictionaryValue> unprotected_prefs_;
-  scoped_ptr<base::DictionaryValue> protected_prefs_;
+  base::DictionaryValue* unprotected_prefs_;
+  base::DictionaryValue* protected_prefs_;
 
   DISALLOW_COPY_AND_ASSIGN(TrackedPreferencesMigrator);
 };
 
-// Invokes |store_cleaner| for every |keys_to_clean|.
-void CleanupPrefStore(
-    const base::Callback<void(const std::string& key)>& store_cleaner,
+// Removes the value and hash of each preference in |keys_to_clean| using
+// |delegate|.
+void TrackedPreferencesMigrator::CleanupPrefStore(
+    TrackedPreferencesMigrationDelegate* delegate,
     const std::set<std::string>& keys_to_clean) {
+  if (!delegate->IsValid())
+    return;
+
+  DCHECK(delegate->GetPrefHashStore());
+
+  scoped_ptr<PrefHashStoreTransaction> hash_transaction(
+      delegate->GetPrefHashStore()->BeginTransaction());
   for (std::set<std::string>::const_iterator it = keys_to_clean.begin();
        it != keys_to_clean.end(); ++it) {
-    store_cleaner.Run(*it);
+    delegate->CleanPreference(*it);
+    hash_transaction->ClearHash(*it);
   }
 }
 
@@ -89,18 +99,24 @@ void CleanupPrefStore(
 // |keys_to_clean| from the source pref store (through |source_store_cleaner|)
 // once the destination pref store they were migrated to was successfully
 // written to disk. Otherwise, executes the cleanup right away.
-void ScheduleSourcePrefStoreCleanup(
-    const base::Callback<void(const base::Closure&)>&
-        register_on_successful_destination_store_write_callback,
-    const base::Callback<void(const std::string& key)>& source_store_cleaner,
+void
+TrackedPreferencesMigrator::ScheduleSourcePrefStoreCleanup(

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

This fits on the previous line I think.

+    TrackedPreferencesMigrationDelegate* source_delegate,
+    TrackedPreferencesMigrationDelegate* destination_delegate,
     const std::set<std::string>& keys_to_clean,
     bool wait_for_commit_to_destination_store) {
+  DCHECK(source_delegate->IsValid());
+  DCHECK(destination_delegate->IsValid());
+
   DCHECK(!keys_to_clean.empty());
   if (wait_for_commit_to_destination_store) {
-    register_on_successful_destination_store_write_callback.Run(
-        base::Bind(&CleanupPrefStore, source_store_cleaner, keys_to_clean));
+    destination_delegate->NotifyOnSuccessfulWrite(base::Bind(
+        &TrackedPreferencesMigrator::CleanupPrefStore,
+        this,
+        source_delegate,
+        base::ConstRef(keys_to_clean)));
   } else {
-    CleanupPrefStore(source_store_cleaner, keys_to_clean);
+    CleanupPrefStore(source_delegate, keys_to_clean);
   }
 }
 
@@ -111,60 +127,80 @@ void ScheduleSourcePrefStoreCleanup(
 void MigratePrefsFromOldToNewStore(const std::set<std::string>& pref_names,
                                    const base::DictionaryValue* old_store,
                                    base::DictionaryValue* new_store,
+                                   PrefHashStore* old_pref_hash_store,
+                                   PrefHashStore* new_pref_hash_store,
+                                   PrefHashStore* legacy_pref_hash_store,
                                    bool* old_store_needs_cleanup,
                                    bool* new_store_altered) {
+  scoped_ptr<PrefHashStoreTransaction> source_transaction(
+      old_pref_hash_store->BeginTransaction());
+  scoped_ptr<PrefHashStoreTransaction> destination_transaction(
+      new_pref_hash_store->BeginTransaction());
+  scoped_ptr<PrefHashStoreTransaction> legacy_transaction(
+      legacy_pref_hash_store->BeginTransaction());
+
   for (std::set<std::string>::const_iterator it = pref_names.begin();
-       it != pref_names.end(); ++it) {
+       it != pref_names.end();
+       ++it) {
     const std::string& pref_name = *it;
 
     const base::Value* value_in_old_store = NULL;
-    if (!old_store->Get(pref_name, &value_in_old_store))
-      continue;
-
-    // Whether this value ends up being copied below or was left behind by a
-    // previous incomplete migration, it should be cleaned up.
-    *old_store_needs_cleanup = true;
-
-    if (new_store->Get(pref_name, NULL))
-      continue;
-
-    // Copy the value from |old_store| to |new_store| rather than moving it to
-    // avoid data loss should |old_store| be flushed to disk without |new_store|
-    // having equivalently been successfully flushed to disk (e.g., on crash or
-    // in cases where |new_store| is read-only following a read error on
-    // startup).
-    new_store->Set(pref_name, value_in_old_store->DeepCopy());
-    *new_store_altered = true;
+    // If the destination does not have a hash for this pref we will
+    // unconditionally attempt to move it.
+    bool destination_hash_missing =
+        !destination_transaction->GetHash(pref_name);

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

Inline this in conditional on line 172; no need to save this boolean up here
early.

+    // If we migrate the value we will also attempt to migrate the hash.
+    bool migrated_value = false;
+    if (old_store->Get(pref_name, &value_in_old_store)) {
+      // Whether this value ends up being copied below or was left behind by a
+      // previous incomplete migration, it should be cleaned up.
+      *old_store_needs_cleanup = true;
+
+      if (!new_store->Get(pref_name, NULL)) {
+        // Copy the value from |old_store| to |new_store| rather than moving it
+        // to avoid data loss should |old_store| be flushed to disk without
+        // |new_store| having equivalently been successfully flushed to disk
+        // (e.g., on crash or in cases where |new_store| is read-only following
+        // a read error on startup).
+        scoped_ptr<base::Value> value_copy(value_in_old_store->DeepCopy());
+        new_store->Set(pref_name, value_copy.release());
+        migrated_value = true;
+        *new_store_altered = true;
+      }
+    }
+
+    if (destination_hash_missing || migrated_value) {
+      const base::Value* old_hash = source_transaction->GetHash(pref_name);
+      if (!old_hash)
+        old_hash = legacy_transaction->GetHash(pref_name);
+      if (old_hash || !destination_hash_missing) {

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

I don't think this conditional is necessary.

You are essentially only skipping the case where |old_hash| is NULL and there is
no destination hash which means |migrated_value| is true and there is no benefit
to not running the code below -- since ImportHash will no-op and
|new_store_altered| is already set to true (sure it's useless to run this code,
but it's also harmless; this conditional makes things look more complex for no
added benefit IMO).

+        destination_transaction->ImportHash(pref_name, old_hash);
+        *new_store_altered = true;
+      }
+    }
   }
 }
 
 TrackedPreferencesMigrator::TrackedPreferencesMigrator(
     const std::set<std::string>& unprotected_pref_names,
     const std::set<std::string>& protected_pref_names,
-    const base::Callback<void(const std::string& key)>&
-        unprotected_store_cleaner,
-    const base::Callback<void(const std::string& key)>& protected_store_cleaner,
-    const base::Callback<void(const base::Closure&)>&
-        register_on_successful_unprotected_store_write_callback,
-    const base::Callback<void(const base::Closure&)>&
-        register_on_successful_protected_store_write_callback,
-    InterceptablePrefFilter* unprotected_pref_filter,
-    InterceptablePrefFilter* protected_pref_filter)
+    scoped_ptr<TrackedPreferencesMigrationDelegate> unprotected_delegate,
+    scoped_ptr<TrackedPreferencesMigrationDelegate> protected_delegate,
+    scoped_ptr<PrefHashStore> legacy_pref_hash_store)
     : unprotected_pref_names_(unprotected_pref_names),
       protected_pref_names_(protected_pref_names),
-      unprotected_store_cleaner_(unprotected_store_cleaner),
-      protected_store_cleaner_(protected_store_cleaner),
-      register_on_successful_unprotected_store_write_callback_(
-          register_on_successful_unprotected_store_write_callback),
-      register_on_successful_protected_store_write_callback_(
-          register_on_successful_protected_store_write_callback) {
+      unprotected_delegate_(unprotected_delegate.Pass()),
+      protected_delegate_(protected_delegate.Pass()),
+      legacy_pref_hash_store_(legacy_pref_hash_store.Pass()),
+      unprotected_prefs_(NULL),
+      protected_prefs_(NULL) {
   // The callbacks bound below will own this TrackedPreferencesMigrator by
   // reference.
-  unprotected_pref_filter->InterceptNextFilterOnLoad(
+  unprotected_delegate_->InterceptLoadedPreferences(
       base::Bind(&TrackedPreferencesMigrator::InterceptFilterOnLoad,
                  this,
                  UNPROTECTED_PREF_FILTER));
-  protected_pref_filter->InterceptNextFilterOnLoad(
+  protected_delegate_->InterceptLoadedPreferences(
       base::Bind(&TrackedPreferencesMigrator::InterceptFilterOnLoad,
                  this,
                  PROTECTED_PREF_FILTER));
@@ -174,17 +210,17 @@ TrackedPreferencesMigrator::~TrackedPreferencesMigrator() {}
 
 void TrackedPreferencesMigrator::InterceptFilterOnLoad(
     PrefFilterID id,
-    const InterceptablePrefFilter::FinalizeFilterOnLoadCallback&
-        finalize_filter_on_load,
-    scoped_ptr<base::DictionaryValue> prefs) {
+    const TrackedPreferencesMigrationDelegate::InterceptCompletionCallback&
+        completion_callback,
+    base::DictionaryValue* prefs) {
   switch (id) {
     case UNPROTECTED_PREF_FILTER:
-      finalize_unprotected_filter_on_load_ = finalize_filter_on_load;
-      unprotected_prefs_ = prefs.Pass();
+      unprotected_intercept_completion_callback_ = completion_callback;
+      unprotected_prefs_ = prefs;
       break;
     case PROTECTED_PREF_FILTER:
-      finalize_protected_filter_on_load_ = finalize_filter_on_load;
-      protected_prefs_ = prefs.Pass();
+      protected_intercept_completion_callback_ = completion_callback;
+      protected_prefs_ = prefs;
       break;
   }
 
@@ -192,6 +228,16 @@ void TrackedPreferencesMigrator::InterceptFilterOnLoad(
 }
 
 void TrackedPreferencesMigrator::MigrateIfReady() {
+  if (!protected_delegate_->IsValid() || !unprotected_delegate_->IsValid()) {

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

This is not possible since the read tasks [1] and the
(un)protected_intercept_completion_callback_'s own the JsonPrefStores [2] so
they (and migration) must complete before these stores can be released after a
ReadPrefs(Async) call.

As mentioned in my first comment on this CL, we would like to eventually make it
such that read tasks are weakly bound, but this is not yet the case (and this CL
makes this harder by passing a raw pointer to the internal |prefs_| object of
the JsonPrefStore rather than having a clear ownership passing model like it
does now).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/json_pr...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/json_pr...

Remove this code.

+    // We started shutting down before both stores had loaded. Don't call either
+    // of the completion callbacks, as the preferences are not fit to be used.
+    unprotected_prefs_ = NULL;
+    unprotected_intercept_completion_callback_.Reset();
+    protected_prefs_ = NULL;
+    protected_intercept_completion_callback_.Reset();
+    return;
+  }
+
   // Wait for both stores to have been read before proceeding.
   if (!protected_prefs_ || !unprotected_prefs_)
     return;
@@ -199,45 +245,50 @@ void TrackedPreferencesMigrator::MigrateIfReady() {
   bool protected_prefs_need_cleanup = false;
   bool unprotected_prefs_altered = false;
   MigratePrefsFromOldToNewStore(unprotected_pref_names_,
-                                protected_prefs_.get(),
-                                unprotected_prefs_.get(),
+                                protected_prefs_,
+                                unprotected_prefs_,
+                                protected_delegate_->GetPrefHashStore(),
+                                unprotected_delegate_->GetPrefHashStore(),
+                                legacy_pref_hash_store_.get(),
                                 &protected_prefs_need_cleanup,
                                 &unprotected_prefs_altered);
   bool unprotected_prefs_need_cleanup = false;
   bool protected_prefs_altered = false;
   MigratePrefsFromOldToNewStore(protected_pref_names_,
-                                unprotected_prefs_.get(),
-                                protected_prefs_.get(),
+                                unprotected_prefs_,
+                                protected_prefs_,
+                                unprotected_delegate_->GetPrefHashStore(),
+                                protected_delegate_->GetPrefHashStore(),
+                                legacy_pref_hash_store_.get(),
                                 &unprotected_prefs_need_cleanup,
                                 &protected_prefs_altered);
 
-  // Hand the processed prefs back to their respective filters.
-  finalize_unprotected_filter_on_load_.Run(unprotected_prefs_.Pass(),
-                                           unprotected_prefs_altered);
-  finalize_protected_filter_on_load_.Run(protected_prefs_.Pass(),
-                                         protected_prefs_altered);
-
+  // Allow processing of the preference stores to continue.
+  unprotected_prefs_ = NULL;
+  unprotected_intercept_completion_callback_.Run(unprotected_prefs_altered);
+  protected_prefs_ = NULL;
+  protected_intercept_completion_callback_.Run(protected_prefs_altered);
   if (unprotected_prefs_need_cleanup) {
     // Schedule a cleanup of the |protected_pref_names_| from the unprotected
     // prefs once the protected prefs were successfully written to disk (or
     // do it immediately if |!protected_prefs_altered|).
-    ScheduleSourcePrefStoreCleanup(
-        register_on_successful_protected_store_write_callback_,
-        unprotected_store_cleaner_,
-        protected_pref_names_,
-        protected_prefs_altered);
+    ScheduleSourcePrefStoreCleanup(unprotected_delegate_.get(),
+                                   protected_delegate_.get(),
+                                   protected_pref_names_,
+                                   protected_prefs_altered);
   }
-
   if (protected_prefs_need_cleanup) {
     // Schedule a cleanup of the |unprotected_pref_names_| from the protected
     // prefs once the unprotected prefs were successfully written to disk (or
     // do it immediately if |!unprotected_prefs_altered|).
-    ScheduleSourcePrefStoreCleanup(
-        register_on_successful_unprotected_store_write_callback_,
-        protected_store_cleaner_,
-        unprotected_pref_names_,
-        unprotected_prefs_altered);
+    ScheduleSourcePrefStoreCleanup(protected_delegate_.get(),
+                                   unprotected_delegate_.get(),
+                                   unprotected_pref_names_,
+                                   unprotected_prefs_altered);
   }
+
+  if (!unprotected_prefs_altered && !protected_prefs_altered)
+    this->legacy_pref_hash_store_->Reset();
 }
 
 }  // namespace
@@ -245,23 +296,13 @@ void TrackedPreferencesMigrator::MigrateIfReady() {
 void SetupTrackedPreferencesMigration(
     const std::set<std::string>& unprotected_pref_names,
     const std::set<std::string>& protected_pref_names,
-    const base::Callback<void(const std::string& key)>&
-        unprotected_store_cleaner,
-    const base::Callback<void(const std::string& key)>& protected_store_cleaner,
-    const base::Callback<void(const base::Closure&)>&
-        register_on_successful_unprotected_store_write_callback,
-    const base::Callback<void(const base::Closure&)>&
-        register_on_successful_protected_store_write_callback,
-    InterceptablePrefFilter* unprotected_pref_filter,
-    InterceptablePrefFilter* protected_pref_filter) {
+    scoped_ptr<TrackedPreferencesMigrationDelegate> unprotected_delegate,
+    scoped_ptr<TrackedPreferencesMigrationDelegate> protected_delegate,
+    scoped_ptr<PrefHashStore> legacy_pref_hash_store) {
   scoped_refptr<TrackedPreferencesMigrator> prefs_migrator(
-      new TrackedPreferencesMigrator(
-          unprotected_pref_names,
-          protected_pref_names,
-          unprotected_store_cleaner,
-          protected_store_cleaner,
-          register_on_successful_unprotected_store_write_callback,
-          register_on_successful_protected_store_write_callback,
-          unprotected_pref_filter,
-          protected_pref_filter));
+      new TrackedPreferencesMigrator(unprotected_pref_names,
+                                     protected_pref_names,
+                                     unprotected_delegate.Pass(),
+                                     protected_delegate.Pass(),
+                                     legacy_pref_hash_store.Pass()));
 }