Fix a race condition in preference metric reporting.

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
new file mode 100644
index 0000000000000000000000000000000000000000..9e0add88d2ba4208debe4bfbfaf2de4f112f7259
--- /dev/null
+++ b/chrome/browser/prefs/pref_hash_store_impl.cc
@@ -0,0 +1,134 @@
+// Copyright 2013 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/pref_hash_store_impl.h"
+
+#include "base/logging.h"
+#include "base/prefs/pref_service.h"
+#include "base/prefs/scoped_user_pref_update.h"
+#include "base/values.h"
+#include "chrome/browser/prefs/pref_hash_tracker.h"
+#include "chrome/common/pref_names.h"
+
+// Implements PrefHashTracker by storing hashes in a PrefService.
+class PrefHashStoreImpl::PrefHashTrackerImpl : public PrefHashTracker {
+ public:
+  // Constructs a PrefHashTrackerImpl that calculates hashes of preference
+  // values using |pref_hash_calculator| and stores them in |local_state|.
+  // Multiple hash trackers can use the same |local_state| with distinct
+  // |hash_store_id|s.
+  //
+  // Values are stored at kProfilePreferenceHashes / |hash_store_id| / path,

[robertshield, 2013/11/28 02:54:26]

nit: should this be prefs::kProfilePreferenceHashes

+  // where path is the full path of the tracked preference.
+  // |hash_store_id| is not expanded but path and kProfilePreferenceHashes

[robertshield, 2013/11/28 02:54:26]

please elaborate slightly on what "expanded" means in this context.

+  // are.
+  PrefHashTrackerImpl(PrefService* local_state,
+                      const std::string& hash_store_id,
+                      const PrefHashCalculator& pref_hash_calculator);
+
+  // Adds |path| to the set of preferences for which hashes will be stored.
+  void Track(const std::string& path);
+
+  // Updates the stored hash for |path|, if |path| has been configured for

[gab, 2013/11/27 23:43:27]

I think this sentence flows better without the comma.

+  // tracking by calling |Track|. |new_value| may be NULL if there is no value
+  // |path|.

[gab, 2013/11/27 23:43:27]

"no value *at* |path|?"

+  virtual void OnPrefValueChanged(const std::string& path,
+                                  const base::Value* new_value) OVERRIDE;
+
+ private:
+  std::set<std::string> tracked_paths_;
+  PrefService* local_state_;
+  std::string hash_store_id_;
+  PrefHashCalculator pref_hash_calculator_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefHashTrackerImpl);
+};
+
+PrefHashStoreImpl::PrefHashTrackerImpl::PrefHashTrackerImpl(
+    PrefService* local_state,
+    const std::string& hash_store_id,
+    const PrefHashCalculator& pref_hash_calculator)
+    : local_state_(local_state),
+      hash_store_id_(hash_store_id),
+      pref_hash_calculator_(pref_hash_calculator) {}
+
+void PrefHashStoreImpl::PrefHashTrackerImpl::Track(const std::string& path) {
+  tracked_paths_.insert(path);

[gab, 2013/11/27 23:43:27]

Why not use the PrefChangeRegistrar for this? Is it not an option for the entire
lifetime of this object?

[erikwright (departed), 2013/11/28 17:48:07]

We're dealing with a PrefStore, not a PrefService.

(1) There is no added value to the Registrar approach, just added complexity
(because we have to use both the PrefStore (for initialization) and the
PrefService (for updates) APIs. You have to make an assumption that the two
objects you have are the correct pref store and pref service.
(2) It may actually be harmful to track the PrefService and not the user prefs
PrefStore. After all, the value of a preference can change for other reasons
than a user pref change(i.e. managed prefs or extensions pref stores could
change a value at runtime, and we don't want to track a hash of that).

+}
+
+void PrefHashStoreImpl::PrefHashTrackerImpl::OnPrefValueChanged(
+    const std::string& path, const base::Value* new_value) {
+  if (tracked_paths_.find(path) == tracked_paths_.end())
+    return;
+
+  DictionaryPrefUpdate update(local_state_, prefs::kProfilePreferenceHashes);
+  DictionaryValue* child_dictionary = NULL;
+
+  // Get the dictionary corresponding to the profile name,

[gab, 2013/11/27 23:43:27]

s/profile name/hash store id

+  // which may have a '.'
+  if (!update->GetDictionaryWithoutPathExpansion(hash_store_id_,
+                                                 &child_dictionary)) {
+    child_dictionary = new DictionaryValue;
+    update->SetWithoutPathExpansion(hash_store_id_, child_dictionary);
+  }
+
+  child_dictionary->SetString(
+      path, pref_hash_calculator_.Calculate(path, new_value));
+}
+
+PrefHashStoreImpl::PrefHashStoreImpl(const std::string& seed,
+                                     PrefService* local_state,
+                                     const std::string& hash_store_id)
+    : local_state_(local_state),
+      hash_store_id_(hash_store_id),
+      pref_hash_calculator_(seed),
+      pref_hash_tracker_(new PrefHashTrackerImpl(
+          local_state_, hash_store_id, pref_hash_calculator_)) {}
+
+PrefHashStore::InitializationResult PrefHashStoreImpl::InitializeTrackedValue(
+    const std::string& path, const base::Value* initial_value) {
+  DCHECK(pref_hash_tracker_.get());
+  if (!pref_hash_tracker_.get())

[gab, 2013/11/27 23:43:27]

Why handle this case with silent success? The contract (i.e., via DCHECK) is
that this should always be called before the |pref_hash_tracker_| is passed
over, right? And thus it should never be NULL.

+    return PrefHashStore::UNCHANGED;
+
+  pref_hash_tracker_->Track(path);
+
+  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);
+
+  std::string last_hash;
+  // First try to get the stored expected hash...
+  if (hashed_prefs && hashed_prefs->GetString(path, &last_hash)) {
+    PrefHashCalculator::ValidationResult validation_result =
+        pref_hash_calculator_.Validate(path, initial_value, last_hash);
+    if (validation_result == PrefHashCalculator::VALID)
+      return PrefHashStore::UNCHANGED;
+
+    // No matter what the reason for the mismatch, we will correct it now.
+    pref_hash_tracker_->OnPrefValueChanged(path, initial_value);
+
+    switch(validation_result) {
+      case PrefHashCalculator::VALID_LEGACY:
+        return PrefHashStore::MIGRATED;
+      case PrefHashCalculator::INVALID:
+        return initial_value ? PrefHashStore::CHANGED : PrefHashStore::CLEARED;
+      default:
+        NOTREACHED() << "Unexpected PrefHashCalculator::ValidationResult: "
+                     << validation_result;
+        return PrefHashStore::INITIALIZED;
+    }
+  }
+
+  // We haven't tracked this preference yet, or the hash in local state was
+  // removed.
+  pref_hash_tracker_->OnPrefValueChanged(path, initial_value);
+  return PrefHashStore::INITIALIZED;
+}
+
+scoped_ptr<PrefHashTracker> PrefHashStoreImpl::CompleteInitialization() {
+  return pref_hash_tracker_.Pass();
+}