Add support for the ExperimentalSwReporterEngine field trial.

BUG=631480

Review-Url: https://codereview.chromium.org/2226133005
Cr-Commit-Position: refs/heads/master@{#418578}

chrome/browser/component_updater/sw_reporter_installer_win.cc

@@ -47,6 +47,8 @@ namespace component_updater {
 
 namespace {
 
+using safe_browsing::SwReporterInvocation;
+
 // These values are used to send UMA information and are replicated in the
 // histograms.xml file, so the order MUST NOT CHANGE.
 enum SRTCompleted {
@@ -121,12 +123,12 @@ void ReportExperimentError(SwReporterExperimentError error) {
 // (This is the default |reporter_runner| function passed to the
 // |SwReporterInstallerTraits| constructor in |RegisterSwReporterComponent|
 // below.)
-void RunSwReporterAfterStartup(
-    const safe_browsing::SwReporterInvocation& invocation,
+void RunSwReportersAfterStartup(
+    const safe_browsing::SwReporterQueue& invocations,
     const base::Version& version) {
   content::BrowserThread::PostAfterStartupTask(
       FROM_HERE, base::ThreadTaskRunnerHandle::Get(),
-      base::Bind(&safe_browsing::RunSwReporter, invocation, version,
+      base::Bind(&safe_browsing::RunSwReporters, invocations, version,
                  base::ThreadTaskRunnerHandle::Get(),
                  base::WorkerPool::GetTaskRunner(true)));
 }
@@ -145,7 +147,7 @@ bool ValidateString(const std::string& str,
 
 // 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.
+// SwReporter should not be run at all.
 void RunExperimentalSwReporter(const base::FilePath& exe_path,
                                const base::Version& version,
                                std::unique_ptr<base::DictionaryValue> manifest,
@@ -158,73 +160,82 @@ void RunExperimentalSwReporter(const base::FilePath& exe_path,
     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) {
+  if (!launch_params->GetAsList(&parameter_list) || parameter_list->empty()) {
     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;
-  }
+  safe_browsing::SwReporterQueue invocations;
+  for (const auto& iter : *parameter_list) {
+    const base::DictionaryValue* invocation_params = nullptr;
+    if (!iter->GetAsDictionary(&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() &&
-       !ValidateString(suffix, std::string(), kMaxSuffixLength))) {
-    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;
 
-  // 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;
-  }
+    // The suffix must be an alphanumeric string. (Empty is fine as long as the
+    // "suffix" key is present.)
+    std::string suffix;
+    if (!invocation_params->GetString("suffix", &suffix) ||
+        !ValidateString(suffix, std::string(), kMaxSuffixLength)) {
+      ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS);
+      return;
+    }
 
-  for (const auto& value : *arguments) {
-    base::string16 argument;
-    if (!value->GetAsString(&argument)) {
+    // 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.)
+    const base::ListValue* arguments = nullptr;
+    if (!invocation_params->GetList("arguments", &arguments)) {
       ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS);
       return;
     }
-    if (!argument.empty())
-      argv.push_back(argument);
-  }
 
-  base::CommandLine command_line(argv);
+    std::vector<base::string16> argv = {exe_path.value()};
+    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);
+    }
 
-  // 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);
+    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);
+
+    // "prompt" is optional, but if present must be a boolean.
+    SwReporterInvocation::Flags flags = 0;
+    const base::Value* prompt_value = nullptr;
+    if (invocation_params->Get("prompt", &prompt_value)) {
+      bool prompt = false;
+      if (!prompt_value->GetAsBoolean(&prompt)) {
+        ReportExperimentError(SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS);
+        return;
+      }
+      if (prompt)
+        flags |= SwReporterInvocation::FLAG_TRIGGER_PROMPT;
+    }
 
-  auto invocation =
-      safe_browsing::SwReporterInvocation::FromCommandLine(command_line);
-  invocation.suffix = suffix;
-  invocation.is_experimental = true;
+    auto invocation = SwReporterInvocation::FromCommandLine(command_line);
+    invocation.suffix = suffix;
+    invocation.flags = flags;
+    invocations.push(invocation);
+  }
 
-  reporter_runner.Run(invocation, version);
+  DCHECK(!invocations.empty());
+  reporter_runner.Run(invocations, version);
 }
 
 }  // namespace
@@ -268,8 +279,15 @@ void SwReporterInstallerTraits::ComponentReady(
     RunExperimentalSwReporter(exe_path, version, std::move(manifest),
                               reporter_runner_);
   } else {
-    reporter_runner_.Run(
-        safe_browsing::SwReporterInvocation::FromFilePath(exe_path), version);
+    auto invocation = SwReporterInvocation::FromFilePath(exe_path);
+    invocation.flags = SwReporterInvocation::FLAG_LOG_TO_RAPPOR |
+                       SwReporterInvocation::FLAG_LOG_EXIT_CODE_TO_PREFS |
+                       SwReporterInvocation::FLAG_TRIGGER_PROMPT |
+                       SwReporterInvocation::FLAG_SEND_REPORTER_LOGS;
+
+    safe_browsing::SwReporterQueue invocations;
+    invocations.push(invocation);
+    reporter_runner_.Run(invocations, version);
   }
 }
 
@@ -405,7 +423,7 @@ void RegisterSwReporterComponent(ComponentUpdateService* cus) {
 
   // Install the component.
   std::unique_ptr<ComponentInstallerTraits> traits(
-      new SwReporterInstallerTraits(base::Bind(&RunSwReporterAfterStartup),
+      new SwReporterInstallerTraits(base::Bind(&RunSwReportersAfterStartup),
                                     is_experimental_engine_supported));
   // |cus| will take ownership of |installer| during installer->Register(cus).
   DefaultComponentInstaller* installer =

chrome/browser/component_updater/sw_reporter_installer_win.h

@@ -12,6 +12,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "chrome/browser/safe_browsing/srt_fetcher_win.h"
 #include "components/component_updater/default_component_installer.h"
 
 class PrefRegistrySimple;
@@ -23,10 +24,6 @@ class FilePath;
 class Version;
 }
 
-namespace safe_browsing {
-struct SwReporterInvocation;
-}
-
 namespace user_prefs {
 class PrefRegistrySyncable;
 }
@@ -45,7 +42,7 @@ enum SwReporterExperimentError {
 
 // Callback for running the software reporter after it is downloaded.
 using SwReporterRunner =
-    base::Callback<void(const safe_browsing::SwReporterInvocation& invocation,
+    base::Callback<void(const safe_browsing::SwReporterQueue& invocations,
                         const base::Version& version)>;
 
 class SwReporterInstallerTraits : public ComponentInstallerTraits {

chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc

@@ -8,7 +8,6 @@
 #include <memory>
 #include <string>
 #include <utility>
-#include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -37,6 +36,8 @@ constexpr char kErrorHistogramName[] = "SoftwareReporter.ExperimentErrors";
 constexpr char kExperimentTag[] = "experiment_tag";
 constexpr char kMissingTag[] = "missing_tag";
 
+using safe_browsing::SwReporterInvocation;
+
 }  // namespace
 
 class SwReporterInstallerTest : public ::testing::Test {
@@ -46,15 +47,16 @@ class SwReporterInstallerTest : public ::testing::Test {
             base::Bind(&SwReporterInstallerTest::SwReporterLaunched,
                        base::Unretained(this))),
         default_version_("1.2.3"),
-        default_path_(L"C:\\full\\path\\to\\download") {}
+        default_path_(L"C:\\full\\path\\to\\download"),
+        launched_version_("0.0.0") {}
 
   ~SwReporterInstallerTest() override {}
 
  protected:
-  void SwReporterLaunched(const safe_browsing::SwReporterInvocation& invocation,
+  void SwReporterLaunched(const safe_browsing::SwReporterQueue& invocations,
                           const base::Version& version) {
     ASSERT_TRUE(launched_invocations_.empty());
-    launched_invocations_.push_back(invocation);
+    launched_invocations_ = invocations;
     launched_version_ = version;
   }
 
@@ -73,14 +75,17 @@ class SwReporterInstallerTest : public ::testing::Test {
     EXPECT_EQ(default_version_, launched_version_);
     ASSERT_EQ(1U, launched_invocations_.size());
 
-    const safe_browsing::SwReporterInvocation& invocation =
-        launched_invocations_[0];
+    const SwReporterInvocation& invocation = launched_invocations_.front();
     EXPECT_EQ(MakeTestFilePath(default_path_),
               invocation.command_line.GetProgram());
     EXPECT_TRUE(invocation.command_line.GetSwitches().empty());
     EXPECT_TRUE(invocation.command_line.GetArgs().empty());
     EXPECT_TRUE(invocation.suffix.empty());
-    EXPECT_FALSE(invocation.is_experimental);
+    EXPECT_EQ(SwReporterInvocation::FLAG_LOG_TO_RAPPOR |
+                  SwReporterInvocation::FLAG_LOG_EXIT_CODE_TO_PREFS |
+                  SwReporterInvocation::FLAG_TRIGGER_PROMPT |
+                  SwReporterInvocation::FLAG_SEND_REPORTER_LOGS,
+              invocation.flags);
   }
 
   // |ComponentReady| asserts that it is run on the UI thread, so we must
@@ -95,7 +100,7 @@ class SwReporterInstallerTest : public ::testing::Test {
   base::FilePath default_path_;
 
   // Results of running |ComponentReady|.
-  std::vector<safe_browsing::SwReporterInvocation> launched_invocations_;
+  safe_browsing::SwReporterQueue launched_invocations_;
   base::Version launched_version_;
 
  private:
@@ -161,8 +166,7 @@ class ExperimentalSwReporterInstallerTest : public SwReporterInstallerTest {
     EXPECT_EQ(default_version_, launched_version_);
     ASSERT_EQ(1U, launched_invocations_.size());
 
-    const safe_browsing::SwReporterInvocation& invocation =
-        launched_invocations_[0];
+    const SwReporterInvocation& invocation = launched_invocations_.front();
     EXPECT_EQ(MakeTestFilePath(default_path_),
               invocation.command_line.GetProgram());
 
@@ -184,7 +188,7 @@ class ExperimentalSwReporterInstallerTest : public SwReporterInstallerTest {
                 invocation.command_line.GetArgs()[0]);
     }
 
-    EXPECT_TRUE(invocation.is_experimental);
+    EXPECT_EQ(0U, invocation.flags);
     histograms_.ExpectTotalCount(kErrorHistogramName, 0);
   }
 
@@ -274,7 +278,8 @@ TEST_F(ExperimentalSwReporterInstallerTest, SingleInvocation) {
       "{\"launch_params\": ["
       "  {"
       "    \"arguments\": [\"--engine=experimental\", \"random argument\"],"
-      "    \"suffix\": \"TestSuffix\""
+      "    \"suffix\": \"TestSuffix\","
+      "    \"prompt\": false"
       "  }"
       "]}";
   traits.ComponentReady(
@@ -285,8 +290,7 @@ TEST_F(ExperimentalSwReporterInstallerTest, SingleInvocation) {
   EXPECT_EQ(default_version_, launched_version_);
   ASSERT_EQ(1U, launched_invocations_.size());
 
-  const safe_browsing::SwReporterInvocation& invocation =
-      launched_invocations_[0];
+  const SwReporterInvocation& invocation = launched_invocations_.front();
   EXPECT_EQ(MakeTestFilePath(default_path_),
             invocation.command_line.GetProgram());
   EXPECT_EQ(2U, invocation.command_line.GetSwitches().size());
@@ -297,7 +301,7 @@ TEST_F(ExperimentalSwReporterInstallerTest, SingleInvocation) {
   ASSERT_EQ(1U, invocation.command_line.GetArgs().size());
   EXPECT_EQ(L"random argument", invocation.command_line.GetArgs()[0]);
   EXPECT_EQ("TestSuffix", invocation.suffix);
-  EXPECT_TRUE(invocation.is_experimental);
+  EXPECT_EQ(0U, invocation.flags);
   histograms_.ExpectTotalCount(kErrorHistogramName, 0);
 }
 
@@ -310,19 +314,73 @@ TEST_F(ExperimentalSwReporterInstallerTest, MultipleInvocations) {
       "{\"launch_params\": ["
       "  {"
       "    \"arguments\": [\"--engine=experimental\", \"random argument\"],"
-      "    \"suffix\": \"TestSuffix\""
+      "    \"suffix\": \"TestSuffix\","
+      "    \"prompt\": false"
       "  },"
       "  {"
       "    \"arguments\": [\"--engine=second\"],"
-      "    \"suffix\": \"SecondSuffix\""
+      "    \"suffix\": \"SecondSuffix\","
+      "    \"prompt\": true"
+      "  },"
+      "  {"
+      "    \"arguments\": [\"--engine=third\"],"
+      "    \"suffix\": \"ThirdSuffix\""
       "  }"
       "]}";
   traits.ComponentReady(
       default_version_, default_path_,
       base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
 
-  // Not supported yet.
-  ExpectLaunchError();
+  // The SwReporter should be launched three times with the given arguments.
+  EXPECT_EQ(default_version_, launched_version_);
+  ASSERT_EQ(3U, launched_invocations_.size());
+
+  {
+    SwReporterInvocation invocation = launched_invocations_.front();
+    launched_invocations_.pop();
+    EXPECT_EQ(MakeTestFilePath(default_path_),
+              invocation.command_line.GetProgram());
+    EXPECT_EQ(2U, invocation.command_line.GetSwitches().size());
+    EXPECT_EQ("experimental",
+              invocation.command_line.GetSwitchValueASCII("engine"));
+    EXPECT_EQ("TestSuffix", invocation.command_line.GetSwitchValueASCII(
+                                kRegistrySuffixSwitch));
+    ASSERT_EQ(1U, invocation.command_line.GetArgs().size());
+    EXPECT_EQ(L"random argument", invocation.command_line.GetArgs()[0]);
+    EXPECT_EQ("TestSuffix", invocation.suffix);
+    EXPECT_EQ(0U, invocation.flags);
+  }
+
+  {
+    SwReporterInvocation invocation = launched_invocations_.front();
+    launched_invocations_.pop();
+    EXPECT_EQ(MakeTestFilePath(default_path_),
+              invocation.command_line.GetProgram());
+    EXPECT_EQ(2U, invocation.command_line.GetSwitches().size());
+    EXPECT_EQ("second", invocation.command_line.GetSwitchValueASCII("engine"));
+    EXPECT_EQ("SecondSuffix", invocation.command_line.GetSwitchValueASCII(
+                                  kRegistrySuffixSwitch));
+    ASSERT_TRUE(invocation.command_line.GetArgs().empty());
+    EXPECT_EQ("SecondSuffix", invocation.suffix);
+    EXPECT_EQ(SwReporterInvocation::FLAG_TRIGGER_PROMPT, invocation.flags);
+  }
+
+  {
+    SwReporterInvocation invocation = launched_invocations_.front();
+    launched_invocations_.pop();
+    EXPECT_EQ(MakeTestFilePath(default_path_),
+              invocation.command_line.GetProgram());
+    EXPECT_EQ(2U, invocation.command_line.GetSwitches().size());
+    EXPECT_EQ("third", invocation.command_line.GetSwitchValueASCII("engine"));
+    EXPECT_EQ("ThirdSuffix", invocation.command_line.GetSwitchValueASCII(
+                                 kRegistrySuffixSwitch));
+    ASSERT_TRUE(invocation.command_line.GetArgs().empty());
+    EXPECT_EQ("ThirdSuffix", invocation.suffix);
+    // A missing "prompt" key means "false".
+    EXPECT_EQ(0U, invocation.flags);
+  }
+
+  histograms_.ExpectTotalCount(kErrorHistogramName, 0);
 }
 
 TEST_F(ExperimentalSwReporterInstallerTest, MissingSuffix) {
@@ -623,4 +681,26 @@ TEST_F(ExperimentalSwReporterInstallerTest, BadTypesInManifest3) {
                                  SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS, 1);
 }
 
+TEST_F(ExperimentalSwReporterInstallerTest, BadTypesInManifest4) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag(kExperimentTag);
+
+  // This has an int instead of a bool for prompt.
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"arguments\": [\"--engine=experimental\"],"
+      "    \"suffix\": \"TestSuffix\","
+      "    \"prompt\": 1"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  // The SwReporter should not be launched, and an error should be logged.
+  EXPECT_TRUE(launched_invocations_.empty());
+  histograms_.ExpectUniqueSample(kErrorHistogramName,
+                                 SW_REPORTER_EXPERIMENT_ERROR_BAD_PARAMS, 1);
+}
 }  // namespace component_updater

chrome/browser/safe_browsing/srt_fetcher_win.h

@@ -6,7 +6,9 @@
 #define CHROME_BROWSER_SAFE_BROWSING_SRT_FETCHER_WIN_H_
 
 #include <limits.h>
+#include <stdint.h>
 
+#include <queue>
 #include <string>
 
 #include "base/callback_forward.h"
@@ -57,12 +59,19 @@ struct SwReporterInvocation {
   // ending in |suffix|. For the canonical version, |suffix| will be empty.
   std::string suffix;
 
-  // The experimental sw_reporter never triggers the prompt, just reports
-  // results through UMA.
-  bool is_experimental = false;
+  // Flags to control optional behaviours. By default all are enabled;
+  // experimental versions of the reporter will turn off the behaviours that
+  // are not yet supported.
+  using Flags = uint32_t;
+  enum : Flags {
+    FLAG_LOG_TO_RAPPOR = 0x1,
+    FLAG_LOG_EXIT_CODE_TO_PREFS = 0x2,
+    FLAG_TRIGGER_PROMPT = 0x4,
+    FLAG_SEND_REPORTER_LOGS = 0x8,
+  };
+  Flags flags = 0;
 
   SwReporterInvocation();
-  SwReporterInvocation(const SwReporterInvocation& other);
 
   static SwReporterInvocation FromFilePath(const base::FilePath& exe_path);
   static SwReporterInvocation FromCommandLine(
@@ -71,18 +80,25 @@ struct SwReporterInvocation {
   bool operator==(const SwReporterInvocation& other) const;
 };
 
+using SwReporterQueue = std::queue<SwReporterInvocation>;
+
 // Tries to run the sw_reporter component, and then schedule the next try. If
 // called multiple times, then multiple sequences of trying to run will happen,
-// yet only one reporter will run per specified period (either
+// yet only one SwReporterQueue will actually run per specified period (either
 // |kDaysBetweenSuccessfulSwReporterRuns| or
-// |kDaysBetweenSwReporterRunsForPendingPrompt|) will actually happen.
-// |invocation| is the details of the SwReporter to execute, and |version| is
-// its version. The task runners are provided to allow tests to provide their
-// own.
-void RunSwReporter(const SwReporterInvocation& invocation,
-                   const base::Version& version,
-                   scoped_refptr<base::TaskRunner> main_thread_task_runner,
-                   scoped_refptr<base::TaskRunner> blocking_task_runner);
+// |kDaysBetweenSwReporterRunsForPendingPrompt|).
+//
+// Each "run" of the sw_reporter component may aggregate the results of several
+// executions of the tool with different command lines. |invocations| is the
+// queue of SwReporters to execute as a single "run". When a new try is
+// scheduled the entire queue is executed.
+//
+// |version| is the version of the tool that will run. The task runners are
+// provided to allow tests to provide their own.
+void RunSwReporters(const SwReporterQueue& invocations,
+                    const base::Version& version,
+                    scoped_refptr<base::TaskRunner> main_thread_task_runner,
+                    scoped_refptr<base::TaskRunner> blocking_task_runner);
 
 // Returns true iff Local State is successfully accessed and indicates the most
 // recent Reporter run terminated with an exit code indicating the presence of