Fix a race condition in preference metric reporting.

chrome/browser/prefs/pref_change_reporter.cc

+// 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_change_reporter.h"
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/prefs/json_pref_store.h"
+#include "base/values.h"
+#include "chrome/browser/prefs/pref_hash_store.h"
+#include "chrome/browser/prefs/pref_hash_tracker.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.
+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
+  kUnregisteredPreference,

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

Looks like you forgot to bring over the definition of this constant.

+#endif
+  prefs::kExtensionKnownDisabled,
+};
+
+// Observes initialization of and changes to a JsonPrefStore. During
+// initialization, compares loaded values to stored hashes and reports changes
+// using UMA. In response to preference changes, updates stored hashes.
+class PrefStoreObserver : public PrefStore::Observer {
+ public:
+  virtual ~PrefStoreObserver();
+
+  // Creates a PrefStoreObserver for |pref_store| using |pref_hash_store|.
+  static void Observe(JsonPrefStore* pref_store,
+                      scoped_ptr<PrefHashStore> pref_hash_store);
+
+  // PrefStore::Observer implementation.
+  virtual void OnPrefValueChanged(const std::string& key) OVERRIDE;
+  virtual void OnInitializationCompleted(bool succeeded) OVERRIDE;
+
+ private:
+  PrefStoreObserver(PrefStore* pref_store,
+                    scoped_ptr<PrefHashStore> pref_hash_store);
+
+  PrefStore* pref_store_;
+  // Used during initialization, for detecting changes.
+  scoped_ptr<PrefHashStore> pref_hash_store_;
+  // Used after initialization, for updating stored hashes.
+  scoped_ptr<PrefHashTracker> pref_hash_tracker_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefStoreObserver);
+};
+
+PrefStoreObserver::~PrefStoreObserver() {}
+
+// static
+void PrefStoreObserver::Observe(JsonPrefStore* pref_store,
+                                scoped_ptr<PrefHashStore> pref_hash_store) {
+  pref_store->AddOwnedObserver(
+      scoped_ptr<PrefStore::Observer>(
+          new PrefStoreObserver(pref_store, pref_hash_store.Pass())));
+}
+
+void PrefStoreObserver::OnPrefValueChanged(const std::string& key) {
+  const base::Value* value = NULL;
+  pref_store_->GetValue(key, &value);
+
+  DCHECK(pref_hash_tracker_.get());
+  if (pref_hash_tracker_.get())
+    pref_hash_tracker_->OnPrefValueChanged(key, value);
+}
+
+void PrefStoreObserver::OnInitializationCompleted(bool succeeded) {

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

I'm not convinced that doing this OnInitializationCompleted() is early enough;
other observers could already have been notified and have started reading
preferences right?

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

In this case, chrome_pref_service_factory explicitly added us first, so we _are_
the first to hear about changes.

+  DCHECK(pref_hash_store_.get());
+  if (!pref_hash_store_.get())
+    return;
+
+  for (size_t i = 0; i < arraysize(kTrackedPrefs); ++i) {
+    // TODO(erikwright): If it's important, have a pointer to the pref_service
+    // in order to skip things that aren't registered.

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

This matters, in particular to handle kUnregisteredPreference on some OSes; we
could find a better way to do this than rely on checking whether it's registered
though.

+    const base::Value* value = NULL;
+    pref_store_->GetValue(kTrackedPrefs[i], &value);
+    PrefHashStore::InitializationResult initialization_result =
+        pref_hash_store_->InitializeTrackedValue(kTrackedPrefs[i], value);
+
+    switch(initialization_result) {
+      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::INITIALIZED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceInitialized",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      default:
+        NOTREACHED() << "Unexpected PrefHashStore::InitializationResult: "
+                     << initialization_result;
+        break;
+    }
+  }
+
+  pref_hash_tracker_ = 
+      PrefHashStore::CreateTracker(pref_hash_store_.Pass());
+}
+
+PrefStoreObserver::PrefStoreObserver(
+    PrefStore* pref_store,
+    scoped_ptr<PrefHashStore> pref_hash_store)
+    : pref_store_(pref_store), pref_hash_store_(pref_hash_store.Pass()) {}
+
+}  // namespace
+
+void ReportPrefStoreChangesToUMA(JsonPrefStore* pref_store,
+                                 scoped_ptr<PrefHashStore> pref_hash_store) {
+  if (pref_hash_store.get())
+    PrefStoreObserver::Observe(pref_store, pref_hash_store.Pass());
+}