Supports the ContainerStartupFailedSignal.

Concierge client's long-running StartContainer method is terminated
by one of two signals: ContainerStarted or ContainerStartupFailed.
After this change, CrostiniManager can respond to the failures as well
as the successes.

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

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -459,6 +459,12 @@ void CrostiniManager::StartContainer(std::string vm_name,
     std::move(callback).Run(ConciergeClientResult::CLIENT_ERROR);
     return;
   }
+  if (!GetConciergeClient()->IsContainerStartupFailedSignalConnected()) {
+    LOG(ERROR) << "Async call to StartContainer can't complete when signal "
+                  "is not connected.";
+    std::move(callback).Run(ConciergeClientResult::CLIENT_ERROR);
+    return;
+  }
   vm_tools::concierge::StartContainerRequest request;
   request.set_vm_name(std::move(vm_name));
   request.set_container_name(std::move(container_name));
@@ -669,6 +675,17 @@ void CrostiniManager::OnContainerStarted(
   start_container_callbacks_.erase(range.first, range.second);
 }
 
+void CrostiniManager::OnContainerStartupFailed(
+    const vm_tools::concierge::ContainerStartedSignal& signal) {
+  // Find the callbacks to call, then erase them from the map.
+  auto range = start_container_callbacks_.equal_range(
+      std::make_pair(signal.vm_name(), signal.container_name()));
+  for (auto it = range.first; it != range.second; it++) {
+    std::move(it->second).Run(ConciergeClientResult::CONTAINER_START_FAILED);
+  }
+  start_container_callbacks_.erase(range.first, range.second);
+}
+
 void CrostiniManager::OnLaunchContainerApplication(
     LaunchContainerApplicationCallback callback,
     base::Optional<vm_tools::concierge::LaunchContainerApplicationResponse>

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -162,6 +162,8 @@ class CrostiniManager : public chromeos::ConciergeClient::Observer {
   // ConciergeClient::Observer:
   void OnContainerStarted(
       const vm_tools::concierge::ContainerStartedSignal& signal) override;
+  void OnContainerStartupFailed(
+      const vm_tools::concierge::ContainerStartedSignal& signal) override;
 
   // Returns the singleton instance of CrostiniManager.
   static CrostiniManager* GetInstance();

chromeos/dbus/concierge_client.cc

@@ -31,7 +31,11 @@ class ConciergeClientImpl : public ConciergeClient {
   }
 
   bool IsContainerStartedSignalConnected() override {
-    return is_signal_connected_;
+    return is_container_started_signal_connected_;
+  }
+
+  bool IsContainerStartupFailedSignalConnected() override {
+    return is_container_startup_failed_signal_connected_;
   }
 
   void CreateDiskImage(
@@ -184,6 +188,14 @@ class ConciergeClientImpl : public ConciergeClient {
                             weak_ptr_factory_.GetWeakPtr()),
         base::BindOnce(&ConciergeClientImpl::OnSignalConnected,
                        weak_ptr_factory_.GetWeakPtr()));
+    concierge_proxy_->ConnectToSignal(
+        vm_tools::concierge::kVmConciergeInterface,
+        vm_tools::concierge::kContainerStartupFailedSignal,
+        base::BindRepeating(
+            &ConciergeClientImpl::OnContainerStartupFailedSignal,
+            weak_ptr_factory_.GetWeakPtr()),
+        base::BindOnce(&ConciergeClientImpl::OnSignalConnected,
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
  private:
@@ -222,23 +234,48 @@ class ConciergeClientImpl : public ConciergeClient {
     }
   }
 
+  void OnContainerStartupFailedSignal(dbus::Signal* signal) {
+    DCHECK_EQ(signal->GetInterface(),
+              vm_tools::concierge::kVmConciergeInterface);
+    DCHECK_EQ(signal->GetMember(),
+              vm_tools::concierge::kContainerStartupFailedSignal);
+
+    vm_tools::concierge::ContainerStartedSignal container_startup_failed_signal;
+    dbus::MessageReader reader(signal);
+    if (!reader.PopArrayOfBytesAsProto(&container_startup_failed_signal)) {
+      LOG(ERROR) << "Failed to parse proto from DBus Signal";
+      return;
+    }
+    // Tell our Observers.
+    for (auto& observer : observer_list_) {
+      observer.OnContainerStartupFailed(container_startup_failed_signal);
+    }
+  }
+
   void OnSignalConnected(const std::string& interface_name,
                          const std::string& signal_name,
                          bool is_connected) {
     DCHECK_EQ(interface_name, vm_tools::concierge::kVmConciergeInterface);
-    DCHECK_EQ(signal_name, vm_tools::concierge::kContainerStartedSignal);
     if (!is_connected) {
       LOG(ERROR)
           << "Failed to connect to Signal. Async StartContainer will not work";
     }
-    is_signal_connected_ = is_connected;
+    if (signal_name == vm_tools::concierge::kContainerStartedSignal) {
+      is_container_started_signal_connected_ = is_connected;
+    } else if (signal_name ==
+               vm_tools::concierge::kContainerStartupFailedSignal) {
+      is_container_startup_failed_signal_connected_ = is_connected;
+    } else {
+      NOTREACHED();
+    }
   }
 
   dbus::ObjectProxy* concierge_proxy_ = nullptr;
 
   base::ObserverList<Observer> observer_list_;
 
-  bool is_signal_connected_ = false;
+  bool is_container_started_signal_connected_ = false;
+  bool is_container_startup_failed_signal_connected_ = false;
 
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.

chromeos/dbus/concierge_client.h

@@ -24,6 +24,12 @@ class CHROMEOS_EXPORT ConciergeClient : public DBusClient {
     virtual void OnContainerStarted(
         const vm_tools::concierge::ContainerStartedSignal& signal) = 0;
 
+    // OnContainerStartupFailed is signaled by Concierge after the long-running
+    // container startup process's failure is detected. Note the signal protocol
+    // buffer type is the same as in OnContainerStarted.
+    virtual void OnContainerStartupFailed(
+        const vm_tools::concierge::ContainerStartedSignal& signal) = 0;
+
    protected:
     virtual ~Observer() = default;
   };
@@ -38,6 +44,10 @@ class CHROMEOS_EXPORT ConciergeClient : public DBusClient {
   // is called.
   virtual bool IsContainerStartedSignalConnected() = 0;
 
+  // IsContainerStartupFailedSignalConnected must return true before
+  // StartContainer is called.
+  virtual bool IsContainerStartupFailedSignalConnected() = 0;
+
   // Creates a disk image for the Termina VM.
   // |callback| is called after the method call finishes.
   virtual void CreateDiskImage(

chromeos/dbus/fake_concierge_client.cc

@@ -23,7 +23,12 @@ void FakeConciergeClient::RemoveObserver(Observer* observer) {
 
 // ConciergeClient override.
 bool FakeConciergeClient::IsContainerStartedSignalConnected() {
-  return is_signal_connected_;
+  return is_container_started_signal_connected_;
+}
+
+// ConciergeClient override.
+bool FakeConciergeClient::IsContainerStartupFailedSignalConnected() {
+  return is_container_startup_failed_signal_connected_;
 }
 
 void FakeConciergeClient::CreateDiskImage(

chromeos/dbus/fake_concierge_client.h

@@ -26,6 +26,10 @@ class CHROMEOS_EXPORT FakeConciergeClient : public ConciergeClient {
   // is called.
   bool IsContainerStartedSignalConnected() override;
 
+  // IsContainerStartupFailedSignalConnected must return true before
+  // StartContainer is called.
+  bool IsContainerStartupFailedSignalConnected() override;
+
   // Fake version of the method that creates a disk image for the Termina VM.
   // Sets create_disk_image_called. |callback| is called after the method
   // call finishes.
@@ -88,7 +92,11 @@ class CHROMEOS_EXPORT FakeConciergeClient : public ConciergeClient {
   bool start_container_called() const { return start_container_called_; }
   // Set ContainerStartedSignalConnected state
   void set_container_started_signal_connected(bool connected) {
-    is_signal_connected_ = connected;
+    is_container_started_signal_connected_ = connected;
+  }
+  // Set ContainerStartedSignalConnected state
+  void set_container_startup_failed_signal_connected(bool connected) {
+    is_container_startup_failed_signal_connected_ = connected;
   }
 
  protected:
@@ -100,7 +108,8 @@ class CHROMEOS_EXPORT FakeConciergeClient : public ConciergeClient {
   bool start_termina_vm_called_ = false;
   bool stop_vm_called_ = false;
   bool start_container_called_ = false;
-  bool is_signal_connected_ = true;
+  bool is_container_started_signal_connected_ = true;
+  bool is_container_startup_failed_signal_connected_ = true;
   base::ObserverList<Observer> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeConciergeClient);