Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc

 // Copyright 2015 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/safe_browsing/srt_fetcher_win.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 20 matching lines @@
     task_runner_ = new base::TestSimpleTaskRunner;
 
     SetSwReporterTestingDelegate(this);
   }
 
   void TearDownInProcessBrowserTestFixture() override {
     SetSwReporterTestingDelegate(nullptr);
   }
 
   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 {
     prompt_trigger_called_ = true;
   }
 
   int LaunchReporter(const SwReporterInvocation& invocation) override {
     ++reporter_launch_count_;
     reporter_launch_parameters_ = invocation;
     return exit_code_to_report_;
@@ skipping 11 matching lines @@
   }
 
   void ExpectToRunAgain(int days) {
     ASSERT_TRUE(task_runner_->HasPendingTask());
     EXPECT_LE(task_runner_->NextPendingTaskDelay(),
               base::TimeDelta::FromDays(days));
     EXPECT_GT(task_runner_->NextPendingTaskDelay(),
               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
     // function might need to be updated if the internals change.
     //
     // The basic sequence is:
 
     // 1. TryToRun kicks the whole thing off. If the reporter should not be
     // launched now (eg. DaysSinceLastReport is too low) it posts a call to
     // itself again. (In a regular task runner this will be scheduled with a
@@ skipping 14 matching lines @@
     // it in a loop to make sure we're past all pending TryToRun calls before
     // LaunchAndWaitForExit will be called.
     //
     // Once a call to LaunchAndWaitForExit has been posted, TryToRun won't be
     // called again until we pump the UI message loop in order to run
     // ReporterDone.
 
     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_);
+      ASSERT_FALSE(launch_ready_notified_);
+      ASSERT_FALSE(reporter_done_notified_);
 
-    // Reset launch_ready_notified_ so that we can tell if TryToRun gets called
-    // again unexpectedly.
-    launch_ready_notified_ = false;
+      // 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());
 
-    // 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.)
+      // 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_;
+    }
+  }
+
+  // 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_);
+
+    int current_launch_count = reporter_launch_count_;
+
+    // Call the pending TryToRun.
     task_runner_->RunPendingTasks();
-    ASSERT_GE(max_expected_launch_count, reporter_launch_count_);
+
+    // We expect that this scheduled another TryToRun. If it scheduled
+    // LaunchAndWaitForExit an unexpected launch is about to happen.
+    ASSERT_TRUE(task_runner_->HasPendingTask());
     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(expected_launch_count, 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 another call to TryToRun should be scheduled, whether or
-    // not LaunchAndWaitForExit was called.
-    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_EQ(current_launch_count, reporter_launch_count_);
   }
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   bool prompt_trigger_called_ = false;
   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;
 };
 
 }  // namespace
 
 IN_PROC_BROWSER_TEST_F(SRTFetcherTest, NothingFound) {
   exit_code_to_report_ = kSwReporterNothingFound;
   RunReporter();
   task_runner_->RunPendingTasks();
   EXPECT_EQ(1, reporter_launch_count_);
@@ skipping 88 matching lines @@
   const base::FilePath path2(L"path2");
   const base::FilePath path3(L"path3");
 
   // Schedule path1 with a day left in the reporting period.
   // The reporter should not launch.
   constexpr int kDaysLeft = 1;
   {
     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.
   // Now the reporter should launch, just once, using path2.
   {
     SCOPED_TRACE("Reporter runs now");
     SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
     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.
   // Now the reporter should launch again using path3. (Tests that the
   // parameters from the first launch aren't reused.)
   {
     SCOPED_TRACE("Previous run still scheduled");
     SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
-    TestReporterLaunchCycle(2, path3);
+    TestReporterLaunchCycle({path3});
   }
 
   // Schedule path3 again in the next reporting period.
   // The reporter should launch again using path3, since enough time has
   // passed, even though the parameters haven't changed.
   {
     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});
+  }
+}
+
 }  // namespace safe_browsing