Multi-strategy based tracking.

chrome/browser/prefs/pref_hash_store_impl.cc

Index: chrome/browser/prefs/pref_hash_store_impl.cc
diff --git a/chrome/browser/prefs/pref_hash_store_impl.cc b/chrome/browser/prefs/pref_hash_store_impl.cc
index 37a60241511547b6eb17df0b51244061615be814..c3e409f187005e900eaa163e5c1f8e487c3c8dbd 100644
--- a/chrome/browser/prefs/pref_hash_store_impl.cc
+++ b/chrome/browser/prefs/pref_hash_store_impl.cc
@@ -68,3 +68,82 @@ void PrefHashStoreImpl::StoreHash(
   child_dictionary->SetString(
       path, pref_hash_calculator_.Calculate(path, new_value));
 }
+
+PrefHashStore::ValueState PrefHashStoreImpl::CheckSplitValue(
+    const std::string& path,
+    const base::DictionaryValue* initial_split_value,
+    std::vector<std::string>* invalid_keys) const {
+  DCHECK(invalid_keys && invalid_keys->empty());
+
+  const bool has_hashes = HasSplitHashesAtPath(path);
+
+  // Treat NULL and empty the same; otherwise we would need to store a hash
+  // for the entire dictionary (or some other special beacon) to
+  // differentiate these two cases which are really the same for
+  // dictionaries.
+  if (!initial_split_value || initial_split_value->empty())
+    return has_hashes ? CLEARED : UNCHANGED;
+
+  if (!has_hashes)
+    return UNKNOWN_VALUE;
+
+  for (base::DictionaryValue::Iterator it(*initial_split_value); !it.IsAtEnd();
+       it.Advance()) {
+    ValueState value_state = CheckValue(path + "." + it.key(), &it.value());
+    switch (value_state) {
+      case CLEARED:
+        // CLEARED doesn't make sense as a NULL value would never be sampled
+        // by the DictionaryValue::Iterator; in fact it is a known weakness of
+        // this current algorithm to not detect the case where a single key is
+        // cleared entirely from the dictionary pref.
+        NOTREACHED();
+        break;
+      case MIGRATED:
+        // Split tracked preferences were introduced after the migration started
+        // so no migration is expected.
+        NOTREACHED();
+        break;
+      case UNCHANGED:
+        break;
+      case CHANGED:  // Falls through.
+      case UNKNOWN_VALUE:
+        // Declare this value invalid, whether it was changed or never seen
+        // before.
+        invalid_keys->push_back(it.key());
+        break;
+    }
+  }
+  return invalid_keys->empty() ? UNCHANGED : CHANGED;
+}
+
+void PrefHashStoreImpl::StoreSplitHash(
+    const std::string& path,
+    const base::DictionaryValue* split_value) {
+  ClearPath(path);

[erikwright (departed), 2013/12/19 15:21:28]

Is it OK that this method might not be atomic?

I'm not sure exactly what DictionaryPrefUpdate does, but I assumed it was some
kind of transactional object for guaranteed atomic operations on a dictionary.

If so, you might want to consider using it. If not, there is a remote chance
that the user adds one extension and then, while you are writing out the hash
(specifically, somewhere between line 123 and line 129) the JSON file is
serialized on the IO thread and we subsequently crash. Maybe too unlikely to
worry about, but the impact would be losing some hashes and thus some
extensions..

[gab, 2013/12/20 18:37:06]

DictionaryPrefUpdate is not transactional: its only purpose is to Notify
observers of the modified pref only after the modification has occurred (it
otherwise forwards all calls directly to its underlying dictionary).

https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/scoped_...

In fact, there is probably no reason to use it in this class other than to be
consistent with other pref updating code...


There is no risk of race either (unless an observer of
prefs::kProfilePreferenceHashes modifies the extension set, but that would just
be silly...) since this and all other changes to prefs happen on the UI thread.

I did change it anyways because you are right that, in a world with a
transactional model to write preferences, this method should be a transaction to
avoid the case of crashing between clear+write and the next write.

The new model makes it correct as far as the notification for
kProfilePreferenceHashes and will allow for an easy transition to a
transactional model.

+
+  if (split_value) {
+    for (base::DictionaryValue::Iterator it(*split_value); !it.IsAtEnd();
+         it.Advance()) {
+      StoreHash(path + "." + it.key(), &it.value());
+    }
+  }
+}
+
+void PrefHashStoreImpl::ClearPath(const std::string& path) {
+  DictionaryPrefUpdate update(local_state_, prefs::kProfilePreferenceHashes);
+  DictionaryValue* child_dictionary = NULL;
+
+  if (update->GetDictionaryWithoutPathExpansion(hash_store_id_,
+                                                &child_dictionary)) {
+    child_dictionary->Remove(path, NULL);
+  }
+}
+
+bool PrefHashStoreImpl::HasSplitHashesAtPath(const std::string& path) const {
+  const base::DictionaryValue* pref_hash_dicts =
+      local_state_->GetDictionary(prefs::kProfilePreferenceHashes);
+  const base::DictionaryValue* hashed_prefs = NULL;
+  pref_hash_dicts->GetDictionaryWithoutPathExpansion(hash_store_id_,
+                                                     &hashed_prefs);
+  return hashed_prefs && hashed_prefs->GetDictionary(path, NULL);

[erikwright (departed), 2013/12/19 15:21:28]

If you are going to rely on this calling pattern for GetDictionary I suggest
augmenting comments and (if necessary) tests to protect it.

[gab, 2013/12/20 18:37:06]

I'll prepare a separate CL for this.

+}