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..fd4ff884cd2d0d4f35588c0fbf4e369cdded68e4 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,19 @@ class TrackedPreferencesMigrator
       PrefFilterID id,
       const InterceptablePrefFilter::FinalizeFilterOnLoadCallback&
           finalize_filter_on_load,
-      scoped_ptr<base::DictionaryValue> prefs);
+      base::DictionaryValue* prefs);
+
+  void CleanupPrefStoreAndMaybeLocalState(

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

s/LocalState/LegacyHashStore ?

here and below (since the API never really mentions that the legacy hash store
has to be Local State).

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

Done.

+      TrackedPreferencesMigrationDelegate* delegate,
+      const std::set<std::string>& keys_to_clean,
+      scoped_refptr<base::RefCountedData<int> > pending_cleanups);
+
+  void ScheduleSourcePrefStoreAndLocalStateCleanup(
+      TrackedPreferencesMigrationDelegate* source_delegate,
+      TrackedPreferencesMigrationDelegate* destination_delegate,
+      const std::set<std::string>& keys_to_clean,
+      bool wait_for_commit_to_destination_store,
+      scoped_refptr<base::RefCountedData<int> > pending_cleanups);
 
   // Proceeds with migration if both |unprotected_prefs_| and |protected_prefs_|
   // have been set.
@@ -56,32 +63,34 @@ 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_;
-
-  InterceptablePrefFilter::FinalizeFilterOnLoadCallback
-      finalize_unprotected_filter_on_load_;
-  InterceptablePrefFilter::FinalizeFilterOnLoadCallback
-      finalize_protected_filter_on_load_;
+  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_;
 
-  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|.

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

Expand method comment.

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

Done.

-void CleanupPrefStore(
-    const base::Callback<void(const std::string& key)>& store_cleaner,
-    const std::set<std::string>& keys_to_clean) {
+void TrackedPreferencesMigrator::CleanupPrefStoreAndMaybeLocalState(
+    TrackedPreferencesMigrationDelegate* delegate,
+    const std::set<std::string>& keys_to_clean,
+    scoped_refptr<base::RefCountedData<int> > pending_commits) {
+  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);
   }
+  if (!--pending_commits->data)

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

s/!--pending_commits->data/--pending_commits->data == 0

is less obscure IMO

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

Done.

+    this->legacy_pref_hash_store_->Reset();

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

Instead of using the ref-counted |pending_commits| logic, I would prefer to put
legacy_hash_store_transaction->ClearHash(*it);
in the loop above and simply have logic in
PrefHashStoreTransactionImpl::ClearHash() to trigger a Reset() when the last
hash is cleared.

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

Done.

 }
 
 // If |wait_for_commit_to_destination_store|: schedules (via
@@ -89,18 +98,23 @@ 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::ScheduleSourcePrefStoreAndLocalStateCleanup(
+    TrackedPreferencesMigrationDelegate* source_delegate,
+    TrackedPreferencesMigrationDelegate* destination_delegate,
     const std::set<std::string>& keys_to_clean,
-    bool wait_for_commit_to_destination_store) {
+    bool wait_for_commit_to_destination_store,
+    scoped_refptr<base::RefCountedData<int> > pending_commits) {
   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::CleanupPrefStoreAndMaybeLocalState,
+        this,

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

One of the reason this was previously bound to a free function was so that the
reference to the JsonPrefStores held by the
finalize_(un)protected_filter_on_load_ callbacks would be released post
migration.

Now the TrackedPreferencesMigrator and its members will live until the cleanup
tasks are complete which is wrong (I realize this wasn't obvious, apologies for
the lack of comments on this).

The model is that the TrackedPreferencesMigrator holds on to the prefs of one
store while the other store is being read; once both stores are read, it
migrates and then immediately frees everything (scheduling optimistic tasks to
maybe clean up later, through weak pointers, if possible).

Either way, I think this is another argument for not introducing the delegate
interface in this CL. Smaller changes FTW.

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

The delegate impl is bound to the pref stores by weak pointers, so it will not
keep them alive. I added comments and some missing checks to make this clear and
enforce it.

The only other state that is referenced (and potentially held alive) is the
PrefHashStore backed by Local State. I don't believe there is any possibility of
this code executing after Local State is gone, so I don't believe we are
introducing any new references.

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

This is not what I meant:

By binding the above callback to "this" you keep the TrackedPreferencesMigrator
alive. The TrackedPreferencesMigrator itself owns the
(un)protected_intercept_completion_callback_ which hold references to their
specific JsonPrefStores.

Before, the above callback was bound to a free function; allowing
TrackedPreferencesMigrator to go away; releasing its references on the
JsonPrefStore held via the callbacks it owns.

Once again, I'm not saying we can't make this work, but please move the addition
of the TrackedPreferencesMigrationDelegate to a follow-up CL; it is non-trivial
and hinders our ability to commit the core logic ASAP.

(I realized after writing this comment that you kind of need the delegate in
your model since otherwise you also need PrefHashStore to support weak
pointers...  :( -- one way to make the above work with delegates would be to
keep using a free function and pass the delegates into the cleanup callbacks
rather than keeping the TrackedPreferencesMigrator around for them to be
available)

+        source_delegate,
+        base::ConstRef(keys_to_clean),
+        pending_commits));
   } else {
-    CleanupPrefStore(source_store_cleaner, keys_to_clean);
+    CleanupPrefStoreAndMaybeLocalState(
+        source_delegate, keys_to_clean, pending_commits);
   }
 }
 
@@ -111,60 +125,75 @@ 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;
+    const base::Value* value_in_new_store = NULL;
+    bool migrate_hash = !destination_transaction->GetHash(pref_name);
+    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, &value_in_new_store)) {

[gab, 2014/06/06 21:55:00]

Remove the |value_in_new_store| variable, you can pass NULL into this Get()
call.

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

Done.

+        // 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());
+        value_in_new_store = value_copy.get();
+        new_store->Set(pref_name, value_copy.release());
+        *new_store_altered = true;
+        migrate_hash = true;
+      }
+    }
+
+    if (migrate_hash) {
+      const base::Value* old_hash = source_transaction->GetHash(pref_name);
+      if (!old_hash)
+        old_hash = legacy_transaction->GetHash(pref_name);
+      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 +203,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;
   }
 
@@ -199,44 +228,53 @@ 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);
-
-  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);
-  }
-
-  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);
+  unprotected_intercept_completion_callback_.Run(unprotected_prefs_altered);
+  protected_intercept_completion_callback_.Run(protected_prefs_altered);
+  scoped_refptr<base::RefCountedData<int> > pending_commits(
+      new base::RefCountedData<int>((unprotected_prefs_altered ? 1 : 0) +
+                                    (protected_prefs_altered ? 1 : 0)));
+  if (!pending_commits->data) {

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

Above I suggested getting rid of |pending_commits|.

Here this would simply turn into:

if (!unprotected_prefs_altered && !protected_prefs_altered) {
  // If neither store was altered in this run, it is safe to immediately reset
the legacy hash store.
  legacy_pref_hash_store_->Reset();
}

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

Done.

+    this->legacy_pref_hash_store_->Reset();
+  } else {

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

Remove this else conditional, the code below needs to run unconditionally (i.e.,
it's possible that neither store was altered yet they both need cleanup -- since
cleanup may have failed in the previous run).

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

Done.

+    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|).
+      ScheduleSourcePrefStoreAndLocalStateCleanup(unprotected_delegate_.get(),
+                                                  protected_delegate_.get(),
+                                                  protected_pref_names_,
+                                                  protected_prefs_altered,
+                                                  pending_commits);
+    }
+    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|).
+      ScheduleSourcePrefStoreAndLocalStateCleanup(protected_delegate_.get(),
+                                                  unprotected_delegate_.get(),
+                                                  unprotected_pref_names_,
+                                                  unprotected_prefs_altered,
+                                                  pending_commits);
+    }
   }
 }
 
@@ -245,23 +283,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()));
 }