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..0bd28675973b53fce745da834b6ff0bd8a72f6d8 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,117 @@ 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,
+// 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) {
+  // On the first startup after a user is enrolled in the field trial, they may

[csharp, 2016/08/25 22:28:14]

They comment section is pretty big now, what about replacing the two paragraphs
with:

"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." or something like that

[Joe Mason, 2016/08/25 23:17:00]

Done.

+  // already have the non-experimental software reporter component installed.
+  // In that case, the component updater will first call |ComponentReady| with
+  // the manifest from the existing component, then download the new component,
+  // then call |ComponentReady| again with the new manifest.
+  //
+  // Only the experimental version of the reporter will have a launch_params
+  // section in the manifest. So if the section is entirely missing, just
+  // return. We don't want to log an error because this is a normal part of
+  // startup, but we also don't want to run the existing version - this user
+  // should wait for the experimental version to be ready.
+  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() ||
+      // For future expansion, the manifest takes a list of invocation
+      // 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
+  // "suffix" key is present.)
+  std::string suffix;
+  const base::Value* suffix_value = nullptr;
+  if (!invocation_params->Get("suffix", &suffix_value) ||
+      !suffix_value->GetAsString(&suffix) ||
+      (!suffix.empty() &&

[csharp, 2016/08/25 22:28:14]

Is this needed? ValidateString is true for an empty string so I don't think it
adds anything

[Joe Mason, 2016/08/25 23:17:00]

It's easy to miss that ValidateString implies accepting an empty string. I
prefer to make it explicit in the condition here.

+       !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) ||
+      !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 +272,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 +296,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 +405,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));