Allow BorealisTask to report failure status

Previously tasks only indicated true/false fro success/fail. Following
crrev.com/c/2467818 which reports errors to users of the context
manager, we make a similar modification to tasks s.t. they report
failures to the context manager.

We similarly extend the unit tests slightly to ensure that errors are
passed from tasks to the ResultCallback.

Bug: b/162562622
Change-Id: I9ce094c00cad8fc13f72da9109effa6f0f91580b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2470276Reviewed-by: Daniel Ng <danielng@google.com>
Commit-Queue: Nic Hollingum <hollingum@google.com>
Cr-Commit-Position: refs/heads/master@{#818319}

chrome/browser/chromeos/borealis/borealis_context.h

@@ -31,7 +31,7 @@ class BorealisContext {
   bool borealis_running() const { return borealis_running_; }
   void set_borealis_running(bool success) { borealis_running_ = success; }
 
-  const std::string& vm_name() { return vm_name_; }
+  const std::string& vm_name() const { return vm_name_; }
   void set_vm_name(std::string vm_name) { vm_name_ = std::move(vm_name); }
 
   const std::string& root_path() const { return root_path_; }

chrome/browser/chromeos/borealis/borealis_context_manager.cc

@@ -35,7 +35,7 @@ const std::string& BorealisContextManager::Result::FailureReason() const {
   return failure_reason_;
 }
 
-const BorealisContext& BorealisContextManager::Result::Success() {
+const BorealisContext& BorealisContextManager::Result::Success() const {
   DCHECK(Ok());
   return *ctx_;
 }

chrome/browser/chromeos/borealis/borealis_context_manager.h

@@ -17,7 +17,12 @@ class BorealisContext;
 class BorealisContextManager : public KeyedService {
  public:
   // A list of possible outcomes for an attempt to startup borealis.
-  enum Status { kSuccess = 0, kMountFailed = 1 };
+  enum Status {
+    kSuccess = 0,
+    kMountFailed = 1,
+    kDiskImageFailed = 2,
+    kStartVmFailed = 3,
+  };
 
   // An attempt to launch borealis. If the launch succeeds, holds a reference to
   // the context created for that launch, otherwise holds an error.
@@ -42,7 +47,7 @@ class BorealisContextManager : public KeyedService {
 
     // In the event of a successful launch, returns a handle to the context
     // created for that launch.
-    const BorealisContext& Success();
+    const BorealisContext& Success() const;
 
    private:
     Status status_;

chrome/browser/chromeos/borealis/borealis_context_manager_impl.cc

@@ -4,10 +4,30 @@
 
 #include "chrome/browser/chromeos/borealis/borealis_context_manager_impl.h"
 
+#include <ostream>
+
 #include "base/logging.h"
 #include "chrome/browser/chromeos/borealis/borealis_context_manager.h"
 #include "chrome/browser/chromeos/borealis/borealis_task.h"
 
+namespace {
+
+std::ostream& operator<<(std::ostream& stream,
+                         borealis::BorealisContextManager::Status status) {
+  switch (status) {
+    case borealis::BorealisContextManager::kSuccess:
+      return stream << "Success";
+    case borealis::BorealisContextManager::kMountFailed:
+      return stream << "Mount Failed";
+    case borealis::BorealisContextManager::kDiskImageFailed:
+      return stream << "Disk Image Failed";
+    case borealis::BorealisContextManager::kStartVmFailed:
+      return stream << "Start VM Failed";
+  }
+}
+
+}  // namespace
+
 namespace borealis {
 
 BorealisContextManagerImpl::BorealisContextManagerImpl(Profile* profile)
@@ -24,7 +44,7 @@ void BorealisContextManagerImpl::StartBorealis(ResultCallback callback) {
   if (!is_borealis_starting_) {
     is_borealis_starting_ = true;
     task_queue_ = GetTasks();
-    NextTask(/*should_continue=*/true);
+    NextTask();
   }
 }
 
@@ -41,17 +61,7 @@ void BorealisContextManagerImpl::AddCallback(ResultCallback callback) {
   callback_queue_.push(std::move(callback));
 }
 
-void BorealisContextManagerImpl::NextTask(bool should_continue) {
-  if (!should_continue) {
-    // TODO(b/168425531): Error handling should be expanded to give more
-    // information about which task failed, why it failed and what should happen
-    // as a result.
-    // For now just use some random error.
-    startup_status_ = kMountFailed;
-    startup_error_ = "A task failed when trying to start Borealis.";
-    OnQueueComplete();
-    return;
-  }
+void BorealisContextManagerImpl::NextTask() {
   if (task_queue_.empty()) {
     context_.set_borealis_running(true);
     is_borealis_running_ = true;
@@ -62,10 +72,23 @@ void BorealisContextManagerImpl::NextTask(bool should_continue) {
   current_task_ = std::move(task_queue_.front());
   task_queue_.pop();
   current_task_->Run(&context_,
-                     base::BindOnce(&BorealisContextManagerImpl::NextTask,
+                     base::BindOnce(&BorealisContextManagerImpl::TaskCallback,
                                     weak_factory_.GetWeakPtr()));
 }
 
+void BorealisContextManagerImpl::TaskCallback(Status status,
+                                              std::string error) {
+  startup_status_ = status;
+  if (startup_status_ == kSuccess) {
+    NextTask();
+    return;
+  }
+  startup_error_ = error;
+  LOG(ERROR) << "Startup failed: failure=" << startup_status_
+             << " message=" << startup_error_;
+  OnQueueComplete();
+}
+
 void BorealisContextManagerImpl::OnQueueComplete() {
   is_borealis_starting_ = false;
   while (!callback_queue_.empty()) {

chrome/browser/chromeos/borealis/borealis_context_manager_impl.h

@@ -33,7 +33,8 @@ class BorealisContextManagerImpl : public BorealisContextManager {
 
  private:
   void AddCallback(ResultCallback callback);
-  void NextTask(bool should_continue);
+  void NextTask();
+  void TaskCallback(Status status, std::string error);
   void OnQueueComplete();
 
   // Returns the result of the startup (i.e. the context if it succeeds, or an

chrome/browser/chromeos/borealis/borealis_context_manager_unittest.cc

@@ -21,19 +21,27 @@ namespace borealis {
 namespace {
 
 MATCHER(IsSuccessResult, "") {
-  return arg.Ok();
+  return arg.Ok() && arg.Success().vm_name() == "test_vm_name";
 }
 
 MATCHER(IsFailureResult, "") {
-  return !arg.Ok();
+  return !arg.Ok() && arg.Failure() == BorealisContextManager::kStartVmFailed &&
+         arg.FailureReason() == "Something went wrong!";
 }
 
 class MockTask : public BorealisTask {
  public:
   explicit MockTask(bool success) : success_(success) {}
   void Run(BorealisContext* context,
-           base::OnceCallback<void(bool)> callback) override {
-    std::move(callback).Run(/*should_continue=*/success_);
+           BorealisTask::CompletionStatusCallback callback) override {
+    if (success_) {
+      context->set_vm_name("test_vm_name");
+      std::move(callback).Run(BorealisContextManager::kSuccess, "");
+    } else {
+      // Just use a random error.
+      std::move(callback).Run(BorealisContextManager::kStartVmFailed,
+                              "Something went wrong!");
+    }
   }
   bool success_ = true;
 };
@@ -110,6 +118,18 @@ TEST_F(BorealisContextManagerTest, GetTasksReturnsCorrectTaskList) {
   EXPECT_FALSE(tasks.empty());
 }
 
+TEST_F(BorealisContextManagerTest, NoTasksImpliesSuccess) {
+  testing::StrictMock<ResultCallbackHandler> callback_expectation;
+
+  BorealisContextManagerImplForTesting context_manager(
+      profile_.get(), /*tasks=*/0, /*success=*/true);
+  EXPECT_CALL(callback_expectation, Callback(testing::_))
+      .WillOnce(testing::Invoke(
+          [](BorealisContextManager::Result result) { result.Ok(); }));
+  context_manager.StartBorealis(callback_expectation.GetCallback());
+  task_environment_.RunUntilIdle();
+}
+
 TEST_F(BorealisContextManagerTest, StartupSucceedsForSuccessfulTask) {
   testing::StrictMock<ResultCallbackHandler> callback_expectation;
 

chrome/browser/chromeos/borealis/borealis_task.cc

@@ -3,13 +3,16 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/borealis/borealis_task.h"
+#include <string>
 
 #include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
 #include "chrome/browser/chromeos/borealis/borealis_context.h"
 #include "chrome/browser/chromeos/borealis/borealis_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "chromeos/dbus/concierge/concierge_service.pb.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 
 namespace borealis {
@@ -31,12 +34,12 @@ void MountDlc::OnMountDlc(
     CompletionStatusCallback callback,
     const chromeos::DlcserviceClient::InstallResult& install_result) {
   if (install_result.error != dlcservice::kErrorNone) {
-    LOG(ERROR) << "Mounting the DLC for Borealis failed: "
-               << install_result.error;
-    std::move(callback).Run(false);
+    std::move(callback).Run(
+        BorealisContextManager::kMountFailed,
+        "Mounting the DLC for Borealis failed: " + install_result.error);
   } else {
     context->set_root_path(install_result.root_path);
-    std::move(callback).Run(true);
+    std::move(callback).Run(BorealisContextManager::kSuccess, "");
   }
 }
 
@@ -67,22 +70,23 @@ void CreateDiskImage::OnCreateDiskImage(
     CompletionStatusCallback callback,
     base::Optional<vm_tools::concierge::CreateDiskImageResponse> response) {
   if (!response) {
-    LOG(ERROR) << "Failed to create disk image for Borealis. Empty response.";
     context->set_disk_path(base::FilePath());
-    std::move(callback).Run(false);
+    std::move(callback).Run(
+        BorealisContextManager::kDiskImageFailed,
+        "Failed to create disk image for Borealis: Empty response.");
     return;
   }
 
   if (response->status() != vm_tools::concierge::DISK_STATUS_EXISTS &&
       response->status() != vm_tools::concierge::DISK_STATUS_CREATED) {
-    LOG(ERROR) << "Failed to create disk image for Borealis: "
-               << response->failure_reason();
     context->set_disk_path(base::FilePath());
-    std::move(callback).Run(false);
+    std::move(callback).Run(BorealisContextManager::kDiskImageFailed,
+                            "Failed to create disk image for Borealis: " +
+                                response->failure_reason());
     return;
   }
   context->set_disk_path(base::FilePath(response->disk_path()));
-  std::move(callback).Run(true);
+  std::move(callback).Run(BorealisContextManager::kSuccess, "");
 }
 
 StartBorealisVm::StartBorealisVm() = default;
@@ -126,24 +130,21 @@ void StartBorealisVm::OnStartBorealisVm(
     CompletionStatusCallback callback,
     base::Optional<vm_tools::concierge::StartVmResponse> response) {
   if (!response) {
-    LOG(ERROR) << "Failed to start Borealis VM. Empty response.";
-    std::move(callback).Run(false);
+    std::move(callback).Run(BorealisContextManager::kStartVmFailed,
+                            "Failed to start Borealis VM: Empty response.");
     return;
   }
 
-  if (response->status() == vm_tools::concierge::VM_STATUS_RUNNING) {
-    std::move(callback).Run(true);
+  if (response->status() == vm_tools::concierge::VM_STATUS_RUNNING ||
+      response->status() == vm_tools::concierge::VM_STATUS_STARTING) {
+    std::move(callback).Run(BorealisContextManager::kSuccess, "");
     return;
   }
 
-  if (response->status() == vm_tools::concierge::VM_STATUS_FAILURE ||
-      response->status() == vm_tools::concierge::VM_STATUS_UNKNOWN) {
-    LOG(ERROR) << "Failed to start Borealis VM: " << response->failure_reason();
-    std::move(callback).Run(false);
-    return;
-  }
-
-  DCHECK_EQ(response->status(), vm_tools::concierge::VM_STATUS_STARTING);
-  std::move(callback).Run(true);
+  std::move(callback).Run(
+      BorealisContextManager::kStartVmFailed,
+      "Failed to start Borealis VM: " + response->failure_reason() + " (code " +
+          base::NumberToString(response->status()) + ")");
 }
+
 }  // namespace borealis

chrome/browser/chromeos/borealis/borealis_task.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_CHROMEOS_BOREALIS_BOREALIS_TASK_H_
 
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/chromeos/borealis/borealis_context_manager.h"
 #include "chromeos/dbus/concierge_client.h"
 #include "chromeos/dbus/dlcservice/dlcservice_client.h"
 
@@ -17,9 +18,11 @@ class BorealisContext;
 // Borealis Context Manager.
 class BorealisTask {
  public:
-  // Callback to be run when the task completes. The |bool| should reflect
-  // if the task succeeded (true) or failed (false).
-  using CompletionStatusCallback = base::OnceCallback<void(bool)>;
+  // Callback to be run when the task completes. The |status| should reflect
+  // if the task succeeded with kSuccess and an empty string. If it fails, a
+  // different status should be used, and an error string provided.
+  using CompletionStatusCallback =
+      base::OnceCallback<void(BorealisContextManager::Status, std::string)>;
   BorealisTask() = default;
   BorealisTask(const BorealisTask&) = delete;
   BorealisTask& operator=(const BorealisTask&) = delete;

chrome/browser/chromeos/borealis/borealis_task_unittest.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "chrome/browser/chromeos/borealis/borealis_context.h"
+#include "chrome/browser/chromeos/borealis/borealis_context_manager.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/dlcservice/fake_dlcservice_client.h"
@@ -14,18 +15,24 @@
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
+using ::testing::_;
+using ::testing::StrNe;
+
 namespace borealis {
 
 namespace {
 
 class CallbackForTesting {
  public:
-  base::OnceCallback<void(bool)> GetCallback() {
+  BorealisTask::CompletionStatusCallback GetCallback() {
     return base::BindOnce(&CallbackForTesting::Callback,
                           base::Unretained(this));
   }
 
-  MOCK_METHOD(void, Callback, (bool), ());
+  MOCK_METHOD(void,
+              Callback,
+              (BorealisContextManager::Status, std::string),
+              ());
 };
 
 class BorealisTasksTest : public testing::Test {
@@ -33,6 +40,10 @@ class BorealisTasksTest : public testing::Test {
   BorealisTasksTest() = default;
   ~BorealisTasksTest() override = default;
 
+  // Disallow copy and assign.
+  BorealisTasksTest(const BorealisTasksTest&) = delete;
+  BorealisTasksTest& operator=(const BorealisTasksTest&) = delete;
+
  protected:
   void SetUp() override {
     chromeos::DBusThreadManager::Initialize();
@@ -67,10 +78,6 @@ class BorealisTasksTest : public testing::Test {
     profile_builder.SetProfileName("defaultprofile");
     profile_ = profile_builder.Build();
   }
-
-  // Disallow copy and assign.
-  BorealisTasksTest(const BorealisTasksTest&) = delete;
-  BorealisTasksTest& operator=(const BorealisTasksTest&) = delete;
 };
 
 TEST_F(BorealisTasksTest, MountDlcSucceedsAndCallbackRanWithResults) {
@@ -79,7 +86,7 @@ TEST_F(BorealisTasksTest, MountDlcSucceedsAndCallbackRanWithResults) {
   EXPECT_EQ(context_->root_path(), "");
 
   testing::StrictMock<CallbackForTesting> callback;
-  EXPECT_CALL(callback, Callback(true));
+  EXPECT_CALL(callback, Callback(BorealisContextManager::kSuccess, _));
 
   MountDlc task;
   task.Run(context_, callback.GetCallback());
@@ -97,7 +104,7 @@ TEST_F(BorealisTasksTest, CreateDiskSucceedsAndCallbackRanWithResults) {
   EXPECT_EQ(context_->disk_path(), base::FilePath());
 
   testing::StrictMock<CallbackForTesting> callback;
-  EXPECT_CALL(callback, Callback(true));
+  EXPECT_CALL(callback, Callback(BorealisContextManager::kSuccess, _));
 
   CreateDiskImage task;
   task.Run(context_, callback.GetCallback());
@@ -117,7 +124,7 @@ TEST_F(BorealisTasksTest,
   EXPECT_EQ(context_->disk_path(), base::FilePath());
 
   testing::StrictMock<CallbackForTesting> callback;
-  EXPECT_CALL(callback, Callback(true));
+  EXPECT_CALL(callback, Callback(BorealisContextManager::kSuccess, _));
 
   CreateDiskImage task;
   task.Run(context_, callback.GetCallback());
@@ -133,7 +140,7 @@ TEST_F(BorealisTasksTest, StartBorealisVmSucceedsAndCallbackRanWithResults) {
   fake_concierge_client_->set_start_vm_response(std::move(response));
 
   testing::StrictMock<CallbackForTesting> callback;
-  EXPECT_CALL(callback, Callback(true));
+  EXPECT_CALL(callback, Callback(BorealisContextManager::kSuccess, _));
 
   StartBorealisVm task;
   task.Run(context_, callback.GetCallback());
@@ -149,7 +156,7 @@ TEST_F(BorealisTasksTest,
   fake_concierge_client_->set_start_vm_response(std::move(response));
 
   testing::StrictMock<CallbackForTesting> callback;
-  EXPECT_CALL(callback, Callback(true));
+  EXPECT_CALL(callback, Callback(BorealisContextManager::kSuccess, _));
 
   StartBorealisVm task;
   task.Run(context_, callback.GetCallback());
@@ -164,7 +171,8 @@ class BorealisTasksTestDlc : public BorealisTasksTest,
 TEST_P(BorealisTasksTestDlc, MountDlcFailsAndCallbackRanWithResults) {
   fake_dlcservice_client_->set_install_error(GetParam());
   testing::StrictMock<CallbackForTesting> callback;
-  EXPECT_CALL(callback, Callback(false));
+  EXPECT_CALL(callback,
+              Callback(BorealisContextManager::kMountFailed, StrNe("")));
 
   MountDlc task;
   task.Run(context_, callback.GetCallback());
@@ -192,7 +200,8 @@ TEST_P(BorealisTasksTestDiskImage, CreateDiskFailsAndCallbackRanWithResults) {
   EXPECT_EQ(context_->disk_path(), base::FilePath());
 
   testing::StrictMock<CallbackForTesting> callback;
-  EXPECT_CALL(callback, Callback(false));
+  EXPECT_CALL(callback,
+              Callback(BorealisContextManager::kDiskImageFailed, StrNe("")));
 
   CreateDiskImage task;
   task.Run(context_, callback.GetCallback());
@@ -223,7 +232,8 @@ TEST_P(BorealisTasksTestsStartBorealisVm,
   fake_concierge_client_->set_start_vm_response(std::move(response));
 
   testing::StrictMock<CallbackForTesting> callback;
-  EXPECT_CALL(callback, Callback(false));
+  EXPECT_CALL(callback,
+              Callback(BorealisContextManager::kStartVmFailed, StrNe("")));
 
   StartBorealisVm task;
   task.Run(context_, callback.GetCallback());