From 4cc6825c7474a7a898c563c35e67c999f6fd5f47 Mon Sep 17 00:00:00 2001 From: Tom Lukaszewicz Date: Mon, 28 Mar 2022 14:55:38 +0000 Subject: [PATCH] [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. Bug: 1298826 Change-Id: Iaa1d7423ca4732ebd0a21c32aea0e0f753bad658 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3553646 Reviewed-by: Hidehiko Abe Commit-Queue: Thomas Lukaszewicz Cr-Commit-Position: refs/heads/main@{#985970} --- chrome/browser/ash/crosapi/browser_manager.cc | 5 +- chrome/browser/ash/crosapi/browser_manager.h | 6 ++ .../ash/crosapi/browser_manager_unittest.cc | 75 +++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/chrome/browser/ash/crosapi/browser_manager.cc b/chrome/browser/ash/crosapi/browser_manager.cc index 8b8c595aef368d..7163ec558da549 100644 --- a/chrome/browser/ash/crosapi/browser_manager.cc +++ b/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( diff --git a/chrome/browser/ash/crosapi/browser_manager.h b/chrome/browser/ash/crosapi/browser_manager.h index 75609ab47d9f8d..fab2c4690a5085 100644 --- a/chrome/browser/ash/crosapi/browser_manager.h +++ b/chrome/browser/ash/crosapi/browser_manager.h @@ -227,6 +227,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 { @@ -286,6 +290,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 diff --git a/chrome/browser/ash/crosapi/browser_manager_unittest.cc b/chrome/browser/ash/crosapi/browser_manager_unittest.cc index a49ee813a09290..b03fd9c4f1794c 100644 --- a/chrome/browser/ash/crosapi/browser_manager_unittest.cc +++ b/chrome/browser/ash/crosapi/browser_manager_unittest.cc @@ -42,12 +42,21 @@ 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() 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. @@ -55,6 +64,7 @@ class BrowserManagerFake : public BrowserManager { 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 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