Fixes a crash when the user's install flow fails and we press Cancel.

Verified fix using a BrowserTest.

Verifying the fix properly meant fleshing out FakeConciergeClient with
settable response protos for all methods.

Found an invalid DCHECK of |state_| in CrostiniInstallerView when testing.

Guard CrostiniRestarterService::Abort.
There used to be a DCHECK, but it shouldn't be. It is a legitimate possibility
to cancel the install flow right when the the container startup was complete.

Bug: 850281
Change-Id: I0cf6bcef1f4f043d9c7e40f3a8baa458fb6f6ba7
Reviewed-on: https://chromium-review.googlesource.com/1089600Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565603}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -130,8 +130,9 @@ class CrostiniRestarter : public base::RefCountedThreadSafe<CrostiniRestarter> {
     if (is_aborted_)
       return;
 
+    CrostiniManager* crostini_manager = CrostiniManager::GetInstance();
     // Finish Restart immediately if testing.
-    if (CrostiniManager::GetInstance()->skip_restart_for_testing()) {
+    if (crostini_manager->skip_restart_for_testing()) {
       content::BrowserThread::PostTask(
           content::BrowserThread::UI, FROM_HERE,
           base::BindOnce(&CrostiniRestarter::FinishRestart,
@@ -140,10 +141,8 @@ class CrostiniRestarter : public base::RefCountedThreadSafe<CrostiniRestarter> {
       return;
     }
 
-    auto* cros_component_manager =
-        g_browser_process->platform_part()->cros_component_manager();
-    if (!cros_component_manager) {
-      // Running in unittest. We still PostTask to prevent races between
+    if (crostini_manager->is_testing()) {
+      // Running in test. We still PostTask to prevent races between
       // observers aborting.
       content::BrowserThread::PostTask(
           content::BrowserThread::UI, FROM_HERE,
@@ -154,7 +153,9 @@ class CrostiniRestarter : public base::RefCountedThreadSafe<CrostiniRestarter> {
               base::FilePath()));
       return;
     }
-
+    auto* cros_component_manager =
+        g_browser_process->platform_part()->cros_component_manager();
+    DCHECK(cros_component_manager);
     cros_component_manager->Load(
         "cros-termina",
         component_updater::CrOSComponentManager::MountPolicy::kMount,
@@ -358,9 +359,12 @@ void CrostiniRestarterService::RunPendingCallbacks(
 
 void CrostiniRestarterService::Abort(CrostiniManager::RestartId restart_id) {
   auto it = restarter_map_.find(restart_id);
-  DCHECK(it != restarter_map_.end())
-      << "Aborting a restarter that already finished";
-
+  if (it == restarter_map_.end()) {
+    // This can happen if a user cancels the install flow at the exact right
+    // moment, for example.
+    LOG(ERROR) << "Aborting a restarter that already finished";
+    return;
+  }
   it->second->Abort();
   ErasePending(it->second.get());
   // Erasing |it| also invalidates |it|, so make a key from |it| now.

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -229,6 +229,9 @@ class CrostiniManager : public chromeos::ConciergeClient::Observer,
   void set_skip_restart_for_testing() { skip_restart_for_testing_ = true; }
   bool skip_restart_for_testing() { return skip_restart_for_testing_; }
 
+  void set_is_testing(bool is_testing) { is_testing_ = is_testing; }
+  bool is_testing() { return is_testing_; }
+
   // Adds a callback to receive notification of container shutdown.
   void AddShutdownContainerCallback(
       Profile* profile,
@@ -340,6 +343,7 @@ class CrostiniManager : public chromeos::ConciergeClient::Observer,
       start_container_callbacks_;
 
   bool skip_restart_for_testing_ = false;
+  bool is_testing_ = false;
 
   // Pending ShutdownContainer callbacks are keyed by <owner_id, vm_name,
   // container_name> string tuples.

chrome/browser/chromeos/crostini/crostini_manager_unittest.cc

@@ -120,6 +120,7 @@ class CrostiniManagerTest : public testing::Test {
     chromeos::DBusThreadManager::GetSetterForTesting()->SetConciergeClient(
         base::WrapUnique(fake_concierge_client_));
     chromeos::DBusThreadManager::Initialize();
+    crostini::CrostiniManager::GetInstance()->set_is_testing(true);
   }
 
   ~CrostiniManagerTest() override { chromeos::DBusThreadManager::Shutdown(); }

chrome/browser/ui/views/crostini/crostini_installer_view.cc

@@ -172,7 +172,7 @@ void CrostiniInstallerView::OnConciergeStarted(ConciergeClientResult result) {
 }
 
 void CrostiniInstallerView::OnDiskImageCreated(ConciergeClientResult result) {
-  DCHECK_EQ(state_, State::INSTALL_IMAGE_LOADER);
+  DCHECK_EQ(state_, State::START_CONCIERGE);
   state_ = State::CREATE_DISK_IMAGE;
   if (result != ConciergeClientResult::SUCCESS) {
     LOG(ERROR) << "Failed to create disk imagewith error code: "
@@ -246,6 +246,7 @@ void CrostiniInstallerView::HandleError(const base::string16& error_message,
     return;
 
   RecordSetupResultHistogram(result);
+  restart_id_ = crostini::CrostiniManager::kUninitializedRestartId;
   state_ = State::ERROR;
   message_label_->SetVisible(true);
   message_label_->SetText(error_message);

chrome/browser/ui/views/crostini/crostini_installer_view_browsertest.cc

@@ -8,6 +8,7 @@
 #include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -18,6 +19,8 @@
 #include "chrome/browser/ui/test/test_browser_dialog.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_concierge_client.h"
 #include "components/crx_file/id_util.h"
 #include "components/prefs/pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -25,7 +28,12 @@
 
 class CrostiniInstallerViewBrowserTest : public DialogBrowserTest {
  public:
-  CrostiniInstallerViewBrowserTest() {}
+  CrostiniInstallerViewBrowserTest()
+      : fake_concierge_client_(new chromeos::FakeConciergeClient()) {
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetConciergeClient(
+        base::WrapUnique(fake_concierge_client_));
+    crostini::CrostiniManager::GetInstance()->set_is_testing(true);
+  }
 
   // DialogBrowserTest:
   void ShowUi(const std::string& name) override {
@@ -55,6 +63,10 @@ class CrostiniInstallerViewBrowserTest : public DialogBrowserTest {
     return ActiveView()->GetDialogClientView()->cancel_button() != nullptr;
   }
 
+ protected:
+  // Owned by chromeos::DBusThreadManager
+  chromeos::FakeConciergeClient* fake_concierge_client_;
+
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
 
@@ -97,3 +109,25 @@ IN_PROC_BROWSER_TEST_F(CrostiniInstallerViewBrowserTest, Cancel) {
           CrostiniInstallerView::SetupResult::kNotStarted),
       1);
 }
+
+IN_PROC_BROWSER_TEST_F(CrostiniInstallerViewBrowserTest, ErrorThenCancel) {
+  base::HistogramTester histogram_tester;
+  ShowUi("default");
+  EXPECT_NE(nullptr, ActiveView());
+  vm_tools::concierge::StartVmResponse response;
+  response.set_success(false);
+  fake_concierge_client_->set_start_vm_response(std::move(response));
+
+  ActiveView()->GetDialogClientView()->AcceptWindow();
+  EXPECT_FALSE(ActiveView()->GetWidget()->IsClosed());
+  base::RunLoop().RunUntilIdle();
+  ActiveView()->GetDialogClientView()->CancelWindow();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(nullptr, ActiveView());
+
+  histogram_tester.ExpectBucketCount(
+      "Crostini.SetupResult",
+      static_cast<base::HistogramBase::Sample>(
+          CrostiniInstallerView::SetupResult::kErrorStartingTermina),
+      1);
+}

chromeos/dbus/fake_concierge_client.cc

@@ -10,7 +10,9 @@
 
 namespace chromeos {
 
-FakeConciergeClient::FakeConciergeClient() {}
+FakeConciergeClient::FakeConciergeClient() {
+  InitializeProtoResponses();
+}
 FakeConciergeClient::~FakeConciergeClient() = default;
 
 // ConciergeClient override.
@@ -37,11 +39,9 @@ void FakeConciergeClient::CreateDiskImage(
     const vm_tools::concierge::CreateDiskImageRequest& request,
     DBusMethodCallback<vm_tools::concierge::CreateDiskImageResponse> callback) {
   create_disk_image_called_ = true;
-  vm_tools::concierge::CreateDiskImageResponse response;
-  response.set_status(vm_tools::concierge::DISK_STATUS_CREATED);
-  response.set_disk_path("foo");
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), create_disk_image_response_));
 }
 
 void FakeConciergeClient::DestroyDiskImage(
@@ -49,69 +49,62 @@ void FakeConciergeClient::DestroyDiskImage(
     DBusMethodCallback<vm_tools::concierge::DestroyDiskImageResponse>
         callback) {
   destroy_disk_image_called_ = true;
-  vm_tools::concierge::DestroyDiskImageResponse response;
-  response.set_status(vm_tools::concierge::DISK_STATUS_DESTROYED);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), destroy_disk_image_response_));
 }
 
 void FakeConciergeClient::ListVmDisks(
     const vm_tools::concierge::ListVmDisksRequest& request,
     DBusMethodCallback<vm_tools::concierge::ListVmDisksResponse> callback) {
   list_vm_disks_called_ = true;
-  vm_tools::concierge::ListVmDisksResponse response;
-  response.set_success(true);
+
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE, base::BindOnce(std::move(callback), list_vm_disks_response_));
 }
 
 void FakeConciergeClient::StartTerminaVm(
     const vm_tools::concierge::StartVmRequest& request,
     DBusMethodCallback<vm_tools::concierge::StartVmResponse> callback) {
   start_termina_vm_called_ = true;
-  vm_tools::concierge::StartVmResponse response;
-  response.set_success(true);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE, base::BindOnce(std::move(callback), start_vm_response_));
 }
 
 void FakeConciergeClient::StopVm(
     const vm_tools::concierge::StopVmRequest& request,
     DBusMethodCallback<vm_tools::concierge::StopVmResponse> callback) {
   stop_vm_called_ = true;
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(true);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE, base::BindOnce(std::move(callback), stop_vm_response_));
 }
 
 void FakeConciergeClient::StartContainer(
     const vm_tools::concierge::StartContainerRequest& request,
     DBusMethodCallback<vm_tools::concierge::StartContainerResponse> callback) {
   start_container_called_ = true;
-  vm_tools::concierge::StartContainerResponse response;
-  response.set_status(vm_tools::concierge::CONTAINER_STATUS_RUNNING);
+
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), start_container_response_));
 }
 
 void FakeConciergeClient::LaunchContainerApplication(
     const vm_tools::concierge::LaunchContainerApplicationRequest& request,
     DBusMethodCallback<vm_tools::concierge::LaunchContainerApplicationResponse>
         callback) {
-  vm_tools::concierge::LaunchContainerApplicationResponse response;
-  response.set_success(true);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE, base::BindOnce(std::move(callback),
+                                launch_container_application_response_));
 }
 
 void FakeConciergeClient::GetContainerAppIcons(
     const vm_tools::concierge::ContainerAppIconRequest& request,
     DBusMethodCallback<vm_tools::concierge::ContainerAppIconResponse>
         callback) {
-  vm_tools::concierge::ContainerAppIconResponse response;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), container_app_icon_response_));
 }
 
 void FakeConciergeClient::WaitForServiceToBeAvailable(
@@ -124,9 +117,39 @@ void FakeConciergeClient::GetContainerSshKeys(
     const vm_tools::concierge::ContainerSshKeysRequest& request,
     DBusMethodCallback<vm_tools::concierge::ContainerSshKeysResponse>
         callback) {
-  vm_tools::concierge::ContainerSshKeysResponse response;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), container_ssh_keys_response_));
+}
+
+void FakeConciergeClient::InitializeProtoResponses() {
+  create_disk_image_response_.Clear();
+  create_disk_image_response_.set_status(
+      vm_tools::concierge::DISK_STATUS_CREATED);
+  create_disk_image_response_.set_disk_path("foo");
+
+  destroy_disk_image_response_.Clear();
+  destroy_disk_image_response_.set_status(
+      vm_tools::concierge::DISK_STATUS_DESTROYED);
+
+  list_vm_disks_response_.Clear();
+  list_vm_disks_response_.set_success(true);
+
+  start_vm_response_.Clear();
+  start_vm_response_.set_success(true);
+
+  stop_vm_response_.Clear();
+  stop_vm_response_.set_success(true);
+
+  start_container_response_.Clear();
+  start_container_response_.set_status(
+      vm_tools::concierge::CONTAINER_STATUS_RUNNING);
+
+  launch_container_application_response_.Clear();
+  launch_container_application_response_.set_success(true);
+
+  container_app_icon_response_.Clear();
+  container_ssh_keys_response_.Clear();
 }
 
 }  // namespace chromeos

chromeos/dbus/fake_concierge_client.h

@@ -123,10 +123,56 @@ class CHROMEOS_EXPORT FakeConciergeClient : public ConciergeClient {
     is_container_startup_failed_signal_connected_ = connected;
   }
 
+  void set_create_disk_image_response(
+      const vm_tools::concierge::CreateDiskImageResponse&
+          create_disk_image_response) {
+    create_disk_image_response_ = create_disk_image_response;
+  }
+  void set_destroy_disk_image_response(
+      const vm_tools::concierge::DestroyDiskImageResponse&
+          destroy_disk_image_response) {
+    destroy_disk_image_response_ = destroy_disk_image_response;
+  }
+  void set_list_vm_disks_response(
+      const vm_tools::concierge::ListVmDisksResponse& list_vm_disks_response) {
+    list_vm_disks_response_ = list_vm_disks_response;
+  }
+  void set_start_vm_response(
+      const vm_tools::concierge::StartVmResponse& start_vm_response) {
+    start_vm_response_ = start_vm_response;
+  }
+  void set_stop_vm_response(
+      const vm_tools::concierge::StopVmResponse& stop_vm_response) {
+    stop_vm_response_ = stop_vm_response;
+  }
+  void set_start_container_response(
+      const vm_tools::concierge::StartContainerResponse&
+          start_container_response) {
+    start_container_response_ = start_container_response;
+  }
+  void set_launch_container_application_response(
+      const vm_tools::concierge::LaunchContainerApplicationResponse&
+          launch_container_application_response) {
+    launch_container_application_response_ =
+        launch_container_application_response;
+  }
+  void set_container_app_icon_response(
+      const vm_tools::concierge::ContainerAppIconResponse&
+          container_app_icon_response) {
+    container_app_icon_response_ = container_app_icon_response;
+  }
+  void set_container_ssh_keys_response(
+      const vm_tools::concierge::ContainerSshKeysResponse&
+          container_ssh_keys_response) {
+    container_ssh_keys_response_ = container_ssh_keys_response;
+  }
+
  protected:
   void Init(dbus::Bus* bus) override {}
 
  private:
+  void InitializeProtoResponses();
+
   bool create_disk_image_called_ = false;
   bool destroy_disk_image_called_ = false;
   bool list_vm_disks_called_ = false;
@@ -135,6 +181,18 @@ class CHROMEOS_EXPORT FakeConciergeClient : public ConciergeClient {
   bool start_container_called_ = false;
   bool is_container_started_signal_connected_ = true;
   bool is_container_startup_failed_signal_connected_ = true;
+
+  vm_tools::concierge::CreateDiskImageResponse create_disk_image_response_;
+  vm_tools::concierge::DestroyDiskImageResponse destroy_disk_image_response_;
+  vm_tools::concierge::ListVmDisksResponse list_vm_disks_response_;
+  vm_tools::concierge::StartVmResponse start_vm_response_;
+  vm_tools::concierge::StopVmResponse stop_vm_response_;
+  vm_tools::concierge::StartContainerResponse start_container_response_;
+  vm_tools::concierge::LaunchContainerApplicationResponse
+      launch_container_application_response_;
+  vm_tools::concierge::ContainerAppIconResponse container_app_icon_response_;
+  vm_tools::concierge::ContainerSshKeysResponse container_ssh_keys_response_;
+
   base::ObserverList<Observer> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeConciergeClient);