Use a Finch Experiment to control the Browser Blacklist

chrome/browser/chrome_elf_init_win.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 "base/metrics/field_trial.h"
+#include "base/metrics/histogram.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/win/registry.h"
+#include "chrome/browser/chrome_elf_init_win.h"
+#include "chrome/common/chrome_version_info.h"
+#include "chrome_elf/blacklist/blacklist.h"
+
+namespace {
+
+const char kBrowserBlacklistTrialName[] = "BrowserBlacklist";
+const char kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";
+
+// This enum is used to define the buckets for an enumerated UMA histogram.
+// Hence,
+//   (a) existing enumerated constants should never be deleted or reordered, and
+//   (b) new constants should only be appended in front of
+//       BLACKLIST_SETUP_EVENT_MAX.
+enum BlacklistSetupEventType {
+  // 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,
+};
+
+void RecordBlacklistSetupEvent(BlacklistSetupEventType blacklist_setup_event) {
+  UMA_HISTOGRAM_ENUMERATION("Blacklist.Setup",
+                            blacklist_setup_event,
+                            BLACKLIST_SETUP_EVENT_MAX);
+}
+
+}  // namespace
+
+void SetupChromeElfInitialization() {
+  if (base::FieldTrialList::FindFullName(kBrowserBlacklistTrialName) ==
+      kBrowserBlacklistTrialEnabledGroupName) {
+    BrowserBlacklistBeaconSetup();
+  } else {
+    // Disable the blacklist for all future runs by removing the beacon.
+    base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER);
+    blacklist_registry_key.DeleteKey(blacklist::kRegistryBeaconPath);
+  }
+}
+
+void BrowserBlacklistBeaconSetup() {
+  base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER,
+                                           blacklist::kRegistryBeaconPath,
+                                           KEY_READ | KEY_WRITE);
+
+  // Find the last recorded blacklist version and the current version.
+  std::wstring blacklist_version(L"");

[robertshield, 2014/01/02 20:46:20]

Use base::string16 rather than wstring.

Also, don't need the (L"") bit since wstrings are by default initialized to the
empty string.

[csharp, 2014/01/02 21:57:59]

Hmm, I thought I got a compile error when I tried that before, but it does work.
Clearly I'm going insane :)

Done.

+  blacklist_registry_key.ReadValue(blacklist::kBeaconVersion,
+                                   &blacklist_version);
+
+  chrome::VersionInfo version_info;
+  base::string16 current_version = base::UTF8ToUTF16(version_info.Version());
+  if (blacklist_version != current_version) {
+    // The blacklist hasn't run for this version yet, so enable it.
+    blacklist_registry_key.WriteValue(blacklist::kBeaconVersion,
+                                      current_version.c_str());
+
+    blacklist_registry_key.WriteValue(blacklist::kBeaconState,
+                                      blacklist::BLACKLIST_ENABLED);
+
+    RecordBlacklistSetupEvent(BLACKLIST_SETUP_ENABLED);
+
+    // Don't try to record the results of the last setup since it could have

[robertshield, 2014/01/02 20:46:20]

explicitly state here that by "results" we mean either
BLACKLIST_SETUP_RAN_SUCCESSFULLY or BLACKLIST_SETUP_FAILED

[csharp, 2014/01/02 21:57:59]

Done.

+    // been from either this version or the previous version (since crashes
+    // occur before we set the version in the registry).
+  } else {
+    // The blacklist version didn't change, so record the results of the
+    // latest setup.
+    DWORD blacklist_state = 0;

[robertshield, 2014/01/02 20:46:20]

= BLACKLIST_SETUP_ENABLED;

[csharp, 2014/01/02 21:57:59]

We are reading the value here, not writing. Setting blacklist_state to a dummy
value so we won't be anything if we fail to read it from the registry.

[robertshield, 2014/01/03 03:21:42]

0 isn't a dummy value though, it's a legitimate enum value - in this case
BLACKLIST_DISABLED (not BLACKLIST_SETUP_ENABLED, sorry I mixed up the enums
earlier).

Since the DWORD is effectively holding one of these enum values, you should
initialize blacklist_state with an identifier from the enum, or a value not
present in it (like -1)...

And you already did this in the latest patch, so huzzah!

+    blacklist_registry_key.ReadValueDW(blacklist::kBeaconState,
+                                       &blacklist_state);
+
+    // Record the results of the latest blacklist setup.
+    if (blacklist_state == blacklist::BLACKLIST_ENABLED) {
+      RecordBlacklistSetupEvent(BLACKLIST_SETUP_RAN_SUCCESSFULLY);
+    } else if (blacklist_state == blacklist::BLACKLIST_SETUP_RUNNING) {
+      RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED);
+
+      // Since the setup failed, mark the blacklist as disabled so we don't
+      // try it again for this version.
+      blacklist_registry_key.WriteValue(blacklist::kBeaconState,
+                                        blacklist::BLACKLIST_DISABLED);
+    }
+  }
+}