Fix a race condition in preference metric reporting.

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
new file mode 100644
index 0000000000000000000000000000000000000000..8056bf468691b80e5daab067fa5a8ac17cb4fb31
--- /dev/null
+++ b/chrome/browser/prefs/pref_hash_filter.cc
@@ -0,0 +1,100 @@
+// Copyright (c) 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_filter.h"
+
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/prefs/persistent_pref_store.h"
+#include "base/values.h"
+#include "chrome/browser/prefs/pref_hash_store.h"
+#include "chrome/common/pref_names.h"
+
+namespace {
+
+// These preferences must be kept in sync with the TrackedPreference enum in
+// tools/metrics/histograms/histograms.xml. To add a new preference, append it
+// to the array and add a corresponding value to the histogram enum. Replace
+// removed preferences with "".
+const char* kTrackedPrefs[] = {
+  prefs::kShowHomeButton,
+  prefs::kHomePageIsNewTabPage,
+  prefs::kHomePage,
+  prefs::kRestoreOnStartup,
+  prefs::kURLsToRestoreOnStartup,
+  prefs::kExtensionsPref,
+  prefs::kGoogleServicesLastUsername,
+  prefs::kSearchProviderOverrides,
+  prefs::kDefaultSearchProviderSearchURL,
+  prefs::kDefaultSearchProviderKeyword,
+  prefs::kDefaultSearchProviderName,
+#if !defined(OS_ANDROID)
+  prefs::kPinnedTabs,
+#else
+  "",
+#endif
+  prefs::kExtensionKnownDisabled,
+};
+
+void ReportValidationResult(PrefHashStore::ValueState value_state,
+                            size_t value_index) {
+  switch (value_state) {
+    case PrefHashStore::UNCHANGED:
+      UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceUnchanged",
+                                value_index, arraysize(kTrackedPrefs));
+      return;
+    case PrefHashStore::CLEARED:
+      UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceCleared",
+                                value_index, arraysize(kTrackedPrefs));
+      return;
+    case PrefHashStore::MIGRATED:
+      UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceMigrated",
+                                value_index, arraysize(kTrackedPrefs));
+      return;
+    case PrefHashStore::CHANGED:
+      UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
+                                value_index, arraysize(kTrackedPrefs));
+      return;
+    case PrefHashStore::UNKNOWN_VALUE:
+      UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceInitialized",
+                                value_index, arraysize(kTrackedPrefs));
+      return;
+  }
+  NOTREACHED() << "Unexpected PrefHashStore::ValueState: " << value_state;
+}
+
+}  // namespace
+
+PrefHashFilter::PrefHashFilter(scoped_ptr<PrefHashStore> pref_hash_store)
+    : pref_hash_store_(pref_hash_store.Pass()),
+      tracked_paths_(kTrackedPrefs, kTrackedPrefs + arraysize(kTrackedPrefs)) {}
+
+PrefHashFilter::~PrefHashFilter() {}
+
+// Updates the stored hash to correspond to the updated preference value.
+void PrefHashFilter::FilterUpdate(PersistentPrefStore* pref_store,
+                                  const std::string& path) {
+  if (tracked_paths_.find(path) == tracked_paths_.end())
+    return;
+
+  const base::Value* value = NULL;
+  pref_store->GetValue(path, &value);
+  pref_hash_store_->StoreValue(path, value);
+}
+
+// Validates loaded preference values according to stored hashes, reports
+// validation results via UMA, and updates hashes in case of mismatch.
+void PrefHashFilter::FilterOnLoad(PersistentPrefStore* pref_store) {
+  for (size_t i = 0; i < arraysize(kTrackedPrefs); ++i) {
+    if (kTrackedPrefs[i][0] == '\0')

[gab, 2013/12/09 20:30:15]

This feels hacky... why not just loop over the set and check for empty() here?

[erikwright (departed), 2013/12/10 16:06:19]

That would involve constructing std::string unnecessarily. Given that we are
looping over a char* array I think it's pretty clear that a '\0' means an empty
string.

[gab, 2013/12/10 20:00:31]

It wouldn't involve building a string; you would simply iterate over the set (in
which the strings already exist) with a const_iterator; and check for empty()
here.

This also allows you to hide kTrackedPrefs away in the constructor's scope;
having both kTrackedPrefs and tracked_paths_ in scope here makes things
confusing IMO.

[erikwright (departed), 2013/12/12 15:53:30]

I can't iterate over the set; I need the index for reporting to UMA.

I could store a vector instead, but then FilterUpdate would need to O(N) search
the vector before calling StoreValue.

I could use both a vector _and_ a set, but then we are in the same situation of
having two in-scope copies of the same data (plus the hidden kTrackedPrefs).

[gab, 2013/12/12 17:20:58]

Right, I just realized that while working with this CL; I will change the
approach in an upcoming CL, but I'm fine with you keeping it like this for now.

See https://codereview.chromium.org/114223002/ for a skeleton of my approach.

+      continue;
+    const base::Value* value = NULL;
+    pref_store->GetValue(kTrackedPrefs[i], &value);
+    PrefHashStore::ValueState value_state =
+        pref_hash_store_->CheckValue(kTrackedPrefs[i], value);
+    ReportValidationResult(value_state, i);
+    if (value_state != PrefHashStore::UNCHANGED)
+      pref_hash_store_->StoreValue(kTrackedPrefs[i], value);
+  }
+}