Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc

Index: chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
diff --git a/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc b/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
index e77870481184d9455df698b491d7641195865259..dd7d2f3d09ca7f6cfedbfdea079f1cf5947eb8e6 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
@@ -38,8 +38,15 @@ class SRTFetcherTest : public InProcessBrowserTest,
   }
 
   void RunReporter(const base::FilePath& exe_path = base::FilePath()) {
-    RunSwReporter(SwReporterInvocation::FromFilePath(exe_path),
-                  base::Version("1.2.3"), task_runner_, task_runner_);
+    SwReporterQueue invocations;
+    invocations.push(SwReporterInvocation::FromFilePath(exe_path));
+    RunSwReporters(invocations, base::Version("1.2.3"), task_runner_,
+                   task_runner_);
+  }
+
+  void RunReporterQueue(const SwReporterQueue& invocations) {
+    RunSwReporters(invocations, base::Version("1.2.3"), task_runner_,
+                   task_runner_);
   }
 
   void TriggerPrompt(Browser* browser, const std::string& version) override {
@@ -71,8 +78,22 @@ class SRTFetcherTest : public InProcessBrowserTest,
               base::TimeDelta::FromDays(days) - base::TimeDelta::FromHours(1));
   }
 
-  void TestReporterLaunchCycle(int expected_launch_count,
-                               const base::FilePath& expected_launch_path) {
+  // Run through the steps needed to launch the reporter, as many times as
+  // needed to launch all the reporters given in |expected_launch_paths|. Test
+  // that each of those launches succeeded. But do not test that ONLY those
+  // launches succeeded.
+  //
+  // After this, if more launches are expected you can call
+  // |TestPartialLaunchCycle| again with another list of paths, to test that
+  // the launch cycle will continue with those paths.
+  //
+  // To test that a list of paths are launched AND NO OTHERS, use
+  // |TestReporterLaunchCycle|.
+  void TestPartialLaunchCycle(
+      const std::vector<base::FilePath>& expected_launch_paths) {
+    reporter_launch_count_ = 0;

[csharp, 2016/09/13 17:09:03]

nit: I'd move these below the comment block to keep them closer to their uses

[Joe Mason, 2016/09/13 19:24:54]

Done.

+    reporter_launch_parameters_ = SwReporterInvocation();
+
     // This test has an unfortunate amount of knowledge of the internals of
     // ReporterRunner, because it needs to pump the right message loops at the
     // right time so that all its internal messages are delivered. This
@@ -107,47 +128,97 @@ class SRTFetcherTest : public InProcessBrowserTest,
     ASSERT_TRUE(task_runner_->HasPendingTask());
     ASSERT_FALSE(reporter_done_notified_);
 
-    // Clear out any pending TryToRun calls. Use a bounded loop so that if the
-    // reporter will never be called, we'll eventually continue. (If
-    // LaunchAndWaitForExit was pending but TryToRun wasn't, this will call
-    // it.)
-    const int max_expected_launch_count = reporter_launch_count_ + 1;
-    for (int i = 0; !launch_ready_notified_ && i < 100; ++i)
+    int current_launch_count = reporter_launch_count_;
+    for (const auto& expected_launch_path : expected_launch_paths) {
+      // If RunReporter was called with no pending messages, and it was already
+      // time to launch the reporter, then |launch_ready_notified_| will
+      // already be true. Otherwise there will be a TryToRun message pending,
+      // which must be processed first.
+      if (!launch_ready_notified_) {
+        task_runner_->RunPendingTasks();
+        // Since we're expecting a launch here, we expect it to schedule
+        // LaunchAndWaitForExit. So NOW |launch_ready_notified_| should be
+        // true.
+        ASSERT_TRUE(task_runner_->HasPendingTask());
+      }
+      ASSERT_TRUE(launch_ready_notified_);
+      ASSERT_EQ(current_launch_count, reporter_launch_count_);
+
+      // Reset |launch_ready_notified_| so that we can tell if TryToRun gets
+      // called again unexpectedly.
+      launch_ready_notified_ = false;
+
+      // Call the pending LaunchAndWaitForExit.
       task_runner_->RunPendingTasks();
-    ASSERT_GE(max_expected_launch_count, reporter_launch_count_);
-
-    // Reset launch_ready_notified_ so that we can tell if TryToRun gets called
-    // again unexpectedly.
-    launch_ready_notified_ = false;
+      ASSERT_FALSE(launch_ready_notified_);
+      ASSERT_FALSE(reporter_done_notified_);
+
+      // At this point LaunchAndWaitForExit has definitely been called if
+      // it's going to be called at all. (If not, TryToRun will have been
+      // scheduled again.)
+      EXPECT_EQ(current_launch_count + 1, reporter_launch_count_);
+      EXPECT_EQ(expected_launch_path,
+                reporter_launch_parameters_.command_line.GetProgram());
+
+      // Pump the UI message loop to process the ReporterDone call (which
+      // will schedule the next TryToRun.) If LaunchAndWaitForExit wasn't
+      // called, this does nothing.
+      base::RunLoop().RunUntilIdle();
+
+      // At this point there are three things that could have happened:
+      //
+      // 1. LaunchAndWaitForExit was not called. There should be a TryToRun
+      // scheduled.
+      //
+      // 2. ReporterDone was called and there was nothing left in the queue
+      // of SwReporterInvocation's. There should be a TryToRun scheduled.
+      //
+      // 3. ReporterDone was called and there were more
+      // SwReporterInvocation's in the queue to run immediately. There should
+      // be a LaunchAndWaitForExit scheduled.
+      //
+      // So in all cases there should be a pending task, and if we are expecting
+      // more launches in this loop, |launch_ready_notified_| will already be
+      // true.
+      ASSERT_TRUE(task_runner_->HasPendingTask());
+
+      // The test task runner does not actually advance the clock. Pretend that
+      // one day has passed. (Otherwise, when we launch the last
+      // SwReporterInvocation in the queue, the next call to TryToRun will
+      // start a whole new launch cycle.)
+      SetDaysSinceLastReport(1);
+
+      reporter_done_notified_ = false;
+      current_launch_count = reporter_launch_count_;
+    }
+  }
 
-    // This will trigger LaunchAndWaitForExit if it's on the queue and hasn't
-    // been called already. If it was called already, this will do nothing.
-    // (There definitely isn't anything queued up yet after
-    // LaunchAndWaitForExit because ReporterDone wasn't called yet.)
-    task_runner_->RunPendingTasks();
-    ASSERT_GE(max_expected_launch_count, reporter_launch_count_);
-    ASSERT_FALSE(launch_ready_notified_);
+  // Run through the steps needed to launch the reporter, as many times as
+  // needed to launch all the reporters given in |expected_launch_paths|. Test
+  // that each of those launches succeeded. Then, run through the steps needed
+  // to launch the reporter again, to test that the launch cycle is complete
+  // (no more reporters will be launched).
+  void TestReporterLaunchCycle(
+      const std::vector<base::FilePath>& expected_launch_paths) {
+    TestPartialLaunchCycle(expected_launch_paths);
+
+    // Now that all expected launches have been tested, run the cycle once more

[csharp, 2016/09/13 17:09:03]

Could you replace this code with a call to TestPArtialLaunchCycle with an empty
list?

[Joe Mason, 2016/09/13 19:24:54]

No, actually. This is just enough different that it's hard to merge it with
TestPartialLaunchCycle, because it needs to call all the steps EVEN THOUGH
nothing is expected to happen. TestPartialLaunchCycle only calls all the steps
if there's something in |expected_launch_paths|. Putting multiple paths through
TestPartialLaunchCycle would make it much more complicated than this split is.

+    // to make sure no more launches happen.
+    ASSERT_TRUE(task_runner_->HasPendingTask());
     ASSERT_FALSE(reporter_done_notified_);
+    ASSERT_FALSE(launch_ready_notified_);
 
-    // At this point LaunchAndWaitForExit has definitely been called if it's
-    // going to be called at all. (If not, TryToRun will have been scheduled
-    // again.)
-    EXPECT_EQ(expected_launch_count, reporter_launch_count_);
-    EXPECT_EQ(expected_launch_path,
-              reporter_launch_parameters_.command_line.GetProgram());
+    int current_launch_count = reporter_launch_count_;
 
-    // Pump the UI message loop to process the ReporterDone call (which will
-    // schedule the next TryToRun.) If LaunchAndWaitForExit wasn't called, this
-    // does nothing.
-    base::RunLoop().RunUntilIdle();
+    // Call the pending TryToRun.
+    task_runner_->RunPendingTasks();
 
-    // At this point another call to TryToRun should be scheduled, whether or
-    // not LaunchAndWaitForExit was called.
+    // We expect that this scheduled another TryToRun. If it scheduled
+    // LaunchAndWaitForExit an unexpected launch is about to happen.
     ASSERT_TRUE(task_runner_->HasPendingTask());
-
-    // Make sure the flags are false for the next launch cycle test.
     ASSERT_FALSE(launch_ready_notified_);
-    reporter_done_notified_ = false;
+    ASSERT_FALSE(reporter_done_notified_);
+    ASSERT_EQ(current_launch_count, reporter_launch_count_);
   }
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
@@ -155,7 +226,11 @@ class SRTFetcherTest : public InProcessBrowserTest,
   int reporter_launch_count_ = 0;
   SwReporterInvocation reporter_launch_parameters_;
   int exit_code_to_report_ = kReporterFailureExitCode;
+
+  // This will be set to true when a call to |LaunchAndWaitForExit| is next in
+  // the task queue.
   bool launch_ready_notified_ = false;
+
   bool reporter_done_notified_ = false;
 };
 
@@ -264,7 +339,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
     SCOPED_TRACE("N days left until next reporter run");
     SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns - kDaysLeft);
     RunReporter(path1);
-    TestReporterLaunchCycle(0, base::FilePath());
+    TestReporterLaunchCycle({});
   }
 
   // Schedule path2 just as we enter the next reporting period.
@@ -275,15 +350,16 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
     RunReporter(path2);
     // Schedule it twice; it should only actually run once.
     RunReporter(path2);
-    TestReporterLaunchCycle(1, path2);
+    TestReporterLaunchCycle({path2});
   }
 
   // Schedule path3 before any more time has passed.
   // The reporter should not launch.
   {
     SCOPED_TRACE("No more time passed");
+    SetDaysSinceLastReport(0);
     RunReporter(path3);
-    TestReporterLaunchCycle(1, path2);
+    TestReporterLaunchCycle({});
   }
 
   // Enter the next reporting period as path3 is still scheduled.
@@ -292,7 +368,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
   {
     SCOPED_TRACE("Previous run still scheduled");
     SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
-    TestReporterLaunchCycle(2, path3);
+    TestReporterLaunchCycle({path3});
   }
 
   // Schedule path3 again in the next reporting period.
@@ -302,7 +378,75 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
     SCOPED_TRACE("Run with same parameters");
     SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
     RunReporter(path3);
-    TestReporterLaunchCycle(3, path3);
+    TestReporterLaunchCycle({path3});
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, MultipleLaunches) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+
+  const base::FilePath path1(L"path1");
+  const base::FilePath path2(L"path2");
+  const base::FilePath path3(L"path3");
+
+  SwReporterQueue invocations;
+  invocations.push(SwReporterInvocation::FromFilePath(path1));
+  invocations.push(SwReporterInvocation::FromFilePath(path2));
+
+  {
+    SCOPED_TRACE("Launch 2 times");
+    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
+    RunReporterQueue(invocations);
+    TestReporterLaunchCycle({path1, path2});
+  }
+
+  // Schedule a launch with 2 elements, then another with the same 2. It should
+  // just run 2 times, not 4.
+  {
+    SCOPED_TRACE("Launch 2 times with retry");
+    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
+    RunReporterQueue(invocations);
+    RunReporterQueue(invocations);
+    TestReporterLaunchCycle({path1, path2});
+  }
+
+  // Schedule a launch with 2 elements, then add a third while the queue is
+  // running.
+  {
+    SCOPED_TRACE("Add third launch while running");
+    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
+    RunReporterQueue(invocations);
+
+    // Only test the cycle once, to process the first element in queue.
+    TestPartialLaunchCycle({path1});
+
+    invocations.push(SwReporterInvocation::FromFilePath(path3));
+    RunReporterQueue(invocations);
+
+    // There is still a 2nd element on the queue - that should execute, and
+    // nothing more.
+    TestReporterLaunchCycle({path2});
+
+    // Time passes... Now the 3-element queue should run.
+    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
+    TestReporterLaunchCycle({path1, path2, path3});
+  }
+
+  // Second launch should not occur after a failure.
+  {
+    SCOPED_TRACE("Launch multiple times with failure");
+    exit_code_to_report_ = kReporterFailureExitCode;
+    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
+    RunReporterQueue(invocations);
+    TestReporterLaunchCycle({path1});
+
+    // If we try again before the reporting period is up, it should not do
+    // anything.
+    TestReporterLaunchCycle({});
+
+    // After enough time has passed, should try the queue again.
+    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
+    TestReporterLaunchCycle({path1});
   }
 }