Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/safe_browsing/srt_fetcher_win.cc

Index: chrome/browser/safe_browsing/srt_fetcher_win.cc
diff --git a/chrome/browser/safe_browsing/srt_fetcher_win.cc b/chrome/browser/safe_browsing/srt_fetcher_win.cc
index d46ed04482d957058e2e2d8c14b6c25a87122959..8a99316285f85b66c68e8a4ba01d4496fae1c962 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_win.cc
@@ -119,7 +119,9 @@ const char kMemoryUsedMetricName[] = "SoftwareReporter.MemoryUsed";
 // Reports metrics about the software reporter via UMA (and sometimes Rappor).
 class UMAHistogramReporter {
  public:
-  explicit UMAHistogramReporter(const std::string& suffix = std::string())
+  UMAHistogramReporter() : UMAHistogramReporter(std::string()) {}
+
+  explicit UMAHistogramReporter(const std::string& suffix)
       : suffix_(suffix),
         registry_key_(suffix.empty() ? kSoftwareRemovalToolRegistryKey
                                      : base::StringPrintf(
@@ -588,20 +590,21 @@ void MaybeFetchSRT(Browser* browser, const base::Version& reporter_version) {
 class ReporterRunner : public chrome::BrowserListObserver {
  public:
   // Starts the sequence of attempts to run the reporter.

[grt (UTC plus 2), 2016/09/02 10:19:57]

"reporters" (please update all comments accordingly

[Joe Mason, 2016/09/02 21:14:34]

Done.

-  static void Run(const SwReporterInvocation& invocation,
+  static void Run(std::unique_ptr<SwReporterQueue> invocations,
                   const base::Version& version,
                   scoped_refptr<base::TaskRunner> main_thread_task_runner,
                   scoped_refptr<base::TaskRunner> blocking_task_runner) {
     if (!instance_)
       instance_ = new ReporterRunner;
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    DCHECK(invocations);
     // There's nothing to do if the path and version of the reporter has not
     // changed, we just keep running the tasks that are running now.
-    if (instance_->invocation_ == invocation && instance_->version_.IsValid() &&
-        instance_->version_ == version)
+    if (instance_->invocations_ && *instance_->invocations_ == *invocations &&

[grt (UTC plus 2), 2016/09/02 10:19:57]

now that i've taken more time to grok ReporterRunner, i think things could be
simplified by having successive invocations walk an iterator through the
invocations_ collection (a vector, in this case). this avoids the whole "make a
copy and pass it along" issue that bugs me, and i think is a bit cleaner. Run()
could reset the iterator back to the front of the collection when it moves the
new collection into place:

    instance_->invocations_ = std::move(invocations);
    instance_->next_invocation_ = instance_->invocations_->begin();

and then ReporterDone() would schedule the next invocation if the iterator isn't
at the end:

    if (next_invocation_ != invocations_->end() &&
        exit_code != kReporterFailureExitCode) {
      ScheduleNextInvocation(*next_invocation_);
      ++next_invocation_;

and reset the iterator to the front when going back to TryToRun:

    } else {
      next_invocation_ = invocations_.begin();
      schedule the next TryToRun

[Joe Mason, 2016/09/02 21:14:35]

I don't think this is any clearer than using a queue. It requires holding state
in the ReporterRunner object, which opens a possibility of the state in the
ReporterRunner to get out of sync with the state captured by tasks that were
posted.

For example, your suggestion for Run() contains a bug - when it moves a new
collection into place, the new collection should be scheduled to execute on the
NEXT TryToRun call. In the meantime the current collection should finish, if
it's already started. (There's a browser test for that.)

So we would need to save a "pending collection" in Run, and then in TryToRun,
swap it with the current collection. Which is basically what happens now, if we
rename invocations_ to pending_invocations_.

So, we could have ReporterRunner save the state more explicitly by renaming
invocations_ to pending_invocations_, and adding a separate running_invocations_
member and next_invocation_ iterator. Run() overwrites pending_invocations_ but
doesn't touch running_invocations_. TryToRun copies pending_invocations_ to
running_invocations_ and resets next_invocation_. ReporterDone increments
next_invocation_.

But that still leaves the possibility that any function could alter
next_invocation_ at the wrong time, or that something could overwrite
running_invocations_ at the wrong time. Using a queue that's modified keeps all
the state in one place, although that "one place" is a queue object that's
passed between several functions so the ownership can seem a bit abstract. The
only real way to mess up the state of the queue is to forget to call pop() after
front().

A middle ground is to get rid of the queue parameters, and store two queues
(pending_invocations_ and running_invocations_) in ReporterRunner.
LaunchNextInvocation and ReporterDone would reference the running_invocations_
queue instead of it being a parameter. What do you think of that approach?

[grt (UTC plus 2), 2016/09/04 10:35:44]

yes, duplicate state is bad. could all state be in the Runner and none bound up
in the callbacks?
ah, yes. is this a requirement, or just the way it happens to be implemented?
would it simplify things to nix that?
i think i like what you have now better if a current set of invocations must
complete before a new set is run.

[macourteau, 2016/09/12 19:53:15]

Done.
I don't think this is a requirement, but I believe the changes I made (which
keep the state in the Runner) keep the same behaviour while making things easier
to understand.

+        instance_->version_.IsValid() && instance_->version_ == version)
       return;
 
-    instance_->invocation_ = invocation;
+    instance_->invocations_ = std::move(invocations);
     instance_->version_ = version;
     instance_->main_thread_task_runner_ = std::move(main_thread_task_runner);
     instance_->blocking_task_runner_ = std::move(blocking_task_runner);
@@ -626,12 +629,41 @@ class ReporterRunner : public chrome::BrowserListObserver {
     BrowserList::RemoveObserver(this);
   }
 
+  // Launches the command line at the head of the queue.
+  void LaunchNextInvocation(std::unique_ptr<SwReporterQueue> invocations) {

[grt (UTC plus 2), 2016/09/02 10:19:57]

nit: "ScheduleNextInvocation" is more fitting

[Joe Mason, 2016/09/02 21:14:35]

I disagree. It posts a task to do the (synchronous) launch on another thread,
but it doesn't add any extra delay, which Schedule implies to me. I'd find it
confusing to use "Schedule" here when there's code in TryToRun to wait for
several days before the next try, which sounds more like a "Schedule" action if
anything does.

Instead I left this as LaunchNextInvocation and renamed Run to
ScheduleInvocations, because Run may not actually cause anything to be executed
immediately.

[grt (UTC plus 2), 2016/09/04 10:35:44]

...which is another way of saying it schedules it to run as soon as possible on
some other thread. :-)  in the context of base/process/launch.h, launching is a
synchronous operation. since that's where my head is, i think "synchronous
launch" when i see "Launch" in a function name.
that's a good improvement. thanks.

[macourteau, 2016/09/12 19:53:15]

Done.

+    DCHECK(invocations);
+    DCHECK(!invocations->empty());
+    auto next_invocation = invocations->front();
+    invocations->pop();
+
+    if (g_testing_delegate_)
+      g_testing_delegate_->NotifyLaunchReady();
+
+    // It's OK to simply |PostTaskAndReplyWithResult| so that
+    // |LaunchAndWaitForExit| doesn't need to access |main_thread_task_runner_|

[grt (UTC plus 2), 2016/09/02 10:19:58]

i find the use of task runners for the main/UI thread work a bit confusing. if i
understand correctly, main_thread_task_runner_ is used for TryToRun, which
DCHECKs that it's on the UI thread. is it possible for the test to let the UI
thread run rather than needing a second task runner on the same thread? could
the testing delegate's methods pop out of the UI message loop when needed? it's
a bit more confusing that this function will end up posting ReporterDone to the
UI thread's task runner rather than main_thread_task_runner_.

[Joe Mason, 2016/09/02 21:14:35]

Agreed. But I don't think I should be changing the working task runners in the
same patch as adding the queue, because it makes the change itself more
confusing.

I'm pretty sure the only things that need to run on the UI thread are
ReporterRunner::Run and MaybeFetchSRT. So I would use a single task_runner for
the blocking LaunchAndWaitForExit method, and have Run dispatch the first
TryToRun call to that task_runner. Then ReporterDone would explicitly dispatch
MaybeFetchSRT to the UI thread and call TryToRun again on the same task_runner.

But that refactor is orthogonal to this feature so it needs its own patch.

[grt (UTC plus 2), 2016/09/04 10:35:44]

Ack.
Agreed.

+    // since the callback is not delayed and the test task runner won't need to
+    // force it.
+    base::PostTaskAndReplyWithResult(
+        blocking_task_runner_.get(), FROM_HERE,
+
+        // Send the head of the queue to the launch callback.
+        base::Bind(&LaunchAndWaitForExit, next_invocation),
+
+        // Send the tail of the queue to the next callback, which will handle
+        // the results of the first launch and then recursively launch
+        // everything in the tail.
+        base::Bind(&ReporterRunner::ReporterDone, base::Unretained(this),
+                   base::Time::Now(), version_, next_invocation,
+                   base::Passed(std::move(invocations))));
+  }
+
   // This method is called on the UI thread when the reporter run has completed.

[grt (UTC plus 2), 2016/09/02 10:19:57]

"the reporter" -> "a reporter"

[Joe Mason, 2016/09/02 21:14:34]

Done.

   // This is run as a task posted from an interruptible worker thread so should
   // be resilient to unexpected shutdown.
   void ReporterDone(const base::Time& reporter_start_time,
                     const base::Version& version,
                     const SwReporterInvocation& finished_invocation,
+                    std::unique_ptr<SwReporterQueue> remaining_invocations,
                     int exit_code) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -640,24 +672,38 @@ class ReporterRunner : public chrome::BrowserListObserver {
 
     base::TimeDelta reporter_running_time =
         base::Time::Now() - reporter_start_time;
-    // Don't continue when the reporter process failed to launch, but still try
-    // again after the regular delay. It's not worth retrying earlier, risking
-    // running too often if it always fails, since not many users fail here.
-    main_thread_task_runner_->PostDelayedTask(
-        FROM_HERE,
-        base::Bind(&ReporterRunner::TryToRun, base::Unretained(this)),
-        base::TimeDelta::FromDays(days_between_reporter_runs_));
+
+    // Don't continue the current queue of reporters if one failed to launch.
+    // As soon as we're not running this queue, schedule the next overall queue
+    // run after the regular delay. (If there was a failure it's not worth
+    // retrying earlier, risking running too often if it always fails, since
+    // not many users fail here.)
+    if (!remaining_invocations->empty() &&
+        exit_code != kReporterFailureExitCode) {
+      LaunchNextInvocation(std::move(remaining_invocations));
+    } else {
+      main_thread_task_runner_->PostDelayedTask(
+          FROM_HERE,
+          base::Bind(&ReporterRunner::TryToRun, base::Unretained(this)),
+          base::TimeDelta::FromDays(days_between_reporter_runs_));
+    }
+
+    // If the reporter failed to launch, do not process the results. (The exit
+    // code itself doesn't need to be logged in this case because
+    // SW_REPORTER_FAILED_TO_START is logged in |LaunchAndWaitForExit|.)
     if (exit_code == kReporterFailureExitCode)
       return;
 
     UMAHistogramReporter uma(finished_invocation.suffix);
     uma.ReportVersion(version);
     uma.ReportExitCode(exit_code);
-    uma.ReportFoundUwS(!finished_invocation.is_experimental /*use_rappor*/);
+    uma.ReportFoundUwS(finished_invocation.flags &
+                       SwReporterInvocation::FLAG_LOG_TO_RAPPOR);
 
     //  Only save results from the canonical version of the software.
     PrefService* local_state = g_browser_process->local_state();
-    if (local_state && !finished_invocation.is_experimental) {
+    if (local_state &&
+        (finished_invocation.flags & SwReporterInvocation::FLAG_LOG_TO_PREFS)) {
       local_state->SetInteger(prefs::kSwReporterLastExitCode, exit_code);
       local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
                             base::Time::Now().ToInternalValue());
@@ -666,8 +712,8 @@ class ReporterRunner : public chrome::BrowserListObserver {
     uma.ReportScanTimes();
     uma.ReportMemoryUsage();
 
-    // Only continue to launch the prompt for the canonical version.
-    if (finished_invocation.is_experimental)
+    if (!(finished_invocation.flags &
+          SwReporterInvocation::FLAG_TRIGGER_PROMPT))
       return;
 
     if (!IsInSRTPromptFieldTrialGroups()) {
@@ -721,22 +767,16 @@ class ReporterRunner : public chrome::BrowserListObserver {
         last_time_triggered +
         base::TimeDelta::FromDays(days_between_reporter_runs_) -
         base::Time::Now());
-    if (next_trigger_delay.ToInternalValue() <= 0 ||
-        // Also make sure the kSwReporterLastTimeTriggered value is not set in
-        // the future.
-        last_time_triggered > base::Time::Now()) {
-      if (g_testing_delegate_)
-        g_testing_delegate_->NotifyLaunchReady();
-
-      // It's OK to simply |PostTaskAndReplyWithResult| so that
-      // |LaunchAndWaitForExit| doesn't need to access
-      // |main_thread_task_runner_| since the callback is not delayed and the
-      // test task runner won't need to force it.
-      base::PostTaskAndReplyWithResult(
-          blocking_task_runner_.get(), FROM_HERE,
-          base::Bind(&LaunchAndWaitForExit, invocation_),
-          base::Bind(&ReporterRunner::ReporterDone, base::Unretained(this),
-                     base::Time::Now(), version_, invocation_));
+    DCHECK(invocations_);
+    if (!invocations_->empty() &&
+        (next_trigger_delay.ToInternalValue() <= 0 ||

[grt (UTC plus 2), 2016/09/02 10:19:57]

don't convert a time or timedelta to its internal value for the sake of math.
the following is more correct and clear:
    const base::Time now = base::Time::now();
    const base::Time next_trigger = last_time_triggered +
        base::TimeDelta::FromDays(days_between_reporter_runs_);
    if (... last_time_triggered > now || next_trigger <= now)

[Joe Mason, 2016/09/02 21:14:35]

Done.

+         // Also make sure the kSwReporterLastTimeTriggered value is not set in
+         // the future.
+         last_time_triggered > base::Time::Now())) {
+      // Make a copy of the queue, since LaunchNextInvocation will pop items
+      // from it. We need to save the full original for the comparison in
+      // Run.
+      LaunchNextInvocation(std::make_unique<SwReporterQueue>(*invocations_));
     } else {
       main_thread_task_runner_->PostDelayedTask(
           FROM_HERE,
@@ -746,7 +786,7 @@ class ReporterRunner : public chrome::BrowserListObserver {
   }
 
   bool first_run_ = true;
-  SwReporterInvocation invocation_;
+  std::unique_ptr<SwReporterQueue> invocations_;
   base::Version version_;
   scoped_refptr<base::TaskRunner> main_thread_task_runner_;
   scoped_refptr<base::TaskRunner> blocking_task_runner_;
@@ -786,14 +826,15 @@ SwReporterInvocation SwReporterInvocation::FromCommandLine(
 
 bool SwReporterInvocation::operator==(const SwReporterInvocation& other) const {
   return command_line.argv() == other.command_line.argv() &&
-         suffix == other.suffix && is_experimental == other.is_experimental;
+         suffix == other.suffix && flags == other.flags;
 }
 
-void RunSwReporter(const SwReporterInvocation& invocation,
-                   const base::Version& version,
-                   scoped_refptr<base::TaskRunner> main_thread_task_runner,
-                   scoped_refptr<base::TaskRunner> blocking_task_runner) {
-  ReporterRunner::Run(invocation, version, std::move(main_thread_task_runner),
+void RunSwReporters(std::unique_ptr<SwReporterQueue> invocations,
+                    const base::Version& version,
+                    scoped_refptr<base::TaskRunner> main_thread_task_runner,
+                    scoped_refptr<base::TaskRunner> blocking_task_runner) {
+  ReporterRunner::Run(std::move(invocations), version,

[grt (UTC plus 2), 2016/09/02 10:19:58]

?  DCHECK(!invocations->empty());
why would it be permissible to start this whole thing if there's nothing to do?
if this does need to be supported, then how about resetting the Runner back to
its first_run_ state and stop scheduling tries when it discovers that it has an
empty collection of invocations?

[Joe Mason, 2016/09/02 21:14:34]

There's a DCHECK for that at the top of ReporterRunner::Run, didn't seem worth
it to check it twice when this is just a thin wrapper.

[grt (UTC plus 2), 2016/09/04 10:35:44]

is there? i see one that DCHECKs that the pointer isn't null, but it looks like
there's production code to handle the case where the container is empty. if an
empty container should be forbidden, then it's good practice to have a DCHECK
for it as early as possible so that a developer who accidentally tries to call
this with an empty container sees the error at the point of the call rather than
somewhere in the depths of the impl where it'll be harder to figure out what
happened.

[macourteau, 2016/09/12 19:53:15]

Done.

+                      std::move(main_thread_task_runner),
                       std::move(blocking_task_runner));
 }