Move preference MACs to the protected preference stores.

chrome/browser/prefs/tracked/tracked_preferences_migration.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/prefs/tracked/tracked_preferences_migration.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #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 {
 
 class TrackedPreferencesMigrator
     : public base::RefCounted<TrackedPreferencesMigrator> {
  public:
   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>;
 
   enum PrefFilterID {
     UNPROTECTED_PREF_FILTER,
     PROTECTED_PREF_FILTER
   };
 
   ~TrackedPreferencesMigrator();
 
   // Stores the data coming in from the filter identified by |id| into this
   // class and then calls MigrateIfReady();
   void InterceptFilterOnLoad(
       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.
   void MigrateIfReady();
 
   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);
   }
 }
 
 // If |wait_for_commit_to_destination_store|: schedules (via
 // |register_on_successful_destination_store_write_callback|) a cleanup of the
 // |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);
   }
 }
 
 // Copies the value of each pref in |pref_names| which is set in |old_store|,
 // but not in |new_store| into |new_store|. Sets |old_store_needs_cleanup| to
 // true if any old duplicates remain in |old_store| and sets |new_store_altered|
 // to true if any value was copied to |new_store|.

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

Update this comment.

 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;
+    // 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;
 
-    // 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 (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 (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));
 }
 
 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;
   }
 
   MigrateIfReady();
 }
 
 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;
 
   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
 
 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()));
 }