Basic IPC communication between Chrome and the SW Reporter.

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 f2f98355f54f0865b9782a261e22085495606bc9..7fb940d921b88048ed8bfdd463f366921ed2a5bd 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_win.cc
@@ -36,6 +36,7 @@
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
+#include "chrome/browser/safe_browsing/srt_chrome_prompt_impl.h"
 #include "chrome/browser/safe_browsing/srt_client_info_win.h"
 #include "chrome/browser/safe_browsing/srt_global_error_win.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -50,11 +51,16 @@
 #include "components/variations/net/variations_http_headers.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
+#include "mojo/edk/embedder/connection_params.h"
+#include "mojo/edk/embedder/pending_process_connection.h"
+#include "mojo/edk/embedder/platform_channel_pair.h"
+#include "mojo/public/cpp/system/message_pipe.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
+#include "third_party/pipa/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
 
 using content::BrowserThread;
 
@@ -165,6 +171,15 @@ const char kEngineErrorCodeMetricName[] = "SoftwareReporter.EngineErrorCode";
 // SoftwareReporterLogsUploadResult enum defined in the histograms.xml file.
 const int kSwReporterLogsUploadResultMax = 30;
 
+const base::Feature kInBrowserCleanerUIFeature{

[grt (UTC plus 2), 2017/03/29 13:01:42]

constexpr

[ftirelo, 2017/03/30 22:13:51]

This is also defined in the header file (I'll need in tests), so kept as const.

[grt (UTC plus 2), 2017/03/31 11:32:33]

can you not leave the declaration in the .h as-is and make the definition on
line 79 constexpr?

[ftirelo, 2017/04/06 15:29:25]

If I use constexpr here and not in the header file, I get a compilation error. I
didn't understand the second half of the comment, since line 79 doesn't seem
related to this CL.

+    "InBrowserCleanerUI", base::FEATURE_DISABLED_BY_DEFAULT};
+
+constexpr char kChromeMojoPipeTokenSwitch[] = "chrome-mojo-pipe-token";

[grt (UTC plus 2), 2017/03/29 13:01:43]

what other component uses this switch? do the two sides share a place where the
constant can be defined only once?

[Joe Mason, 2017/03/29 13:31:01]

The SRT, which is in a separate repo.

The constant could be defined in the pipa repo alongside chrome_prompt.mojom,
but I'm not sure it's worth adding another header file to that repo just for
this.

(Although we could move all the other SRT command-line flags to that repo too if
we do that.)

[ftirelo, 2017/03/30 22:13:51]

As soon as I land crrev/2782243002, I will move shared constants to that subdir,
so they can be shared with the SRT.

[ftirelo, 2017/03/30 22:13:52]

This will be landed in //components/chrome_cleaner as soon as I land
crrev/2782243002.

+
+// DO NOT SUBMIT. This is for debugging only and will be removed before landing
+// this CL.
+constexpr bool kDebugOnlyForceReporterRun = true;
+
 // Reports metrics about the software reporter via UMA (and sometimes Rappor).
 class UMAHistogramReporter {
  public:
@@ -553,19 +568,64 @@ void DisplaySRTPrompt(const base::FilePath& download_path) {
     global_error->ShowBubbleView(browser);
 }
 
-// This function is called from a worker thread to launch the SwReporter and
-// wait for termination to collect its exit code. This task could be
-// interrupted by a shutdown at any time, so it shouldn't depend on anything
-// external that could be shut down beforehand.
-int LaunchAndWaitForExit(const SwReporterInvocation& invocation) {
+// Class responsible for launching the reporter process and waiting for its

[grt (UTC plus 2), 2017/03/29 13:01:43]

maybe here is a good place to document this class's relation to threads --
created and destroyed on UI thread, launching on BG thread, IPC hookup/teardown
on IO thread, etc.

[ftirelo, 2017/03/30 22:13:51]

Done.

+// completion. If feature InBrowserCleanerUI is enabled, this object will also
+// be responsible for starting the ChromePromptImpl object in the IO thread and
+// controlling its lifetime.
+class SwReporterProcess : public base::RefCounted<SwReporterProcess> {
+ public:
+  explicit SwReporterProcess(const SwReporterInvocation& invocation)
+      : invocation_(invocation) {}
+  virtual ~SwReporterProcess();

[grt (UTC plus 2), 2017/03/29 13:01:42]

why are these methods virutal?

[ftirelo, 2017/03/30 22:13:51]

No need to, since they are in the anonymous namespace. I may move them out for
testing and want to mock the class, but if needed I'll bring the virtuals back.

+
+  // This function is called from a worker thread to launch the SwReporter and
+  // wait for termination to collect its exit code. This task could be
+  // interrupted by a shutdown at any time, so it shouldn't depend on anything
+  // external that could be shut down beforehand.
+  virtual int LaunchAndWaitForExit();
+
+  virtual const SwReporterInvocation& invocation() const { return invocation_; }
+
+ private:
+  // Starts a new IPC service implementing the ChromePrompt interface and
+  // launches a new reporter process that can connect to the IPC.
+  base::Process LaunchConnectedReporterProcess();
+
+  // Starts a new instance of ChromePromptImpl to receive requests from the
+  // reporter. Must be run in the IO thread.
+  void CreateChromePromptImpl(mojo::ScopedMessagePipeHandle my_pipe);
+
+  // The invocation for the current reporter process.
+  SwReporterInvocation invocation_;
+
+  // Implementation of the ChromePrompt service to be used by the current
+  // reporter process.
+  std::unique_ptr<ChromePromptImpl> chrome_prompt_impl_;

[Joe Mason, 2017/03/29 13:04:31]

This file could use a comment ... somewhere ... explaining the lifetime of
chrome_prompt_impl_. It's not very obvious from reading the code that the pipe
is only used from within LaunchAndWaitForExit so it can be destroyed as soon as
that function finishes.

[ftirelo, 2017/03/30 22:13:51]

Done.

+};
+
+SwReporterProcess::~SwReporterProcess() {
+  if (!chrome_prompt_impl_)
+    return;
+
+  BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
+      ->PostTask(FROM_HERE, base::Bind(&base::DeletePointer<ChromePromptImpl>,

[grt (UTC plus 2), 2017/03/29 13:01:42]

nit:
      ->DeleteSoon(FROM_HERE, chrome_prompt_impl_.release());

[ftirelo, 2017/03/30 22:13:51]

Done.

+                                       chrome_prompt_impl_.release()));
+}
+
+int SwReporterProcess::LaunchAndWaitForExit() {

[grt (UTC plus 2), 2017/03/29 13:01:42]

nit: consider renaming this LaunchAndWaitForExitOnBackgroundThread

[ftirelo, 2017/03/30 22:13:52]

Done.

   if (g_testing_delegate_)
-    return g_testing_delegate_->LaunchReporter(invocation);
+    return g_testing_delegate_->LaunchReporter(invocation_);
+
   base::Process reporter_process =
-      base::LaunchProcess(invocation.command_line, base::LaunchOptions());
+      base::FeatureList::IsEnabled(kInBrowserCleanerUIFeature)
+          ? LaunchConnectedReporterProcess()
+          : base::LaunchProcess(invocation_.command_line,
+                                base::LaunchOptions());
+
   // This exit code is used to identify that a reporter run didn't happen, so
   // the result should be ignored and a rerun scheduled for the usual delay.
   int exit_code = kReporterFailureExitCode;
-  UMAHistogramReporter uma(invocation.suffix);
+  UMAHistogramReporter uma(invocation_.suffix);
   if (reporter_process.IsValid()) {
     uma.RecordReporterStep(SW_REPORTER_START_EXECUTION);
     bool success = reporter_process.WaitForExit(&exit_code);
@@ -576,6 +636,51 @@ int LaunchAndWaitForExit(const SwReporterInvocation& invocation) {
   return exit_code;
 }
 
+base::Process SwReporterProcess::LaunchConnectedReporterProcess() {
+  DCHECK(base::FeatureList::IsEnabled(kInBrowserCleanerUIFeature));
+
+  mojo::edk::PendingProcessConnection pending_process_connection;
+  std::string mojo_pipe_token;
+  mojo::ScopedMessagePipeHandle mojo_pipe =
+      pending_process_connection.CreateMessagePipe(&mojo_pipe_token);
+  invocation_.command_line.AppendSwitchASCII(kChromeMojoPipeTokenSwitch,
+                                             mojo_pipe_token);
+
+  mojo::edk::PlatformChannelPair channel;
+  base::HandlesToInheritVector handles_to_inherit;
+  channel.PrepareToPassClientHandleToChildProcess(&invocation_.command_line,
+                                                  &handles_to_inherit);
+
+  base::LaunchOptions launch_options;
+  launch_options.handles_to_inherit = &handles_to_inherit;
+  base::Process reporter_process =
+      base::LaunchProcess(invocation_.command_line, launch_options);

[Joe Mason, 2017/03/29 13:04:31]

Should return early here if !reporter_process.IsValid. Otherwise
LaunchAndWaitForExit will return immediately, but CreateChromePromptImpl will
still be scheduled.

[ftirelo, 2017/03/30 22:13:51]

Done.

+

[grt (UTC plus 2), 2017/03/29 13:01:42]

?
  if (!reporter_process.IsValid())
    return reporter_process;
?

[ftirelo, 2017/03/30 22:13:51]

Done.

+  pending_process_connection.Connect(
+      reporter_process.Handle(),
+      mojo::edk::ConnectionParams(channel.PassServerHandle()));
+
+  // ChromePromptImpl tasks will need to run in the IO thread. There is no
+  // need to synchronize its creation, since the client end will wait for this
+  // initialization to be done before sending requests.
+  BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
+      ->PostTask(FROM_HERE,
+                 base::Bind(&SwReporterProcess::CreateChromePromptImpl,
+                            base::Unretained(this), base::Passed(&mojo_pipe)));

[grt (UTC plus 2), 2017/03/29 13:01:42]

i think this could result in walking off the end of the earth under the
following contrived scenario:
- process creation on line 656 appears to succeed
- this task is posted to the IO thread
- remote process dies a horrible death, causing line 631 to return immediately
- ReporterDone is posted to and run on the UI thread
- SwReporterProcess instance is destroyed on UI thread
- IO thread wakes up and tries to run the posted task
- UAF

[Joe Mason, 2017/03/29 13:04:31]

And there's another race condition here - what happens if the reporter
subprocess exits before it connects to the mojo pipe? (A crash on start, for
instance.) Then we have:

UI thread:
launch subprocess
post to IO thread -> CreateChromePromptImpl
reporter_process.WaitForExit (returns quickly)
PostTaskAndReplyWithResult calls ReporterDone
on exit from ReporterDone, SwReporterProcess is deleted

IO thread:
CreateChromePromptImpl called with invalid this pointer

I think this can be fixed by removing "Unretained" so the CreateChromePromptImpl
task retains a reference, as well as ReporterDone. So if the child process exits
immediately, we create a useless chrome_prompt_impl_ and then immediately free
it, but that's ok in this corner case.

[ftirelo, 2017/03/30 22:13:52]

Please take a look at the new implementation to see if all race conditions have
been fixed.

[ftirelo, 2017/03/30 22:13:52]

Please take a look at the new implementation to see if all race conditions have
been fixed.

+
+  return reporter_process;
+}
+
+void SwReporterProcess::CreateChromePromptImpl(
+    mojo::ScopedMessagePipeHandle mojo_pipe) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  chrome_cleaner::mojom::ChromePromptRequest chrome_prompt_request;
+  chrome_prompt_request.Bind(std::move(mojo_pipe));
+  chrome_prompt_impl_ =

[grt (UTC plus 2), 2017/03/29 13:01:43]

you have a data race here between this store (on the IO thread) and the load on
line 607 (on the UI thread). you could possibly fix it by shunting destruction
of the SwReporterProcess instance itself to the IO thread whenever
kInBrowserCleanerUIFeature is enabled.

[ftirelo, 2017/03/30 22:13:51]

Please take a look at the new implementation to see if all race conditions have
been fixed.

+      base::MakeUnique<ChromePromptImpl>(std::move(chrome_prompt_request));
+}
+
 }  // namespace
 
 void DisplaySRTPromptForTesting(const base::FilePath& download_path) {
@@ -771,11 +876,14 @@ class ReporterRunner : public chrome::BrowserListObserver {
     base::TaskRunner* task_runner =
         g_testing_delegate_ ? g_testing_delegate_->BlockingTaskRunner()
                             : blocking_task_runner_.get();
+    scoped_refptr<SwReporterProcess> sw_reporter_process(

[grt (UTC plus 2), 2017/03/29 13:01:42]

please document the expected lifetime of SwReporterProcess. it appears that it's
meant to be destroyed on the UI thread when ReporterDone exits. what, other than
the callback below, holds a ref to it? must it be refcounted, or can you use a
more clear/obvious ownership model?

[Joe Mason, 2017/03/29 13:31:01]

With scoped_refptr we can have references to it in ReporterDone (which always
runs after LaunchAndWaitForExit due to PostTaskAndReply) and
CreateChromePromptImpl, which ensures SwReporterProcess isn't deleted too early
even on launch failures.

A unique_ptr passed to ReporterDone, which then deletes the SwReporterProcess on
the IO thread, would also work because it would ensure that the
SwReporterProcess delete is posted after any CreateChromePromptImpl.

[ftirelo, 2017/03/30 22:13:51]

Done.

[ftirelo, 2017/03/30 22:13:52]

Done.

+        new SwReporterProcess(next_invocation));

[Joe Mason, 2017/03/29 13:04:31]

I think make_scoped_refptr is preferred here.

[ftirelo, 2017/03/30 22:13:52]

Done.

     base::PostTaskAndReplyWithResult(
         task_runner, FROM_HERE,
-        base::Bind(&LaunchAndWaitForExit, next_invocation),
+        base::Bind(&SwReporterProcess::LaunchAndWaitForExit,
+                   base::Unretained(sw_reporter_process.get())),

[Joe Mason, 2017/03/29 13:31:00]

Nit: if you do keep scoped_refptr, I think it makes sense to get rid of
Unretained here. It's not totally necessary since we know ReporterDone runs
after LaunchAndWait, but it's a bit safer in case of refactoring.

[ftirelo, 2017/03/30 22:13:51]

Done.

         base::Bind(&ReporterRunner::ReporterDone, base::Unretained(this), Now(),
-                   version_, next_invocation));
+                   version_, sw_reporter_process));

[grt (UTC plus 2), 2017/03/29 13:01:42]

nit: std::move(sw_reporter_process)

[ftirelo, 2017/03/30 22:13:51]

Saved both callbacks in auxiliary variables to enforce evaluation order
(sw_reporter_process) was used by both methods posted by
PostTaskAndReplyWithResult().

   }
 
   // This method is called on the UI thread when an invocation of the reporter
@@ -783,7 +891,7 @@ class ReporterRunner : public chrome::BrowserListObserver {
   // thread so should be resilient to unexpected shutdown.
   void ReporterDone(const base::Time& reporter_start_time,
                     const base::Version& version,
-                    const SwReporterInvocation& finished_invocation,
+                    scoped_refptr<SwReporterProcess> sw_reporter_process,
                     int exit_code) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -813,6 +921,7 @@ class ReporterRunner : public chrome::BrowserListObserver {
     if (exit_code == kReporterFailureExitCode)
       return;
 
+    auto finished_invocation = sw_reporter_process->invocation();
     UMAHistogramReporter uma(finished_invocation.suffix);
     uma.ReportVersion(version);
     uma.ReportExitCode(exit_code);
@@ -895,11 +1004,12 @@ class ReporterRunner : public chrome::BrowserListObserver {
     const base::Time next_trigger(
         last_time_triggered +
         base::TimeDelta::FromDays(days_between_reporter_runs_));
-    if (!pending_invocations_.empty() &&
-        (next_trigger <= now ||
-         // Also make sure the kSwReporterLastTimeTriggered value is not set in
-         // the future.
-         last_time_triggered > now)) {
+    if (kDebugOnlyForceReporterRun ||
+        (!pending_invocations_.empty() &&
+         (next_trigger <= now ||
+          // Also make sure the kSwReporterLastTimeTriggered value is not set in
+          // the future.
+          last_time_triggered > now))) {
       const base::Time last_time_sent_logs = base::Time::FromInternalValue(
           local_state->GetInt64(prefs::kSwReporterLastTimeSentReport));
       const base::Time next_time_send_logs =