[M101 Merge][lacros] Prevent KeepAlive from blocking Lacros restart

Currently when lacros is terminated with a KeepAlive hold it is
automatically relaunched in a windowless state.

This behavior is in conflict with desired restart behavior where the
process is terminated and immediately relaunched with the expectation
that the previous session is restored (all profiles & windows).

This Cl addresses this issue by preventing KeepAlive from starting
lacros in a windowless state if a relaunch was explicitly requested.

(cherry picked from commit 4cc6825)

Bug: 1298826
Change-Id: I79815c77ae3a22c6cf9b727a9c0469713f8121fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3553646
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#985970}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3567169
Cr-Commit-Position: refs/branch-heads/4951@{#398}
Cr-Branched-From: 27de622-refs/heads/main@{#982481}

chrome/browser/ash/crosapi/browser_manager.cc

@@ -1244,8 +1244,11 @@ void BrowserManager::StopKeepAlive(Feature feature) {
 }
 
 void BrowserManager::LaunchForKeepAliveIfNecessary() {
+  // KeepAlive should not start lacros in a windowless state if a relaunch has
+  // been requested. Lacros restart will instead be handled in
+  // `OnLacrosChromeTerminated()`.
   if (state_ == State::STOPPED && !shutdown_requested_ &&
-      !keep_alive_features_.empty()) {
+      !keep_alive_features_.empty() && !relaunch_requested_) {
     CHECK(browser_util::IsLacrosEnabled());
     CHECK(browser_util::IsLacrosAllowedToLaunch());
     MaybeStart(browser_util::InitialBrowserAction(

chrome/browser/ash/crosapi/browser_manager.h

@@ -225,6 +225,10 @@ class BrowserManager : public session_manager::SessionManagerObserver,
   // for a graceful exit.
   void Shutdown();
 
+  void set_relaunch_requested_for_testing(bool relaunch_requested) {
+    relaunch_requested_ = relaunch_requested;
+  }
+
   // Parameters used to launch Lacros that are calculated on a background
   // sequence. Public so that it can be used from private static functions.
   struct LaunchParamsFromBackground {
@@ -284,6 +288,8 @@ class BrowserManager : public session_manager::SessionManagerObserver,
   FRIEND_TEST_ALL_PREFIXES(BrowserManagerTest, LacrosKeepAlive);
   FRIEND_TEST_ALL_PREFIXES(BrowserManagerTest,
                            LacrosKeepAliveReloadsWhenUpdateAvailable);
+  FRIEND_TEST_ALL_PREFIXES(BrowserManagerTest,
+                           LacrosKeepAliveDoesNotBlockRestart);
   friend class apps::StandaloneBrowserExtensionApps;
   // App service require the lacros-chrome to keep alive for web apps to:
   // 1. Have lacros-chrome running before user open the browser so we can

chrome/browser/ash/crosapi/browser_manager_unittest.cc

@@ -42,19 +42,29 @@ class BrowserManagerFake : public BrowserManager {
                      component_updater::ComponentUpdateService* update_service)
       : BrowserManager(std::move(browser_loader), update_service) {}
   ~BrowserManagerFake() override = default;
+
+  // BrowserManager:
   void Start(
       browser_util::InitialBrowserAction initial_browser_action) override {
     ++start_count_;
+    initial_browser_action_ = initial_browser_action.action;
     SetState(State::STARTING);
   }
-  int start_count() { return start_count_; }
+
+  int start_count() const { return start_count_; }
+
+  mojom::InitialBrowserAction initial_browser_action() const {
+    return initial_browser_action_;
+  }
+
   void SetStatePublic(State state) { SetState(state); }
 
   // Make the State enum publicly available.
   using BrowserManager::State;
 
  private:
   int start_count_ = 0;
+  mojom::InitialBrowserAction initial_browser_action_;
 };
 
 }  // namespace
@@ -250,4 +260,69 @@ TEST_F(BrowserManagerTest, NewWindowReloadsWhenUpdateAvailable) {
                                    /*should_trigger_session_restore=*/false);
 }
 
+TEST_F(BrowserManagerTest, LacrosKeepAliveDoesNotBlockRestart) {
+  AddRegularUser("user@test.com");
+  browser_util::SetProfileMigrationCompletedForUser(
+      local_state_.Get(), ash::ProfileHelper::Get()
+                              ->GetUserByProfile(&testing_profile_)
+                              ->username_hash());
+  EXPECT_TRUE(browser_util::IsLacrosEnabled());
+  EXPECT_TRUE(browser_util::IsLacrosAllowedToLaunch());
+
+  using State = BrowserManagerFake::State;
+  EXPECT_EQ(fake_browser_manager_->start_count(), 0);
+
+  // Attempt to mount the Lacros image. Will not start as it does not meet the
+  // automatic start criteria.
+  EXPECT_CALL(*browser_loader_, Load(_))
+      .WillOnce([](BrowserLoader::LoadCompletionCallback callback) {
+        std::move(callback).Run(base::FilePath("/run/lacros"),
+                                browser_util::LacrosSelection::kRootfs);
+      });
+  fake_browser_manager_->InitializeAndStart();
+  EXPECT_EQ(fake_browser_manager_->start_count(), 0);
+
+  fake_browser_manager_->SetStatePublic(State::UNAVAILABLE);
+  EXPECT_EQ(fake_browser_manager_->start_count(), 0);
+
+  // Creating a ScopedKeepAlive does not start Lacros.
+  std::unique_ptr<BrowserManager::ScopedKeepAlive> keep_alive =
+      fake_browser_manager_->KeepAlive(BrowserManager::Feature::kTestOnly);
+  EXPECT_EQ(fake_browser_manager_->start_count(), 0);
+
+  // Simulate a Lacros termination, keep alive should launch Lacros in a
+  // windowless state.
+  auto simulate_lacros_termination = [&]() {
+    fake_browser_manager_->SetStatePublic(State::TERMINATING);
+    fake_browser_manager_->OnLacrosChromeTerminated();
+  };
+  simulate_lacros_termination();
+  EXPECT_EQ(fake_browser_manager_->start_count(), 1);
+  EXPECT_EQ(fake_browser_manager_->initial_browser_action(),
+            mojom::InitialBrowserAction::kDoNotOpenWindow);
+
+  // Terminating again causes keep alive to again start Lacros in a windowless
+  // state.
+  simulate_lacros_termination();
+  EXPECT_EQ(fake_browser_manager_->start_count(), 2);
+  EXPECT_EQ(fake_browser_manager_->initial_browser_action(),
+            mojom::InitialBrowserAction::kDoNotOpenWindow);
+
+  // Request a relaunch. Keep alive should not start Lacros in a windowless
+  // state but Lacros should instead start with the kRestoreLastSession action.
+  fake_browser_manager_->set_relaunch_requested_for_testing(true);
+  simulate_lacros_termination();
+  EXPECT_EQ(fake_browser_manager_->start_count(), 3);
+  EXPECT_EQ(fake_browser_manager_->initial_browser_action(),
+            mojom::InitialBrowserAction::kRestoreLastSession);
+
+  // Resetting the relaunch requested bit should cause keep alive to start
+  // Lacros in a windowless state.
+  fake_browser_manager_->set_relaunch_requested_for_testing(false);
+  simulate_lacros_termination();
+  EXPECT_EQ(fake_browser_manager_->start_count(), 4);
+  EXPECT_EQ(fake_browser_manager_->initial_browser_action(),
+            mojom::InitialBrowserAction::kDoNotOpenWindow);
+}
+
 }  // namespace crosapi