Reland "GuestOsSharePath mark paths no longer shared on VM shutdown"

This reverts commit 476e192f.

Reason for revert: reland with fixed test

Original change's description:
> Revert "GuestOsSharePath mark paths no longer shared on VM shutdown"
>
> This reverts commit 56a15a3f.
>
> Reason for revert: Fails unit tests on linux-chromeos-rel. See
> https://crbug.com/1158303 for details.
>
> Original change's description:
> > GuestOsSharePath mark paths no longer shared on VM shutdown
> >
> > Bug: 1144138
> > Change-Id: I970293c61c9686ab32f574957c028bd3db754ccd
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2589049
> > Auto-Submit: Joel Hockey <joelhockey@chromium.org>
> > Commit-Queue: Jason Lin <lxj@google.com>
> > Reviewed-by: Jason Lin <lxj@google.com>
> > Cr-Commit-Position: refs/heads/master@{#836531}
>
> TBR=joelhockey@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,lxj@google.com
>
> Change-Id: I4ba6d44fa5b448a4d6899a69dd5848a103a5614c
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1144138, 1158303
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2588930
> Reviewed-by: Kush Sinha <sinhak@chromium.org>
> Commit-Queue: Kush Sinha <sinhak@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#836575}

TBR=joelhockey@chromium.org,sinhak@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,lxj@google.com

# Not skipping CQ checks because this is a reland.

Bug: 1144138
Bug: 1158303
Change-Id: I1e53443a212950ac2d66bb736ce3350798d95201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2591027Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836853}

chrome/browser/chromeos/file_manager/path_util_unittest.cc

@@ -735,6 +735,7 @@ TEST_F(FileManagerPathUtilConvertUrlTest, ConvertPathToArcUrl_MyDriveLegacy) {
 }
 
 TEST_F(FileManagerPathUtilConvertUrlTest, ConvertPathToArcUrl_MyDriveArcvm) {
+  chromeos::DBusThreadManager::Initialize();
   auto* command_line = base::CommandLine::ForCurrentProcess();
   command_line->InitFromArgv({"", "--enable-arcvm"});
   EXPECT_TRUE(arc::IsArcVmEnabled());

chrome/browser/chromeos/guest_os/guest_os_share_path.cc

@@ -10,7 +10,6 @@
 #include "base/optional.h"
 #include "base/task/thread_pool.h"
 #include "chrome/browser/chromeos/arc/session/arc_session_manager.h"
-#include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/drive/drive_integration_service.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
@@ -184,6 +183,9 @@ GuestOsSharePath::GuestOsSharePath(Profile* profile)
       file_watcher_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
           {base::MayBlock(), base::TaskPriority::USER_VISIBLE})),
       seneschal_callback_(base::BindRepeating(LogErrorResult)) {
+  auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile_);
+  crostini_manager->AddVmShutdownObserver(this);
+
   if (auto* vmgr = file_manager::VolumeManager::Get(profile_)) {
     vmgr->AddObserver(this);
   }
@@ -202,6 +204,9 @@ GuestOsSharePath::~GuestOsSharePath() = default;
 
 void GuestOsSharePath::Shutdown() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile_);
+  crostini_manager->RemoveVmShutdownObserver(this);
+
   for (auto& shared_path : shared_paths_) {
     if (shared_path.second.watcher) {
       file_watcher_task_runner_->DeleteSoon(
@@ -480,12 +485,7 @@ void GuestOsSharePath::UnsharePath(const std::string& vm_name,
                                    SuccessCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (auto* info = FindSharedPathInfo(path)) {
-    info->vm_names.erase(vm_name);
-    if (info->vm_names.empty()) {
-      if (info->watcher) {
-        file_watcher_task_runner_->DeleteSoon(FROM_HERE,
-                                              info->watcher.release());
-      }
+    if (RemoveSharedPathInfo(*info, vm_name)) {
       shared_paths_.erase(path);
     }
   }
@@ -589,6 +589,17 @@ bool GuestOsSharePath::IsPathShared(const std::string& vm_name,
   }
 }
 
+void GuestOsSharePath::OnVmShutdown(const std::string& vm_name) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  for (auto it = shared_paths_.begin(); it != shared_paths_.end();) {
+    if (RemoveSharedPathInfo(it->second, vm_name)) {
+      shared_paths_.erase(it++);
+    } else {
+      ++it;
+    }
+  }
+}
+
 void GuestOsSharePath::OnVolumeMounted(chromeos::MountError error_code,
                                        const file_manager::Volume& volume) {
   if (error_code != chromeos::MountError::MOUNT_ERROR_NONE) {
@@ -756,4 +767,16 @@ SharedPathInfo* GuestOsSharePath::FindSharedPathInfo(
   return &it->second;
 }
 
+bool GuestOsSharePath::RemoveSharedPathInfo(SharedPathInfo& info,
+                                            const std::string& vm_name) {
+  info.vm_names.erase(vm_name);
+  if (info.vm_names.empty()) {
+    if (info.watcher) {
+      file_watcher_task_runner_->DeleteSoon(FROM_HERE, info.watcher.release());
+    }
+    return true;
+  }
+  return false;
+}
+
 }  // namespace guest_os

chrome/browser/chromeos/guest_os/guest_os_share_path.h

@@ -41,6 +41,7 @@ struct SharedPathInfo {
 // Handles sharing and unsharing paths from the Chrome OS host to guest VMs via
 // seneschal.
 class GuestOsSharePath : public KeyedService,
+                         public chromeos::VmShutdownObserver,
                          public file_manager::VolumeManagerObserver,
                          public drivefs::DriveFsHostObserver {
  public:
@@ -113,6 +114,9 @@ class GuestOsSharePath : public KeyedService,
   // Returns true if |path| or a parent is shared with |vm_name|.
   bool IsPathShared(const std::string& vm_name, base::FilePath path) const;
 
+  // chromeos::VmShutdownObserver
+  void OnVmShutdown(const std::string& vm_name) override;
+
   // file_manager::VolumeManagerObserver
   void OnVolumeMounted(chromeos::MountError error_code,
                        const file_manager::Volume& volume) override;
@@ -155,6 +159,10 @@ class GuestOsSharePath : public KeyedService,
 
   // Returns info for specified path or nullptr if not found.
   SharedPathInfo* FindSharedPathInfo(const base::FilePath& path);
+  // Removes |vm_name| from |info.vm_names| if it exists, and deletes the
+  // |info.watcher| if |info.path| is not shared with any other VMs.  Returns
+  // true if path is no longer shared with any VMs.
+  bool RemoveSharedPathInfo(SharedPathInfo& info, const std::string& vm_name);
 
   Profile* profile_;
   // Task runner for FilePathWatchers to be created, run, and be destroyed on.

chrome/browser/chromeos/guest_os/guest_os_share_path_factory.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/guest_os/guest_os_share_path_factory.h"
 
+#include "chrome/browser/chromeos/crostini/crostini_manager_factory.h"
 #include "chrome/browser/chromeos/guest_os/guest_os_share_path.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -25,7 +26,9 @@ GuestOsSharePathFactory* GuestOsSharePathFactory::GetInstance() {
 GuestOsSharePathFactory::GuestOsSharePathFactory()
     : BrowserContextKeyedServiceFactory(
           "GuestOsSharePath",
-          BrowserContextDependencyManager::GetInstance()) {}
+          BrowserContextDependencyManager::GetInstance()) {
+  DependsOn(crostini::CrostiniManagerFactory::GetInstance());
+}
 
 GuestOsSharePathFactory::~GuestOsSharePathFactory() = default;
 

chrome/browser/chromeos/guest_os/guest_os_share_path_unittest.cc

@@ -1044,6 +1044,14 @@ TEST_F(GuestOsSharePathTest, IsPathShared) {
         shared_path_.Append("a/b"), shared_path_.DirName(), root_}) {
     EXPECT_FALSE(guest_os_share_path_->IsPathShared("not-shared", path));
   }
+
+  // IsPathShared should be false after VM shutdown.
+  vm_tools::concierge::VmStoppedSignal signal;
+  signal.set_name(crostini::kCrostiniDefaultVmName);
+  signal.set_owner_id("test");
+  crostini::CrostiniManager::GetForProfile(profile())->OnVmStopped(signal);
+  EXPECT_FALSE(guest_os_share_path_->IsPathShared(
+      crostini::kCrostiniDefaultVmName, shared_path_));
 }
 
 }  // namespace guest_os