Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/component_updater/sw_reporter_installer_win.cc

Index: chrome/browser/component_updater/sw_reporter_installer_win.cc
diff --git a/chrome/browser/component_updater/sw_reporter_installer_win.cc b/chrome/browser/component_updater/sw_reporter_installer_win.cc
index cad962c7d29a8f83f4a2a8fb84770271fee87a19..fc0e1e684911926cf2a98e197c867fe599591bae 100644
--- a/chrome/browser/component_updater/sw_reporter_installer_win.cc
+++ b/chrome/browser/component_updater/sw_reporter_installer_win.cc
@@ -6,6 +6,7 @@
 
 #include <stdint.h>
 
+#include <algorithm>
 #include <map>
 #include <memory>
 #include <string>
@@ -14,6 +15,8 @@
 
 #include "base/base_paths.h"
 #include "base/bind.h"
+#include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
@@ -21,10 +24,12 @@
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_tokenizer.h"
+#include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/threading/worker_pool.h"
 #include "base/time/time.h"
 #include "base/win/registry.h"
+#include "base/win/windows_version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/safe_browsing/srt_fetcher_win.h"
 #include "chrome/browser/safe_browsing/srt_field_trial_win.h"
@@ -35,14 +40,15 @@
 #include "components/prefs/pref_registry_simple.h"
 #include "components/update_client/update_client.h"
 #include "components/update_client/utils.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace component_updater {
 
 namespace {
 
-// These two sets of values are used to send UMA information and are replicated
-// in the histograms.xml file, so the order MUST NOT CHANGE.
+// These values are used to send UMA information and are replicated in the
+// histograms.xml file, so the order MUST NOT CHANGE.
 enum SRTCompleted {
   SRT_COMPLETED_NOT_YET = 0,
   SRT_COMPLETED_YES = 1,
@@ -67,6 +73,9 @@ const wchar_t kExitCodeValueName[] = L"ExitCode";
 const wchar_t kUploadResultsValueName[] = L"UploadResults";
 const wchar_t kVersionValueName[] = L"Version";
 
+constexpr base::Feature kExperimentalEngineFeature{
+    "ExperimentalSwReporterEngine", base::FEATURE_DISABLED_BY_DEFAULT};
+
 void SRTHasCompleted(SRTCompleted value) {
   UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.Cleaner.HasCompleted", value,
                             SRT_COMPLETED_MAX);
@@ -103,6 +112,11 @@ void ReportUploadsWithUma(const base::string16& upload_results) {
   UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.LastUploadResult", last_result);
 }
 
+void ReportExperimentError(SwReporterExperimentError error) {
+  UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.ExperimentErrors", error,
+                            SW_REPORTER_EXPERIMENT_ERROR_MAX);
+}
+
 // Run the software reporter on the next Chrome startup after it's downloaded.
 // (This is the default |reporter_runner| function passed to the
 // |SwReporterInstallerTraits| constructor in |RegisterSwReporterComponent|
@@ -117,11 +131,109 @@ void RunSwReporterAfterStartup(
                  base::WorkerPool::GetTaskRunner(true)));
 }
 
+// Ensures |str| contains only alphanumeric characters and characters from
+// |extras|, and is not longer than |max_length|.
+bool ValidateString(const std::string& str,
+                    const std::string& extras,
+                    size_t max_length) {
+  return str.size() <= max_length &&
+         std::all_of(str.cbegin(), str.cend(), [&extras](char c) {
+           return base::IsAsciiAlpha(c) || base::IsAsciiDigit(c) ||
+                  extras.find(c) != std::string::npos;
+         });
+}
+
+// Reads the command-line params and an UMA histogram suffix from the manifest,

[grt (UTC plus 2), 2016/08/26 10:31:56]

nit: "Lifts the bottle and drinks the beer." shouldn't have a comma before "and"

+// and launch the SwReporter with those parameters. If anything goes wrong the

[grt (UTC plus 2), 2016/08/26 10:31:56]

nit: launches

+// SwReporter should not be run at all, instead of falling back to the default.
+void RunExperimentalSwReporter(const base::FilePath& exe_path,
+                               const base::Version& version,
+                               std::unique_ptr<base::DictionaryValue> manifest,
+                               const SwReporterRunner& reporter_runner) {
+  // The experiment requires launch_params so if they aren't present just
+  // return. This isn't an error because the user could get into the experiment
+  // group before they've downloaded the experiment component.
+  base::Value* launch_params = nullptr;
+  if (!manifest->Get("launch_params", &launch_params))
+    return;
+
+  const base::ListValue* parameter_list = nullptr;
+  if (!launch_params->GetAsList(&parameter_list) || parameter_list->empty() ||

[grt (UTC plus 2), 2016/08/26 10:31:56]

replace the last two conditions with "parameter_list->GetSize() != 1"

+      // For future expansion, the manifest takes a list of invocation

[grt (UTC plus 2), 2016/08/26 10:31:56]

nit: i find a multi-line comment in the middle of a condition like this reduces
readability. if this comment is needed, put it before the "if".

+      // parameters, but currently we only support a single invocation.
+      parameter_list->GetSize() > 1) {
+    ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS);
+    return;
+  }
+
+  const base::DictionaryValue* invocation_params = nullptr;
+  if (!parameter_list->GetDictionary(0, &invocation_params)) {
+    ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS);
+    return;
+  }
+
+  // Max length of the registry and histogram suffix. Fairly arbitrary: the
+  // Windows registry accepts much longer keys, but we need to display this
+  // string in histograms as well.
+  constexpr size_t kMaxSuffixLength = 80;
+
+  // The suffix must be an alphanumeric string. (Empty is fine as long as the

[grt (UTC plus 2), 2016/08/26 10:31:56]

nit: either make the parenthetical remark a part of the previous sentence, or
make it non-parenthetical. or simplify to something like:

  // The suffix must be an alphanumeric string if not empty.

+  // "suffix" key is present.)
+  std::string suffix;
+  const base::Value* suffix_value = nullptr;
+  if (!invocation_params->Get("suffix", &suffix_value) ||
+      !suffix_value->GetAsString(&suffix) ||

[grt (UTC plus 2), 2016/08/26 10:31:56]

nix |suffix_value| and replace these two lines with:
  !invocation_params->GetString("suffix", &suffix)
since you don't need to distinguish between "is not present" and "is present,
but is not a string"

+      (!suffix.empty() &&

[grt (UTC plus 2), 2016/08/26 10:31:56]

i agree that this check should be removed. it doesn't add anything beyond the
comment above, yet it makes the code harder to read. additionally, it introduces
an an extra check to short-circuit the following function call that will only
happen in the exceptional error condition. in the majority of cases, it's a few
more wasted cycles (and bytes in the binary).

+       !ValidateString(suffix, std::string(), kMaxSuffixLength))) {
+    ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS);
+    return;
+  }
+
+  // Build a command line for the reporter out of the executable path and the
+  // arguments from the manifest. (The "arguments" key must be present, but
+  // it's ok if it's an empty list or a list of empty strings.)
+  std::vector<base::string16> argv = {exe_path.value()};
+  const base::Value* arguments_value = nullptr;
+  const base::ListValue* arguments = nullptr;
+  if (!invocation_params->Get("arguments", &arguments_value) ||

[grt (UTC plus 2), 2016/08/26 10:31:56]

nix |arguments_value| and replace with:
  !invocation_params->GetList("arguments", &arguments)

+      !arguments_value->GetAsList(&arguments)) {
+    ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS);
+    return;
+  }
+
+  for (const auto& value : *arguments) {
+    base::string16 argument;
+    if (!value->GetAsString(&argument)) {
+      ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS);
+      return;
+    }
+    if (!argument.empty())
+      argv.push_back(argument);
+  }
+
+  base::CommandLine command_line(argv);
+
+  // Add the histogram suffix to the command-line as well, so that the
+  // reporter will add the same suffix to registry keys where it writes
+  // metrics.
+  if (!suffix.empty())
+    command_line.AppendSwitchASCII("registry-suffix", suffix);
+
+  auto invocation =
+      safe_browsing::SwReporterInvocation::FromCommandLine(command_line);
+  invocation.suffix = suffix;
+  invocation.is_experimental = true;
+
+  reporter_runner.Run(invocation, version);
+}
+
 }  // namespace
 
 SwReporterInstallerTraits::SwReporterInstallerTraits(
-    const SwReporterRunner& reporter_runner)
-    : reporter_runner_(reporter_runner) {}
+    const SwReporterRunner& reporter_runner,
+    bool is_experimental_engine_supported)
+    : reporter_runner_(reporter_runner),
+      is_experimental_engine_supported_(is_experimental_engine_supported) {}
 
 SwReporterInstallerTraits::~SwReporterInstallerTraits() {}
 
@@ -152,8 +264,13 @@ void SwReporterInstallerTraits::ComponentReady(
     std::unique_ptr<base::DictionaryValue> manifest) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   const base::FilePath exe_path(install_dir.Append(kSwReporterExeName));
-  reporter_runner_.Run(
-      safe_browsing::SwReporterInvocation::FromFilePath(exe_path), version);
+  if (IsExperimentalEngineEnabled()) {
+    RunExperimentalSwReporter(exe_path, version, std::move(manifest),
+                              reporter_runner_);
+  } else {
+    reporter_runner_.Run(
+        safe_browsing::SwReporterInvocation::FromFilePath(exe_path), version);
+  }
 }
 
 base::FilePath SwReporterInstallerTraits::GetRelativeInstallDir() const {
@@ -171,13 +288,39 @@ std::string SwReporterInstallerTraits::GetName() const {
 
 update_client::InstallerAttributes
 SwReporterInstallerTraits::GetInstallerAttributes() const {
-  return update_client::InstallerAttributes();
+  update_client::InstallerAttributes attributes;
+  if (IsExperimentalEngineEnabled()) {
+    // Pass the "tag" parameter to the installer; it will be used to choose
+    // which binary is downloaded.
+    constexpr char kTagParam[] = "tag";
+    const std::string tag = variations::GetVariationParamValueByFeature(
+        kExperimentalEngineFeature, kTagParam);
+
+    // If the tag is not a valid attribute (see the regexp in
+    // ComponentInstallerTraits::InstallerAttributes), set it to a valid but
+    // unrecognized value so that nothing will be downloaded.
+    constexpr size_t kMaxAttributeLength = 256;
+    constexpr char kExtraAttributeChars[] = "-.,;+_=";
+    if (tag.empty() ||
+        !ValidateString(tag, kExtraAttributeChars, kMaxAttributeLength)) {
+      ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_TAG);
+      attributes[kTagParam] = "missing_tag";
+    } else {
+      attributes[kTagParam] = tag;
+    }
+  }
+  return attributes;
 }
 
 std::vector<std::string> SwReporterInstallerTraits::GetMimeTypes() const {
   return std::vector<std::string>();
 }
 
+bool SwReporterInstallerTraits::IsExperimentalEngineEnabled() const {
+  return is_experimental_engine_supported_ &&
+         base::FeatureList::IsEnabled(kExperimentalEngineFeature);
+}
+
 void RegisterSwReporterComponent(ComponentUpdateService* cus) {
   if (!safe_browsing::IsSwReporterEnabled())
     return;
@@ -254,9 +397,16 @@ void RegisterSwReporterComponent(ComponentUpdateService* cus) {
     }
   }
 
+  // The experiment is only enabled on x86. There's no way to check this in the
+  // variations config so we'll hard-code it.
+  const bool is_experimental_engine_supported =
+      base::win::OSInfo::GetInstance()->architecture() ==
+      base::win::OSInfo::X86_ARCHITECTURE;
+
   // Install the component.
   std::unique_ptr<ComponentInstallerTraits> traits(
-      new SwReporterInstallerTraits(base::Bind(&RunSwReporterAfterStartup)));
+      new SwReporterInstallerTraits(base::Bind(&RunSwReporterAfterStartup),
+                                    is_experimental_engine_supported));
   // |cus| will take ownership of |installer| during installer->Register(cus).
   DefaultComponentInstaller* installer =
       new DefaultComponentInstaller(std::move(traits));