Migrate ForEachCurrentBrowser and ForEachCurrentAndNewBrowser to BrowserWindowInterface APIs [chromium/src : main]

0 views

Skip to first unread message

Qikai Zhong (Gerrit)

unread,

Sep 16, 2025, 6:20:32 AM (6 days ago) Sep 16

to Alex Carutasu, Tom Lukaszewicz, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, chrome-gr...@chromium.org

Attention needed from Alex Carutasu and Tom Lukaszewicz

Qikai Zhong added 1 comment

File chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc

Line 182, Patchset 4:

TabStripModel* TabLifecycleUnitSource::GetFocusedTabStripModel() const {
  if (focused_tab_strip_model_for_testing_) {
    // Additional safety check: ensure the testing TabStripModel is still valid
    if (focused_tab_strip_model_for_testing_->count() == 0 &&
        focused_tab_strip_model_for_testing_->active_index() == -1) {
      return nullptr;
    }
    return focused_tab_strip_model_for_testing_;
  }

  Browser* const focused_browser = chrome::FindBrowserWithActiveWindow();
  if (!focused_browser) {
    return nullptr;
  }

  return focused_browser->tab_strip_model();
}

Qikai Zhong . resolved

Patchset 1 caused a UAF.
It is caught by test TabDeclutterControllerBrowserTest.TestInactiveBrowserDoesNotShowNudge.
The commit e176ce82cc68a95dd24022fb71f457cd95c75d3b modified BrowserTabStripTracker destructor from using BrowserList::ForEachCurrentBrowser to ForEachCurrentBrowserWindowInterfaceOrderedByActivation, changing the observer notification timing.

UAF Trigger Sequence
Test Setup: TestInactiveBrowserDoesNotShowNudge creates a second browser and sets focused_tab_strip_model_for_testing_ to point to its TabStripModel
Browser Destruction: Second browser begins destruction process
TabStripModel Destruction: TabStripModel enters partial destruction state (count=0, active_index=-1) but object memory not yet freed
Observer Notification: TabLifecycleUnitSource::OnBrowserRemoved() triggered
UAF Access: UpdateFocusedTab() → GetFocusedTabStripModel() returns the partially-destroyed TabStripModel → GetActiveWebContents() accesses invalid data → Crash
Key Technical Insight
The UAF occurs because object destruction timing differs from pointer invalidation timing. The focused_tab_strip_model_for_testing_ pointer remains valid while the TabStripModel object is in an intermediate destruction state with invalid internal data.

Deterministic Nature
The consistent crash indicates a deterministic execution order, not randomness. The API change altered the specific timing when observers are notified relative to object lifecycle events.

Fix Implementation in Patchset 3

Two-layer safety mechanism:

Proactive cleanup: Clear focused_tab_strip_model_for_testing_ in OnBrowserRemoved()
Defensive validation: Check for invalid state (count=0, active_index=-1) in GetFocusedTabStripModel()
Summarized by AI.

Open in Gerrit

Related details

Attention is currently required from:

Alex Carutasu
Tom Lukaszewicz

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alex Carutasu (Gerrit)

unread,

Sep 16, 2025, 2:25:35 PM (6 days ago) Sep 16

to chrome-gr...@chromium.org

Attention needed from Qikai Zhong and Tom Lukaszewicz

Qikai Zhong has uploaded the change for review

Alex Carutasu removed chrome-gr...@chromium.org from reviewers of this change.

Commit message

Migrate ForEachCurrentBrowser and ForEachCurrentAndNewBrowser to BrowserWindowInterface APIs

This CL migrates all usages of BrowserList::ForEachCurrentBrowser() and
BrowserList::ForEachCurrentAndNewBrowser() to the corresponding
BrowserWindowInterface iterator APIs.

API migrations:
- ForEachCurrentBrowser() → ForEachCurrentBrowserWindowInterfaceOrderedByActivation()
- ForEachCurrentAndNewBrowser() → ForEachCurrentAndNewBrowserWindowInterfaceOrderedByActivation()

Will remove ForEachCurrentBrowser() and ForEachCurrentAndNewBrowser() after 2 weeks

Files changed:
- chrome/browser/ui/browser_tab_strip_tracker.h
- chrome/browser/ui/browser_tab_strip_tracker.cc
- chrome/browser/lifetime/browser_close_manager.cc
- chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc

This is part of the ongoing effort to decouple clients from direct
Browser dependencies and enable them to access Browser-scoped features
through BrowserWindowInterface handles instead.

Fixed a use-after-free caused by migrating from ForEachCurrentBrowser to ForEachCurrentBrowserWindowInterfaceOrderedByActivation.

The migration maintains identical iteration behavior:
- Same enumeration order (browsers_ordered_by_activation)
- Same handling of browsers added during iteration (kEnumerateNewBrowser flag)
- Callbacks updated to return bool and access Browser via GetBrowserForMigrationOnly()

Bug: TBD

Change-Id: Iadc8d89e50e0cacc63d7167ac5fd35a3506b8b0b

Change diff

Change information

Files:

M chrome/browser/lifetime/browser_close_manager.cc
M chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc
M chrome/browser/ui/browser_tab_strip_tracker.cc
M chrome/browser/ui/browser_tab_strip_tracker.h

Change size: M

Delta: 4 files changed, 89 insertions(+), 44 deletions(-)