From c3b0bcd0d3b2f49d5126f0298796c0cdcfdc2921 Mon Sep 17 00:00:00 2001
From: Allen Bauer <[email protected]>
Date: Wed, 23 Sep 2026 14:32:26 -0700
Subject: [PATCH] Harden AnimationBuilder against layer deletions during scheduling.

For certain cases where a started animation is immediately finished when
scheduled, the callbacks invoked might destroy the view on which the
layer is animating. Any subsequence animation sequences for that layer
are now invalid and can lead to a UAF as they're being scheduled. This
tracks the lifetime of the layers while the AnimationBuilder instance
is scheduling the animations.

Change-Id: I759edbe4bebbb86c5f31b83b5930a8484d966d1b
Bug: 561997480
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8423046
Reviewed-by: Thomas Lukaszewicz <[email protected]>
Commit-Queue: Allen Bauer <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1703780}
---
diff --git a/ui/views/animation/animation_builder.cc b/ui/views/animation/animation_builder.cc
index cd58a25..8780291 100644
--- a/ui/views/animation/animation_builder.cc
+++ b/ui/views/animation/animation_builder.cc
@@ -5,6 +5,8 @@
 #include "ui/views/animation/animation_builder.h"
 
 #include <algorithm>
+#include <iterator>
+#include <memory>
 #include <tuple>
 #include <utility>
 #include <vector>
@@ -14,6 +16,7 @@
 #include "base/functional/callback.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"
 #include "base/no_destructor.h"
 #include "base/time/time.h"
 #include "ui/compositor/layer.h"
@@ -28,6 +31,15 @@
 #include "ui/views/animation/animation_sequence_block.h"
 
 namespace views {
+namespace {
+// Used during scheduling to track whether a layer had been destroyed due to
+// a previously scheduled animation sequence terminating early.
+struct LayerSequences {
+  base::WeakPtr<ui::Layer> layer;
+  std::vector<std::unique_ptr<ui::LayerAnimationSequence>> sequences;
+};
+
+}  // namespace
 
 AnimationBuilder::Observer::Observer() = default;
 
@@ -220,10 +232,30 @@
     animation_observer_.release();
   }
 
+  std::vector<LayerSequences> layer_sequences;
   for (auto it = layer_animation_sequences_.begin();
        it != layer_animation_sequences_.end();) {
     auto* const target = it->first;
     auto end_it = layer_animation_sequences_.upper_bound(target);
+    LayerSequences this_layer_sequences;
+    this_layer_sequences.layer = target->AsWeakPtr();
+    std::transform(it, end_it,
+                   std::back_inserter(this_layer_sequences.sequences),
+                   [](auto& it) { return std::move(it.second); });
+    layer_sequences.emplace_back(std::move(this_layer_sequences));
+    it = end_it;
+  }
+
+  for (auto& layer_sequence : layer_sequences) {
+    if (!layer_sequence.layer) {
+      // If the layer had been destroyed from an earlier animation sequence
+      // terminating early, make sure all remaining sequences are also aborted.
+      for (auto& sequence : layer_sequence.sequences) {
+        sequence->Abort(nullptr);
+      }
+      continue;
+    }
+    auto* const target = layer_sequence.layer.get();
 
     if (abort_handle_) {
       abort_handle_->AddLayer(target);
@@ -233,11 +265,13 @@
     if (preemption_strategy_) {
       settings.SetPreemptionStrategy(preemption_strategy_.value());
     }
+
     std::vector<ui::LayerAnimationSequence*> sequences;
-    std::transform(it, end_it, std::back_inserter(sequences),
-                   [](auto& it) { return it.second.release(); });
+    std::transform(layer_sequence.sequences.begin(),
+                   layer_sequence.sequences.end(),
+                   std::back_inserter(sequences),
+                   [](auto& sequence) { return sequence.release(); });
     target->GetAnimator()->StartTogether(std::move(sequences));
-    it = end_it;
   }
 }
 
diff --git a/ui/views/animation/animation_builder_unittest.cc b/ui/views/animation/animation_builder_unittest.cc
index fa80469..047f732 100644
--- a/ui/views/animation/animation_builder_unittest.cc
+++ b/ui/views/animation/animation_builder_unittest.cc
@@ -10,6 +10,7 @@
 
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
+#include "base/test/bind.h"
 #include "base/test/gtest_util.h"
 #include "base/time/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -1277,4 +1278,72 @@
             expected_end_transform);
 }
 
+// Regression test for the ~AnimationBuilder destructor loop dereferencing
+// stale ui::Layer* multimap keys after a re-entrant client callback deletes a
+// later animation target.
+//
+// The destructor iterates `layer_animation_sequences_` (keyed by bare
+// ui::Layer*, in pointer order) and calls target->GetAnimator() /
+// StartTogether() per target. StartTogether() synchronously dispatches
+// LayerAnimationObserver callbacks (e.g. via preemption of a conflicting
+// animation, Step() of an expired sequence, or
+// FinishAnyAnimationWithZeroDuration()). Production callbacks delete Views
+// and their Layers from these callbacks (e.g. ash
+// AshNotificationView::AnimateResizeAfterRemoval -> RemoveChildViewT()). If
+// such a callback deletes a *later* target of the builder, the loop resumes
+// with a dangling Layer* and dereferences freed memory.
+//
+// Under ASan this test reports heap-use-after-free in ui::Layer::GetAnimator
+// called from views::AnimationBuilder::~AnimationBuilder.
+TEST_F(AnimationBuilderTest, DestructorLoopUsesLayerDeletedByCallback) {
+  auto owner_a = std::make_unique<TestAnimatibleLayerOwner>();
+  auto owner_b = std::make_unique<TestAnimatibleLayerOwner>();
+
+  // The multimap is ordered by Layer* value; make `first` the target that the
+  // destructor loop visits first and `victim` the one visited afterwards.
+  const bool a_first = owner_a->layer() < owner_b->layer();
+  std::unique_ptr<TestAnimatibleLayerOwner>& first =
+      a_first ? owner_a : owner_b;
+  std::unique_ptr<TestAnimatibleLayerOwner>& victim =
+      a_first ? owner_b : owner_a;
+
+  ui::Layer* first_layer = first->layer();
+  ui::Layer* victim_layer = victim->layer();
+
+  // Builder 1: leave a pending bounds animation on `first` whose
+  // ended/aborted callback destroys `victim` and its Layer. This models the
+  // shipping ash grouped-notification callbacks
+  // (on_notification_slid_out / on_animation_aborted), which synchronously
+  // delete sibling child Views (and thus their Layers) via RemoveChildViewT.
+  {
+    AnimationBuilder builder;
+    builder.OnEnded(base::BindLambdaForTesting([&]() { victim.reset(); }));
+    builder.OnAborted(base::BindLambdaForTesting([&]() { victim.reset(); }));
+    builder.Once()
+        .SetDuration(base::Seconds(1))
+        .SetBounds(first_layer, gfx::Rect(10, 10, 100, 100));
+  }
+  ASSERT_TRUE(victim);
+  ASSERT_TRUE(first_layer->GetAnimator()->is_animating());
+
+  // Builder 2: one builder with sequences for both layers, mirroring
+  // AshNotificationView::AnimateResizeAfterRemoval() which adds a bounds
+  // sequence for every remaining grouped child to a single builder. Its
+  // destructor starts the `first` sequence; the animator's default
+  // IMMEDIATELY_SET_NEW_TARGET preemption synchronously finishes builder 1's
+  // conflicting animation, running the callback above which frees
+  // `victim_layer` while it is still a not-yet-visited key of the destructor
+  // loop. The loop then resumes and dereferences the freed Layer.
+  {
+    AnimationBuilder builder;
+    builder.Once()
+        .SetDuration(base::Seconds(1))
+        .SetBounds(first_layer, gfx::Rect(0, 0, 50, 50))
+        .SetBounds(victim_layer, gfx::Rect(0, 0, 60, 60));
+  }  // Heap-use-after-free: ~AnimationBuilder -> victim_layer->GetAnimator().
+
+  // Not reached under ASan.
+  EXPECT_FALSE(victim);
+}
+
 }  // namespace views

diff --git a/ui/views/animation/animation_builder_unittest.cc b/ui/views/animation/animation_builder_unittest.cc
index fa80469..047f732 100644
--- a/ui/views/animation/animation_builder_unittest.cc
+++ b/ui/views/animation/animation_builder_unittest.cc
@@ -10,6 +10,7 @@
 
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
+#include "base/test/bind.h"
 #include "base/test/gtest_util.h"
 #include "base/time/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -1277,4 +1278,72 @@
             expected_end_transform);
 }
 
+// Regression test for the ~AnimationBuilder destructor loop dereferencing
+// stale ui::Layer* multimap keys after a re-entrant client callback deletes a
+// later animation target.
+//
+// The destructor iterates `layer_animation_sequences_` (keyed by bare
+// ui::Layer*, in pointer order) and calls target->GetAnimator() /
+// StartTogether() per target. StartTogether() synchronously dispatches
+// LayerAnimationObserver callbacks (e.g. via preemption of a conflicting
+// animation, Step() of an expired sequence, or
+// FinishAnyAnimationWithZeroDuration()). Production callbacks delete Views
+// and their Layers from these callbacks (e.g. ash
+// AshNotificationView::AnimateResizeAfterRemoval -> RemoveChildViewT()). If
+// such a callback deletes a *later* target of the builder, the loop resumes
+// with a dangling Layer* and dereferences freed memory.
+//
+// Under ASan this test reports heap-use-after-free in ui::Layer::GetAnimator
+// called from views::AnimationBuilder::~AnimationBuilder.
+TEST_F(AnimationBuilderTest, DestructorLoopUsesLayerDeletedByCallback) {
+  auto owner_a = std::make_unique<TestAnimatibleLayerOwner>();
+  auto owner_b = std::make_unique<TestAnimatibleLayerOwner>();
+
+  // The multimap is ordered by Layer* value; make `first` the target that the
+  // destructor loop visits first and `victim` the one visited afterwards.
+  const bool a_first = owner_a->layer() < owner_b->layer();
+  std::unique_ptr<TestAnimatibleLayerOwner>& first =
+      a_first ? owner_a : owner_b;
+  std::unique_ptr<TestAnimatibleLayerOwner>& victim =
+      a_first ? owner_b : owner_a;
+
+  ui::Layer* first_layer = first->layer();
+  ui::Layer* victim_layer = victim->layer();
+
+  // Builder 1: leave a pending bounds animation on `first` whose
+  // ended/aborted callback destroys `victim` and its Layer. This models the
+  // shipping ash grouped-notification callbacks
+  // (on_notification_slid_out / on_animation_aborted), which synchronously
+  // delete sibling child Views (and thus their Layers) via RemoveChildViewT.
+  {
+    AnimationBuilder builder;
+    builder.OnEnded(base::BindLambdaForTesting([&]() { victim.reset(); }));
+    builder.OnAborted(base::BindLambdaForTesting([&]() { victim.reset(); }));
+    builder.Once()
+        .SetDuration(base::Seconds(1))
+        .SetBounds(first_layer, gfx::Rect(10, 10, 100, 100));
+  }
+  ASSERT_TRUE(victim);
+  ASSERT_TRUE(first_layer->GetAnimator()->is_animating());
+
+  // Builder 2: one builder with sequences for both layers, mirroring
+  // AshNotificationView::AnimateResizeAfterRemoval() which adds a bounds
+  // sequence for every remaining grouped child to a single builder. Its
+  // destructor starts the `first` sequence; the animator's default
+  // IMMEDIATELY_SET_NEW_TARGET preemption synchronously finishes builder 1's
+  // conflicting animation, running the callback above which frees
+  // `victim_layer` while it is still a not-yet-visited key of the destructor
+  // loop. The loop then resumes and dereferences the freed Layer.
+  {
+    AnimationBuilder builder;
+    builder.Once()
+        .SetDuration(base::Seconds(1))
+        .SetBounds(first_layer, gfx::Rect(0, 0, 50, 50))
+        .SetBounds(victim_layer, gfx::Rect(0, 0, 60, 60));
+  }  // Heap-use-after-free: ~AnimationBuilder -> victim_layer->GetAnimator().
+
+  // Not reached under ASan.
+  EXPECT_FALSE(victim);
+}
+
 }  // namespace views