Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/component_updater/sw_reporter_installer_win_unittest.cc

 // Copyright 2016 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 "chrome/browser/component_updater/sw_reporter_installer_win.h"
 
+#include <map>
 #include <memory>
+#include <string>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
+#include "base/json/json_reader.h"
 #include "base/macros.h"
+#include "base/metrics/field_trial.h"
+#include "base/strings/stringprintf.h"
+#include "base/test/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/safe_browsing/srt_fetcher_win.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace component_updater {
 
+namespace {
+
+constexpr char kExperimentGroupName[] = "ExperimentalSwReporterEngine";
+
+// These MUST match the values for SwReporterExperimentError in histograms.xml.
+enum ExperimentError {

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

What about putting this into the header (with a slight more specific name) so
you don't need to declare it in the cc and the unit test?

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

Done.

+  EXPERIMENT_ERROR_BAD_TAG = 0,
+  EXPERIMENT_ERROR_BAD_PARAMS = 1,
+  EXPERIMENT_ERROR_MAX,
+};
+
+};  // namespace
+
 class SwReporterInstallerTest : public ::testing::Test {
  public:
   SwReporterInstallerTest()
-      : traits_(base::Bind(&SwReporterInstallerTest::SwReporterLaunched,
-                           base::Unretained(this))),
+      : launched_callback_(
+            base::Bind(&SwReporterInstallerTest::SwReporterLaunched,
+                       base::Unretained(this))),
         default_version_("1.2.3"),
         default_path_(L"C:\\full\\path\\to\\download") {}
 

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

nit: Please add a virtual deconstructor

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

Done.

  protected:
-  // Each test fixture inherits from |SwReporterInstallerTest|, and friendship
-  // is not transitive so they will not have access to |ComponentReady|. Use
-  // this helper function to call it.
-  void TestComponentReady(const base::Version& version,
-                          const base::FilePath& path) {
-    traits_.ComponentReady(version, path,
-                           std::make_unique<base::DictionaryValue>());
-  }
-
   void SwReporterLaunched(const safe_browsing::SwReporterInvocation& invocation,
                           const base::Version& version) {
     ASSERT_TRUE(launched_invocations_.empty());
     launched_invocations_.push_back(invocation);
     launched_version_ = version;
   }
 
   base::FilePath MakeTestFilePath(const base::FilePath& path) const {
     return path.Append(L"software_reporter_tool.exe");
   }
 
+  // Expects that the SwReporter was launched exactly once, with no arguments.
+  void ExpectDefaultInvocation() const {
+    EXPECT_EQ(default_version_, launched_version_);
+    ASSERT_EQ(1U, launched_invocations_.size());
+
+    const safe_browsing::SwReporterInvocation& invocation =
+        launched_invocations_[0];
+    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);
+  }
+
   // |ComponentReady| asserts that it is run on the UI thread, so we must
   // create test threads before calling it.
   content::TestBrowserThreadBundle threads_;
 
-  // The traits object to test.
-  SwReporterInstallerTraits traits_;
+  // Bound callback to the |SwReporterLaunched| method.
+  SwReporterRunner launched_callback_;
 
   // Default parameters for |ComponentReady|.
   base::Version default_version_;
   base::FilePath default_path_;
 
   // Results of running |ComponentReady|.
   std::vector<safe_browsing::SwReporterInvocation> launched_invocations_;
   base::Version launched_version_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SwReporterInstallerTest);
 };
 
+// This class contains extended setup that is only used for tests of the
+// experimental reporter.
+class ExperimentalSwReporterInstallerTest : public SwReporterInstallerTest {
+ public:
+  ExperimentalSwReporterInstallerTest() {}
+

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

nit: Please add a virtual deconstructor

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

Done.

+ protected:
+  void CreateFeatureWithoutTag() {
+    std::map<std::string, std::string> params;
+    CreateFeatureWithParams(params);
+  }
+
+  void CreateFeatureWithTag(const std::string& tag) {
+    std::map<std::string, std::string> params;
+    params["tag"] = tag;
+    CreateFeatureWithParams(params);
+  }
+
+  void CreateFeatureWithParams(

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

nit: Worth making this private to ensure users call one of the above function?

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

There's no reason a unit test couldn't create a custom param list. I think it
makes sense for this to be available even if it's not used right now.

+      const std::map<std::string, std::string>& params) {
+    // Assign the given variation params to the experiment group until
+    // |variations_| goes out of scope when the test exits. This will also
+    // create a FieldTrial for this group.
+    variations_ = std::make_unique<variations::testing::VariationParamsManager>(
+        kExperimentGroupName, params);
+
+    // Create a feature list containing only the field trial for this group,
+    // and enable it for the length of the test.
+    base::FieldTrial* trial = base::FieldTrialList::Find(kExperimentGroupName);
+    ASSERT_TRUE(trial);
+    auto feature_list = std::make_unique<base::FeatureList>();
+    feature_list->RegisterFieldTrialOverride(
+        kExperimentGroupName, base::FeatureList::OVERRIDE_ENABLE_FEATURE,
+        trial);
+    scoped_feature_list_.InitWithFeatureList(std::move(feature_list));
+  }
+
+  // Expects that the SwReporter was launched exactly once, with the given
+  // |expected_suffix| and one |expected_additional_argument| on the
+  // command-line. (|expected_additional_argument| mainly exists to test that
+  // arguments are included at all, so there is no need to test for
+  // combinations of multiple arguments and switches in this function.)
+  void ExpectExperimentalInvocation(
+      const std::string& expected_suffix,
+      const base::string16& expected_additional_argument) {
+    EXPECT_EQ(default_version_, launched_version_);
+    ASSERT_EQ(1U, launched_invocations_.size());
+
+    const safe_browsing::SwReporterInvocation& invocation =
+        launched_invocations_[0];
+    EXPECT_EQ(MakeTestFilePath(default_path_),
+              invocation.command_line.GetProgram());
+    if (expected_suffix.empty()) {
+      EXPECT_TRUE(invocation.command_line.GetSwitches().empty());
+      EXPECT_TRUE(invocation.suffix.empty());
+    } else {
+      EXPECT_EQ(1U, invocation.command_line.GetSwitches().size());
+      EXPECT_EQ(expected_suffix,
+                invocation.command_line.GetSwitchValueASCII("registry-suffix"));
+      EXPECT_EQ(expected_suffix, invocation.suffix);
+    }
+    if (expected_additional_argument.empty()) {

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

nit: Please add a blank line above

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

Done.

+      EXPECT_TRUE(invocation.command_line.GetArgs().empty());
+    } else {
+      EXPECT_EQ(1U, invocation.command_line.GetArgs().size());
+      EXPECT_EQ(expected_additional_argument,
+                invocation.command_line.GetArgs()[0]);
+    }
+    EXPECT_TRUE(invocation.is_experimental);
+    histograms_.ExpectTotalCount("SoftwareReporter.ExperimentErrors", 0);
+  }
+
+  std::unique_ptr<variations::testing::VariationParamsManager> variations_;
+  base::test::ScopedFeatureList scoped_feature_list_;
+  base::HistogramTester histograms_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExperimentalSwReporterInstallerTest);
+};
+
 TEST_F(SwReporterInstallerTest, Default) {
-  TestComponentReady(default_version_, default_path_);
+  SwReporterInstallerTraits traits(launched_callback_, false);
 
-  // The SwReporter should be launched exactly once, with no arguments.
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_TRUE(attributes.empty());
+
+  traits.ComponentReady(default_version_, default_path_,
+                        std::make_unique<base::DictionaryValue>());
+  ExpectDefaultInvocation();
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, NoExperimentConfig) {
+  // Even if the experiment is supported on this hardware, the user shouldn't
+  // be enrolled unless enabled through variations.
+  SwReporterInstallerTraits traits(launched_callback_, true);
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_TRUE(attributes.empty());
+
+  traits.ComponentReady(default_version_, default_path_,
+                        std::make_unique<base::DictionaryValue>());
+  ExpectDefaultInvocation();
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, ExperimentUnsupported) {
+  // Even if the experiment config is enabled in variations, the user shouldn't
+  // be enrolled if the hardware doesn't support it.
+  SwReporterInstallerTraits traits(launched_callback_, false);
+  CreateFeatureWithTag("experiment_tag");
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_TRUE(attributes.empty());
+
+  traits.ComponentReady(default_version_, default_path_,
+                        std::make_unique<base::DictionaryValue>());
+  ExpectDefaultInvocation();
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, ExperimentMissingTag) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithoutTag();
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("missing_tag", attributes["tag"]);
+  histograms_.ExpectUniqueSample("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_TAG, 1);
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, ExperimentInvalidTag) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("tag with invalid whitespace chars");
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("missing_tag", attributes["tag"]);
+  histograms_.ExpectUniqueSample("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_TAG, 1);
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, ExperimentTagTooLong) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  std::string tag_too_long(500, 'x');
+  CreateFeatureWithTag(tag_too_long);
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("missing_tag", attributes["tag"]);
+  histograms_.ExpectUniqueSample("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_TAG, 1);
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, ExperimentEmptyTag) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("");
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("missing_tag", attributes["tag"]);

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

nit: Make a const for "missing_tag" since it is used so frequently?

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

Done.

+  histograms_.ExpectUniqueSample("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_TAG, 1);
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, SingleInvocation) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");

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

make a const for experiment_tag too?

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

Done.

+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("experiment_tag", attributes["tag"]);
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"arguments\": [\"--engine=experimental\", \"random argument\"],"
+      "    \"suffix\": \"Experimental\""

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

nit: what about making the suffix just "suffix", to make it easier to tell apart
from the engine

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

Done.

+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  // The SwReporter should be launched once with the given arguments.
   EXPECT_EQ(default_version_, launched_version_);
   ASSERT_EQ(1U, launched_invocations_.size());
 
   const safe_browsing::SwReporterInvocation& invocation =
       launched_invocations_[0];
   EXPECT_EQ(MakeTestFilePath(default_path_),
             invocation.command_line.GetProgram());
-  EXPECT_TRUE(invocation.command_line.GetSwitches().empty());
-  EXPECT_TRUE(invocation.command_line.GetArgs().empty());
+  EXPECT_EQ(2U, invocation.command_line.GetSwitches().size());
+  EXPECT_EQ("experimental",
+            invocation.command_line.GetSwitchValueASCII("engine"));
+  EXPECT_EQ("Experimental",
+            invocation.command_line.GetSwitchValueASCII("registry-suffix"));
+  ASSERT_EQ(1U, invocation.command_line.GetArgs().size());
+  EXPECT_EQ(L"random argument", invocation.command_line.GetArgs()[0]);
+  EXPECT_EQ("Experimental", invocation.suffix);
+  EXPECT_TRUE(invocation.is_experimental);
+  histograms_.ExpectTotalCount("SoftwareReporter.ExperimentErrors", 0);
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, MissingSuffix) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"arguments\": [\"random argument\"]"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  ExpectExperimentalInvocation("", L"random argument");
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, EmptySuffix) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"suffix\": \"\","
+      "    \"arguments\": [\"random argument\"]"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  ExpectExperimentalInvocation("", L"random argument");
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, MissingSuffixAndArgs) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  ExpectExperimentalInvocation("", L"");
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, EmptySuffixAndArgs) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"suffix\": \"\","
+      "    \"arguments\": []"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  ExpectExperimentalInvocation("", L"");
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, EmptySuffixAndArgs2) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"suffix\": \"\","
+      "    \"arguments\": [\"\"]"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  ExpectExperimentalInvocation("", L"");
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, MissingArguments) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"suffix\": \"Experimental\""
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  ExpectExperimentalInvocation("Experimental", L"");
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, EmptyArguments) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"suffix\": \"Experimental\","
+      "    \"arguments\": []"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  ExpectExperimentalInvocation("Experimental", L"");
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, EmptyArguments2) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"suffix\": \"Experimental\","
+      "    \"arguments\": [\"\"]"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  ExpectExperimentalInvocation("Experimental", L"");
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, EmptyManifest) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("experiment_tag", attributes["tag"]);

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

The first 5 lines of this function seem to be repeated in a lot of tests, move
to a helper function?

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

Done.

+
+  static constexpr char kTestManifest[] = "{}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest)));
+
+  // The SwReporter should not be launched, but no error should be logged.
+  // (This tests the case where a non-experimental version of the reporter,
+  // which does not have "launch_params" in its manifest, is already present.)
+  EXPECT_TRUE(launched_invocations_.empty());
+  histograms_.ExpectTotalCount("SoftwareReporter.ExperimentErrors", 0);
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, EmptyLaunchParams) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("experiment_tag", attributes["tag"]);
+
+  static constexpr char kTestManifest[] = "{\"launch_params\": []}";

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

It seems a bit weird that launch_params =[] is and error, but launch_params=[{}]
works. It feels like they should probably be the same

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

As we discussed, I'm made the "suffix" and "arguments" keys mandatory. So now:

launch_params = [] -> error
launch_params = [{}] -> error
launch_params = [{"suffix": ""}] -> error (no args)
launch_params = [{"arguments": []}] -> error (no suffix)
launch_params = [{"suffix": "", "arguments": []}] -> OK! 

And that last line is the canonical way to create the control group (no suffix
or arguments given).

+  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("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_PARAMS, 1);
+
+  static constexpr char kTestManifest2[] = "{\"launch_params\": {}}";

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

Maybe move this into it's own test (if the helper reduces the setup code to 1
call), they line 499 is just checking 1 instead of 2 (it takes a second to
realize why that is 2).

This idea also applies to the rest of the test where the counts go up

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

Actually there's no point testing the tag each time - SingleInvocation gives
enough coverage. Done.

+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest2)));
+
+  // The SwReporter should not be launched, and an error should be logged.
+  EXPECT_TRUE(launched_invocations_.empty());
+  histograms_.ExpectUniqueSample("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_PARAMS, 2);
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, BadSuffix) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("experiment_tag", attributes["tag"]);
+
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"arguments\": [\"--engine=experimental\"],"
+      "    \"suffix\": \"invalid whitespace characters\""
+      "  }"
+      "]}";
+  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("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_PARAMS, 1);
+
+  static constexpr char kTestManifest2[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"arguments\": [\"--engine=experimental\"],"
+      "    \"suffix\": \"%s\""
+      "  }"
+      "]}";
+  std::string suffix_too_long(500, 'x');
+  std::string manifest =
+      base::StringPrintf(kTestManifest2, suffix_too_long.c_str());
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(manifest)));
+
+  // The SwReporter should not be launched, and an error should be logged.
+  EXPECT_TRUE(launched_invocations_.empty());
+  histograms_.ExpectUniqueSample("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_PARAMS, 2);
+}
+
+TEST_F(ExperimentalSwReporterInstallerTest, BadTypesInManifest) {
+  SwReporterInstallerTraits traits(launched_callback_, true);
+  CreateFeatureWithTag("experiment_tag");
+
+  update_client::InstallerAttributes attributes =
+      traits.GetInstallerAttributes();
+  EXPECT_EQ(1U, attributes.size());
+  EXPECT_EQ("experiment_tag", attributes["tag"]);
+
+  // This has a string instead of a list for "arguments".
+  static constexpr char kTestManifest[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"arguments\": \"--engine=experimental\","
+      "    \"suffix\": \"Experimental\""
+      "  }"
+      "]}";
+  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("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_PARAMS, 1);
+
+  // This has the invocation parameters as direct children of "launch_params",
+  // instead of using a list.
+  static constexpr char kTestManifest2[] =
+      "{\"launch_params\": "
+      "  {"
+      "    \"arguments\": [\"--engine=experimental\"],"
+      "    \"suffix\": \"Experimental\""
+      "  }"
+      "}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest2)));
+
+  // The SwReporter should not be launched, and an error should be logged.
+  EXPECT_TRUE(launched_invocations_.empty());
+  histograms_.ExpectUniqueSample("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_PARAMS, 2);
+
+  // This has a list for suffix as well as for arguments.
+  static constexpr char kTestManifest3[] =
+      "{\"launch_params\": ["
+      "  {"
+      "    \"arguments\": [\"--engine=experimental\"],"
+      "    \"suffix\": [\"Experimental\"]"
+      "  }"
+      "]}";
+  traits.ComponentReady(
+      default_version_, default_path_,
+      base::DictionaryValue::From(base::JSONReader::Read(kTestManifest3)));
+
+  // The SwReporter should not be launched, and an error should be logged.
+  EXPECT_TRUE(launched_invocations_.empty());
+  histograms_.ExpectUniqueSample("SoftwareReporter.ExperimentErrors",
+                                 EXPERIMENT_ERROR_BAD_PARAMS, 3);
 }
 
 }  // namespace component_updater