New terminal works better with upgrade dialog.

Several tweaks made to upgrade flow to make it play better with the
new terminal. Since the terminal launches its window prior to
starting crostini, we move the upgrade availability check to an earlier
phase of LaunchCrostiniApp. Also, when the user decides not to upgrade,
we don't restart crostini twice. (Previously, this could happen because
the upgrade process needs to shut down crostini first).

Bug: 1024693
Change-Id: I46ee09877a7092d91894b80bc798ed99e4f855ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2108300Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751581}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -928,6 +928,8 @@ void CrostiniManager::MaybeUpdateCrostini() {
     // |component_manager| may be nullptr in unit tests.
     return;
   }
+  // This is a new user session, perhaps using an old CrostiniManager.
+  container_upgrade_prompt_shown_.clear();
   base::ThreadPool::PostTaskAndReply(
       FROM_HERE, {base::MayBlock()},
       base::BindOnce(CrostiniManager::CheckPathsAndComponents),

chrome/browser/chromeos/crostini/crostini_upgrader.cc

@@ -232,7 +232,9 @@ void CrostiniUpgrader::PowerChanged(
   // less conservative check is possible, but we can expect users to have access
   // to a charger.
   power_status_good_ = proto.battery_state() !=
-                       power_manager::PowerSupplyProperties::DISCHARGING;
+                           power_manager::PowerSupplyProperties::DISCHARGING ||
+                       proto.external_power() !=
+                           power_manager::PowerSupplyProperties::DISCONNECTED;
 
   auto* pmc = chromeos::PowerManagerClient::Get();
   pmc_observer_.Remove(pmc);

chrome/browser/chromeos/crostini/crostini_util.cc

@@ -99,21 +99,15 @@ void OnCrostiniRestarted(Profile* profile,
                          Browser* browser,
                          base::OnceClosure callback,
                          crostini::CrostiniResult result) {
-  auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile);
-
-  if (crostini_manager->ShouldPromptContainerUpgrade(container_id)) {
-    chromeos::CrostiniUpgraderDialog::Show(std::move(callback));
+  if (crostini::MaybeShowCrostiniDialogBeforeLaunch(profile, result)) {
+    VLOG(1) << "Crostini restart blocked by dialog";
     return;
   }
+
   if (result != crostini::CrostiniResult::SUCCESS) {
     OnLaunchFailed(app_id, result);
     if (browser && browser->window())
       browser->window()->Close();
-    if (result == crostini::CrostiniResult::OFFLINE_WHEN_UPGRADE_REQUIRED ||
-        result == crostini::CrostiniResult::LOAD_COMPONENT_FAILED) {
-      ShowCrostiniUpdateComponentView(profile,
-                                      crostini::CrostiniUISurface::kAppList);
-    }
     return;
   }
   std::move(callback).Run();
@@ -382,33 +376,28 @@ void AddSpinner(crostini::CrostiniManager::RestartId restart_id,
   }
 }
 
-void LaunchCrostiniApp(Profile* profile,
-                       const std::string& app_id,
-                       int64_t display_id,
-                       const std::vector<storage::FileSystemURL>& files,
-                       LaunchCrostiniAppCallback callback) {
-  // Policies can change under us, and crostini may now be forbidden.
-  if (!CrostiniFeatures::Get()->IsUIAllowed(profile)) {
-    return std::move(callback).Run(false, "Crostini UI not allowed");
+bool MaybeShowCrostiniDialogBeforeLaunch(Profile* profile,
+                                         CrostiniResult result) {
+  if (result == CrostiniResult::OFFLINE_WHEN_UPGRADE_REQUIRED ||
+      result == CrostiniResult::LOAD_COMPONENT_FAILED) {
+    ShowCrostiniUpdateComponentView(profile, CrostiniUISurface::kAppList);
+    VLOG(1) << "Update Component dialog";
+    return true;
   }
+  return false;
+}
+
+void LaunchCrostiniAppImpl(
+    Profile* profile,
+    const std::string& app_id,
+    int64_t display_id,
+    const std::vector<storage::FileSystemURL>& files,
+    base::Optional<crostini::CrostiniRegistryService::Registration>
+        registration,
+    LaunchCrostiniAppCallback callback) {
   auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile);
   crostini::CrostiniRegistryService* registry_service =
       crostini::CrostiniRegistryServiceFactory::GetForProfile(profile);
-  base::Optional<crostini::CrostiniRegistryService::Registration> registration =
-      registry_service->GetRegistration(app_id);
-  if (!registration) {
-    RecordAppLaunchHistogram(CrostiniAppLaunchAppType::kUnknownApp);
-    return std::move(callback).Run(
-        false, "LaunchCrostiniApp called with an unknown app_id: " + app_id);
-  }
-  if (crostini_manager->IsUncleanStartup()) {
-    // Prompt for user-restart.
-    if (!ShowCrostiniRecoveryView(profile,
-                                  crostini::CrostiniUISurface::kAppList, app_id,
-                                  display_id, std::move(callback))) {
-      return;
-    }
-  }
   // Store these as we move |registration| into LaunchContainerApplication().
   const std::string vm_name = registration->VmName();
   const std::string container_name = registration->ContainerName();
@@ -419,14 +408,6 @@ void LaunchCrostiniApp(Profile* profile,
     DCHECK(files.empty());
     RecordAppLaunchHistogram(CrostiniAppLaunchAppType::kTerminal);
 
-    // At this point, we know that Crostini UI is allowed.
-    if (!crostini_manager->IsCrosTerminaInstalled() ||
-        !CrostiniFeatures::Get()->IsEnabled(profile)) {
-      crostini::CrostiniInstaller::GetForProfile(profile)->ShowDialog(
-          CrostiniUISurface::kAppList);
-      return std::move(callback).Run(false, "Crostini not installed");
-    }
-
     GURL vsh_in_crosh_url = GenerateVshInCroshUrl(
         profile, vm_name, container_name, std::vector<std::string>());
 
@@ -475,6 +456,57 @@ void LaunchCrostiniApp(Profile* profile,
       base::TimeDelta::FromMilliseconds(kDelayBeforeSpinnerMs));
 }
 
+void LaunchCrostiniApp(Profile* profile,
+                       const std::string& app_id,
+                       int64_t display_id,
+                       const std::vector<storage::FileSystemURL>& files,
+                       LaunchCrostiniAppCallback callback) {
+  // Policies can change under us, and crostini may now be forbidden.
+  if (!CrostiniFeatures::Get()->IsUIAllowed(profile)) {
+    return std::move(callback).Run(false, "Crostini UI not allowed");
+  }
+  auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile);
+
+  // At this point, we know that Crostini UI is allowed.
+  if (app_id == GetTerminalId() &&
+      (!crostini_manager->IsCrosTerminaInstalled() ||
+       !CrostiniFeatures::Get()->IsEnabled(profile))) {
+    crostini::CrostiniInstaller::GetForProfile(profile)->ShowDialog(
+        CrostiniUISurface::kAppList);
+    return std::move(callback).Run(false, "Crostini not installed");
+  }
+
+  crostini::CrostiniRegistryService* registry_service =
+      crostini::CrostiniRegistryServiceFactory::GetForProfile(profile);
+  base::Optional<crostini::CrostiniRegistryService::Registration> registration =
+      registry_service->GetRegistration(app_id);
+  if (!registration) {
+    RecordAppLaunchHistogram(CrostiniAppLaunchAppType::kUnknownApp);
+    return std::move(callback).Run(
+        false, "LaunchCrostiniApp called with an unknown app_id: " + app_id);
+  }
+
+  if (crostini_manager->IsUncleanStartup()) {
+    // Prompt for user-restart.
+    if (!ShowCrostiniRecoveryView(profile,
+                                  crostini::CrostiniUISurface::kAppList, app_id,
+                                  display_id, std::move(callback))) {
+      return;
+    }
+  }
+
+  if (crostini_manager->ShouldPromptContainerUpgrade(
+          ContainerId(registration->VmName(), registration->ContainerName()))) {
+    chromeos::CrostiniUpgraderDialog::Show(
+        base::BindOnce(&LaunchCrostiniAppImpl, profile, app_id, display_id,
+                       files, std::move(registration), std::move(callback)));
+    VLOG(1) << "Upgrade dialog";
+    return;
+  }
+  LaunchCrostiniAppImpl(profile, app_id, display_id, files,
+                        std::move(registration), std::move(callback));
+}
+
 void LoadIcons(Profile* profile,
                const std::vector<std::string>& app_ids,
                int resource_size_in_dip,
@@ -648,4 +680,9 @@ std::vector<int64_t> GetTicksForDiskSize(int64_t min_size,
   return ticks;
 }
 
+const ContainerId& DefaultContainerId() {
+  static const base::NoDestructor<ContainerId> container_id(
+      kCrostiniDefaultVmName, kCrostiniDefaultContainerName);
+  return *container_id;
+}
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_util.h

@@ -13,6 +13,7 @@
 #include "base/files/file_path.h"
 #include "base/optional.h"
 #include "base/values.h"
+#include "chrome/browser/chromeos/crostini/crostini_simple_types.h"
 #include "storage/browser/file_system/file_system_url.h"
 #include "ui/base/resource/scale_factor.h"
 
@@ -71,6 +72,10 @@ bool ShouldAllowContainerUpgrade(Profile* profile);
 // the configuration specified by CrostiniAnsiblePlaybook user policy.
 bool ShouldConfigureDefaultContainer(Profile* profile);
 
+// Returns whether a dialog from Crostini is blocking the immediate launch.
+bool MaybeShowCrostiniDialogBeforeLaunch(Profile* profile,
+                                         CrostiniResult result);
+
 // Launches the Crostini app with ID of |app_id| on the display with ID of
 // |display_id|. |app_id| should be a valid Crostini app list id.
 void LaunchCrostiniApp(Profile* profile,
@@ -234,6 +239,8 @@ void UpdateContainerPref(Profile* profile,
                          const std::string& key,
                          base::Value value);
 
+const ContainerId& DefaultContainerId();
+
 }  // namespace crostini
 
 #endif  // CHROME_BROWSER_CHROMEOS_CROSTINI_CROSTINI_UTIL_H_

chrome/browser/extensions/api/terminal/terminal_private_api.cc

@@ -284,6 +284,13 @@ void TerminalPrivateOpenTerminalProcessFunction::OnCrostiniRestarted(
     int tab_id,
     const std::vector<std::string>& arguments,
     crostini::CrostiniResult result) {
+  if (crostini::MaybeShowCrostiniDialogBeforeLaunch(
+          Profile::FromBrowserContext(browser_context()), result)) {
+    const std::string msg = "Waiting for component update dialog response";
+    LOG(ERROR) << msg;
+    Respond(Error(msg));
+    return;
+  }
   startup_status->OnCrostiniRestarted(result);
   if (result == crostini::CrostiniResult::SUCCESS) {
     OpenProcess(user_id_hash, tab_id, arguments);
@@ -292,14 +299,6 @@ void TerminalPrivateOpenTerminalProcessFunction::OnCrostiniRestarted(
         base::StringPrintf("Error starting crostini for terminal: %d", result);
     LOG(ERROR) << msg;
     Respond(Error(msg));
-
-    // Special handling to update terminal component.
-    if (result == crostini::CrostiniResult::OFFLINE_WHEN_UPGRADE_REQUIRED ||
-        result == crostini::CrostiniResult::LOAD_COMPONENT_FAILED) {
-      ShowCrostiniUpdateComponentView(
-          Profile::FromBrowserContext(browser_context()),
-          crostini::CrostiniUISurface::kAppList);
-    }
   }
 }
 
@@ -328,7 +327,6 @@ void TerminalPrivateOpenTerminalProcessFunction::OpenOnRegistryTaskRunner(
   chromeos::ProcessProxyRegistry* registry =
       chromeos::ProcessProxyRegistry::Get();
   const base::CommandLine cmdline{arguments};
-
   std::string terminal_id;
   bool success = registry->OpenProcess(cmdline, user_id_hash, output_callback,
                                        &terminal_id);

chrome/browser/extensions/api/terminal/terminal_private_browsertest.cc

@@ -58,7 +58,7 @@ IN_PROC_BROWSER_TEST_F(TerminalPrivateBrowserTest, OpenTerminalProcessChecks) {
   // (LOAD_COMPONENT_FAILED), but the point of the test is to see that we get
   // past the vmshell allowed checks.
   crostini_features.set_allowed(true);
-  ExpectJsResult(script, "Error starting crostini for terminal: 22");
+  ExpectJsResult(script, "Waiting for component update dialog response");
 
   // openTerminalProcess not defined.
   ExpectJsResult("typeof chrome.terminalPrivate.openTerminalProcess",

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_dialog.cc

@@ -69,12 +69,16 @@ bool CrostiniUpgraderDialog::CanCloseDialog() const {
 }
 
 namespace {
-void RestartAndRunLaunchClosure(
-    base::WeakPtr<crostini::CrostiniManager> crostini_manager,
-    base::OnceClosure launch_closure) {
+void RunLaunchClosure(base::WeakPtr<crostini::CrostiniManager> crostini_manager,
+                      base::OnceClosure launch_closure,
+                      bool restart_required) {
   if (!crostini_manager) {
     return;
   }
+  if (!restart_required) {
+    std::move(launch_closure).Run();
+    return;
+  }
   crostini_manager->RestartCrostini(
       crostini::kCrostiniDefaultVmName, crostini::kCrostiniDefaultContainerName,
       base::BindOnce(
@@ -102,9 +106,9 @@ void CrostiniUpgraderDialog::OnDialogShown(content::WebUI* webui) {
                             crostini::kCrostiniDefaultContainerName));
 
   upgrader_ui_ = static_cast<CrostiniUpgraderUI*>(webui->GetController());
-  upgrader_ui_->set_launch_closure(base::BindOnce(
-      &RestartAndRunLaunchClosure, crostini_manager->GetWeakPtr(),
-      std::move(launch_closure_)));
+  upgrader_ui_->set_launch_callback(
+      base::BindOnce(&RunLaunchClosure, crostini_manager->GetWeakPtr(),
+                     std::move(launch_closure_)));
   return SystemWebDialogDelegate::OnDialogShown(webui);
 }
 

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_page_handler.cc

@@ -23,13 +23,13 @@ CrostiniUpgraderPageHandler::CrostiniUpgraderPageHandler(
         pending_page_handler,
     mojo::PendingRemote<chromeos::crostini_upgrader::mojom::Page> pending_page,
     base::OnceClosure close_dialog_callback,
-    base::OnceClosure launch_closure)
+    base::OnceCallback<void(bool)> launch_callback)
     : web_contents_{web_contents},
       upgrader_ui_delegate_{upgrader_ui_delegate},
       receiver_{this, std::move(pending_page_handler)},
       page_{std::move(pending_page)},
       close_dialog_callback_{std::move(close_dialog_callback)},
-      launch_closure_{std::move(launch_closure)} {
+      launch_callback_{std::move(launch_callback)} {
   upgrader_ui_delegate_->AddObserver(this);
 }
 
@@ -87,18 +87,22 @@ void CrostiniUpgraderPageHandler::Cancel() {
 }
 
 void CrostiniUpgraderPageHandler::Launch() {
-  std::move(launch_closure_).Run();
+  std::move(launch_callback_).Run(true);
 }
 
 void CrostiniUpgraderPageHandler::CancelBeforeStart() {
   base::UmaHistogramEnumeration(crostini::kUpgradeDialogEventHistogram,
                                 crostini::UpgradeDialogEvent::kNotStarted);
   upgrader_ui_delegate_->CancelBeforeStart();
+  if (launch_callback_) {
+    // Running launch closure - no upgrade wanted, no need to restart crostini.
+    std::move(launch_callback_).Run(false);
+  }
 }
 
 void CrostiniUpgraderPageHandler::Close() {
-  if (launch_closure_) {
-    std::move(launch_closure_).Run();
+  if (launch_callback_) {
+    std::move(launch_callback_).Run(true);
   }
   std::move(close_dialog_callback_).Run();
 }

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_page_handler.h

@@ -33,7 +33,7 @@ class CrostiniUpgraderPageHandler
       mojo::PendingRemote<chromeos::crostini_upgrader::mojom::Page>
           pending_page,
       base::OnceClosure close_dialog_callback,
-      base::OnceClosure launch_closure);
+      base::OnceCallback<void(bool)> launch_callback);
   ~CrostiniUpgraderPageHandler() override;
 
   // chromeos::crostini_upgrader::mojom::PageHandler:
@@ -69,7 +69,7 @@ class CrostiniUpgraderPageHandler
   mojo::Receiver<chromeos::crostini_upgrader::mojom::PageHandler> receiver_;
   mojo::Remote<chromeos::crostini_upgrader::mojom::Page> page_;
   base::OnceClosure close_dialog_callback_;
-  base::OnceClosure launch_closure_;
+  base::OnceCallback<void(bool)> launch_callback_;
 
   base::WeakPtrFactory<CrostiniUpgraderPageHandler> weak_ptr_factory_{this};
 

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_ui.cc

@@ -144,7 +144,7 @@ void CrostiniUpgraderUI::CreatePageHandler(
       // |this|.
       base::BindOnce(&CrostiniUpgraderUI::OnWebUICloseDialog,
                      base::Unretained(this)),
-      std::move(launch_closure_));
+      std::move(launch_callback_));
 }
 
 void CrostiniUpgraderUI::OnWebUICloseDialog() {

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_ui.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_CHROMEOS_CROSTINI_UPGRADER_CROSTINI_UPGRADER_UI_H_
 #define CHROME_BROWSER_UI_WEBUI_CHROMEOS_CROSTINI_UPGRADER_CROSTINI_UPGRADER_UI_H_
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader.mojom.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -27,8 +28,8 @@ class CrostiniUpgraderUI
   ~CrostiniUpgraderUI() override;
 
   bool can_close() { return can_close_; }
-  void set_launch_closure(base::OnceClosure(launch_closure)) {
-    launch_closure_ = std::move(launch_closure);
+  void set_launch_callback(base::OnceCallback<void(bool)>(launch_callback)) {
+    launch_callback_ = std::move(launch_callback);
   }
 
   // Instantiates implementor of the
@@ -53,7 +54,7 @@ class CrostiniUpgraderUI
       page_factory_receiver_{this};
 
   // Not owned. Passed to |page_handler_|
-  base::OnceClosure launch_closure_;
+  base::OnceCallback<void(bool)> launch_callback_;
 
   bool can_close_ = false;