Use a Finch Experiment to control the Browser Blacklist

chrome/browser/chrome_browser_main_win.cc

 // Copyright (c) 2012 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/chrome_browser_main_win.h"
 
 #include <windows.h>
 #include <shellapi.h>
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/metro.h"
 #include "base/win/windows_version.h"
 #include "base/win/wrapped_window_proc.h"
 #include "chrome/browser/browser_util_win.h"
 #include "chrome/browser/install_verification/win/install_verification.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_shortcut_manager.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/ui/simple_message_box.h"
 #include "chrome/browser/ui/uninstall_browser_prompt.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/env_vars.h"
 #include "chrome/installer/launcher_support/chrome_launcher_support.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/helper.h"
 #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"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "installer_util_strings/installer_util_strings.h"
 #include "ui/base/cursor/cursor_loader_win.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/l10n/l10n_util_win.h"
 #include "ui/base/ui_base_switches.h"
 #include "ui/base/win/message_box_win.h"
 #include "ui/gfx/platform_font_win.h"
 #include "ui/gfx/switches.h"
 
 namespace {
 
 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 =
       reinterpret_cast<base::win::WinProcExceptionFilter>(
           ::GetProcAddress(::GetModuleHandle(
                                chrome::kBrowserProcessExecutableName),
                            "CrashForException"));
   exception_filter = base::win::SetWinProcExceptionFilter(exception_filter);
   DCHECK(!exception_filter);
 }
@@ skipping 172 matching lines @@
 void ChromeBrowserMainPartsWin::PostBrowserStart() {
   ChromeBrowserMainParts::PostBrowserStart();
 
   // Set up a task to verify installed modules in the current process. Use a
   // delay to reduce the impact on startup time.
   content::BrowserThread::GetMessageLoopProxyForThread(
       content::BrowserThread::UI)->PostDelayedTask(
           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
 void ChromeBrowserMainPartsWin::PrepareRestartOnCrashEnviroment(
     const CommandLine& parsed_command_line) {
   // Clear this var so child processes don't show the dialog by default.
   scoped_ptr<base::Environment> env(base::Environment::Create());
   env->UnSetVar(env_vars::kShowRestart);
 
   // For non-interactive tests we don't restart on crash.
@@ skipping 159 matching lines @@
   if (resource_id)
     return l10n_util::GetStringUTF16(resource_id);
   return base::string16();
 }
 
 // static
 void ChromeBrowserMainPartsWin::SetupInstallerUtilStrings() {
   CR_DEFINE_STATIC_LOCAL(TranslationDelegate, delegate, ());
   installer::SetTranslationDelegate(&delegate);
 }