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..cb1d544039787cf1e8035fd729435b870defc480
--- /dev/null
+++ b/chrome/browser/prefs/pref_hash_filter.cc
@@ -0,0 +1,97 @@
+// 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,
+};
+
+}  // namespace
+
+PrefHashFilter::PrefHashFilter(scoped_ptr<PrefHashStore> pref_hash_store)
+    : pref_hash_store_(pref_hash_store.Pass()),
+      tracked_paths_(kTrackedPrefs, kTrackedPrefs + arraysize(kTrackedPrefs)) {}

[gab, 2013/12/06 17:23:38]

This works?! i.e., "const char*" can act as an iterator?! Cool, didn't know that
:)!

[erikwright (departed), 2013/12/06 19:00:47]

An iterator is basically anything that supports operator++(), operator*(),
operator==().

STL collection iterator range constructors look roughly like this:

template <typename Value> class collection {
 public:
  ..
  template <typename Iterator> collection(Iterator begin, Iterator end);
  ..
}

And they only require that Iterator::operator*() returns a type that can be
passed to a single-argument constructor for Value.

+
+PrefHashFilter::~PrefHashFilter() {}
+
+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);
+}
+
+void PrefHashFilter::FilterOnLoad(PersistentPrefStore* pref_store) {
+  for (size_t i = 0; i < arraysize(kTrackedPrefs); ++i) {
+    if (kTrackedPrefs[i] == NULL)

[gab, 2013/12/06 17:23:38]

Checking for NULL here is no longer correct; please use the set for this logic
now (and for the looping).

This makes me think that we may want to have the kTrackedPrefs defined as a
static const[] in the local-scope of the constructor? The only downside I guess
is that we can't use the initializer list if we do this :(.

[gab, 2013/12/06 18:38:07]

In fact this makes me think that a pref_hash_filter_unittest.cc is probably
called for to avoid bugs like this one :)!

[erikwright (departed), 2013/12/06 19:00:47]

As mentioned, the test for this class is clearly required, but seeking feedback
on the design from you before writing it.

+      continue;
+    const base::Value* value = NULL;
+    pref_store->GetValue(kTrackedPrefs[i], &value);
+    PrefHashStore::ValueState value_state =
+        pref_hash_store_->CheckValue(kTrackedPrefs[i], value);
+
+    switch (value_state) {
+      case PrefHashStore::UNCHANGED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceUnchanged",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      case PrefHashStore::CLEARED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceCleared",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      case PrefHashStore::MIGRATED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceMigrated",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      case PrefHashStore::CHANGED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      case PrefHashStore::UNKNOWN_VALUE:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceInitialized",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      default:

[gab, 2013/12/06 17:23:38]

Remove default block here as well (ref comment I made further below).

[erikwright (departed), 2013/12/09 17:59:40]

Done.

+        NOTREACHED() << "Unexpected PrefHashStore::InitializationResult: "
+                     << value_state;
+        break;
+    }
+
+    if (value_state != PrefHashStore::UNCHANGED)
+      pref_hash_store_->StoreValue(kTrackedPrefs[i], value);
+  }
+}