Adds error handling support for the SwReporter launcher.

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 <initializer_list>
 #include <set>
 #include <tuple>
 #include <utility>
@@ skipping 12 matching lines @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/lifetime/keep_alive_types.h"
 #include "chrome/browser/lifetime/scoped_keep_alive.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/srt_chrome_prompt_impl.h"
 #include "chrome/browser/safe_browsing/srt_client_info_win.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/test/test_browser_dialog.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/chrome_cleaner/public/constants/constants.h"
 #include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
 #include "components/component_updater/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing_db/safe_browsing_prefs.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/scoped_ipc_support.h"
 #include "mojo/edk/system/core.h"
 #include "testing/multiprocess_func_list.h"
 
 namespace safe_browsing {
 
 namespace {
 
 using chrome_cleaner::mojom::ElevationStatus;
 using chrome_cleaner::mojom::PromptAcceptance;
 
 // Special switches passed by the parent process (test case) to the reporter
 // child process to indicate the behavior that should be mocked.
 constexpr char kExitCodeToReturnSwitch[] = "exit-code-to-return";
 constexpr char kReportUwSFoundSwitch[] = "report-uws-found";
 constexpr char kReportElevationRequiredSwitch[] = "report-elevation-required";
 constexpr char kExpectedPromptAcceptanceSwitch[] = "expected-prompt-acceptance";
+constexpr char kExpectedReporterFailureSwitch[] = "expected-reporter-crash";
 
 // The exit code to be returned in case of failure in the child process.
 // This should never be passed as the expected exit code to be reported by
 // tests.
 constexpr int kFailureExitCode = -1;
 
 // Pass the |prompt_acceptance| to the mock child process in command line
 // switch kExpectedPromptAcceptanceSwitch.
 void AddPromptAcceptanceToCommandLine(PromptAcceptance prompt_acceptance,
                                       base::CommandLine* command_line) {
@@ skipping 16 matching lines @@
     if (chrome_cleaner::mojom::IsKnownEnumValue(prompt_acceptance))
       return prompt_acceptance;
   }
   return PromptAcceptance::UNSPECIFIED;
 }
 
 // Pointer to ChromePromptPtr object to be used by the child process. The
 // object must be created, deleted, and accessed on the IPC thread only.
 chrome_cleaner::mojom::ChromePromptPtr* g_chrome_prompt_ptr = nullptr;
 
+// Potential failures on the reporter process that should be handled by the

[Joe Mason, 2017/04/24 14:53:48]

Nit: "in the reporter process". Or "of the reporter process".

[ftirelo, 2017/04/24 15:47:46]

Done.

+// parent process.
+enum class MockedReporterFailure {
+  kNone = 0,
+  // Crashes at specific moments in the reporter when connected to the IPC.
+  kCrashOnStartup = 1,
+  kCrashAfterConnectedToParentProcess = 2,
+  kCrashWhileWaitingForResponse = 3,
+  kCrashAfterReceivedResponse = 4,
+  // Once an IPC message is sent by the reporter, the parent process will
+  // report a bad message that will lead to an invocation of OnConnectionError.
+  kBadMessageReported = 5,
+};
+
+void AddExpectedCrashToCommandLine(MockedReporterFailure reporter_failure,
+                                   base::CommandLine* command_line) {
+  command_line->AppendSwitchASCII(
+      kExpectedReporterFailureSwitch,
+      base::IntToString(static_cast<int>(reporter_failure)));
+}
+
+void CrashIf(MockedReporterFailure reporter_failure) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  const std::string& expected_str =
+      command_line->GetSwitchValueASCII(kExpectedReporterFailureSwitch);
+  int val = -1;
+  if (base::StringToInt(expected_str, &val) &&
+      static_cast<MockedReporterFailure>(val) == reporter_failure) {
+    exit(kFailureExitCode);
+  }
+}
+
 // The callback function to be passed to ChromePrompt::PromptUser to check if
 // the prompt accepted returned by the parent process is equal to
 // |expected_prompt_acceptance|. Will set |expected_value_received| with the
 // comparison result, so that the main thread can report failure. Will invoke
 // |done| callback when done.
 void PromptUserCallback(const base::Closure& done,
                         PromptAcceptance expected_prompt_acceptance,
                         bool* expected_value_received,
                         PromptAcceptance prompt_acceptance) {
   *expected_value_received = prompt_acceptance == expected_prompt_acceptance;
   // It's safe to delete the ChromePromptPtr object here, since it will not be
   // used anymore by the child process.
   delete g_chrome_prompt_ptr;
   g_chrome_prompt_ptr = nullptr;
+  CrashIf(MockedReporterFailure::kCrashAfterReceivedResponse);
   done.Run();
 }
 
 // Mocks the sending of scan results from the child process to the parent
 // process. Obtains the behavior to be mocked from special switches in
 // |command_line|. Sets |expected_value_received| as true if the parent
 // process replies with the expected prompt acceptance value.
 void SendScanResults(const std::string& chrome_mojo_pipe_token,
                      const base::CommandLine& command_line,
                      const base::Closure& done,
@@ skipping 16 matching lines @@
     uws->observed_behaviours = chrome_cleaner::mojom::ObservedBehaviours::New();
     removable_uws_found.push_back(std::move(uws));
   }
   const ElevationStatus elevation_status =
       command_line.HasSwitch(kReportElevationRequiredSwitch)
           ? ElevationStatus::REQUIRED
           : ElevationStatus::NOT_REQUIRED;
   const PromptAcceptance expected_prompt_acceptance =
       PromptAcceptanceFromCommandLine(command_line);
 
+  // This task is posted to the IPC thread so that it will happen after the
+  // request is sent to the parent process and before the response gets
+  // handled on the IPC thread.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&CrashIf,
+                 MockedReporterFailure::kCrashWhileWaitingForResponse));
+
   (*g_chrome_prompt_ptr)
       ->PromptUser(
           std::move(removable_uws_found), elevation_status,
           base::Bind(&PromptUserCallback, done, expected_prompt_acceptance,
                      expected_value_received));
 }
 
 // Connects to the parent process and sends mocked scan results. Returns true
 // if connection was successful and the prompt acceptance results sent by the
 // parent process are the same as expected.
 bool ConnectAndSendDataToParentProcess(
     const std::string& chrome_mojo_pipe_token,
     const base::CommandLine& command_line) {
   DCHECK(!chrome_mojo_pipe_token.empty());
 
   mojo::edk::Init();
   base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
   base::Thread io_thread("IPCThread");
   if (!io_thread.StartWithOptions(options))
     return false;
   mojo::edk::ScopedIPCSupport ipc_support(
       io_thread.task_runner(),
       mojo::edk::ScopedIPCSupport::ShutdownPolicy::CLEAN);
   mojo::edk::SetParentPipeHandleFromCommandLine();
+
+  CrashIf(MockedReporterFailure::kCrashAfterConnectedToParentProcess);
+
   base::MessageLoop message_loop;
   base::RunLoop run_loop;
   // After the response from the parent process is received, this will post a
   // task to unblock the child process's main thread.
   auto done = base::Bind(
       [](scoped_refptr<base::SequencedTaskRunner> main_runner,
          base::Closure quit_closure) {
         main_runner->PostTask(FROM_HERE, std::move(quit_closure));
       },
       base::SequencedTaskRunnerHandle::Get(),
       base::Passed(run_loop.QuitClosure()));
 
   bool expected_value_received = false;
   io_thread.task_runner()->PostTask(
       FROM_HERE, base::Bind(&SendScanResults, chrome_mojo_pipe_token,
                             command_line, done, &expected_value_received));
+
   run_loop.Run();
 
   return expected_value_received;
 }
 
 // Mocks a Software Reporter process that returns an exit code specified by
 // command line switch kExitCodeToReturnSwitch. If a Mojo IPC is available,
 // this will also connect to the parent process and send mocked scan results
 // to the parent process using data passed as command line switches.
 MULTIPROCESS_TEST_MAIN(MockSwReporterProcess) {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  CrashIf(MockedReporterFailure::kCrashOnStartup);
   const std::string& str =
       command_line->GetSwitchValueASCII(kExitCodeToReturnSwitch);
   const std::string& chrome_mojo_pipe_token = command_line->GetSwitchValueASCII(
       chrome_cleaner::kChromeMojoPipeTokenSwitch);
   int exit_code_to_report = kFailureExitCode;
   bool success = base::StringToInt(str, &exit_code_to_report) &&
                  (chrome_mojo_pipe_token.empty() ||
                   ConnectAndSendDataToParentProcess(chrome_mojo_pipe_token,
                                                     *command_line));
   return success ? exit_code_to_report : kFailureExitCode;
 }
 
+// Decorates a ChromePromptImpl object to simulate failures indentified by the
+// parent process and keep track of errors handled. By default, deletes all
+// actions to the decorated object.
+class ReportBadMessageChromePromptImpl : public ChromePromptImpl {
+ public:
+  ReportBadMessageChromePromptImpl(
+      chrome_cleaner::mojom::ChromePromptRequest request,
+      bool bad_message_expected,
+      bool* bad_message_reported)
+      : ChromePromptImpl(std::move(request)),
+        bad_message_expected_(bad_message_expected),
+        bad_message_reported_(bad_message_reported) {
+    DCHECK(bad_message_reported_);
+  }
+  ~ReportBadMessageChromePromptImpl() = default;
+
+  void PromptUser(
+      std::vector<chrome_cleaner::mojom::UwSPtr> removable_uws_found,
+      chrome_cleaner::mojom::ElevationStatus elevation_status,
+      const chrome_cleaner::mojom::ChromePrompt::PromptUserCallback& callback) {
+    if (bad_message_expected_)
+      mojo::ReportBadMessage("bad message");
+
+    ChromePromptImpl::PromptUser(std::move(removable_uws_found),
+                                 elevation_status, callback);
+  }
+
+  void OnConnectionError(const std::string& message) override {
+    *bad_message_reported_ = true;
+
+    ChromePromptImpl::OnConnectionError(message);
+  }
+
+ private:
+  bool bad_message_expected_ = false;
+  bool* bad_message_reported_ = nullptr;
+};
+
 // Parameters for this test:
 //  - bool in_browser_cleaner_ui: indicates if InBrowserCleanerUI experiment
 //    is enabled; if so, the parent and the child processes will communicate
 //    via a Mojo IPC;
 //  - ElevationStatus elevation_status: indicates if the scan results sent by
 //    the child process should consider that elevation will be required for
-//    cleanup.
+//    cleanup;

[Joe Mason, 2017/04/24 14:53:48]

The most trivial nit ever: Since these lines are already delimited with "-" at
the start, I suggest ending them all with . instead of ; so that you don't need
to update line endings again if another bullet point is added.

[ftirelo, 2017/04/24 15:47:46]

Done.

+//  - MockedReporterFailure expected_reporter_failure_: indicates errors that
+//    should be simulated on the reporter process.
 class SRTFetcherTest
     : public InProcessBrowserTest,
       public SwReporterTestingDelegate,
-      public ::testing::WithParamInterface<std::tuple<bool, ElevationStatus>> {
+      public ::testing::WithParamInterface<
+          std::tuple<bool, ElevationStatus, MockedReporterFailure>> {
  public:
   void SetUpInProcessBrowserTestFixture() override {
     SetSwReporterTestingDelegate(this);
 
-    std::tie(in_browser_cleaner_ui_, elevation_status_) = GetParam();
+    std::tie(in_browser_cleaner_ui_, elevation_status_,
+             expected_reporter_failure_) = GetParam();
     // The config should only accept elevation_status_ if InBrowserCleanerUI
     // feature is enabled.
     ASSERT_TRUE(elevation_status_ == ElevationStatus::NOT_REQUIRED ||
                 in_browser_cleaner_ui_);
 
     if (in_browser_cleaner_ui_)
       scoped_feature_list_.InitAndEnableFeature(kInBrowserCleanerUIFeature);
     else
       scoped_feature_list_.InitAndDisableFeature(kInBrowserCleanerUIFeature);
   }
@@ skipping 36 matching lines @@
   // process.
   // Records the launch and parameters used for further verification.
   base::Process LaunchReporter(
       const SwReporterInvocation& invocation,
       const base::LaunchOptions& launch_options) override {
     ++reporter_launch_count_;
     reporter_launch_parameters_.push_back(invocation);
     if (first_launch_callback_)
       std::move(first_launch_callback_).Run();
 
+    if (exit_code_to_report_ == kReporterNotLaunchedExitCode)
+      return base::Process();  // IsValid() will return false.
+
     base::CommandLine command_line(
         base::GetMultiProcessTestChildBaseCommandLine());
     command_line.AppendArguments(invocation.command_line,
                                  /*include_program=*/false);
     command_line.AppendSwitchASCII(kExitCodeToReturnSwitch,
                                    base::IntToString(exit_code_to_report_));
+
     if (in_browser_cleaner_ui_) {
       AddPromptAcceptanceToCommandLine(PromptAcceptance::DENIED, &command_line);
       if (exit_code_to_report_ == chrome_cleaner::kSwReporterCleanupNeeded) {
         command_line.AppendSwitch(kReportUwSFoundSwitch);
         if (elevation_status_ == ElevationStatus::REQUIRED)
           command_line.AppendSwitch(kReportElevationRequiredSwitch);
       }
     }
+
+    if (expected_reporter_failure_ != MockedReporterFailure::kNone)
+      AddExpectedCrashToCommandLine(expected_reporter_failure_, &command_line);
+
+    bad_message_expected_ = expected_reporter_failure_ ==
+                            MockedReporterFailure::kBadMessageReported;
+    bad_message_reported_ = false;
+
     base::SpawnChildResult result = base::SpawnMultiProcessTestChild(
         "MockSwReporterProcess", command_line, launch_options);
     return std::move(result.process);
   }
 
   // Returns the test's idea of the current time.
   base::Time Now() const override { return mock_time_task_runner_->Now(); }
 
   // Returns a task runner to use when launching the reporter (which is
   // normally a blocking action).
   base::TaskRunner* BlockingTaskRunner() const override {
     // Use the test's main task runner so that we don't need to pump another
     // message loop. Since the test calls LaunchReporter instead of actually
     // doing a blocking reporter launch, it doesn't matter that the task runner
     // doesn't have the MayBlock trait.
     return mock_time_task_runner_.get();
   }
 
+  std::unique_ptr<ChromePromptImpl> CreateChromePromptImpl(
+      chrome_cleaner::mojom::ChromePromptRequest request) {
+    return base::MakeUnique<ReportBadMessageChromePromptImpl>(
+        std::move(request), bad_message_expected_, &bad_message_reported_);
+  }
+
   // Schedules a single reporter to run.
   void RunReporter(int exit_code_to_report,
                    const base::FilePath& exe_path = base::FilePath()) {
     exit_code_to_report_ = exit_code_to_report;
     auto invocation = SwReporterInvocation::FromFilePath(exe_path);
     invocation.supported_behaviours =
         SwReporterInvocation::BEHAVIOUR_LOG_EXIT_CODE_TO_PREFS |
         SwReporterInvocation::BEHAVIOUR_TRIGGER_PROMPT |
         SwReporterInvocation::BEHAVIOUR_ALLOW_SEND_REPORTER_LOGS;
 
@@ skipping 68 matching lines @@
                               bool expect_prompt) {
     EXPECT_TRUE(mock_time_task_runner_->HasPendingTask());
     reporter_launch_count_ = 0;
     reporter_launch_parameters_.clear();
     prompt_trigger_called_ = false;
 
     mock_time_task_runner_->FastForwardBy(
         base::TimeDelta::FromDays(days_until_launch));
 
     EXPECT_EQ(expected_launch_count, reporter_launch_count_);
-    EXPECT_EQ(expect_prompt, prompt_trigger_called_);
+    // Note: a bad message error will not prevent the prompt that is shown to
+    // the user today. Once we hook the new prompt here, we will need to review
+    // this expectation.
+    EXPECT_EQ(expect_prompt &&
+                  (expected_reporter_failure_ == MockedReporterFailure::kNone ||
+                   expected_reporter_failure_ ==
+                       MockedReporterFailure::kBadMessageReported),
+              prompt_trigger_called_);
+
+    EXPECT_EQ(bad_message_expected_, bad_message_reported_);
   }
 
   // Expects that after |days_until_launched| days, the reporter will be
   // launched once with each path in |expected_launch_paths|, and TriggerPrompt
   // will be called if and only if |expect_prompt| is true.
   void ExpectReporterLaunches(
       int days_until_launch,
       const std::vector<base::FilePath>& expected_launch_paths,
       bool expect_prompt) {
     ExpectReporterLaunches(days_until_launch, expected_launch_paths.size(),
@@ skipping 46 matching lines @@
 
   // A task runner with a mock clock.
   scoped_refptr<base::TestMockTimeTaskRunner> mock_time_task_runner_ =
       new base::TestMockTimeTaskRunner();
 
   // The task runner that was in use before installing |mock_time_task_runner_|.
   scoped_refptr<base::SingleThreadTaskRunner> saved_task_runner_;
 
   bool in_browser_cleaner_ui_;
   ElevationStatus elevation_status_;
+  MockedReporterFailure expected_reporter_failure_;
+
+  // Indicates if a bad message error should be simulated by the parent
+  // process.
+  bool bad_message_expected_;
+  // Set by ReportBadMessageChromePromptImpl if a bad message error is reported.
+  bool bad_message_reported_;
 
   bool prompt_trigger_called_ = false;
   int reporter_launch_count_ = 0;
   std::vector<SwReporterInvocation> reporter_launch_parameters_;
-  int exit_code_to_report_ = kReporterFailureExitCode;
+  int exit_code_to_report_ = kReporterNotLaunchedExitCode;
 
   // A callback to invoke when the first reporter of a queue is launched. This
   // can be used to perform actions in the middle of a queue of reporters which
   // all launch on the same mock clock tick.
   base::OnceClosure first_launch_callback_;
 
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 class SRTFetcherPromptTest : public DialogBrowserTest {
@@ skipping 68 matching lines @@
   mock_time_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(10));
 
   // On opening the new browser, the prompt should be shown and the reporter
   // should be scheduled to run later.
   EXPECT_EQ(1, reporter_launch_count_);
   EXPECT_TRUE(prompt_trigger_called_);
   ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
 
 IN_PROC_BROWSER_TEST_P(SRTFetcherTest, Failure) {
-  RunReporter(kReporterFailureExitCode);
+  RunReporter(kReporterNotLaunchedExitCode);
   ExpectReporterLaunches(0, 1, false);
   ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
 
 IN_PROC_BROWSER_TEST_P(SRTFetcherTest, RunDaily) {
   PrefService* local_state = g_browser_process->local_state();
   local_state->SetBoolean(prefs::kSwReporterPendingPrompt, true);
   SetDaysSinceLastTriggered(kDaysBetweenSuccessfulSwReporterRuns - 1);
   ASSERT_GT(kDaysBetweenSuccessfulSwReporterRuns - 1,
             kDaysBetweenSwReporterRunsForPendingPrompt);
@@ skipping 122 matching lines @@
 
     // Time passes... Now the 3-element queue should run.
     ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns,
                            {path1, path2, path3}, false);
     ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
   }
 
   // Second launch should not occur after a failure.
   {
     SCOPED_TRACE("Launch multiple times with failure");
-    RunReporterQueue(kReporterFailureExitCode, invocations);
+    RunReporterQueue(kReporterNotLaunchedExitCode, invocations);
     ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns, {path1},
                            false);
     ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 
     // If we try again before the reporting period is up, it should not do
     // anything.
     ExpectReporterLaunches(0, {}, false);
 
     // After enough time has passed, should try the queue again.
     ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns, {path1},
@@ skipping 81 matching lines @@
     ExpectLoggingSwitches(reporter_launch_parameters_[1], true);
     ExpectLastReportSentInTheLastHour();
   }
   ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
 
 INSTANTIATE_TEST_CASE_P(
     NoInBrowserCleanerUI,
     SRTFetcherTest,
     testing::Combine(testing::Values(false),
-                     testing::Values(ElevationStatus::NOT_REQUIRED)));
+                     testing::Values(ElevationStatus::NOT_REQUIRED),
+                     testing::Values(MockedReporterFailure::kNone,
+                                     MockedReporterFailure::kCrashOnStartup)));
 
 INSTANTIATE_TEST_CASE_P(
     InBrowserCleanerUI,
     SRTFetcherTest,
-    testing::Combine(testing::Values(true),
-                     testing::Values(ElevationStatus::NOT_REQUIRED,
-                                     ElevationStatus::REQUIRED)));
+    testing::Combine(
+        testing::Values(true),
+        testing::Values(ElevationStatus::NOT_REQUIRED,
+                        ElevationStatus::REQUIRED),
+        testing::Values(
+            MockedReporterFailure::kNone,
+            MockedReporterFailure::kCrashOnStartup,
+            MockedReporterFailure::kCrashAfterConnectedToParentProcess,
+            MockedReporterFailure::kCrashWhileWaitingForResponse,
+            MockedReporterFailure::kCrashAfterReceivedResponse,
+            MockedReporterFailure::kBadMessageReported)));
 
 // This provide tests which allows explicit invocation of the SRT Prompt
 // useful for checking dialog layout or any other interactive functionality
 // tests. See docs/testing/test_browser_dialog.md for description of the
 // testing framework.
 IN_PROC_BROWSER_TEST_F(SRTFetcherPromptTest, InvokeDialog_SRTErrorNoFile) {
   RunDialog();
 }
 
 IN_PROC_BROWSER_TEST_F(SRTFetcherPromptTest, InvokeDialog_SRTErrorFile) {
   RunDialog();
 }
 
 }  // namespace safe_browsing