Separate storage for protected preferences into Protected Preferences file.

chrome/browser/prefs/pref_hash_filter.cc

Index: chrome/browser/prefs/pref_hash_filter.cc
diff --git a/chrome/browser/prefs/pref_hash_filter.cc b/chrome/browser/prefs/pref_hash_filter.cc
index b937c358faf770b5f379d1bda8d8698f0f993093..381fbace615328531738d57fd6b13025bba6487c 100644
--- a/chrome/browser/prefs/pref_hash_filter.cc
+++ b/chrome/browser/prefs/pref_hash_filter.cc
@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/prefs/persistent_pref_store.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/pref_store.h"
 #include "base/strings/string_number_conversions.h"
@@ -91,6 +92,42 @@ void PrefHashFilter::ClearResetTime(PrefService* user_prefs) {
   user_prefs->ClearPref(prefs::kPreferenceResetTime);
 }
 
+void PrefHashFilter::MigrateValues(PersistentPrefStore* source,
+                                   PersistentPrefStore* destination) {
+  scoped_ptr<PrefHashStoreTransaction> transaction =
+      pref_hash_store_->BeginTransaction();

[gab, 2014/04/01 18:55:06]

Is pref_hash_store_ expected to be the underlying hash store for |source| or for
|destination| (I'm guessing |source| since this is how it's used in this
method). I think a meta-comment in the interface is necessary to clarify this.

[gab, 2014/04/01 19:24:14]

Ah, now I understand (from seeing how it's actually used in
https://codereview.chromium.org/218583003/). This PrefHashFilter is actually a
throw away PrefHashFilter using the protected configuration and
|pref_hash_store_| is a disposable copy of |source|'s PrefHashStore.

I think the API comment should still be clarified to mention this, this method
is easier to reason about if the reader can assume |pref_hash_store_| isn't the
underlying store for |source| nor |destination|.

(also note that this means that flushing the |transaction| below is irrelevant
IMO)

[erikwright (departed), 2014/04/01 19:46:08]

The method comment for MigrateValues says, in part:

"Values are migrated if they are protected according to this filter's
configuration, the corresponding key has no value in |destination|, and the
value in |source| is trusted according to this filter's PrefHashStore."

Is that sufficient?

[gab, 2014/04/02 17:02:03]

Perhaps adding something like:

"This PrefHashFilter shouldn't be tied to |source| nor |destination|."

?

I don't really like this approach overall, but it's the easiest short term
solution.

+  for (TrackedPreferencesMap::const_iterator it = tracked_paths_.begin();
+       it != tracked_paths_.end();
+       ++it) {
+    const base::Value* source_value = NULL;
+    if (source->GetValue(it->first, &source_value) &&
+        !destination->GetValue(it->first, NULL)) {
+      base::DictionaryValue temp_dictionary;
+      // Copy the value from |source| into a suitable place for a
+      // TrackedPreference to act on it.
+      temp_dictionary.Set(it->first, source_value->DeepCopy());
+      // Check whether the value is correct according to our MAC. May remove the
+      //  value from |temp_dictionary|.

[gab, 2014/04/01 18:55:06]

nit: Remove extra space at beginning of this line.

+      it->second->EnforceAndReport(&temp_dictionary, transaction.get());
+      // Now take the value as it appears in |temp_dictionary| and put it in
+      // |destination|.
+      scoped_ptr<base::Value> checked_value;
+      if (temp_dictionary.Remove(it->first, &checked_value))
+        destination->SetValue(it->first, checked_value.release());
+    }
+    source->RemoveValue(it->first);
+  }
+
+  // Order these such that a crash at any point is still recoverable. We assume
+  // that they are configured such that the writes will occur on worker threads
+  // in the order that we asked for them.
+  destination->CommitPendingWrite();
+  transaction.reset();

[gab, 2014/04/01 18:55:06]

This reset() won't actually force a CommitPendingWrite() to the underlying
storage (right?), isn't that a problem?

If pref_hash_store_ is also the underlying store for |source| then maybe this is
okay as it becomes irrelevant after |destination| was flushed? In fact does the
explicit reset() even matter?

[erikwright (departed), 2014/04/01 19:46:08]

It doesn't matter in practice, not the least because the hash store that is used
is actually a copy of the original hash store (see ProfilePrefStoreManager). But
if it were hypothetically going to have an impact on anything, it would
presumably be most appropriate to persist changes _after_ persisting the
destination and before persisting the source (on the assumption that this could
hypothetically be colocated with source).

Basically, I think it's appropriate for us to explicitly decide where to end the
transaction, and this location seems like the least surprising.

+  // If we crash here, we will just delete the values from |source| in a future
+  // invocation of MigrateValues.
+  source->CommitPendingWrite();
+}
+
 void PrefHashFilter::Initialize(const PrefStore& pref_store) {
   scoped_ptr<PrefHashStoreTransaction> hash_store_transaction(
       pref_hash_store_->BeginTransaction());