From 57d1130a111f0795a081643da604bacf8eb3f5ff Mon Sep 17 00:00:00 2001
From: Tommy Steimel <steimel@chromium.org>
Date: Thu, 17 Sep 2026 07:49:13 -0700
Subject: [PATCH] pip occlusion: Prevent issues when widgets are destroyed during update

PictureInPictureOcclusionTracker users can close or open widgets when
informed of occlusion changes. This in turn can mean that the
PictureInPictureOcclusionTracker can still be iterating on a set of
widgets where one if deleted.

This CL updates the PictureInPictureOcclusionTracker to properly handle
widgets getting deleted during iteration by using a temporary list of
weak pointers to the observed widgets.

Bug: 560251733
Change-Id: I9586a5455ee8c041ec8d889d0bac155abbeafaf4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8415426
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Benjamin Keen <bkeen@google.com>
Cr-Commit-Position: refs/heads/main@{#1699801}
---
diff --git a/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.cc b/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.cc
index 1276886b..f60bf22 100644
--- a/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.cc
+++ b/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.h"
 
+#include "base/auto_reset.h"
+#include "base/task/sequenced_task_runner.h"
 #include "chrome/browser/picture_in_picture/picture_in_picture_occlusion_observer.h"
 
 namespace {
@@ -285,9 +287,39 @@
 }
 
 void PictureInPictureOcclusionTracker::UpdateAllObserverStates() {
+  if (is_updating_observer_states_) {
+    if (!has_scheduled_reupdate_) {
+      has_scheduled_reupdate_ = true;
+      base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              &PictureInPictureOcclusionTracker::UpdateAllObserverStates,
+              weak_factory_.GetWeakPtr()));
+    }
+    return;
+  }
+
+  base::AutoReset<bool> auto_reset(&is_updating_observer_states_, true);
+  has_scheduled_reupdate_ = false;
+
+  // Snapshot widgets to update using WeakPtrs to protect against deletion
+  // or container reallocation during observer callbacks.
+  std::vector<base::WeakPtr<views::Widget>> widgets_to_update;
+  widgets_to_update.reserve(observed_widget_data_.size());
   for (const auto& [widget, observed_widget_data] : observed_widget_data_) {
     if (observed_widget_data.number_of_direct_observers > 0) {
-      UpdateObserverStateForWidget(widget);
+      widgets_to_update.push_back(widget->GetWeakPtr());
+    }
+  }
+
+  for (const auto& weak_widget : widgets_to_update) {
+    if (!weak_widget) {
+      continue;
+    }
+    auto iter = observed_widget_data_.find(weak_widget.get());
+    if (iter != observed_widget_data_.end() &&
+        iter->second.number_of_direct_observers > 0) {
+      UpdateObserverStateForWidget(weak_widget.get());
     }
   }
 }
@@ -295,6 +327,7 @@
 void PictureInPictureOcclusionTracker::UpdateObserverStateForWidget(
     views::Widget* widget,
     bool force_update) {
+  base::WeakPtr<views::Widget> weak_widget = widget->GetWeakPtr();
   const gfx::Rect observer_bounds = widget->GetWindowBoundsInScreen();
   bool occluded = false;
   for (const auto& [picture_in_picture_widget, observed_widget_data] :
@@ -335,7 +368,10 @@
     occluded = iter->second.forced_occlusion_state.value();
   }
   for (auto& observer : observers_) {
-    if (observer.occludable_widget() == widget) {
+    if (!weak_widget) {
+      break;
+    }
+    if (observer.occludable_widget() == weak_widget.get()) {
       observer.occlusion_observer()->OnOcclusionStateChanged(occluded);
     }
   }
diff --git a/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.h b/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.h
index 0b8c7de..3d7b09f0 100644
--- a/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.h
+++ b/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.h
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/containers/flat_map.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/scoped_multi_source_observation.h"
 #include "base/timer/timer.h"
@@ -181,6 +182,16 @@
   // Used to ensure that frequent `OnWidgetBoundsChanged()` calls from dragging
   // a window don't calculate occlusion too often.
   base::OneShotTimer bounds_changed_throttle_timer_;
+
+  // True if we are currently notifying observers in
+  // `UpdateAllObserverStates()`.
+  bool is_updating_observer_states_ = false;
+
+  // True if a task to run `UpdateAllObserverStates()` has already been posted
+  // to handle re-entrant update requests.
+  bool has_scheduled_reupdate_ = false;
+
+  base::WeakPtrFactory<PictureInPictureOcclusionTracker> weak_factory_{this};
 };
 
 #endif  // CHROME_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_OCCLUSION_TRACKER_H_
diff --git a/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker_unittest.cc b/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker_unittest.cc
index b84f4d3..4184658 100644
--- a/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker_unittest.cc
+++ b/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker_unittest.cc
@@ -341,4 +341,111 @@
   testing::Mock::VerifyAndClearExpectations(&observer);
 }
 
+TEST_F(PictureInPictureOcclusionTrackerTest,
+       ClosingPiPWidgetDuringUpdateAllObserverStatesDoesNotCrash) {
+  MockPictureInPictureOcclusionObserver observer1;
+  ScopedPictureInPictureOcclusionObservation observation1(&observer1);
+  MockPictureInPictureOcclusionObserver observer2;
+  ScopedPictureInPictureOcclusionObservation observation2(&observer2);
+
+  std::unique_ptr<views::Widget> picture_in_picture_widget =
+      CreatePictureInPictureWidget();
+  picture_in_picture_widget->SetBounds({0, 0, 200, 200});
+
+  std::unique_ptr<views::Widget> occludable_widget1 =
+      CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+  occludable_widget1->Show();
+  occludable_widget1->SetBounds({300, 0, 200, 200});
+
+  std::unique_ptr<views::Widget> occludable_widget2 =
+      CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+  occludable_widget2->Show();
+  occludable_widget2->SetBounds({0, 0, 200, 200});
+
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(false));
+  observation1.Observe(occludable_widget1.get());
+  EXPECT_CALL(observer2, OnOcclusionStateChanged(true));
+  observation2.Observe(occludable_widget2.get());
+  testing::Mock::VerifyAndClearExpectations(&observer1);
+  testing::Mock::VerifyAndClearExpectations(&observer2);
+
+  WaitForBoundsChangedDebounce();
+
+  // When observer1 is notified of occlusion, synchronously destroy the PiP
+  // widget. This triggers OnWidgetDestroying which re-entrantly calls
+  // UpdateAllObserverStates(). Both observers should safely become unoccluded.
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(true)).WillOnce([&]() {
+    picture_in_picture_widget.reset();
+  });
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(false));
+  EXPECT_CALL(observer2, OnOcclusionStateChanged(false));
+
+  // Move occludable_widget1 under the PiP widget, triggering
+  // UpdateAllObserverStates(). Note that occludable_widget1 is moving, not
+  // picture_in_picture_widget, so picture_in_picture_widget is not on the call
+  // stack when destroyed.
+  occludable_widget1->SetBounds({50, 50, 200, 200});
+  WaitForBoundsChangedDebounce();
+}
+
+TEST_F(PictureInPictureOcclusionTrackerTest,
+       AddingObserversDuringUpdateAllObserverStatesDoesNotCrash) {
+  MockPictureInPictureOcclusionObserver observer1;
+  ScopedPictureInPictureOcclusionObservation observation1(&observer1);
+  MockPictureInPictureOcclusionObserver observer2;
+  ScopedPictureInPictureOcclusionObservation observation2(&observer2);
+
+  std::unique_ptr<views::Widget> picture_in_picture_widget =
+      CreatePictureInPictureWidget();
+  picture_in_picture_widget->SetBounds({0, 0, 50, 50});
+
+  std::unique_ptr<views::Widget> occludable_widget1 =
+      CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+  occludable_widget1->Show();
+  occludable_widget1->SetBounds({100, 100, 50, 50});
+
+  std::unique_ptr<views::Widget> occludable_widget2 =
+      CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+  occludable_widget2->Show();
+  occludable_widget2->SetBounds({200, 200, 50, 50});
+
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(false));
+  observation1.Observe(occludable_widget1.get());
+  EXPECT_CALL(observer2, OnOcclusionStateChanged(false));
+  observation2.Observe(occludable_widget2.get());
+  testing::Mock::VerifyAndClearExpectations(&observer1);
+  testing::Mock::VerifyAndClearExpectations(&observer2);
+
+  WaitForBoundsChangedDebounce();
+
+  std::vector<std::unique_ptr<views::Widget>> new_widgets;
+  std::vector<std::unique_ptr<MockPictureInPictureOcclusionObserver>>
+      new_observers;
+  std::vector<std::unique_ptr<ScopedPictureInPictureOcclusionObservation>>
+      new_observations;
+
+  // When observer1 is notified of occlusion, synchronously add 50 new observed
+  // widgets. This forces observed_widget_data_ (a base::flat_map) to reallocate
+  // its internal vector during iteration.
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(true)).WillOnce([&]() {
+    for (int i = 0; i < 50; ++i) {
+      auto widget =
+          CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+      widget->Show();
+      auto obs = std::make_unique<MockPictureInPictureOcclusionObserver>();
+      auto observation =
+          std::make_unique<ScopedPictureInPictureOcclusionObservation>(
+              obs.get());
+      observation->Observe(widget.get());
+      new_widgets.push_back(std::move(widget));
+      new_observers.push_back(std::move(obs));
+      new_observations.push_back(std::move(observation));
+    }
+  });
+  EXPECT_CALL(observer2, OnOcclusionStateChanged(true));
+
+  picture_in_picture_widget->SetBounds({0, 0, 500, 500});
+  WaitForBoundsChangedDebounce();
+}
+
 }  // namespace

diff --git a/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker_unittest.cc b/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker_unittest.cc
index b84f4d3..4184658 100644
--- a/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker_unittest.cc
+++ b/chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker_unittest.cc
@@ -341,4 +341,111 @@
   testing::Mock::VerifyAndClearExpectations(&observer);
 }
 
+TEST_F(PictureInPictureOcclusionTrackerTest,
+       ClosingPiPWidgetDuringUpdateAllObserverStatesDoesNotCrash) {
+  MockPictureInPictureOcclusionObserver observer1;
+  ScopedPictureInPictureOcclusionObservation observation1(&observer1);
+  MockPictureInPictureOcclusionObserver observer2;
+  ScopedPictureInPictureOcclusionObservation observation2(&observer2);
+
+  std::unique_ptr<views::Widget> picture_in_picture_widget =
+      CreatePictureInPictureWidget();
+  picture_in_picture_widget->SetBounds({0, 0, 200, 200});
+
+  std::unique_ptr<views::Widget> occludable_widget1 =
+      CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+  occludable_widget1->Show();
+  occludable_widget1->SetBounds({300, 0, 200, 200});
+
+  std::unique_ptr<views::Widget> occludable_widget2 =
+      CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+  occludable_widget2->Show();
+  occludable_widget2->SetBounds({0, 0, 200, 200});
+
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(false));
+  observation1.Observe(occludable_widget1.get());
+  EXPECT_CALL(observer2, OnOcclusionStateChanged(true));
+  observation2.Observe(occludable_widget2.get());
+  testing::Mock::VerifyAndClearExpectations(&observer1);
+  testing::Mock::VerifyAndClearExpectations(&observer2);
+
+  WaitForBoundsChangedDebounce();
+
+  // When observer1 is notified of occlusion, synchronously destroy the PiP
+  // widget. This triggers OnWidgetDestroying which re-entrantly calls
+  // UpdateAllObserverStates(). Both observers should safely become unoccluded.
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(true)).WillOnce([&]() {
+    picture_in_picture_widget.reset();
+  });
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(false));
+  EXPECT_CALL(observer2, OnOcclusionStateChanged(false));
+
+  // Move occludable_widget1 under the PiP widget, triggering
+  // UpdateAllObserverStates(). Note that occludable_widget1 is moving, not
+  // picture_in_picture_widget, so picture_in_picture_widget is not on the call
+  // stack when destroyed.
+  occludable_widget1->SetBounds({50, 50, 200, 200});
+  WaitForBoundsChangedDebounce();
+}
+
+TEST_F(PictureInPictureOcclusionTrackerTest,
+       AddingObserversDuringUpdateAllObserverStatesDoesNotCrash) {
+  MockPictureInPictureOcclusionObserver observer1;
+  ScopedPictureInPictureOcclusionObservation observation1(&observer1);
+  MockPictureInPictureOcclusionObserver observer2;
+  ScopedPictureInPictureOcclusionObservation observation2(&observer2);
+
+  std::unique_ptr<views::Widget> picture_in_picture_widget =
+      CreatePictureInPictureWidget();
+  picture_in_picture_widget->SetBounds({0, 0, 50, 50});
+
+  std::unique_ptr<views::Widget> occludable_widget1 =
+      CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+  occludable_widget1->Show();
+  occludable_widget1->SetBounds({100, 100, 50, 50});
+
+  std::unique_ptr<views::Widget> occludable_widget2 =
+      CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+  occludable_widget2->Show();
+  occludable_widget2->SetBounds({200, 200, 50, 50});
+
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(false));
+  observation1.Observe(occludable_widget1.get());
+  EXPECT_CALL(observer2, OnOcclusionStateChanged(false));
+  observation2.Observe(occludable_widget2.get());
+  testing::Mock::VerifyAndClearExpectations(&observer1);
+  testing::Mock::VerifyAndClearExpectations(&observer2);
+
+  WaitForBoundsChangedDebounce();
+
+  std::vector<std::unique_ptr<views::Widget>> new_widgets;
+  std::vector<std::unique_ptr<MockPictureInPictureOcclusionObserver>>
+      new_observers;
+  std::vector<std::unique_ptr<ScopedPictureInPictureOcclusionObservation>>
+      new_observations;
+
+  // When observer1 is notified of occlusion, synchronously add 50 new observed
+  // widgets. This forces observed_widget_data_ (a base::flat_map) to reallocate
+  // its internal vector during iteration.
+  EXPECT_CALL(observer1, OnOcclusionStateChanged(true)).WillOnce([&]() {
+    for (int i = 0; i < 50; ++i) {
+      auto widget =
+          CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+      widget->Show();
+      auto obs = std::make_unique<MockPictureInPictureOcclusionObserver>();
+      auto observation =
+          std::make_unique<ScopedPictureInPictureOcclusionObservation>(
+              obs.get());
+      observation->Observe(widget.get());
+      new_widgets.push_back(std::move(widget));
+      new_observers.push_back(std::move(obs));
+      new_observations.push_back(std::move(observation));
+    }
+  });
+  EXPECT_CALL(observer2, OnOcclusionStateChanged(true));
+
+  picture_in_picture_widget->SetBounds({0, 0, 500, 500});
+  WaitForBoundsChangedDebounce();
+}
+
 }  // namespace