0

Close unmanaged Boca SWA instances

This change configures the OnTask window tracker to observe the browser
list outside active OnTask sessions to pre-emptively close unmanaged
Boca instances, especially those that can be spawned through
non-conventional flows.

Bug: b:377997889
Change-Id: Ibb773020c80be10dc4db19c6d83c9f2e7f34984d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6041246
Reviewed-by: April Zhou <aprilzhou@google.com>
Reviewed-by: Luke Cao <caott@google.com>
Commit-Queue: Vignesh Shenvi <vshenvi@google.com>
Cr-Commit-Position: refs/heads/main@{#1401638}
This commit is contained in:
Vignesh Shenvi
2025-01-02 16:04:42 -08:00
committed by Chromium LUCI CQ
parent 0af7085e84
commit b5ef6441ff
11 changed files with 161 additions and 55 deletions

@ -31,6 +31,7 @@ static_library("on_task") {
"//ash/constants",
"//ash/public/cpp",
"//ash/webui/boca_ui",
"//ash/webui/system_apps/public:system_web_app_type",
"//base",
"//chrome/browser/apps/app_service",

@ -49,7 +49,7 @@ LockedSessionWindowTrackerFactory::BuildServiceInstanceForBrowserContext(
auto on_task_blocklist =
std::make_unique<OnTaskBlocklist>(std::move(url_blocklist_manager));
return std::make_unique<LockedSessionWindowTracker>(
std::move(on_task_blocklist));
std::move(on_task_blocklist), context);
}
content::BrowserContext*

@ -13,20 +13,24 @@
#include "ash/public/cpp/shell_window_ids.h"
#include "ash/shell.h"
#include "ash/webui/boca_ui/url_constants.h"
#include "ash/webui/system_apps/public/system_web_app_type.h"
#include "ash/wm/screen_pinning_controller.h"
#include "base/containers/contains.h"
#include "base/functional/bind.h"
#include "base/strings/string_util.h"
#include "base/values.h"
#include "chrome/browser/platform_util.h"
#include "chrome/browser/ui/ash/system_web_apps/system_web_app_ui_utils.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_strip_model_delegate.h"
#include "chromeos/ash/components/boca/boca_role_util.h"
#include "chromeos/ash/components/boca/boca_window_observer.h"
#include "chromeos/ash/components/boca/on_task/activity/active_tab_tracker.h"
#include "chromeos/ash/components/boca/on_task/notification_constants.h"
#include "chromeos/ash/components/boca/on_task/on_task_notifications_manager.h"
#include "chromeos/ash/components/browser_context_helper/browser_context_helper.h"
#include "chromeos/strings/grit/chromeos_strings.h"
#include "components/sessions/content/session_tab_helper.h"
#include "content/public/browser/browser_thread.h"
@ -50,11 +54,23 @@ Browser* LockedSessionWindowTracker::GetBrowserWithTab(
}
LockedSessionWindowTracker::LockedSessionWindowTracker(
std::unique_ptr<OnTaskBlocklist> on_task_blocklist)
std::unique_ptr<OnTaskBlocklist> on_task_blocklist,
content::BrowserContext* context)
: on_task_blocklist_(std::move(on_task_blocklist)),
notifications_manager_(ash::boca::OnTaskNotificationsManager::Create()) {}
is_consumer_profile_(ash::boca_util::IsConsumer(
ash::BrowserContextHelper::Get()->GetUserByBrowserContext(context))),
notifications_manager_(ash::boca::OnTaskNotificationsManager::Create()) {
// Set up window tracker to observe app instances only on consumer devices.
// This will enable us to filter out unmanaged app instances.
if (is_consumer_profile_) {
BrowserList::GetInstance()->AddObserver(this);
}
}
LockedSessionWindowTracker::~LockedSessionWindowTracker() {
if (is_consumer_profile_) {
BrowserList::GetInstance()->RemoveObserver(this);
}
CleanupWindowTracker();
}
@ -80,9 +96,6 @@ void LockedSessionWindowTracker::InitializeBrowserInfoForTracking(
}
browser_ = browser;
browser_->tab_strip_model()->AddObserver(this);
if (!browser_list_observation_.IsObserving()) {
browser_list_observation_.Observe(BrowserList::GetInstance());
}
}
void LockedSessionWindowTracker::RefreshUrlBlocklist() {
@ -100,15 +113,48 @@ void LockedSessionWindowTracker::RefreshUrlBlocklist() {
void LockedSessionWindowTracker::MaybeCloseBrowser(
base::WeakPtr<Browser> weak_browser_ptr) {
if (!weak_browser_ptr) {
return;
}
// The browser window needs to be closed if:
// 1. It is a duplicate instance of the Boca SWA outside the one being
// tracked.
// 2. It is an unmanaged instance of the Boca SWA spawned through
// non-conventional means.
// 3. It is not a Boca app instance and the tracking window happens to be in
// locked fullscreen mode.
// 4. It is an oauth popup and the oauth operation has completed.
//
// The inverse checks below ensure we do not attempt to close the window if
// they do not fall under any of the scenarios outlined above.
Browser* const browser = weak_browser_ptr.get();
// If tracking browser is in locked fullscreen mode, we may need to explicitly
// close a browser when either a new window is opened from the OnTask SWA that
// is blocked, but is not closed or when an OAuth is completed, but since
// OnTask prevents windows from closing, we need to manually close that window
// when the OAuth is completed.
if (!browser || browser == browser_ ||
(browser_ && !platform_util::IsBrowserLockedFullscreen(browser_)) ||
(browser->is_type_app_popup() && oauth_in_progress_)) {
if (browser == browser_) {
// Same instance as the one being tracked. Skip close.
return;
}
if (!browser_ && browser->IsLockedForOnTask()) {
// New instance that has been prepared for OnTask but is not being tracked
// yet. Skip close because it is a managed instance.
return;
}
if (browser->is_type_app_popup() && oauth_in_progress_) {
// Oauth popup and oauth is still in progress. Skip close.
return;
}
bool is_boca_app_instance =
ash::IsBrowserForSystemWebApp(browser, ash::SystemWebAppType::BOCA);
if (browser_ && !platform_util::IsBrowserLockedFullscreen(browser_) &&
!is_boca_app_instance) {
// New instance that is not a Boca SWA instance and was spawned when the
// Boca SWA instance being tracked is not in locked fullscreen mode. Skip
// close.
return;
}
if (!browser_ && !is_boca_app_instance) {
// New instance that is not a Boca SWA instance and is spawned when there is
// no Boca SWA instance being tracked. Skip close for now.
return;
}
browser->window()->Close();
@ -158,7 +204,6 @@ bool LockedSessionWindowTracker::CanOpenNewPopup() {
void LockedSessionWindowTracker::CleanupWindowTracker() {
if (browser_) {
browser_->tab_strip_model()->RemoveObserver(this);
browser_list_observation_.Reset();
}
if (on_task_blocklist_) {
on_task_blocklist_->CleanupBlocklist();

@ -9,19 +9,18 @@
#include "base/memory/singleton.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/scoped_observation.h"
#include "chrome/browser/ui/browser_list_observer.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "chromeos/ash/components/boca/on_task/on_task_blocklist.h"
#include "chromeos/ash/components/boca/on_task/on_task_notifications_manager.h"
#include "components/keyed_service/core/keyed_service.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_observer.h"
class Browser;
class BrowserList;
namespace ash::boca {
class BocaWindowObserver;
@ -43,8 +42,8 @@ class LockedSessionWindowTracker : public KeyedService,
public:
static Browser* GetBrowserWithTab(content::WebContents* tab);
explicit LockedSessionWindowTracker(
std::unique_ptr<OnTaskBlocklist> on_task_blocklist);
LockedSessionWindowTracker(std::unique_ptr<OnTaskBlocklist> on_task_blocklist,
content::BrowserContext* context);
LockedSessionWindowTracker(const LockedSessionWindowTracker&) = delete;
LockedSessionWindowTracker& operator=(const LockedSessionWindowTracker&) =
delete;
@ -122,11 +121,10 @@ class LockedSessionWindowTracker : public KeyedService,
bool can_start_navigation_throttle_ = true;
bool oauth_in_progress_ = false;
const std::unique_ptr<OnTaskBlocklist> on_task_blocklist_;
const bool is_consumer_profile_;
std::unique_ptr<ash::boca::OnTaskNotificationsManager> notifications_manager_;
raw_ptr<Browser> browser_ = nullptr;
base::ScopedObservation<BrowserList, BrowserListObserver>
browser_list_observation_{this};
base::ObserverList<ash::boca::BocaWindowObserver> observers_;
base::WeakPtrFactory<LockedSessionWindowTracker> weak_pointer_factory_{this};

@ -93,9 +93,9 @@ class FakeOnTaskNotificationsManagerDelegate
// browser test, right now it's very hard to tell when does tab strip update
// happens.
class OnTaskLockedSessionWindowTrackerTest : public BrowserWithTestWindowTest {
public:
protected:
void SetUp() override {
// TODO(crbug.com/36741761):Change this to reguar mock when remigrated to
// TODO(crbug.com/36741761): Change this to regular mock when remigrated to
// browser test.
ON_CALL(boca_window_observer_, OnActiveTabChanged(_))
.WillByDefault(Return());
@ -125,7 +125,7 @@ class OnTaskLockedSessionWindowTrackerTest : public BrowserWithTestWindowTest {
auto on_task_blocklist = std::make_unique<OnTaskBlocklist>(
std::move(url_blocklist_manager));
return std::make_unique<LockedSessionWindowTracker>(
std::move(on_task_blocklist));
std::move(on_task_blocklist), context);
}));
ASSERT_TRUE(base::test::RunUntil([&]() {
return (LockedSessionWindowTrackerFactory::GetForBrowserContext(
@ -141,6 +141,7 @@ class OnTaskLockedSessionWindowTrackerTest : public BrowserWithTestWindowTest {
ash::boca::OnTaskNotificationsManager::CreateForTest(
std::move(fake_notifications_delegate)));
}
void TearDown() override {
task_environment()->RunUntilIdle();
@ -676,6 +677,11 @@ TEST_F(OnTaskLockedSessionWindowTrackerTest,
TEST_F(OnTaskLockedSessionWindowTrackerTest,
NewBrowserWindowsDontOpenDuringLockedFullscreen) {
// Set up relevant features for browser instance observation.
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitWithFeatures(
/*enabled_features=*/{ash::features::kBoca, ash::features::kBocaConsumer},
/*disabled_features=*/{});
CreateWindowTrackerServiceForTesting();
auto* const window_tracker =
LockedSessionWindowTrackerFactory::GetForBrowserContext(profile());
@ -717,6 +723,11 @@ TEST_F(OnTaskLockedSessionWindowTrackerTest,
}
TEST_F(OnTaskLockedSessionWindowTrackerTest, NewBrowserPopupIsRegistered) {
// Set up relevant features for browser instance observation.
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitWithFeatures(
/*enabled_features=*/{ash::features::kBoca, ash::features::kBocaConsumer},
/*disabled_features=*/{});
CreateWindowTrackerServiceForTesting();
auto* const window_tracker =
LockedSessionWindowTrackerFactory::GetForBrowserContext(profile());
@ -732,6 +743,11 @@ TEST_F(OnTaskLockedSessionWindowTrackerTest, NewBrowserPopupIsRegistered) {
}
TEST_F(OnTaskLockedSessionWindowTrackerTest, BrowserClose) {
// Set up relevant features for browser instance observation.
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitWithFeatures(
/*enabled_features=*/{ash::features::kBoca, ash::features::kBocaConsumer},
/*disabled_features=*/{});
CreateWindowTrackerServiceForTesting();
auto* const window_tracker =
LockedSessionWindowTrackerFactory::GetForBrowserContext(profile());

@ -81,7 +81,8 @@ void OnTaskSystemWebAppManagerImpl::LaunchSystemWebAppAsync(
if (instance) {
const SessionID active_window_id =
instance->GetActiveSystemWebAppWindowID();
instance->PrepareSystemWebAppWindowForOnTask(active_window_id);
instance->PrepareSystemWebAppWindowForOnTask(
active_window_id, /*close_bundle_content=*/true);
}
std::move(callback).Run(launch_result.state ==
apps::LaunchResult::State::kSuccess);
@ -209,7 +210,8 @@ void OnTaskSystemWebAppManagerImpl::RemoveTabsWithTabIds(
}
void OnTaskSystemWebAppManagerImpl::PrepareSystemWebAppWindowForOnTask(
SessionID window_id) {
SessionID window_id,
bool close_bundle_content) {
Browser* const browser = GetBrowserWindowWithID(window_id);
if (!browser) {
return;
@ -225,16 +227,18 @@ void OnTaskSystemWebAppManagerImpl::PrepareSystemWebAppWindowForOnTask(
aura::Window* const native_window = browser->window()->GetNativeWindow();
native_window->SetProperty(chromeos::kSupportsFloatedStateKey, false);
// Remove all tabs with pre-existing content. This is to de-dupe content and
// ensure that the tabs are set up for locked mode.
std::set<SessionID> tab_ids_to_remove;
for (int idx = browser->tab_strip_model()->count() - 1; idx > 0; --idx) {
content::WebContents* const tab =
browser->tab_strip_model()->GetWebContentsAt(idx);
const SessionID tab_id = sessions::SessionTabHelper::IdForTab(tab);
tab_ids_to_remove.insert(tab_id);
// Remove all tabs with pre-existing content if specified. This is to normally
// de-dupe content and ensure that the tabs are set up for locked mode.
if (close_bundle_content) {
std::set<SessionID> tab_ids_to_remove;
for (int idx = browser->tab_strip_model()->count() - 1; idx > 0; --idx) {
content::WebContents* const tab =
browser->tab_strip_model()->GetWebContentsAt(idx);
const SessionID tab_id = sessions::SessionTabHelper::IdForTab(tab);
tab_ids_to_remove.insert(tab_id);
}
RemoveTabsWithTabIds(window_id, tab_ids_to_remove);
}
RemoveTabsWithTabIds(window_id, tab_ids_to_remove);
}
SessionID OnTaskSystemWebAppManagerImpl::GetActiveTabID() {

@ -46,7 +46,8 @@ class OnTaskSystemWebAppManagerImpl : public OnTaskSystemWebAppManager {
void RemoveTabsWithTabIds(
SessionID window_id,
const std::set<SessionID>& tab_ids_to_remove) override;
void PrepareSystemWebAppWindowForOnTask(SessionID window_id) override;
void PrepareSystemWebAppWindowForOnTask(SessionID window_id,
bool close_bundle_content) override;
SessionID GetActiveTabID() override;
void SwitchToTab(SessionID tab_id) override;

@ -46,11 +46,13 @@ constexpr char kTestUrl[] = "https://www.test.com";
class LockedSessionWindowTrackerMock : public LockedSessionWindowTracker {
public:
explicit LockedSessionWindowTrackerMock(Profile* profile)
: LockedSessionWindowTracker(std::make_unique<OnTaskBlocklist>(
std::make_unique<policy::URLBlocklistManager>(
profile->GetPrefs(),
policy::policy_prefs::kUrlBlocklist,
policy::policy_prefs::kUrlAllowlist))) {}
: LockedSessionWindowTracker(
std::make_unique<OnTaskBlocklist>(
std::make_unique<policy::URLBlocklistManager>(
profile->GetPrefs(),
policy::policy_prefs::kUrlBlocklist,
policy::policy_prefs::kUrlAllowlist)),
profile) {}
~LockedSessionWindowTrackerMock() override = default;
MOCK_METHOD(void,
@ -292,7 +294,7 @@ IN_PROC_BROWSER_TEST_F(OnTaskSystemWebAppManagerImplBrowserTest,
// Verify that the tab is cleaned up after window prep.
system_web_app_manager.PrepareSystemWebAppWindowForOnTask(
boca_app_browser->session_id());
boca_app_browser->session_id(), /*close_bundle_content=*/true);
EXPECT_TRUE(boca_app_browser->IsLockedForOnTask());
views::Widget* const widget = views::Widget::GetWidgetForNativeWindow(
boca_app_browser->window()->GetNativeWindow());
@ -303,5 +305,29 @@ IN_PROC_BROWSER_TEST_F(OnTaskSystemWebAppManagerImplBrowserTest,
EXPECT_EQ(boca_app_browser->tab_strip_model()->count(), 1);
}
IN_PROC_BROWSER_TEST_F(OnTaskSystemWebAppManagerImplBrowserTest,
PreparingSystemWebAppWindowAndPreservingContent) {
// Launch Boca app for testing purposes.
OnTaskSystemWebAppManagerImpl system_web_app_manager(profile());
base::test::TestFuture<bool> launch_future;
system_web_app_manager.LaunchSystemWebAppAsync(launch_future.GetCallback());
ASSERT_TRUE(launch_future.Get());
Browser* const boca_app_browser = FindBocaSystemWebAppBrowser();
ASSERT_THAT(boca_app_browser, NotNull());
EXPECT_EQ(boca_app_browser->tab_strip_model()->count(), 1);
// Create tab so we can verify it does not get cleaned up after window prep.
system_web_app_manager.CreateBackgroundTabWithUrl(
boca_app_browser->session_id(), GURL(kTestUrl),
LockedNavigationOptions::BLOCK_NAVIGATION);
ASSERT_EQ(boca_app_browser->tab_strip_model()->count(), 2);
// Verify that the tab is not destroyed after window prep.
system_web_app_manager.PrepareSystemWebAppWindowForOnTask(
boca_app_browser->session_id(), /*close_bundle_content=*/false);
EXPECT_TRUE(boca_app_browser->IsLockedForOnTask());
EXPECT_EQ(boca_app_browser->tab_strip_model()->count(), 2);
}
} // namespace
} // namespace ash::boca

@ -67,7 +67,8 @@ void OnTaskSessionManager::OnSessionStarted(
system_web_app_manager_->GetActiveSystemWebAppWindowID();
window_id.is_valid()) {
// Prepare the pre-existing Boca SWA instance for OnTask.
system_web_app_manager_->PrepareSystemWebAppWindowForOnTask(window_id);
system_web_app_manager_->PrepareSystemWebAppWindowForOnTask(
window_id, /*close_bundle_content=*/true);
system_web_app_manager_->SetWindowTrackerForSystemWebAppWindow(
window_id, {&active_tab_tracker_, this});
} else {
@ -218,12 +219,22 @@ void OnTaskSessionManager::OnAppReloaded() {
return;
}
// Prepare the SWA for OnTask without closing bundle content outside an active
// session. This is needed to prevent the window tracker from filtering out
// and closing the app instance.
system_web_app_manager_->PrepareSystemWebAppWindowForOnTask(
window_id, /*close_bundle_content=*/false);
// Only restore tabs and set up window tracker if there is an active session.
// This ensures we do not inadvertently block URLs.
if (!active_session_id_.has_value()) {
return;
}
system_web_app_manager_->PrepareSystemWebAppWindowForOnTask(window_id);
// Prepare the SWA for OnTask and close bundle content. This is to de-dupe
// content and ensure that they are set up for locked mode.
system_web_app_manager_->PrepareSystemWebAppWindowForOnTask(
window_id, /*close_bundle_content=*/true);
system_web_app_manager_->SetWindowTrackerForSystemWebAppWindow(
window_id, {&active_tab_tracker_, this});

@ -83,7 +83,7 @@ class OnTaskSystemWebAppManagerMock : public OnTaskSystemWebAppManager {
(override));
MOCK_METHOD(void,
PrepareSystemWebAppWindowForOnTask,
(SessionID window_id),
(SessionID window_id, bool close_bundle_content),
(override));
MOCK_METHOD(SessionID, GetActiveTabID, (), (override));
MOCK_METHOD(void, SwitchToTab, (SessionID tab_id), (override));
@ -218,7 +218,8 @@ TEST_F(OnTaskSessionManagerTest,
session_manager_->active_tab_tracker(), session_manager_.get()};
Sequence s;
EXPECT_CALL(*system_web_app_manager_ptr_,
PrepareSystemWebAppWindowForOnTask(kWindowId))
PrepareSystemWebAppWindowForOnTask(kWindowId,
/*close_bundle_content=*/true))
.Times(1)
.InSequence(s);
EXPECT_CALL(
@ -687,7 +688,7 @@ TEST_F(OnTaskSessionManagerTest, OnAppReloadWithNoActiveWindow) {
EXPECT_CALL(*system_web_app_manager_ptr_, GetActiveSystemWebAppWindowID())
.WillOnce(Return(SessionID::InvalidValue()));
EXPECT_CALL(*system_web_app_manager_ptr_,
PrepareSystemWebAppWindowForOnTask(_))
PrepareSystemWebAppWindowForOnTask(_, _))
.Times(0);
session_manager_->OnAppReloaded();
}
@ -719,8 +720,8 @@ TEST_F(OnTaskSessionManagerTest, RestoreTabsOnAppReload) {
SetWindowTrackerForSystemWebAppWindow(kWindowId, kWindowObservers))
.Times(AtLeast(1));
EXPECT_CALL(*system_web_app_manager_ptr_,
PrepareSystemWebAppWindowForOnTask(kWindowId))
.Times(1)
PrepareSystemWebAppWindowForOnTask(kWindowId, _))
.Times(AtLeast(1))
.InSequence(s);
EXPECT_CALL(*system_web_app_manager_ptr_,
CreateBackgroundTabWithUrl(
@ -768,8 +769,8 @@ TEST_F(OnTaskSessionManagerTest, LockWindowOnAppReload) {
SetWindowTrackerForSystemWebAppWindow(kWindowId, kWindowObservers))
.Times(AtLeast(1));
EXPECT_CALL(*system_web_app_manager_ptr_,
PrepareSystemWebAppWindowForOnTask(kWindowId))
.Times(1)
PrepareSystemWebAppWindowForOnTask(kWindowId, _))
.Times(AtLeast(1))
.InSequence(s);
EXPECT_CALL(*system_web_app_manager_ptr_, SetPinStateForSystemWebAppWindow(
/*pinned=*/true, kWindowId))

@ -56,10 +56,13 @@ class OnTaskSystemWebAppManager {
SessionID window_id,
const std::set<SessionID>& tab_ids_to_remove) = 0;
// Sets up the specified Boca SWA window for OnTask. This may remove all
// pre-existing content tabs except for the homepage one, normally required
// when the app instance was restored manually.
virtual void PrepareSystemWebAppWindowForOnTask(SessionID window_id) = 0;
// Sets up the specified Boca SWA window for OnTask. Setting
// `close_bundle_content` will remove all pre-existing content tabs except for
// the homepage one, normally required at the onset of a new session or when
// the app instance was restored manually in the middle of an active session.
virtual void PrepareSystemWebAppWindowForOnTask(
SessionID window_id,
bool close_bundle_content) = 0;
// Returns a valid tab id associated with the active tab. If
// there is no valid active tab, it returns `SessionID::InvalidValue()`.