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"

[gab, 2014/06/17 02:00:06]

Remove this include which is now in the header.

[erikwright (departed), 2014/06/17 19:07:24]

Done.

 #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"
+#include "chrome/browser/prefs/tracked/dictionary_hash_store_contents.h"
+#include "chrome/browser/prefs/tracked/hash_store_contents.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,
+      scoped_ptr<PrefHashStore> unprotected_pref_hash_store,
+      scoped_ptr<PrefHashStore> protected_pref_hash_store,
+      scoped_ptr<HashStoreContents> legacy_pref_hash_store,
       InterceptablePrefFilter* unprotected_pref_filter,
       InterceptablePrefFilter* protected_pref_filter);
 
  private:
   friend class base::RefCounted<TrackedPreferencesMigrator>;
 
   enum PrefFilterID {
     UNPROTECTED_PREF_FILTER,
     PROTECTED_PREF_FILTER
   };
@@ skipping 20 matching lines @@
   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<PrefHashStore> unprotected_pref_hash_store_;
+  scoped_ptr<PrefHashStore> protected_pref_hash_store_;
+  scoped_ptr<HashStoreContents> legacy_pref_hash_store_;
+
   scoped_ptr<base::DictionaryValue> unprotected_prefs_;
   scoped_ptr<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,
     const std::set<std::string>& keys_to_clean) {
@@ skipping 21 matching lines @@
   } else {
     CleanupPrefStore(source_store_cleaner, 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|.
 void MigratePrefsFromOldToNewStore(const std::set<std::string>& pref_names,
-                                   const base::DictionaryValue* old_store,
+                                   base::DictionaryValue* old_store,
                                    base::DictionaryValue* new_store,
+                                   PrefHashStore* new_hash_store,
                                    bool* old_store_needs_cleanup,
-                                   bool* new_store_altered) {
+                                   bool* new_store_altered,
+                                   HashStoreContents* legacy_hash_store) {

[gab, 2014/06/17 02:00:06]

Personal preference: put this param between |new_hash_store| and
|old_store_needs_cleanup|.

[erikwright (departed), 2014/06/17 19:07:24]

Done.

+  const base::DictionaryValue* old_hash_store_contents =
+      DictionaryHashStoreContents(old_store).GetContents();
+  const base::DictionaryValue* legacy_hash_store_contents =
+      legacy_hash_store->GetContents();
+  scoped_ptr<PrefHashStoreTransaction> new_hash_store_transaction(
+      new_hash_store->BeginTransaction(scoped_ptr<HashStoreContents>(
+          new DictionaryHashStoreContents(new_store))));
+
   for (std::set<std::string>::const_iterator it = pref_names.begin();
-       it != pref_names.end(); ++it) {
-    const std::string& pref_name = *it;
+       it != pref_names.end();
+       ++it) {
+     const std::string& pref_name = *it;
+     const base::Value* value_in_old_store = NULL;
 
-    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 =
+        !new_hash_store_transaction->HasHash(pref_name);
+    // 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).
+        new_store->Set(pref_name, value_in_old_store->DeepCopy());
+        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 = NULL;
+      if (old_hash_store_contents)
+        old_hash_store_contents->Get(pref_name, &old_hash);
+      if (!old_hash && legacy_hash_store_contents)
+        legacy_hash_store_contents->Get(pref_name, &old_hash);

[gab, 2014/06/17 02:00:06]

I would like to log an UMA stat if we migrate any MACs from Local State in this
method (to get a graph of the migration panning out).

i.e. set a bool here if |old_hash != NULL| and report a
"Settings.MACsMigratedFromLegacyStore" with histogram type
"enum=BooleanMigrated" at the end of this method.

+      if (old_hash || !destination_hash_missing) {

[gab, 2014/06/17 02:00:06]

I think this would be less subtle (and more readable) as:

if (old_hash) {
  new_hash_store_transaction->ImportHash(pref_name, old_hash);
  *new_store_altered = true;
} else if (!destination_hash_missing) {
  new_hash_store_transaction->ClearHash(pref_name);
  *new_store_altered = true;
}

Furthermore, this makes it such that ImportHash() can explicitly not support
NULL hashes, via contract and DCHECK (simplifying that method as well).

[erikwright (departed), 2014/06/17 19:07:24]

Done.

+        new_hash_store_transaction->ImportHash(pref_name, old_hash);
+        *new_store_altered = true;
+      }
+    }
   }
 }
 
+void CleanupMigratedHashes(const std::set<std::string>& migrated_pref_names,

[gab, 2014/06/17 02:00:06]

Move this method just below ScheduleSourcePrefStoreCleanup() to keep cleanup
related methods together.

[erikwright (departed), 2014/06/17 19:07:24]

Done.

+                           PrefHashStore* origin_pref_hash_store,
+                           base::DictionaryValue* origin_pref_store) {
+  scoped_ptr<PrefHashStoreTransaction> transaction(
+      origin_pref_hash_store->BeginTransaction(scoped_ptr<HashStoreContents>(
+          new DictionaryHashStoreContents(origin_pref_store))));
+  for (std::set<std::string>::const_iterator it = migrated_pref_names.begin();
+       it != migrated_pref_names.end();
+       ++it) {
+    transaction->ClearHash(*it);
+  }
+}
+
 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,
+    scoped_ptr<PrefHashStore> unprotected_pref_hash_store,
+    scoped_ptr<PrefHashStore> protected_pref_hash_store,
+    scoped_ptr<HashStoreContents> legacy_pref_hash_store,
     InterceptablePrefFilter* unprotected_pref_filter,
     InterceptablePrefFilter* protected_pref_filter)
     : 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) {
+          register_on_successful_protected_store_write_callback),
+      unprotected_pref_hash_store_(unprotected_pref_hash_store.Pass()),
+      protected_pref_hash_store_(protected_pref_hash_store.Pass()),
+      legacy_pref_hash_store_(legacy_pref_hash_store.Pass()) {
   // The callbacks bound below will own this TrackedPreferencesMigrator by
   // reference.
   unprotected_pref_filter->InterceptNextFilterOnLoad(
       base::Bind(&TrackedPreferencesMigrator::InterceptFilterOnLoad,
                  this,
                  UNPROTECTED_PREF_FILTER));
   protected_pref_filter->InterceptNextFilterOnLoad(
       base::Bind(&TrackedPreferencesMigrator::InterceptFilterOnLoad,
                  this,
                  PROTECTED_PREF_FILTER));
@@ skipping 23 matching lines @@
 void TrackedPreferencesMigrator::MigrateIfReady() {
   // 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(),
+                                unprotected_pref_hash_store_.get(),
                                 &protected_prefs_need_cleanup,
-                                &unprotected_prefs_altered);
+                                &unprotected_prefs_altered,
+                                legacy_pref_hash_store_.get());
   bool unprotected_prefs_need_cleanup = false;
   bool protected_prefs_altered = false;
   MigratePrefsFromOldToNewStore(protected_pref_names_,
                                 unprotected_prefs_.get(),
                                 protected_prefs_.get(),
+                                protected_pref_hash_store_.get(),
                                 &unprotected_prefs_need_cleanup,
-                                &protected_prefs_altered);
+                                &protected_prefs_altered,
+                                legacy_pref_hash_store_.get());
+
+  if (!unprotected_prefs_altered && !protected_prefs_altered) {
+    // Clean up any MACs that might have been previously migrated from the
+    // various stores. It's safe to leave them behind for a little while as they
+    // will be ignored unless the corresponding value is _also_ present. The
+    // cleanup must be deferred until the MACs have been written to their target
+    // stores, and doing so in a subsequent launch is easier than within the
+    // same process.
+    CleanupMigratedHashes(unprotected_pref_names_,
+                          protected_pref_hash_store_.get(),
+                          protected_prefs_.get());
+    CleanupMigratedHashes(protected_pref_names_,
+                          unprotected_pref_hash_store_.get(),
+                          unprotected_prefs_.get());
+    legacy_pref_hash_store_->Reset();
+  }
 
   // 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
@@ skipping 22 matching lines @@
 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,
+    scoped_ptr<PrefHashStore> unprotected_pref_hash_store,
+    scoped_ptr<PrefHashStore> protected_pref_hash_store,
+    scoped_ptr<HashStoreContents> legacy_pref_hash_store,
     InterceptablePrefFilter* unprotected_pref_filter,
     InterceptablePrefFilter* protected_pref_filter) {
   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_hash_store.Pass(),
+          protected_pref_hash_store.Pass(),
+          legacy_pref_hash_store.Pass(),
           unprotected_pref_filter,
           protected_pref_filter));
 }