From 424ab5d800ff8f1d3c2fc776f9ef00862fc54474 Mon Sep 17 00:00:00 2001
From: Zhenyao Mo <zmo@chromium.org>
Date: Tue, 14 Jul 2026 13:20:19 -0700
Subject: [PATCH] cc: Defer SetNeedsRedraw while iterating tile priority queues

AssignGpuMemoryToTiles() and the eviction helpers it uses notify the
client of each tile's state change with set_needs_redraw=true while a
Raster/EvictionTilePriorityQueue is still being iterated. From the
posted CheckIfMoreTilesNeedToBePrepared task this may re-enter
Scheduler::ProcessScheduledActions() and trigger ActivateSyncTree(),
which can remove tilings still referenced by the live priority-queue
iterators.

Apply the same defer pattern used by MarkTilesOutOfMemory(): pass
set_needs_redraw=false for each tile, accumulate whether any
required-for-draw tile changed state, and request a single redraw once
the queues and the PrioritizedTiles derived from them have been fully
consumed. This covers the eviction loops and the solid-color path in
AssignGpuMemoryToTiles(), as well as TrimPrepaintTiles() and
ReduceTileMemoryWhenIdle() which share the same helpers.

Bug: 501604761
Change-Id: I4b78af9286a1c8f1a0cda2f30f2249c8ab6dba44
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8088758
Commit-Queue: Zhenyao Mo <zmo@chromium.org>
Reviewed-by: vikas soni <vikassoni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1662124}
---
diff --git a/cc/tiles/tile_manager.cc b/cc/tiles/tile_manager.cc
index 890ab156..27f120ed 100644
--- a/cc/tiles/tile_manager.cc
+++ b/cc/tiles/tile_manager.cc
@@ -508,8 +508,13 @@
   // Note: we don't need to flush anything here, even though this is a case
   // where frames are not being produced. The resource pool will itself issue a
   // flush after a few seconds when a resource becomes unused.
+  bool freed_required_for_draw_tile = false;
   FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit(
-      nullptr, limit, kVisiblePriority, &usage);
+      nullptr, limit, kVisiblePriority, &usage, &freed_required_for_draw_tile);
+  if (freed_required_for_draw_tile) {
+    client_->SetNeedsRedraw(/*animation_only=*/false,
+                            /*skip_if_inside_draw=*/true);
+  }
 }
 
 void TileManager::TrimPrepaintTiles() {
@@ -518,6 +523,7 @@
   std::unique_ptr<EvictionTilePriorityQueue> eviction_priority_queue =
       client_->BuildEvictionQueue();
   bool has_eligible_used_tiles = false;
+  bool freed_required_for_draw_tile = false;
   for (; !eviction_priority_queue->IsEmpty(); eviction_priority_queue->Pop()) {
     const auto& prioritized_tile = eviction_priority_queue->Top();
     Tile* tile = prioritized_tile.tile();
@@ -546,7 +552,8 @@
       // PictureLayerTiling::ComputePriorityForTile() sets the bin to EVENTUALLY
       // regardless (because the client doesn't have valid priorities).
       // We don't want to keep these tiles, so no DCHECK() or exclusion here.
-      FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(tile);
+      freed_required_for_draw_tile |=
+          FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(tile);
     } else {
       // Tile has been used recently, reset this so that if it's not used until
       // the next reclaim task, then we know it has been at least
@@ -556,6 +563,11 @@
       has_eligible_used_tiles = true;
     }
   }
+  eviction_priority_queue.reset();
+  if (freed_required_for_draw_tile) {
+    client_->SetNeedsRedraw(/*animation_only=*/false,
+                            /*skip_if_inside_draw=*/true);
+  }
 
   // Reschedule the task, since there are tiles that would be eligible to evict
   // if they were old enough. Note that we don't choose the smallest delay
@@ -717,9 +729,22 @@
       !prioritized_work.tiles_to_raster.empty() &&
       prioritized_work.tiles_to_raster.front().tile()->required_for_draw());
 
+  const bool required_for_draw_tile_state_changed =
+      prioritized_work.required_for_draw_tile_state_changed;
+
   // Schedule tile tasks.
   ScheduleTasks(std::move(prioritized_work));
 
+  // If we trigger SetNeedsRedraw() while iterating the priority queues, we may
+  // end up triggering Scheduler::ProcessScheduledActions(), which is
+  // inefficient and may in turn trigger ActivateSyncTree() and other actions
+  // that remove tiles still referenced by the queues. Defer the redraw until
+  // the queues and the prioritized tiles they produced have been consumed.
+  if (required_for_draw_tile_state_changed) {
+    client_->SetNeedsRedraw(/*animation_only=*/false,
+                            /*skip_if_inside_draw=*/true);
+  }
+
   TRACE_EVENT_INSTANT("cc", "DidPrepareTiles", "state", BasicStateAsValue());
   return true;
 }
@@ -823,7 +848,8 @@
 TileManager::FreeTileResourcesUntilUsageIsWithinLimit(
     std::unique_ptr<EvictionTilePriorityQueue> eviction_priority_queue,
     const MemoryUsage& limit,
-    MemoryUsage* usage) {
+    MemoryUsage* usage,
+    bool* freed_required_for_draw_tile) {
   while (usage->Exceeds(limit)) {
     if (!eviction_priority_queue) {
       eviction_priority_queue = client_->BuildEvictionQueue();
@@ -833,7 +859,8 @@
 
     Tile* tile = eviction_priority_queue->Top().tile();
     *usage -= MemoryUsage::FromTile(tile);
-    FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(tile);
+    *freed_required_for_draw_tile |=
+        FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(tile);
     eviction_priority_queue->Pop();
   }
   return eviction_priority_queue;
@@ -844,7 +871,8 @@
     std::unique_ptr<EvictionTilePriorityQueue> eviction_priority_queue,
     const MemoryUsage& limit,
     const TilePriority& other_priority,
-    MemoryUsage* usage) {
+    MemoryUsage* usage,
+    bool* freed_required_for_draw_tile) {
   while (usage->Exceeds(limit)) {
     if (!eviction_priority_queue) {
       eviction_priority_queue = client_->BuildEvictionQueue();
@@ -858,7 +886,8 @@
 
     Tile* tile = prioritized_tile.tile();
     *usage -= MemoryUsage::FromTile(tile);
-    FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(tile);
+    *freed_required_for_draw_tile |=
+        FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(tile);
     eviction_priority_queue->Pop();
   }
   return eviction_priority_queue;
@@ -940,7 +969,9 @@
       if (is_solid_color) {
         tile->draw_info().set_solid_color(color);
         client_->NotifyTileStateChanged(tile, /*update_damage=*/true,
-                                        /*set_needs_redraw=*/true);
+                                        /*set_needs_redraw=*/false);
+        work_to_schedule.required_for_draw_tile_state_changed |=
+            tile->required_for_draw();
         continue;
       }
     }
@@ -1003,7 +1034,8 @@
     eviction_priority_queue =
         FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit(
             std::move(eviction_priority_queue), scheduled_tile_memory_limit,
-            priority, &memory_usage);
+            priority, &memory_usage,
+            &work_to_schedule.required_for_draw_tile_state_changed);
     bool memory_usage_is_within_limit =
         !memory_usage.Exceeds(scheduled_tile_memory_limit);
 
@@ -1056,7 +1088,8 @@
   // didn't reduce memory. This ensures that we always release as many resources
   // as possible to stay within the memory limit.
   eviction_priority_queue = FreeTileResourcesUntilUsageIsWithinLimit(
-      std::move(eviction_priority_queue), hard_memory_limit, &memory_usage);
+      std::move(eviction_priority_queue), hard_memory_limit, &memory_usage,
+      &work_to_schedule.required_for_draw_tile_state_changed);
 
   // At this point, if we ran out of memory when allocating resources and we
   // couldn't go past even the NOW bin, this means we have evicted resources
@@ -1168,13 +1201,17 @@
   }
 }
 
-void TileManager::FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(
+bool TileManager::FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(
     Tile* tile) {
   TRACE_EVENT0("viz", __PRETTY_FUNCTION__);
   bool was_ready_to_draw = tile->draw_info().IsReadyToDraw();
   FreeResourcesForTile(tile);
+  // Do not request a redraw here; this is always called while a priority queue
+  // holding raw tile/tiling pointers is being iterated. The caller is
+  // responsible for requesting a single redraw once iteration is complete.
   client_->NotifyTileStateChanged(tile, /*update_damage=*/was_ready_to_draw,
-                                  /*set_needs_redraw=*/true);
+                                  /*set_needs_redraw=*/false);
+  return tile->required_for_draw();
 }
 
 void TileManager::PartitionImagesForCheckering(
@@ -1916,13 +1953,25 @@
       !work_to_schedule.tiles_to_raster.empty() &&
       work_to_schedule.tiles_to_raster.front().tile()->required_for_draw());
 
+  const bool required_for_draw_tile_state_changed =
+      work_to_schedule.required_for_draw_tile_state_changed;
+
   // |tiles_that_need_to_be_rasterized| will be empty when we reach a
   // steady memory state. Keep scheduling tasks until we reach this state.
   if (!work_to_schedule.tiles_to_raster.empty()) {
     ScheduleTasks(std::move(work_to_schedule));
+    if (required_for_draw_tile_state_changed) {
+      client_->SetNeedsRedraw(/*animation_only=*/false,
+                              /*skip_if_inside_draw=*/true);
+    }
     return;
   }
 
+  if (required_for_draw_tile_state_changed) {
+    client_->SetNeedsRedraw(/*animation_only=*/false,
+                            /*skip_if_inside_draw=*/true);
+  }
+
   // If we're not in SMOOTHNESS_TAKES_PRIORITY  mode, we should unlock all
   // images since we're technically going idle here at least for this frame.
   if (global_state_.tree_priority != SMOOTHNESS_TAKES_PRIORITY) {
diff --git a/cc/tiles/tile_manager.h b/cc/tiles/tile_manager.h
index 008d4c0..c1e6fa09 100644
--- a/cc/tiles/tile_manager.h
+++ b/cc/tiles/tile_manager.h
@@ -296,6 +296,11 @@
     // are not necessarily associated with any tile.
     std::vector<DrawImage> extra_prepaint_images;
     CheckerImageTracker::ImageDecodeQueue checker_image_decode_queue;
+    // True if a tile that is required for draw had its state changed (e.g.
+    // resource freed or solid color set) while assigning gpu memory. The
+    // caller should request a redraw once it has finished using the tile and
+    // tiling pointers held by this struct.
+    bool required_for_draw_tile_state_changed = false;
   };
 
   // Frees the resources of all occluded tiles.
@@ -309,7 +314,9 @@
 
   // True if tile resources are present and freed.
   void FreeResourcesForTile(Tile* tile);
-  void FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(Tile* tile);
+  // Returns true if `tile` is required for draw, in which case the caller
+  // should request a redraw once it has finished iterating tiles.
+  bool FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(Tile* tile);
   scoped_refptr<TileTask> CreateRasterTask(
       const PrioritizedTile& prioritized_tile,
       const TargetColorParams& target_color_params,
@@ -319,13 +326,15 @@
   FreeTileResourcesUntilUsageIsWithinLimit(
       std::unique_ptr<EvictionTilePriorityQueue> eviction_priority_queue,
       const MemoryUsage& limit,
-      MemoryUsage* usage);
+      MemoryUsage* usage,
+      bool* freed_required_for_draw_tile);
   std::unique_ptr<EvictionTilePriorityQueue>
   FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit(
       std::unique_ptr<EvictionTilePriorityQueue> eviction_priority_queue,
       const MemoryUsage& limit,
-      const TilePriority& oother_priority,
-      MemoryUsage* usage);
+      const TilePriority& other_priority,
+      MemoryUsage* usage,
+      bool* freed_required_for_draw_tile);
   bool TilePriorityViolatesMemoryPolicy(const TilePriority& priority);
   bool AreRequiredTilesReadyToDraw(RasterTilePriorityQueue::Type type) const;
   void CheckIfMoreTilesNeedToBePrepared();
diff --git a/cc/tiles/tile_manager_unittest.cc b/cc/tiles/tile_manager_unittest.cc
index eba043b..b7eaf7f9 100644
--- a/cc/tiles/tile_manager_unittest.cc
+++ b/cc/tiles/tile_manager_unittest.cc
@@ -46,6 +46,7 @@
 #include "cc/test/test_task_graph_runner.h"
 #include "cc/test/test_tile_priorities.h"
 #include "cc/tiles/eviction_tile_priority_queue.h"
+#include "cc/tiles/picture_layer_tiling_set.h"
 #include "cc/tiles/raster_tile_priority_queue.h"
 #include "cc/tiles/tile.h"
 #include "cc/tiles/tile_priority.h"
@@ -1897,6 +1898,89 @@
   }
 }
 
+class TileManagerDeferredRedrawTest : public TestLayerTreeHostBase {
+ public:
+  // Host impl that simulates a client which removes tilings synchronously when
+  // notified of a tile state change with `set_needs_redraw` set. TileManager
+  // must defer SetNeedsRedraw() until it has finished iterating its priority
+  // queues so that it is safe for the client to do this.
+  class ReentrantHostImpl : public FakeLayerTreeHostImpl {
+   public:
+    using FakeLayerTreeHostImpl::FakeLayerTreeHostImpl;
+
+    void NotifyTileStateChanged(const Tile* tile,
+                                bool update_damage,
+                                bool set_needs_redraw) override {
+      if (set_needs_redraw && tilings_to_remove_) {
+        ++notify_with_redraw_count_;
+        PictureLayerTilingSet* tilings = tilings_to_remove_;
+        tilings_to_remove_ = nullptr;
+        tilings->RemoveAllTilings();
+      }
+    }
+
+    void SetNeedsRedraw(bool animation_only,
+                        bool skip_if_inside_draw) override {
+      ++set_needs_redraw_count_;
+    }
+
+    void set_tilings_to_remove(PictureLayerTilingSet* tilings) {
+      tilings_to_remove_ = tilings;
+    }
+    int notify_with_redraw_count() const { return notify_with_redraw_count_; }
+    int set_needs_redraw_count() const { return set_needs_redraw_count_; }
+
+   private:
+    raw_ptr<PictureLayerTilingSet> tilings_to_remove_ = nullptr;
+    int notify_with_redraw_count_ = 0;
+    int set_needs_redraw_count_ = 0;
+  };
+
+  std::unique_ptr<FakeLayerTreeHostImpl> CreateHostImpl(
+      const LayerTreeSettings& settings,
+      TaskRunnerProvider* task_runner_provider,
+      TaskGraphRunner* task_graph_runner) override {
+    return std::make_unique<ReentrantHostImpl>(settings, task_runner_provider,
+                                               task_graph_runner);
+  }
+
+  std::unique_ptr<LayerTreeFrameSink> CreateLayerTreeFrameSink() override {
+    return FakeLayerTreeFrameSink::CreateSoftware();
+  }
+
+  ReentrantHostImpl* reentrant_host_impl() {
+    return static_cast<ReentrantHostImpl*>(host_impl());
+  }
+};
+
+TEST_F(TileManagerDeferredRedrawTest, DeferRedrawWhileFreeingTileResources) {
+  const gfx::Size layer_bounds(1000, 1000);
+  host_impl()->active_tree()->SetDeviceViewportRect(gfx::Rect(layer_bounds));
+  SetupDefaultTrees(layer_bounds);
+
+  std::vector<Tile*> active_tiles =
+      active_layer()->HighResTiling()->AllTilesForTesting();
+  ASSERT_GT(active_tiles.size(), 1u);
+  host_impl()->tile_manager()->InitializeTilesWithResourcesForTesting(
+      active_tiles);
+
+  // Reduce the memory limit so that all active tiles will be evicted on the
+  // next PrepareTiles().
+  auto global_state = host_impl()->global_tile_state();
+  global_state.hard_memory_limit_in_bytes = 0u;
+  global_state.soft_memory_limit_in_bytes = 0u;
+  reentrant_host_impl()->set_tilings_to_remove(active_layer()->tilings());
+
+  // Freeing tile resources notifies the client of each tile's state change but
+  // must defer SetNeedsRedraw() until the priority queues are no longer in use.
+  // No tile state change notification should request a redraw while resources
+  // are being freed; instead a single SetNeedsRedraw() should be issued
+  // afterwards.
+  host_impl()->tile_manager()->PrepareTiles(global_state);
+  EXPECT_EQ(0, reentrant_host_impl()->notify_with_redraw_count());
+  EXPECT_LE(1, reentrant_host_impl()->set_needs_redraw_count());
+}
+
 class TileManagerOcclusionTest : public TileManagerTest {
  public:
   LayerTreeSettings CreateSettings() override {

diff --git a/cc/tiles/tile_manager_unittest.cc b/cc/tiles/tile_manager_unittest.cc
index eba043b..b7eaf7f9 100644
--- a/cc/tiles/tile_manager_unittest.cc
+++ b/cc/tiles/tile_manager_unittest.cc
@@ -46,6 +46,7 @@
 #include "cc/test/test_task_graph_runner.h"
 #include "cc/test/test_tile_priorities.h"
 #include "cc/tiles/eviction_tile_priority_queue.h"
+#include "cc/tiles/picture_layer_tiling_set.h"
 #include "cc/tiles/raster_tile_priority_queue.h"
 #include "cc/tiles/tile.h"
 #include "cc/tiles/tile_priority.h"
@@ -1897,6 +1898,89 @@
   }
 }
 
+class TileManagerDeferredRedrawTest : public TestLayerTreeHostBase {
+ public:
+  // Host impl that simulates a client which removes tilings synchronously when
+  // notified of a tile state change with `set_needs_redraw` set. TileManager
+  // must defer SetNeedsRedraw() until it has finished iterating its priority
+  // queues so that it is safe for the client to do this.
+  class ReentrantHostImpl : public FakeLayerTreeHostImpl {
+   public:
+    using FakeLayerTreeHostImpl::FakeLayerTreeHostImpl;
+
+    void NotifyTileStateChanged(const Tile* tile,
+                                bool update_damage,
+                                bool set_needs_redraw) override {
+      if (set_needs_redraw && tilings_to_remove_) {
+        ++notify_with_redraw_count_;
+        PictureLayerTilingSet* tilings = tilings_to_remove_;
+        tilings_to_remove_ = nullptr;
+        tilings->RemoveAllTilings();
+      }
+    }
+
+    void SetNeedsRedraw(bool animation_only,
+                        bool skip_if_inside_draw) override {
+      ++set_needs_redraw_count_;
+    }
+
+    void set_tilings_to_remove(PictureLayerTilingSet* tilings) {
+      tilings_to_remove_ = tilings;
+    }
+    int notify_with_redraw_count() const { return notify_with_redraw_count_; }
+    int set_needs_redraw_count() const { return set_needs_redraw_count_; }
+
+   private:
+    raw_ptr<PictureLayerTilingSet> tilings_to_remove_ = nullptr;
+    int notify_with_redraw_count_ = 0;
+    int set_needs_redraw_count_ = 0;
+  };
+
+  std::unique_ptr<FakeLayerTreeHostImpl> CreateHostImpl(
+      const LayerTreeSettings& settings,
+      TaskRunnerProvider* task_runner_provider,
+      TaskGraphRunner* task_graph_runner) override {
+    return std::make_unique<ReentrantHostImpl>(settings, task_runner_provider,
+                                               task_graph_runner);
+  }
+
+  std::unique_ptr<LayerTreeFrameSink> CreateLayerTreeFrameSink() override {
+    return FakeLayerTreeFrameSink::CreateSoftware();
+  }
+
+  ReentrantHostImpl* reentrant_host_impl() {
+    return static_cast<ReentrantHostImpl*>(host_impl());
+  }
+};
+
+TEST_F(TileManagerDeferredRedrawTest, DeferRedrawWhileFreeingTileResources) {
+  const gfx::Size layer_bounds(1000, 1000);
+  host_impl()->active_tree()->SetDeviceViewportRect(gfx::Rect(layer_bounds));
+  SetupDefaultTrees(layer_bounds);
+
+  std::vector<Tile*> active_tiles =
+      active_layer()->HighResTiling()->AllTilesForTesting();
+  ASSERT_GT(active_tiles.size(), 1u);
+  host_impl()->tile_manager()->InitializeTilesWithResourcesForTesting(
+      active_tiles);
+
+  // Reduce the memory limit so that all active tiles will be evicted on the
+  // next PrepareTiles().
+  auto global_state = host_impl()->global_tile_state();
+  global_state.hard_memory_limit_in_bytes = 0u;
+  global_state.soft_memory_limit_in_bytes = 0u;
+  reentrant_host_impl()->set_tilings_to_remove(active_layer()->tilings());
+
+  // Freeing tile resources notifies the client of each tile's state change but
+  // must defer SetNeedsRedraw() until the priority queues are no longer in use.
+  // No tile state change notification should request a redraw while resources
+  // are being freed; instead a single SetNeedsRedraw() should be issued
+  // afterwards.
+  host_impl()->tile_manager()->PrepareTiles(global_state);
+  EXPECT_EQ(0, reentrant_host_impl()->notify_with_redraw_count());
+  EXPECT_LE(1, reentrant_host_impl()->set_needs_redraw_count());
+}
+
 class TileManagerOcclusionTest : public TileManagerTest {
  public:
   LayerTreeSettings CreateSettings() override {