Add UMA to report Preferences File Corruption

chrome/browser/prefs/pref_metrics_service.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_metrics_service.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
+#include "base/json/json_string_value_serializer.h"
+#include "base/md5.h"
 #include "base/metrics/histogram.h"
+#include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_shutdown.h"
+// Accessing the Device ID API here is a layering violation.
+// TODO(bbudge) Move the API so it's usable here.
+// http://crbug.com/276485
+#include "chrome/browser/extensions/api/music_manager_private/device_id.h"
+#include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
+#include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/prefs/synced_pref_change_registrar.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_prepopulate_data.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
+#include "grit/browser_resources.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "ui/base/resource/resource_bundle.h"
 
 namespace {
 
-// Converts a host name into a domain name for easier matching.
-std::string GetDomainFromHost(const std::string& host) {
-  return net::registry_controlled_domains::GetDomainAndRegistry(
-      host,
-      net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
+const int kSessionStartupPrefValueMax = SessionStartupPref::kPrefValueMax;
+
+// 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,
+  prefs::kPinnedTabs,
+};
+
+bool BrowserShuttingDown() {
+  return browser_shutdown::GetShutdownType() != browser_shutdown::NOT_VALID;
 }
 
-const int kSessionStartupPrefValueMax = SessionStartupPref::kPrefValueMax;
-
 }  // namespace
 
 PrefMetricsService::PrefMetricsService(Profile* profile)
-    : profile_(profile) {
+    : weak_factory_(this),
+      profile_(profile),
+      prefs_(profile_->GetPrefs()),
+      local_state_(g_browser_process->local_state()),
+      tracked_pref_paths_(kTrackedPrefs),
+      tracked_pref_path_count_(arraysize(kTrackedPrefs)) {
+  // Check that the pref is registered; otherwise some browser_tests crash.
+  if (prefs_->FindPreference(prefs::kGoogleServicesUsername))
+    profile_name_ = profile_->GetProfileName();
+
+  pref_hash_seed_ = ResourceBundle::GetSharedInstance().GetRawDataResource(
+      IDR_PREF_HASH_SEED_BIN).as_string();
+
   RecordLaunchPrefs();
 
+  // Don't track preferences in browser tests. Otherwise, tests that change
+  // prefs after shutdown begins will write to local state, which will DCHECK
+  // when trying to write.
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType))
+    InitializePrefObservers();
+
+#if !defined(OS_ANDROID)
+  // We need the machine id to compute pref value hashes. Fetch that, and then
+  // call CheckTrackedPreferences in the callback.
+  extensions::api::DeviceId::GetDeviceId(
+      "PrefMetricsService",  // non-empty string to obfuscate the device id.
+      Bind(&PrefMetricsService::GetDeviceIdCallback,
+           weak_factory_.GetWeakPtr()));
+#else
+  // Android has no GetDeviceId.
+  CheckTrackedPreferences();
+#endif  // !defined(OS_ANDROID)
+
   PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
   synced_pref_change_registrar_.reset(new SyncedPrefChangeRegistrar(prefs));
 
   RegisterSyncedPrefObservers();
 }
 
+PrefMetricsService::PrefMetricsService(Profile* profile,
+                                       PrefService* local_state,
+                                       const std::string& device_id,
+                                       const char** tracked_pref_paths,
+                                       int tracked_pref_path_count)
+    : weak_factory_(this),
+      profile_(profile),
+      prefs_(profile->GetPrefs()),
+      local_state_(local_state),
+      device_id_(device_id),
+      tracked_pref_paths_(tracked_pref_paths),
+      tracked_pref_path_count_(tracked_pref_path_count) {
+  CheckTrackedPreferences();
+  InitializePrefObservers();
+}
+
 PrefMetricsService::~PrefMetricsService() {
 }
 
 void PrefMetricsService::RecordLaunchPrefs() {
-  PrefService* prefs = profile_->GetPrefs();
-  bool show_home_button = prefs->GetBoolean(prefs::kShowHomeButton);
-  bool home_page_is_ntp = prefs->GetBoolean(prefs::kHomePageIsNewTabPage);
+  bool show_home_button = prefs_->GetBoolean(prefs::kShowHomeButton);
+  bool home_page_is_ntp = prefs_->GetBoolean(prefs::kHomePageIsNewTabPage);
   UMA_HISTOGRAM_BOOLEAN("Settings.ShowHomeButton", show_home_button);
   if (show_home_button) {
     UMA_HISTOGRAM_BOOLEAN("Settings.GivenShowHomeButton_HomePageIsNewTabPage",
                           home_page_is_ntp);
   }
 
   // For non-NTP homepages, see if the URL comes from the same TLD+1 as a known
   // search engine.  Note that this is only an approximation of search engine
   // use, due to both false negatives (pages that come from unknown TLD+1 X but
   // consist of a search box that sends to known TLD+1 Y) and false positives
   // (pages that share a TLD+1 with a known engine but aren't actually search
   // pages, e.g. plus.google.com).
   if (!home_page_is_ntp) {
-    GURL homepage_url(prefs->GetString(prefs::kHomePage));
+    GURL homepage_url(prefs_->GetString(prefs::kHomePage));
     if (homepage_url.is_valid()) {
       UMA_HISTOGRAM_ENUMERATION(
           "Settings.HomePageEngineType",
           TemplateURLPrepopulateData::GetEngineType(homepage_url),
           SEARCH_ENGINE_MAX);
     }
   }
 
-  int restore_on_startup = prefs->GetInteger(prefs::kRestoreOnStartup);
+  int restore_on_startup = prefs_->GetInteger(prefs::kRestoreOnStartup);
   UMA_HISTOGRAM_ENUMERATION("Settings.StartupPageLoadSettings",
                             restore_on_startup, kSessionStartupPrefValueMax);
   if (restore_on_startup == SessionStartupPref::kPrefValueURLs) {
-    const ListValue* url_list = prefs->GetList(prefs::kURLsToRestoreOnStartup);
+    const ListValue* url_list = prefs_->GetList(prefs::kURLsToRestoreOnStartup);
     UMA_HISTOGRAM_CUSTOM_COUNTS("Settings.StartupPageLoadURLs",
                                 url_list->GetSize(), 1, 50, 20);
     // Similarly, check startup pages for known search engine TLD+1s.
     std::string url_text;
     for (size_t i = 0; i < url_list->GetSize(); i++) {
       if (url_list->GetString(i, &url_text)) {
         GURL start_url(url_text);
         if (start_url.is_valid()) {
           UMA_HISTOGRAM_ENUMERATION(
               "Settings.StartupPageEngineTypes",
               TemplateURLPrepopulateData::GetEngineType(start_url),
               SEARCH_ENGINE_MAX);
         }
       }
     }
   }
 }
 
+// static
+void PrefMetricsService::RegisterPrefs(PrefRegistrySimple* registry) {
+  // Register the top level dictionary to map profile names to dictionaries of
+  // tracked preferences.
+  registry->RegisterDictionaryPref(prefs::kProfilePreferenceHashes);
+}
+
 void PrefMetricsService::RegisterSyncedPrefObservers() {
   LogHistogramValueCallback booleanHandler = base::Bind(
       &PrefMetricsService::LogBooleanPrefChange, base::Unretained(this));
 
   AddPrefObserver(prefs::kShowHomeButton, "ShowHomeButton", booleanHandler);
   AddPrefObserver(prefs::kHomePageIsNewTabPage, "HomePageIsNewTabPage",
                   booleanHandler);
 
   AddPrefObserver(prefs::kRestoreOnStartup, "StartupPageLoadSettings",
                   base::Bind(&PrefMetricsService::LogIntegerPrefChange,
@@ skipping 57 matching lines @@
   const ListValue* items = NULL;
   if (!value->GetAsList(&items))
     return;
   for (size_t i = 0; i < items->GetSize(); ++i) {
     const Value *item_value = NULL;
     if (items->Get(i, &item_value))
       item_callback.Run(histogram_name, item_value);
   }
 }
 
+void PrefMetricsService::GetDeviceIdCallback(const std::string& device_id) {
+  device_id_ = device_id;
+  CheckTrackedPreferences();
+}
+
+// To detect changes to Preferences that happen outside of Chrome, we hash
+// selected pref values and save them in local state. CheckTrackedPreferences
+// compares the saved values to the values observed in the profile's prefs. A
+// dictionary of dictionaries in local state holds the hashed values, grouped by
+// profile. To make the system more resistant to spoofing, pref values are
+// hashed with the pref path and the device id.
+void PrefMetricsService::CheckTrackedPreferences() {
+  // Don't attempt to change local state during browser shutdown.
+  if (BrowserShuttingDown())

[battre, 2013/09/02 08:33:46]

Don't you want to move this down so that it becomes:

if (!BrowserShuttingDown()) {
  UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
      i, tracked_pref_path_count_);
} else {
  UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChangedInShutdown", i,
...);
}
?

Alternatively, you could put this into
PrefMetricsService::UpdateTrackedPreference.

[bbudge, 2013/09/04 15:00:09]

I added these guards to get some browser_tests to pass. Since I modified
PrefMetricsService to not check for pref tampering during testing, I don't think
I need these two guard if's any more so I'm removing them.

Do you think it's important to detect pref changes at shutdown?

On 2013/09/02 08:33:46, battre wrote:

+    return;
+
+  const base::DictionaryValue* pref_hash_dicts =
+      local_state_->GetDictionary(prefs::kProfilePreferenceHashes);
+  // Get the hashed prefs dictionary if it exists. If it doesn't, it will be
+  // created if we set preference values below.
+  const base::DictionaryValue* hashed_prefs = NULL;
+  pref_hash_dicts->GetDictionary(profile_name_, &hashed_prefs);
+  for (int i = 0; i < tracked_pref_path_count_; ++i) {
+    // Skip prefs that haven't been registered (e.g. in browser_tests).
+    if (!prefs_->FindPreference(tracked_pref_paths_[i]))
+      continue;
+
+    bool unchanged = true;

[battre, 2013/09/02 08:33:46]

how about removing one negation and calling it changed?

[bbudge, 2013/09/04 15:00:09]

Done.

+    const base::Value* value = prefs_->GetUserPrefValue(tracked_pref_paths_[i]);
+    if (value) {
+      std::string value_hash =
+          GetHashedPrefValue(tracked_pref_paths_[i], value);
+      std::string last_hash;
+      if (hashed_prefs &&
+          hashed_prefs->GetString(tracked_pref_paths_[i], &last_hash)) {
+        if (value_hash != last_hash) {
+          unchanged = false;
+          // Record that the preference changed from its last value.
+          UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
+              i, tracked_pref_path_count_);
+          UpdateTrackedPreference(tracked_pref_paths_[i]);
+        }
+      } else {
+        unchanged = false;
+        // Record that we haven't tracked this preference yet, or the hash in
+        // local state was removed.
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceInitialized",
+            i, tracked_pref_path_count_);
+        UpdateTrackedPreference(tracked_pref_paths_[i]);
+      }
+    } else {
+      // There is no preference set. Remove any hashed value from local state
+      // and if one was present, record that a pref was cleared.
+      if (RemoveTrackedPreference(tracked_pref_paths_[i])) {
+        unchanged = false;
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceCleared",
+            i, tracked_pref_path_count_);
+      }
+    }
+    if (unchanged) {
+      UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceUnchanged",
+          i, tracked_pref_path_count_);
+    }
+  }
+}
+
+void PrefMetricsService::UpdateTrackedPreference(const char* path) {
+  // Don't attempt to change local state during browser shutdown.
+  if (BrowserShuttingDown())
+    return;
+
+  const base::Value* value = prefs_->GetUserPrefValue(path);
+  if (value) {
+    DictionaryPrefUpdate update(local_state_, prefs::kProfilePreferenceHashes);
+    base::DictionaryValue* dict = update.Get();
+    if (dict) {
+      dict->SetString(GetHashedPrefPath(path),
+                      GetHashedPrefValue(path, value));
+    }
+  } else {
+    RemoveTrackedPreference(path);
+  }
+}
+
+bool PrefMetricsService::RemoveTrackedPreference(const char* path) {
+  DictionaryPrefUpdate update(local_state_, prefs::kProfilePreferenceHashes);
+  base::DictionaryValue* dict = update.Get();
+  if (dict)
+    return dict->Remove(GetHashedPrefPath(path), NULL);
+
+  return false;
+}
+
+std::string PrefMetricsService::GetHashedPrefPath(const char* path) {
+  std::string hash_pref_path(profile_name_);
+  hash_pref_path.append(".");
+  hash_pref_path.append(path);
+  return hash_pref_path;
+}
+
+std::string PrefMetricsService::GetHashedPrefValue(
+    const char* path,
+    const base::Value* value) {
+  DCHECK(value);
+  std::string value_string;
+  JSONStringValueSerializer serializer(&value_string);
+  serializer.Serialize(*value);

[battre, 2013/09/02 08:33:46]

This is pretty brittle. If somebody modifies the serialization it will trigger a
spike in the UMA. Currently, the code is deterministic because dictionaries are
std::maps instead of std::hash_maps that don't preserve order. This could change
for example. Or whitespacing could change. Do you want to store concrete values
in the unit tests that trigger in these cases?

[bbudge, 2013/09/04 15:00:09]

Good point. I added some test code to protect against this.

+
+  base::MD5Context context;
+  base::MD5Init(&context);
+  // Salt the hash with an initial seed, the device id, and the pref path.
+  base::MD5Update(&context, pref_hash_seed_);
+  base::MD5Update(&context, device_id_);
+  base::MD5Update(&context, path);
+  base::MD5Update(&context, value_string);
+  base::MD5Digest digest;
+  base::MD5Final(&digest, &context);
+  return base::MD5DigestToBase16(digest);
+}
+
+void PrefMetricsService::InitializePrefObservers() {
+  pref_registrar_.Init(prefs_);
+  for (int i = 0; i < tracked_pref_path_count_; ++i) {
+    pref_registrar_.Add(
+        tracked_pref_paths_[i],
+        base::Bind(&PrefMetricsService::UpdateTrackedPreference,
+                   weak_factory_.GetWeakPtr(),
+                   tracked_pref_paths_[i]));
+  }
+}
+
 // static
 PrefMetricsService::Factory* PrefMetricsService::Factory::GetInstance() {
   return Singleton<PrefMetricsService::Factory>::get();
 }
 
 // static
 PrefMetricsService* PrefMetricsService::Factory::GetForProfile(
     Profile* profile) {
   return static_cast<PrefMetricsService*>(
       GetInstance()->GetServiceForBrowserContext(profile, true));
@@ skipping 19 matching lines @@
 }
 
 bool PrefMetricsService::Factory::ServiceIsNULLWhileTesting() const {
   return false;
 }
 
 content::BrowserContext* PrefMetricsService::Factory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
   return chrome::GetBrowserContextRedirectedInIncognito(context);
 }