crostini: Wire up VmStopping signal

Instead of relying in Chrome to tell CrostiniManager when a VM is
stopping (so we don't know when it's vmc or otherwise triggered, let
Concierge tell us as the authority. We still note it on StopVm calls as
that lets us know slightly (1 dbus call) sooner.

Bug: b:245410022
Test: CQ
Change-Id: I6709efb8f12d2c2b3aab1cc835e90f6ff45004c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4223851
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Fergus Dall <sidereal@google.com>
Commit-Queue: David Munro <davidmunro@google.com>
Cr-Commit-Position: refs/heads/main@{#1103214}

chrome/browser/ash/crostini/crostini_manager.cc

@@ -2768,12 +2768,25 @@ void CrostiniManager::OnVmStopped(
   if (signal.owner_id() != owner_id_)
     return;
   if (running_vms_.find(signal.name()) == running_vms_.end()) {
-    LOG(ERROR) << "Ignoring VmStopped for " << signal.name();
+    LOG(WARNING) << "Ignoring VmStopped for " << signal.name();
     return;
   }
   OnVmStoppedCleanup(signal.name());
 }
 
+void CrostiniManager::OnVmStopping(
+    const vm_tools::concierge::VmStoppingSignal& signal) {
+  if (signal.owner_id() != owner_id_) {
+    return;
+  }
+  auto iter = running_vms_.find(signal.name());
+  if (iter == running_vms_.end()) {
+    LOG(WARNING) << "Ignoring VmStopping for " << signal.name();
+    return;
+  }
+  iter->second.state = VmState::STOPPING;
+}
+
 void CrostiniManager::OnContainerShutdown(
     const vm_tools::cicerone::ContainerShutdownSignal& signal) {
   if (signal.owner_id() != owner_id_)

chrome/browser/ash/crostini/crostini_manager.h

@@ -519,6 +519,8 @@ class CrostiniManager : public KeyedService,
   // ConciergeClient::VmObserver:
   void OnVmStarted(const vm_tools::concierge::VmStartedSignal& signal) override;
   void OnVmStopped(const vm_tools::concierge::VmStoppedSignal& signal) override;
+  void OnVmStopping(
+      const vm_tools::concierge::VmStoppingSignal& signal) override;
 
   // CiceroneClient::Observer:
   void OnContainerStarted(

chrome/browser/ash/guest_os/guest_os_session_tracker.cc

@@ -155,6 +155,10 @@ bool GuestOsSessionTracker::IsRunning(const GuestId& id) {
   return guests_.contains(id);
 }
 
+bool GuestOsSessionTracker::IsVmStopping(const std::string& vm_name) {
+  return stopping_vms_.contains(vm_name);
+}
+
 // ash::ConciergeClient::VmObserver overrides.
 void GuestOsSessionTracker::OnVmStarted(
     const vm_tools::concierge::VmStartedSignal& signal) {
@@ -172,6 +176,7 @@ void GuestOsSessionTracker::OnVmStopped(
     return;
   }
   vms_.erase(signal.name());
+  stopping_vms_.erase(signal.name());
   std::vector<GuestId> ids;
   for (const auto& pair : guests_) {
     if (pair.first.vm_name != signal.name()) {
@@ -184,6 +189,11 @@ void GuestOsSessionTracker::OnVmStopped(
   }
 }
 
+void GuestOsSessionTracker::OnVmStopping(
+    const vm_tools::concierge::VmStoppingSignal& signal) {
+  stopping_vms_.insert(signal.name());
+}
+
 // ash::CiceroneClient::Observer overrides.
 void GuestOsSessionTracker::OnContainerStarted(
     const vm_tools::cicerone::ContainerStartedSignal& signal) {

chrome/browser/ash/guest_os/guest_os_session_tracker.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_ASH_GUEST_OS_GUEST_OS_SESSION_TRACKER_H_
 
 #include "base/callback_list.h"
+#include "base/containers/flat_set.h"
 #include "base/files/file_path.h"
 #include "base/functional/callback_forward.h"
 #include "chrome/browser/ash/guest_os/guest_id.h"
@@ -82,6 +83,10 @@ class GuestOsSessionTracker : protected ash::ConciergeClient::VmObserver,
   // Returns true if a guest is running, false otherwise.
   bool IsRunning(const GuestId& id);
 
+  // Returns true if a VM is known to be shutting down, false for running or
+  // stopped.
+  bool IsVmStopping(const std::string& vm_name);
+
   void AddGuestForTesting(const GuestId& id,
                           const std::string& token = "test_token");
   void AddGuestForTesting(const GuestId& id,
@@ -96,6 +101,8 @@ class GuestOsSessionTracker : protected ash::ConciergeClient::VmObserver,
   // ash::ConciergeClient::VmObserver overrides.
   void OnVmStarted(const vm_tools::concierge::VmStartedSignal& signal) override;
   void OnVmStopped(const vm_tools::concierge::VmStoppedSignal& signal) override;
+  void OnVmStopping(
+      const vm_tools::concierge::VmStoppingSignal& signal) override;
 
   // ash::CiceroneClient::Observer overrides.
   void OnContainerStarted(
@@ -127,6 +134,7 @@ class GuestOsSessionTracker : protected ash::ConciergeClient::VmObserver,
                                const std::string& container_name);
   std::string owner_id_;
   base::flat_map<std::string, vm_tools::concierge::VmInfo> vms_;
+  base::flat_set<std::string> stopping_vms_;
   base::flat_map<GuestId, GuestInfo> guests_;
   base::flat_map<std::string, GuestId> tokens_to_guests_;
 

chrome/browser/ash/guest_os/guest_os_session_tracker_unittest.cc

@@ -322,4 +322,15 @@ TEST_F(GuestOsSessionTrackerTest, GetGuestIdForToken) {
       container_shutdown_signal_);
   ASSERT_EQ(absl::nullopt, tracker_.GetGuestIdForToken(token_));
 }
+
+TEST_F(GuestOsSessionTrackerTest, IsVmStopping) {
+  vm_tools::concierge::VmStoppingSignal signal;
+  signal.set_name("vm_name");
+  FakeConciergeClient()->NotifyVmStopping(signal);
+  ASSERT_TRUE(tracker_.IsVmStopping(signal.name()));
+
+  FakeConciergeClient()->NotifyVmStopped(vm_shutdown_signal_);
+  ASSERT_FALSE(tracker_.IsVmStopping(signal.name()));
+}
+
 }  // namespace guest_os

chromeos/ash/components/dbus/concierge/concierge_client.cc

@@ -74,6 +74,10 @@ class ConciergeClientImpl : public ConciergeClient {
     return is_vm_stopped_signal_connected_;
   }
 
+  bool IsVmStoppingSignalConnected() override {
+    return is_vm_stopping_signal_connected_;
+  }
+
   bool IsDiskImageProgressSignalConnected() override {
     return is_disk_import_progress_signal_connected_;
   }
@@ -306,6 +310,12 @@ class ConciergeClientImpl : public ConciergeClient {
                             weak_ptr_factory_.GetWeakPtr()),
         base::BindOnce(&ConciergeClientImpl::OnSignalConnected,
                        weak_ptr_factory_.GetWeakPtr()));
+    concierge_proxy_->ConnectToSignal(
+        concierge::kVmConciergeInterface, concierge::kVmStoppingSignal,
+        base::BindRepeating(&ConciergeClientImpl::OnVmStoppingSignal,
+                            weak_ptr_factory_.GetWeakPtr()),
+        base::BindOnce(&ConciergeClientImpl::OnSignalConnected,
+                       weak_ptr_factory_.GetWeakPtr()));
     concierge_proxy_->ConnectToSignal(
         concierge::kVmConciergeInterface, concierge::kDiskImageProgressSignal,
         base::BindRepeating(&ConciergeClientImpl::OnDiskImageProgress,
@@ -421,6 +431,22 @@ class ConciergeClientImpl : public ConciergeClient {
       observer.OnVmStopped(vm_stopped_signal);
   }
 
+  void OnVmStoppingSignal(dbus::Signal* signal) {
+    DCHECK_EQ(signal->GetInterface(), concierge::kVmConciergeInterface);
+    DCHECK_EQ(signal->GetMember(), concierge::kVmStoppingSignal);
+
+    concierge::VmStoppingSignal vm_stopping_signal;
+    dbus::MessageReader reader(signal);
+    if (!reader.PopArrayOfBytesAsProto(&vm_stopping_signal)) {
+      LOG(ERROR) << "Failed to parse proto from DBus Signal";
+      return;
+    }
+
+    for (auto& observer : vm_observer_list_) {
+      observer.OnVmStopping(vm_stopping_signal);
+    }
+  }
+
   void OnDiskImageProgress(dbus::Signal* signal) {
     DCHECK_EQ(signal->GetInterface(), concierge::kVmConciergeInterface);
     DCHECK_EQ(signal->GetMember(), concierge::kDiskImageProgressSignal);
@@ -450,6 +476,8 @@ class ConciergeClientImpl : public ConciergeClient {
       is_vm_stopped_signal_connected_ = is_connected;
     } else if (signal_name == concierge::kDiskImageProgressSignal) {
       is_disk_import_progress_signal_connected_ = is_connected;
+    } else if (signal_name == concierge::kVmStoppingSignal) {
+      is_vm_stopping_signal_connected_ = is_connected;
     } else {
       NOTREACHED();
     }
@@ -466,6 +494,7 @@ class ConciergeClientImpl : public ConciergeClient {
 
   bool is_vm_started_signal_connected_ = false;
   bool is_vm_stopped_signal_connected_ = false;
+  bool is_vm_stopping_signal_connected_ = false;
   bool is_disk_import_progress_signal_connected_ = false;
 
   // Note: This should remain the last member so it'll be destroyed and

chromeos/ash/components/dbus/concierge/concierge_client.h

@@ -46,6 +46,10 @@ class COMPONENT_EXPORT(CONCIERGE) ConciergeClient
     virtual void OnVmStopped(
         const vm_tools::concierge::VmStoppedSignal& signal) = 0;
 
+    // OnVmStopping is signaled by Concierge when a VM is stopping.
+    virtual void OnVmStopping(
+        const vm_tools::concierge::VmStoppingSignal& signal) {}
+
    protected:
     virtual ~VmObserver() = default;
   };
@@ -82,10 +86,11 @@ class COMPONENT_EXPORT(CONCIERGE) ConciergeClient
   // Adds an observer for disk image operations.
   virtual void RemoveDiskImageObserver(DiskImageObserver* observer) = 0;
 
-  // IsVmSartedSignalConnected and IsVmStoppedSignalConnected must return true
+  // IsVmStartedSignalConnected and IsVmStoppedSignalConnected must return true
   // before RestartCrostini is called.
   virtual bool IsVmStartedSignalConnected() = 0;
   virtual bool IsVmStoppedSignalConnected() = 0;
+  virtual bool IsVmStoppingSignalConnected() = 0;
 
   // IsDiskImageProgressSignalConnected must return true before
   // ImportDiskImage is called.

chromeos/ash/components/dbus/concierge/fake_concierge_client.cc

@@ -81,6 +81,10 @@ bool FakeConciergeClient::IsVmStoppedSignalConnected() {
   return is_vm_stopped_signal_connected_;
 }
 
+bool FakeConciergeClient::IsVmStoppingSignalConnected() {
+  return is_vm_stopping_signal_connected_;
+}
+
 bool FakeConciergeClient::IsDiskImageProgressSignalConnected() {
   return is_disk_image_progress_signal_connected_;
 }
@@ -413,12 +417,19 @@ void FakeConciergeClient::NotifyVmStarted(
 
 void FakeConciergeClient::NotifyVmStopped(
     const vm_tools::concierge::VmStoppedSignal& signal) {
-  // Now GetVmInfo can return success.
+  // Now GetVmInfo can no longer succeed.
   get_vm_info_response_->set_success(false);
   for (auto& observer : vm_observer_list_)
     observer.OnVmStopped(signal);
 }
 
+void FakeConciergeClient::NotifyVmStopping(
+    const vm_tools::concierge::VmStoppingSignal& signal) {
+  for (auto& observer : vm_observer_list_) {
+    observer.OnVmStopping(signal);
+  }
+}
+
 bool FakeConciergeClient::HasVmObservers() const {
   return !vm_observer_list_.empty();
 }

chromeos/ash/components/dbus/concierge/fake_concierge_client.h

@@ -37,6 +37,7 @@ class COMPONENT_EXPORT(CONCIERGE) FakeConciergeClient : public ConciergeClient {
 
   bool IsVmStartedSignalConnected() override;
   bool IsVmStoppedSignalConnected() override;
+  bool IsVmStoppingSignalConnected() override;
   bool IsDiskImageProgressSignalConnected() override;
   void CreateDiskImage(
       const vm_tools::concierge::CreateDiskImageRequest& request,
@@ -210,6 +211,9 @@ class COMPONENT_EXPORT(CONCIERGE) FakeConciergeClient : public ConciergeClient {
   void set_vm_stopped_signal_connected(bool connected) {
     is_vm_stopped_signal_connected_ = connected;
   }
+  void set_vm_stopping_signal_connected(bool connected) {
+    is_vm_stopping_signal_connected_ = connected;
+  }
   void set_disk_image_progress_signal_connected(bool connected) {
     is_disk_image_progress_signal_connected_ = connected;
   }
@@ -333,6 +337,7 @@ class COMPONENT_EXPORT(CONCIERGE) FakeConciergeClient : public ConciergeClient {
 
   void NotifyVmStarted(const vm_tools::concierge::VmStartedSignal& signal);
   void NotifyVmStopped(const vm_tools::concierge::VmStoppedSignal& signal);
+  void NotifyVmStopping(const vm_tools::concierge::VmStoppingSignal& signal);
   bool HasVmObservers() const;
 
   void NotifyConciergeStopped();
@@ -383,6 +388,7 @@ class COMPONENT_EXPORT(CONCIERGE) FakeConciergeClient : public ConciergeClient {
 
   bool is_vm_started_signal_connected_ = true;
   bool is_vm_stopped_signal_connected_ = true;
+  bool is_vm_stopping_signal_connected_ = true;
   bool is_disk_image_progress_signal_connected_ = true;
 
   bool wait_for_service_to_be_available_response_ = true;