Use a Finch Experiment to control the Browser Blacklist

chrome/browser/chrome_browser_main_win.cc

Index: chrome/browser/chrome_browser_main_win.cc
diff --git a/chrome/browser/chrome_browser_main_win.cc b/chrome/browser/chrome_browser_main_win.cc
index 38ffa8220b7144dc92eb406cfa8512e2d65f13b0..c126ca10fe27d652f9c198c26858929acd51a3da 100644
--- a/chrome/browser/chrome_browser_main_win.cc
+++ b/chrome/browser/chrome_browser_main_win.cc
@@ -13,6 +13,8 @@
 #include "base/environment.h"
 #include "base/files/file_path.h"
 #include "base/i18n/rtl.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/scoped_native_library.h"
@@ -39,6 +41,7 @@
 #include "chrome/installer/util/install_util.h"
 #include "chrome/installer/util/l10n_string_util.h"
 #include "chrome/installer/util/shell_util.h"
+#include "chrome_elf/blacklist/blacklist.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/main_function_params.h"
 #include "grit/app_locale_settings.h"
@@ -59,6 +62,23 @@ typedef HRESULT (STDAPICALLTYPE* RegisterApplicationRestartProc)(
     const wchar_t* command_line,
     DWORD flags);
 
+const char kBrowserBlacklistTrialName[] = "BrowserBlacklist";
+const char kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";
+
+enum BlacklistSetupEvent {
+  // The blacklist beacon has placed to enable the browser blacklisting.
+  BLACKLIST_SETUP_ENABLED = 0,
+
+  // The blacklist was successfully enabled.
+  BLACKLIST_SETUP_RAN_SUCCESSFULLY,
+
+  // The blacklist setup code failed to execute.
+  BLACKLIST_SETUP_FAILED,
+
+  // Always keep this at the end.
+  BLACKLIST_SETUP_EVENT_MAX,
+};

[Ilya Sherman, 2013/12/27 01:22:50]

Please document that this histogram is used to back an UMA_HISTOGRAM_ENUMERATION
metric, and hence (a) existing constants should never be deleted or re-arranged;
and (b) new constants should only ever be added at the end of the histogram. 
See [ https://sites.google.com/a/chromium.org/dev/Home/user-metrics ] if you'd
like a template comment.  (Feel free to improve the template as well, I just now
wrote it, so it's only had one pair of eyes on it.)

[csharp, 2014/01/02 19:55:36]

Added the comment although I reworded part b to say new elements should be
inserted in front of BLACKLIST_SETUP_EVENT_MAX. I'm not quite sure how or if the
comment template should be adjusted to mention this fact (since the _MAX value
isn't really part of the histogram and it can be moved further down).

+
 void InitializeWindowProcExceptions() {
   // Get the breakpad pointer from chrome.exe
   base::win::WinProcExceptionFilter exception_filter =
@@ -251,6 +271,83 @@ void ChromeBrowserMainPartsWin::PostBrowserStart() {
           FROM_HERE,
           base::Bind(&VerifyInstallation),
           base::TimeDelta::FromSeconds(45));
+
+  // Setup the blacklist beacon if this client is in the experiment to use
+  // the browser blacklist.
+  if (base::FieldTrialList::FindFullName(kBrowserBlacklistTrialName) ==
+      kBrowserBlacklistTrialEnabledGroupName) {
+    // Find the current state of the blacklist.
+    DWORD blacklist_state;

[robertshield, 2013/12/27 03:24:29]

= 0

[csharp, 2014/01/02 19:55:36]

Done.

+    DWORD blacklist_state_size = sizeof(blacklist_state);
+    ::RegGetValue(HKEY_CURRENT_USER,

[robertshield, 2013/12/27 03:24:29]

Please use base/win/registry.h instead of the advapi32 functions while in
chrome/

[csharp, 2014/01/02 19:55:36]

Done.

+                  blacklist::kRegistryBeaconPath,
+                  blacklist::kBeaconState,
+                  RRF_RT_REG_DWORD,
+                  NULL,
+                  &blacklist_state,
+                  &blacklist_state_size);
+
+    if (blacklist_state == blacklist::BLACKLIST_ENABLED) {
+      // The blacklist was setup successfully.
+      UMA_HISTOGRAM_ENUMERATION("Blacklist.Setup",
+                                BLACKLIST_SETUP_RAN_SUCCESSFULLY,
+                                BLACKLIST_SETUP_EVENT_MAX);
+    } else if (blacklist_state == blacklist::BLACKLIST_SETUP_RUNNING) {
+      // The blacklist setup failed.
+      UMA_HISTOGRAM_ENUMERATION("Blacklist.Setup",
+                                BLACKLIST_SETUP_FAILED,
+                                BLACKLIST_SETUP_EVENT_MAX);

[Ilya Sherman, 2013/12/27 01:22:50]

Please create a wrapper method to emit to this histogram, so that all of the
shared parts of the call are just in one place.

[csharp, 2014/01/02 19:55:36]

Done.

+
+      // Since the setup failed, mark the blacklist as disabled so we don't
+      // try it again.
+      DWORD blacklist_state = blacklist::BLACKLIST_DISABLED;
+      ::RegSetKeyValue(HKEY_CURRENT_USER,
+                       blacklist::kRegistryBeaconPath,
+                       blacklist::kBeaconState,
+                       REG_DWORD,
+                       &blacklist_state,
+                       sizeof(blacklist_state));
+    }
+
+    // Find the version the blacklist was last enabled for.
+    wchar_t key_data[255];

[robertshield, 2013/12/27 03:24:29]

 = {}

[csharp, 2014/01/02 19:55:36]

Done.

+    DWORD key_data_size = sizeof(key_data);
+    ::RegGetValue(HKEY_CURRENT_USER,
+                  blacklist::kRegistryBeaconPath,
+                  blacklist::kBeaconVersion,
+                  RRF_RT_REG_SZ,
+                  NULL,
+                  key_data,
+                  &key_data_size);
+
+    // If the blacklist hasn't been enabled for this version yet, enable it.
+    chrome::VersionInfo version_info;
+    base::string16 version = UTF8ToUTF16(version_info.Version());
+    if (key_data != version) {
+      ::RegSetKeyValue(HKEY_CURRENT_USER,
+                       blacklist::kRegistryBeaconPath,
+                       blacklist::kBeaconVersion,
+                       REG_SZ,
+                       version.c_str(),
+                       version.size() * sizeof(wchar_t));
+
+      DWORD blacklist_state = blacklist::BLACKLIST_ENABLED;
+      ::RegSetKeyValue(HKEY_CURRENT_USER,
+                       blacklist::kRegistryBeaconPath,
+                       blacklist::kBeaconState,
+                       REG_DWORD,
+                       &blacklist_state,
+                       sizeof(blacklist_state));
+
+      // Record the blacklist getting enabled for this version.
+      UMA_HISTOGRAM_ENUMERATION("Blacklist.Setup",
+                                BLACKLIST_SETUP_ENABLED,
+                                BLACKLIST_SETUP_EVENT_MAX);
+    }
+  } else {
+    // Disable the blacklist for future runs by removing the beacon.
+    ::RegDeleteKey(HKEY_CURRENT_USER, blacklist::kRegistryBeaconPath);
+  }

[Ilya Sherman, 2013/12/27 01:22:50]

I've mostly only looked at the histograms-related code in this CL, but it seems
wrong to me to include so much extra code in chrome_browser_main_win.cc,
especially inline in this method.  Can you factor this out into a separate
method, preferably in a more specialized implementation file?

[robertshield, 2013/12/27 03:24:29]

+1

[csharp, 2014/01/02 19:55:36]

Done.

 }
 
 // static