Chrome Settings Hardening

chrome/browser/prefs/chrome_pref_service_factory.cc

Index: chrome/browser/prefs/chrome_pref_service_factory.cc
diff --git a/chrome/browser/prefs/chrome_pref_service_factory.cc b/chrome/browser/prefs/chrome_pref_service_factory.cc
index a459ea8c81cc042e0218861a0c7f5332495f89fb..0194b079e78d7dc8e1312f224b34581a857bb775 100644
--- a/chrome/browser/prefs/chrome_pref_service_factory.cc
+++ b/chrome/browser/prefs/chrome_pref_service_factory.cc
@@ -8,6 +8,7 @@
 #include "base/debug/trace_event.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
+#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/default_pref_store.h"
 #include "base/prefs/json_pref_store.h"
@@ -23,6 +24,7 @@
 #include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/prefs/pref_service_syncable_factory.h"
 #include "chrome/browser/ui/profile_error_dialog.h"
+#include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -44,6 +46,71 @@ using content::BrowserThread;
 
 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. Each
+// tracked preference must be given a unique reporting ID.
+const PrefHashFilter::TrackedPreference kTrackedPrefs[] = {
+  { 0, prefs::kShowHomeButton, true },
+  { 1, prefs::kHomePageIsNewTabPage, true },
+  { 2, prefs::kHomePage, true },
+  { 3, prefs::kRestoreOnStartup, true },
+  { 4, prefs::kURLsToRestoreOnStartup, true },
+  { 5, prefs::kExtensionsPref, false },
+  { 6, prefs::kGoogleServicesLastUsername, true },
+  { 7, prefs::kSearchProviderOverrides, true },
+  { 8, prefs::kDefaultSearchProviderSearchURL, true },
+  { 9, prefs::kDefaultSearchProviderKeyword, true },
+  { 10, prefs::kDefaultSearchProviderName, true },
+#if !defined(OS_ANDROID)
+  { 11, prefs::kPinnedTabs, true },
+#endif
+  { 12, prefs::kExtensionKnownDisabled, true },
+  { 13, prefs::kProfileResetPromptMemento, true },
+};
+
+// The count of tracked preferences IDs across all platforms.
+const size_t kTrackedPrefsReportingIDsCount = 14;

[gab, 2014/01/02 18:40:22]

@erikwright pointed out that this might not be necessary.

I added it so that the same count is reported on every platform (e.g., if the
last kTrackedPref is ifdef'ed out on some platforms).

Is this required that the count reported via the histogram macros be the same
across all platforms (or even across multiple calls to the same histogram -- see
pref_hash_filter.cc for the histograms themselves)?

[Mark P, 2014/01/02 19:32:46]

erikwright@ is correct that this is not strictly necessary.  As long as your
emissions to your UMA enum histogram are in the valid range (i.e., the max
number you emit on a particular platform is less than the number of buckets you
pass to the UMA_HISTOGRAM_ENUMERATION macro), then you should be fine.  This
appears to be the case.

*However*, I'd prefer you keep the code as-is, using the same number of buckets
on every platform.  The dashboard code uses conflicting buckets for error
detection.  Most of your clients will emit to buckets such as [1, 2), [12, 13),
etc.  You'll never normally hit the overflow [14, infinity) bucket.  But suppose
one of your clients on a platform that uses a smaller maximum bucket has a bit
error or corrupted memory or something and emits to the overflow bucket.  Now
the dashboard will see a [12, infinity) count along with a set of normal [12,
13) counts.  I'm not sure how smart it'll be to handle this case, and even if it
does handle it well now, it might cause issues with our plans for better
removing bad data in the future.  In other words, I'd feel more comfortable if
the histogram used the same bucketing on all platforms.

+COMPILE_ASSERT(kTrackedPrefsReportingIDsCount >= arraysize(kTrackedPrefs),
+               need_to_increment_ids_count);
+
+PrefHashFilter::EnforcementLevel GetSettingsEnforcementLevel() {
+  static const char kSettingsEnforcementExperiment[] = "SettingsEnforcement";
+  struct {
+    PrefHashFilter::EnforcementLevel level;

[erikwright (departed), 2014/01/03 19:08:13]

nit: I would reverse this since, intuitively, a map maps from the first field to
the second (key-value, not "value-key").

Also, consider using an actual map like:

std::map<string, level> m;
m.insert("a", A);
m.insert("b", B);

if (trial) {
  map::iterator it = m.find(group_name);
  if (it != m.end())
    return *it;
}
return default;

I'm not sure if it will be more or less clear. Slightly less efficient (NlogN?)
but N is small and this is only done once per profile I think, so code
simplicity should dominate.

[gab, 2014/01/03 19:26:09]

Reversed the mapping (i.e., name first).

I still prefer the current form though. Building a map in local scope just to
perform a single search in it seems counter-intuitive to me.

+    const char* level_name;
+  } static const kEnforcementLevelMap[] = {
+    {
+      PrefHashFilter::NO_ENFORCEMENT,
+      "no_enforcement"
+    },
+    {
+      PrefHashFilter::ENFORCE,
+      "enforce"
+    },
+    {
+      PrefHashFilter::ENFORCE_NO_SEEDING,
+      "enforce_no_seeding"
+    },
+    {
+      PrefHashFilter::ENFORCE_NO_SEEDING_NO_MIGRATION,
+      "enforce_no_seeding_no_migration"
+    },
+  };
+
+  base::FieldTrial* trial =
+      base::FieldTrialList::Find(kSettingsEnforcementExperiment);
+  if (trial) {
+    const std::string& group_name = trial->group_name();
+    for (size_t i = 0; i < arraysize(kEnforcementLevelMap); ++i) {
+      if (kEnforcementLevelMap[i].level_name == group_name)
+        return kEnforcementLevelMap[i].level;
+    }
+  }
+  // TODO(gab): Switch default to ENFORCE_ALL when field trial config is up.
+  return PrefHashFilter::NO_ENFORCEMENT;
+}
+
 // Shows notifications which correspond to PersistentPrefStore's reading errors.
 void HandleReadError(PersistentPrefStore::PrefReadError error) {
   // Sample the histogram also for the successful case in order to get a
@@ -126,8 +193,13 @@ void PrepareBuilder(
           new CommandLinePrefStore(CommandLine::ForCurrentProcess())));
   factory->set_read_error_callback(base::Bind(&HandleReadError));
   scoped_ptr<PrefFilter> pref_filter;
-  if (pref_hash_store)
-    pref_filter.reset(new PrefHashFilter(pref_hash_store.Pass()));
+  if (pref_hash_store) {
+    pref_filter.reset(new PrefHashFilter(pref_hash_store.Pass(),
+                                         kTrackedPrefs,
+                                         arraysize(kTrackedPrefs),
+                                         kTrackedPrefsReportingIDsCount,
+                                         GetSettingsEnforcementLevel()));
+  }
   factory->set_user_prefs(
       new JsonPrefStore(
           pref_filename,