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..1ffecf02e0a5854c9ebf7613bcaebbdb47778111 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,
@@ -117,11 +123,134 @@ void RunSwReporterAfterStartup(
                  base::WorkerPool::GetTaskRunner(true)));
 }
 
+// Constants and helper functions for the Experimental Engine feature.
+
+constexpr base::Feature kExperimentalEngineFeature{

[csharp, 2016/08/25 19:47:27]

The constants and enum can probably be moved up to live with the other constants
and enum, it shouldn't matter if they aren't too close to the function since
their names should be all that is needed here

[Joe Mason, 2016/08/25 22:03:20]

I see I'm outnumbered here. Done.

+    "ExperimentalSwReporterEngine", base::FEATURE_DISABLED_BY_DEFAULT};
+
+// Max length of an installer attribute (taken from the regexp in
+// ComponentInstallerTraits::InstallerAttributes).
+constexpr size_t kMaxAttributeLength = 256;
+
+// 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;
+
+// These MUST match the values for SwReporterExperimentError in histograms.xml.
+enum ExperimentError {
+  EXPERIMENT_ERROR_BAD_TAG = 0,
+  EXPERIMENT_ERROR_BAD_PARAMS = 1,
+  EXPERIMENT_ERROR_MAX,
+};
+
+void ReportExperimentError(ExperimentError error) {
+  UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.ExperimentErrors", error,
+                            EXPERIMENT_ERROR_MAX);
+}
+
+// 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,
+// and launch the SwReporter with those parameters. If anything goes wrong the
+// 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) {
+  // Don't log an error if the launch_params section is entirely missing.  This
+  // can happen when the user already has an older version of the software
+  // reporter component, so |ComponentReady| is called on startup before the
+  // experimental version is downloaded.
+  //
+  // Also don't run the old version, though. We want this user to run the

[csharp, 2016/08/25 19:47:27]

nit: This comment doesn't really add much since the old version can't run here
at all. Remove?

[Joe Mason, 2016/08/25 22:03:20]

Actually the old version can run here, if we're not careful - this comment is
explaining why we return early. I reworded it to be more clear.

+  // experimental version once it's been downloaded.
+  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()) {
+    ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+    return;
+  }
+
+  // For now we only support running the reporter once, so only look at the

[csharp, 2016/08/25 19:47:28]

Nit: Instead of this comments what about just adding to the if statement above
"|| parameter_list->size() > 1", since it would be an error for this to get get
multiple args

[Joe Mason, 2016/08/25 22:03:19]

Done.

+  // first set of arguments in the list.
+  const base::DictionaryValue* invocation_params = nullptr;
+  if (!parameter_list->GetDictionary(0, &invocation_params)) {
+    ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+    return;
+  }
+
+  // The suffix is optional. If present, it must be a short alphanumeric string.
+  std::string suffix;
+  const base::Value* suffix_value = nullptr;
+  if (invocation_params->Get("suffix", &suffix_value)) {
+    if (!suffix_value->GetAsString(&suffix)) {
+      ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+      return;
+    }
+    if (!suffix.empty() &&

[csharp, 2016/08/25 19:47:28]

nit: I wonder if it'd be clear to merge this with the above statement"
if (!(invocation_params->Get("suffix", &suffix_value) && !suffix.empty() &&
ValidateString(suffix, std::string(), kMaxSuffixLength))"

[Joe Mason, 2016/08/25 22:03:20]

Done.

+        !ValidateString(suffix, std::string(), kMaxSuffixLength)) {
+      ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+      return;
+    }
+  }
+
+  // Build a command line for the reporter out of the executable path and the
+  // (optional) arguments from the manifest.
+  std::vector<base::string16> argv = {exe_path.value()};
+  const base::Value* arguments_value = nullptr;
+  if (invocation_params->Get("arguments", &arguments_value)) {
+    const base::ListValue* arguments = nullptr;
+    if (!arguments_value->GetAsList(&arguments)) {
+      ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+      return;
+    }
+    for (const std::unique_ptr<base::Value>& value : *arguments) {
+      base::string16 argument;
+      if (!value->GetAsString(&argument)) {
+        ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+        return;
+      }
+      if (!argument.empty())

[csharp, 2016/08/25 19:47:27]

An empty argument seems like an error, move into the above if statement?

[Joe Mason, 2016/08/25 22:03:19]

I don't see any reason to ban an explicit empty string. It's a natural way to
write "no arguments". Look at the EmptyArguments and EmptyArguments2 unit tests
- it'd be confusing for one to work and the other to fail.

+        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,6 +281,11 @@ void SwReporterInstallerTraits::ComponentReady(
     std::unique_ptr<base::DictionaryValue> manifest) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   const base::FilePath exe_path(install_dir.Append(kSwReporterExeName));
+  if (IsExperimentalEngineEnabled()) {
+    RunExperimentalSwReporter(exe_path, version, std::move(manifest),
+                              reporter_runner_);
+    return;

[csharp, 2016/08/25 19:47:27]

nit: Instead of a return here what about just adding an else for the next run
command

[Joe Mason, 2016/08/25 22:03:19]

Done.

+  }
   reporter_runner_.Run(
       safe_browsing::SwReporterInvocation::FromFilePath(exe_path), version);
 }
@@ -171,13 +305,36 @@ 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.
+    if (tag.empty() || !ValidateString(tag, "_.,;+_=", kMaxAttributeLength)) {

[csharp, 2016/08/25 19:47:28]

nit: Please put "_.,;+_=" into a named constant

[Joe Mason, 2016/08/25 22:03:20]

Done.

+      ReportExperimentError(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 +411,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));