Remove shared ownership of CrostiniRestarter

It seems to be possible for shared references to CrostiniRestarter to
outlive CrostiniManager. Since all of CrostiniRestarter assumes that
CrostiniManager still exists, this can cause crashes. Switch
CrostiniRestarter to being exclusivly owned by CrostiniManager and
handing out weak pointers to callbacks.

Bug: 1040797
Change-Id: Ib65c79c7c81111aad88435b3c95da0c64226e791
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1996112Reviewed-by: Nic Hollingum <hollingum@google.com>
Commit-Queue: Fergus Dall <sidereal@google.com>
Cr-Commit-Position: refs/heads/master@{#733496}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -127,8 +127,7 @@ CrostiniManager::RestartOptions& CrostiniManager::RestartOptions::operator=(
     RestartOptions&&) = default;
 
 class CrostiniManager::CrostiniRestarter
-    : public base::RefCountedThreadSafe<CrostiniRestarter>,
-      public crostini::VmShutdownObserver,
+    : public crostini::VmShutdownObserver,
       public chromeos::disks::DiskMountManager::Observer,
       public chromeos::SchedulerConfigurationManagerBase::Observer {
  public:
@@ -167,13 +166,15 @@ class CrostiniManager::CrostiniRestarter
       base::PostTask(
           FROM_HERE, {content::BrowserThread::UI},
           base::BindOnce(&CrostiniRestarter::StartLxdContainerFinished,
-                         base::WrapRefCounted(this), CrostiniResult::SUCCESS));
+                         weak_ptr_factory_.GetWeakPtr(),
+                         CrostiniResult::SUCCESS));
       return;
     }
 
     StartStage(mojom::InstallerState::kInstallImageLoader);
-    crostini_manager_->InstallTerminaComponent(base::BindOnce(
-        &CrostiniRestarter::LoadComponentFinished, base::WrapRefCounted(this)));
+    crostini_manager_->InstallTerminaComponent(
+        base::BindOnce(&CrostiniRestarter::LoadComponentFinished,
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
   void AddObserver(CrostiniManager::RestartObserver* observer) {
@@ -227,9 +228,6 @@ class CrostiniManager::CrostiniRestarter
   bool is_aborted() const { return is_aborted_; }
   CrostiniResult result_ = CrostiniResult::NEVER_FINISHED;
 
- private:
-  friend class base::RefCountedThreadSafe<CrostiniRestarter>;
-
   ~CrostiniRestarter() override {
     // Do not record results if this restart was triggered by the installer.
     // The crostini installer has its own histograms that should be kept
@@ -246,6 +244,7 @@ class CrostiniManager::CrostiniRestarter
       mount_manager->RemoveObserver(this);
   }
 
+ private:
   void StartStage(mojom::InstallerState stage) {
     for (auto& observer : observer_list_) {
       observer.OnStageStarted(stage);
@@ -274,8 +273,8 @@ class CrostiniManager::CrostiniRestarter
     // Set the pref here, after we first successfully install something
     profile_->GetPrefs()->SetBoolean(crostini::prefs::kCrostiniEnabled, true);
     StartStage(mojom::InstallerState::kStartConcierge);
-    crostini_manager_->StartConcierge(
-        base::BindOnce(&CrostiniRestarter::ConciergeStarted, this));
+    crostini_manager_->StartConcierge(base::BindOnce(
+        &CrostiniRestarter::ConciergeStarted, weak_ptr_factory_.GetWeakPtr()));
   }
 
   void ConciergeStarted(bool is_started) {
@@ -300,8 +299,8 @@ class CrostiniManager::CrostiniRestarter
         base::FilePath(vm_name_),
         vm_tools::concierge::StorageLocation::STORAGE_CRYPTOHOME_ROOT,
         disk_size_available,
-        base::BindOnce(&CrostiniRestarter::CreateDiskImageFinished, this,
-                       disk_size_available));
+        base::BindOnce(&CrostiniRestarter::CreateDiskImageFinished,
+                       weak_ptr_factory_.GetWeakPtr(), disk_size_available));
   }
 
   void CreateDiskImageFinished(int64_t disk_size_available,
@@ -347,7 +346,8 @@ class CrostiniManager::CrostiniRestarter
     StartStage(mojom::InstallerState::kStartTerminaVm);
     crostini_manager_->StartTerminaVm(
         vm_name_, disk_path_, num_cores_disabled,
-        base::BindOnce(&CrostiniRestarter::StartTerminaVmFinished, this));
+        base::BindOnce(&CrostiniRestarter::StartTerminaVmFinished,
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
   void StartTerminaVmFinished(bool success) {
@@ -368,13 +368,15 @@ class CrostiniManager::CrostiniRestarter
             crostini::prefs::kReportCrostiniUsageEnabled) &&
         vm_name_ == kCrostiniDefaultVmName &&
         container_name_ == kCrostiniDefaultContainerName) {
-      crostini_manager_->GetTerminaVmKernelVersion(base::BindOnce(
-          &CrostiniRestarter::GetTerminaVmKernelVersionFinished, this));
+      crostini_manager_->GetTerminaVmKernelVersion(
+          base::BindOnce(&CrostiniRestarter::GetTerminaVmKernelVersionFinished,
+                         weak_ptr_factory_.GetWeakPtr()));
     }
     StartStage(mojom::InstallerState::kCreateContainer);
     crostini_manager_->CreateLxdContainer(
         vm_name_, container_name_,
-        base::BindOnce(&CrostiniRestarter::CreateLxdContainerFinished, this));
+        base::BindOnce(&CrostiniRestarter::CreateLxdContainerFinished,
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
   void GetTerminaVmKernelVersionFinished(
@@ -410,7 +412,7 @@ class CrostiniManager::CrostiniRestarter
         options_.container_username.value_or(
             DefaultContainerUserNameForProfile(profile_)),
         base::BindOnce(&CrostiniRestarter::SetUpLxdContainerUserFinished,
-                       this));
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
   void SetUpLxdContainerUserFinished(bool success) {
@@ -430,7 +432,8 @@ class CrostiniManager::CrostiniRestarter
     StartStage(mojom::InstallerState::kStartContainer);
     crostini_manager_->StartLxdContainer(
         vm_name_, container_name_,
-        base::BindOnce(&CrostiniRestarter::StartLxdContainerFinished, this));
+        base::BindOnce(&CrostiniRestarter::StartLxdContainerFinished,
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
   void StartLxdContainerFinished(CrostiniResult result) {
@@ -468,8 +471,8 @@ class CrostiniManager::CrostiniRestarter
       StartStage(mojom::InstallerState::kFetchSshKeys);
       crostini_manager_->GetContainerSshKeys(
           vm_name_, container_name_,
-          base::BindOnce(&CrostiniRestarter::GetContainerSshKeysFinished, this,
-                         info->username));
+          base::BindOnce(&CrostiniRestarter::GetContainerSshKeysFinished,
+                         weak_ptr_factory_.GetWeakPtr(), info->username));
     } else {
       FinishRestart(result);
     }
@@ -583,6 +586,8 @@ class CrostiniManager::CrostiniRestarter
   bool is_running_ = false;
 
   static CrostiniManager::RestartId next_restart_id_;
+
+  base::WeakPtrFactory<CrostiniRestarter> weak_ptr_factory_{this};
 };
 
 CrostiniManager::RestartId
@@ -1848,20 +1853,25 @@ CrostiniManager::RestartId CrostiniManager::RestartCrostiniWithOptions(
     return kUninitializedRestartId;
   }
 
-  auto restarter = base::MakeRefCounted<CrostiniRestarter>(
+  auto restarter = std::make_unique<CrostiniRestarter>(
       profile_, this, std::move(vm_name), std::move(container_name),
       std::move(options), std::move(callback));
+  auto restart_id = restarter->restart_id();
   if (observer)
     restarter->AddObserver(observer);
   auto key = ContainerId(restarter->vm_name(), restarter->container_name());
-  restarters_by_container_.emplace(key, restarter->restart_id());
-  restarters_by_id_[restarter->restart_id()] = restarter;
+  restarters_by_container_.emplace(key, restart_id);
+  restarters_by_id_[restart_id] = std::move(restarter);
   if (restarters_by_container_.count(key) > 1) {
     VLOG(1) << "Already restarting " << key;
   } else {
-    restarter->Restart();
+    // ::Restart needs to be called after the restarter is inserted into
+    // restarters_by_id_ because some tests will make the restart process
+    // complete before ::Restart returns.
+    restarters_by_id_[restart_id]->Restart();
   }
-  return restarter->restart_id();
+
+  return restart_id;
 }
 
 void CrostiniManager::AbortRestartCrostini(
@@ -1901,8 +1911,7 @@ void CrostiniManager::OnAbortRestartCrostini(
   // Kick off the "next" (in no order) pending Restart() if any.
   auto pending_it = restarters_by_container_.find(key);
   if (pending_it != restarters_by_container_.end()) {
-    auto restarter = restarters_by_id_[pending_it->second];
-    restarter->Restart();
+    restarters_by_id_[pending_it->second]->Restart();
   }
 
   std::move(callback).Run();
@@ -2814,7 +2823,7 @@ void CrostiniManager::RemoveCrostini(std::string vm_name,
           },
           crostini_remover));
 
-  for (auto restarter_it : restarters_by_id_) {
+  for (const auto& restarter_it : restarters_by_id_) {
     AbortRestartCrostini(restarter_it.first, abort_callback);
   }
 }
@@ -2830,11 +2839,11 @@ void CrostiniManager::FinishRestart(CrostiniRestarter* restarter,
                                     CrostiniResult result) {
   auto key = ContainerId(restarter->vm_name(), restarter->container_name());
   auto range = restarters_by_container_.equal_range(key);
-  std::vector<scoped_refptr<CrostiniRestarter>> pending_restarters;
+  std::vector<std::unique_ptr<CrostiniRestarter>> pending_restarters;
   // Erase first, because restarter->RunCallback() may modify our maps.
   for (auto it = range.first; it != range.second; ++it) {
     CrostiniManager::RestartId restart_id = it->second;
-    pending_restarters.emplace_back(restarters_by_id_[restart_id]);
+    pending_restarters.emplace_back(std::move(restarters_by_id_[restart_id]));
     restarters_by_id_.erase(restart_id);
   }
   restarters_by_container_.erase(range.first, range.second);

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -848,7 +848,7 @@ class CrostiniManager : public KeyedService,
   std::multimap<ContainerId, CrostiniManager::RestartId>
       restarters_by_container_;
 
-  std::map<CrostiniManager::RestartId, scoped_refptr<CrostiniRestarter>>
+  std::map<CrostiniManager::RestartId, std::unique_ptr<CrostiniRestarter>>
       restarters_by_id_;
 
   // True when the installer dialog is showing. At that point, it is invalid