From 657d71230c27d629349f6219e72da4f176f900a0 Mon Sep 17 00:00:00 2001
From: Daniel Cheng <dcheng@chromium.org>
Date: Fri, 02 Oct 2026 13:08:38 -0700
Subject: [PATCH] Deschedule tabs in ThumbnailSchedulerImpl::RemoveTab

`ThumbnailTabHelper` was migrated to be owned by `TabFeatures` in commit
f7c68946aca7c2b7d975a63a12d9626e13292292. `ThumbnailTabHelper`
transitively owns `ThumbnailReadinessTracker` (via `TabStateTracker`),
which previously relied on `WebContentsDestroyed()` to reset the tab's
capture readiness state. However, `TabFeatures` (and thus
`ThumbnailTabHelper`) is destroyed before `WebContents`, so the cleanup
logic in the aforementioned `WebContentsDestroyed()` method is no longer
reachable.

This matters because resetting the capture readiness state results in
the following call chain:
  1. `TabStateTracker::PageReadinessChanged()` (via a
     `base::RepeatingCallback`)
  2. `ThumbnailCaptureDriver::UpdatePageReadiness()`
  3. `ThumbnailCaptureDriver::UpdateSchedulingPriority()`
  4. `ThumbnailSchedulerImpl::SetTabCapturePriority()`
  5. `ThumbnailSchedulerImpl::Schedule()`

`ThumbnailSchedulerImpl` owns a `TabNode` per registered tab in `tabs_`
and maintains 4 `base::LinkedList<TabNode>` fields that track `TabNode`s
that are waiting to capture or actively capturing: updating the priority
removes the `TabNode` from any list it's currently in before moving it
into the appropriate list (if any).

`base::LinkedList<T>` is an intrusive linked list; `T` needs to inherit
from `base::LinkNode`. Critically:
- `base::LinkNode` is used in performance-critical code so the
  `previous_` and `next_` fields do not use `raw_ptr`
- `base::LinkNode` does not automatically remove itself from its
  `base::LinkedList<T>` in its destructor

When a `ThumbnailCaptureDriver` is destroyed, its destructor calls
`ThumbnailSchedulerImpl::RemoveTab()`, which just erased the
entry from `tabs_` without resetting its priority. With all these
factors combined, this results in scenarios where:
- a `TabNode` is destroyed by `RemoveTab()` while still linked in one of
  the lists in the global `ThumbnailSchedulerImpl`
- the memory is reused for something else
- the thumbnail scheduler scribbles over the dangling `TabNode`

In the case of the linked crash, the "something else" is a `BindState`
object that backs a `base::OnceCallback` and the callback invokes its
bound functor with invalid arguments, leading to a crash.

Fix `RemoveTab()` to reset the tab's capture priority to `kNone` before
erasing it from `tabs_`. Because descheduling an actively capturing tab
during `~ThumbnailCaptureDriver()` synchronously calls back into
`TabStateTracker::StopCapture()`, adjust the member declaration order in
`ThumbnailTabHelper` and `TabStateTracker` so `background_capturer_` and
`scoped_capture_` outlive `capture_driver_` during teardown.

Bug: 565866967
Bug: 568437149
Change-Id: I5850a6aa3af0e461c56e02962b9ab3dfb9b3190d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8499464
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Dana Fried <dfried@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1710360}
---
diff --git a/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl.cc b/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl.cc
index 2beca26e1..f8ce26a3 100644
--- a/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl.cc
+++ b/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl.cc
@@ -41,6 +41,10 @@
 }
 
 void ThumbnailSchedulerImpl::RemoveTab(TabCapturer* tab) {
+  // Explicitly set the priority to `kNone` to ensure the corresponding
+  // `TabNode` is unlinked from `hi_prio_waiting_`, `lo_prio_waiting_`,
+  // `hi_prio_capturing_`, or `lo_prio_capturing_` as appropriate.
+  SetTabCapturePriority(tab, TabCapturePriority::kNone);
   int num_removed = tabs_.erase(tab);
   DCHECK_EQ(1, num_removed) << "removed a tab that was never added";
 }
diff --git a/chrome/browser/ui/thumbnails/thumbnail_tab_helper.cc b/chrome/browser/ui/thumbnails/thumbnail_tab_helper.cc
index 7382496..2f3a9f5e 100644
--- a/chrome/browser/ui/thumbnails/thumbnail_tab_helper.cc
+++ b/chrome/browser/ui/thumbnails/thumbnail_tab_helper.cc
@@ -225,15 +225,16 @@
 
   const raw_ptr<ThumbnailTabHelper> thumbnail_tab_helper_;
 
+  // Scoped request for video capture. Declared before `capture_driver_` because
+  // `~ThumbnailCaptureDriver()` may call back into `StopCapture()`.
+  base::ScopedClosureRunner scoped_capture_;
+
   ThumbnailCaptureDriver capture_driver_{
       this, &thumbnail_tab_helper_->GetScheduler()};
   ThumbnailReadinessTracker readiness_tracker_;
 
   // Where we are in the page lifecycle.
   CaptureReadiness page_readiness_ = CaptureReadiness::kNotReady;
-
-  // Scoped request for video capture.
-  base::ScopedClosureRunner scoped_capture_;
 };
 
 // ThumbnailTabHelper ----------------------------------------------------
@@ -278,12 +279,12 @@
 ThumbnailTabHelper::ThumbnailTabHelper(tabs::TabInterface& tab,
                                        content::WebContents* contents)
     : content::WebContentsObserver(contents),
-      state_(std::make_unique<TabStateTracker>(this, contents)),
       background_capturer_(std::make_unique<BackgroundThumbnailVideoCapturer>(
           contents,
           base::BindRepeating(
               &ThumbnailTabHelper::StoreThumbnailForBackgroundCapture,
               base::Unretained(this)))),
+      state_(std::make_unique<TabStateTracker>(this, contents)),
       thumbnail_(base::MakeRefCounted<ThumbnailImage>(
           state_.get(),
           DiscardedTabThumbnailData::TakeThumbnailDataIfAvailable(contents))),
diff --git a/chrome/browser/ui/thumbnails/thumbnail_tab_helper.h b/chrome/browser/ui/thumbnails/thumbnail_tab_helper.h
index 733d3bd3..73acb2d 100644
--- a/chrome/browser/ui/thumbnails/thumbnail_tab_helper.h
+++ b/chrome/browser/ui/thumbnails/thumbnail_tab_helper.h
@@ -100,11 +100,11 @@
   // Copy info from the most recent frame we have captured.
   ThumbnailCaptureInfo last_frame_capture_info_;
 
+  std::unique_ptr<BackgroundThumbnailCapturer> background_capturer_;
+
   // Private implementation of state tracking.
   std::unique_ptr<TabStateTracker> state_;
 
-  std::unique_ptr<BackgroundThumbnailCapturer> background_capturer_;
-
   // Times for computing metrics.
   base::TimeTicks start_video_capture_time_;
 
diff --git a/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl_unittest.cc b/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl_unittest.cc
index 4c62235..09a0274 100644
--- a/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl_unittest.cc
+++ b/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl_unittest.cc
@@ -194,3 +194,19 @@
       &tabs_[1], ThumbnailSchedulerImpl::TabCapturePriority::kNone);
   EXPECT_THAT(TabScheduledStates(), ElementsAre(false, false, false, false));
 }
+
+TEST_F(ThumbnailSchedulerImplTest, RemoveTabWhileCapturing) {
+  FakeTabCapturer removed_tab;
+  scheduler_.AddTab(&removed_tab);
+  scheduler_.SetTabCapturePriority(
+      &removed_tab, ThumbnailSchedulerImpl::TabCapturePriority::kHigh);
+  EXPECT_TRUE(removed_tab.capture_permitted());
+
+  scheduler_.RemoveTab(&removed_tab);
+
+  scheduler_.SetTabCapturePriority(
+      &tabs_[0], ThumbnailSchedulerImpl::TabCapturePriority::kHigh);
+  scheduler_.SetTabCapturePriority(
+      &tabs_[1], ThumbnailSchedulerImpl::TabCapturePriority::kHigh);
+  EXPECT_THAT(TabScheduledStates(), ElementsAre(true, true, false, false));
+}
diff --git a/chrome/browser/ui/thumbnails/thumbnail_tab_helper_interactive_uitest.cc b/chrome/browser/ui/thumbnails/thumbnail_tab_helper_interactive_uitest.cc
index 48f1a3e8..9f8f8957 100644
--- a/chrome/browser/ui/thumbnails/thumbnail_tab_helper_interactive_uitest.cc
+++ b/chrome/browser/ui/thumbnails/thumbnail_tab_helper_interactive_uitest.cc
@@ -256,3 +256,26 @@
       VerifyTabIsNotLoadedAndNeedsReloading(1),
       VerifyTabIsNotLoadedAndNeedsReloading(2), WaitForAndVerifyThumbnail(1));
 }
+
+IN_PROC_BROWSER_TEST_F(ThumbnailTabHelperUpdatedInteractiveTest,
+                       CloseObservedTabThenObserveAnotherTab) {
+  std::unique_ptr<ThumbnailImage::Subscription> first_subscription;
+  std::unique_ptr<ThumbnailImage::Subscription> second_subscription;
+
+  RunTestSequence(
+      AddInstrumentedTab(kFirstTab, chrome::ChromeUINewTabURLAsGURL(), 0),
+      WaitForWebContentsReady(kFirstTab),
+      AddInstrumentedTab(kSecondTab, chrome::ChromeUINewTabURLAsGURL(), 1),
+      WaitForWebContentsReady(kSecondTab), Do([&]() {
+        auto* tab_strip = browser()->GetTabStripModel();
+        first_subscription =
+            ThumbnailTabHelper::From(tab_strip->GetTabAtIndex(0))
+                ->thumbnail()
+                ->Subscribe();
+        tab_strip->CloseWebContentsAt(0, TabCloseTypes::CLOSE_NONE);
+        second_subscription =
+            ThumbnailTabHelper::From(tab_strip->GetTabAtIndex(0))
+                ->thumbnail()
+                ->Subscribe();
+      }));
+}

diff --git a/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl_unittest.cc b/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl_unittest.cc
index 4c62235..09a0274 100644
--- a/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl_unittest.cc
+++ b/chrome/browser/ui/thumbnails/thumbnail_scheduler_impl_unittest.cc
@@ -194,3 +194,19 @@
       &tabs_[1], ThumbnailSchedulerImpl::TabCapturePriority::kNone);
   EXPECT_THAT(TabScheduledStates(), ElementsAre(false, false, false, false));
 }
+
+TEST_F(ThumbnailSchedulerImplTest, RemoveTabWhileCapturing) {
+  FakeTabCapturer removed_tab;
+  scheduler_.AddTab(&removed_tab);
+  scheduler_.SetTabCapturePriority(
+      &removed_tab, ThumbnailSchedulerImpl::TabCapturePriority::kHigh);
+  EXPECT_TRUE(removed_tab.capture_permitted());
+
+  scheduler_.RemoveTab(&removed_tab);
+
+  scheduler_.SetTabCapturePriority(
+      &tabs_[0], ThumbnailSchedulerImpl::TabCapturePriority::kHigh);
+  scheduler_.SetTabCapturePriority(
+      &tabs_[1], ThumbnailSchedulerImpl::TabCapturePriority::kHigh);
+  EXPECT_THAT(TabScheduledStates(), ElementsAre(true, true, false, false));
+}
diff --git a/chrome/browser/ui/thumbnails/thumbnail_tab_helper_interactive_uitest.cc b/chrome/browser/ui/thumbnails/thumbnail_tab_helper_interactive_uitest.cc
index 48f1a3e8..9f8f8957 100644
--- a/chrome/browser/ui/thumbnails/thumbnail_tab_helper_interactive_uitest.cc
+++ b/chrome/browser/ui/thumbnails/thumbnail_tab_helper_interactive_uitest.cc
@@ -256,3 +256,26 @@
       VerifyTabIsNotLoadedAndNeedsReloading(1),
       VerifyTabIsNotLoadedAndNeedsReloading(2), WaitForAndVerifyThumbnail(1));
 }
+
+IN_PROC_BROWSER_TEST_F(ThumbnailTabHelperUpdatedInteractiveTest,
+                       CloseObservedTabThenObserveAnotherTab) {
+  std::unique_ptr<ThumbnailImage::Subscription> first_subscription;
+  std::unique_ptr<ThumbnailImage::Subscription> second_subscription;
+
+  RunTestSequence(
+      AddInstrumentedTab(kFirstTab, chrome::ChromeUINewTabURLAsGURL(), 0),
+      WaitForWebContentsReady(kFirstTab),
+      AddInstrumentedTab(kSecondTab, chrome::ChromeUINewTabURLAsGURL(), 1),
+      WaitForWebContentsReady(kSecondTab), Do([&]() {
+        auto* tab_strip = browser()->GetTabStripModel();
+        first_subscription =
+            ThumbnailTabHelper::From(tab_strip->GetTabAtIndex(0))
+                ->thumbnail()
+                ->Subscribe();
+        tab_strip->CloseWebContentsAt(0, TabCloseTypes::CLOSE_NONE);
+        second_subscription =
+            ThumbnailTabHelper::From(tab_strip->GetTabAtIndex(0))
+                ->thumbnail()
+                ->Subscribe();
+      }));
+}