fix heap-use-after-free when aborting an animation

Under certain circumstances, such as aborting an animation and then
reassigning a unique_ptr to a new CallbackLayerAnimationObserver (eg
InkDropRipple::SnapToState(InkDropState ink_drop_state), the logic to
detect deletion in CallbackLayerAnimationObserver ends up causing a
heap-use-after-free because
CallbackLayerAnimationObserver::CheckAllSequencesCompleted() overwrites
the destroyed_ pointer that CallbackLayerAnimationObserver::SetActive() set.

In order to fix this issue, use of the destroyed_ pointer was replaced
with local WeakPtrs in order to detect deletion.

Change-Id: I051657fae929eb4d7200f965561e9d9fb21b44f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1714626Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Jeremy Apthorp <jeremya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683029}

AUTHORS

@@ -444,6 +444,7 @@ Joe Knoll <joe.knoll@workday.com>
 Joe Thomas <mhx348@motorola.com>
 Joel Stanley <joel@jms.id.au>
 Johannes Rudolph <johannes.rudolph@googlemail.com>
+John Kleinschmidt <kleinschmidtorama@gmail.com>
 John Yani <vanuan@gmail.com>
 John Yoo <nearbyh13@gmail.com>
 Johnson Lin <johnson.lin@intel.com>

ui/compositor/callback_layer_animation_observer.cc

@@ -38,22 +38,18 @@ CallbackLayerAnimationObserver::CallbackLayerAnimationObserver(
           &CallbackLayerAnimationObserver::DummyAnimationStartedCallback)),
       animation_ended_callback_(animation_ended_callback) {}
 
-CallbackLayerAnimationObserver::~CallbackLayerAnimationObserver() {
-  if (destroyed_)
-    *destroyed_ = true;
-}
+CallbackLayerAnimationObserver::~CallbackLayerAnimationObserver() {}
 
 void CallbackLayerAnimationObserver::SetActive() {
   active_ = true;
 
-  bool destroyed = false;
-  destroyed_ = &destroyed;
+  base::WeakPtr<CallbackLayerAnimationObserver> weak_this =
+      weak_factory_.GetWeakPtr();
 
   CheckAllSequencesStarted();
 
-  if (destroyed)
+  if (!weak_this)
     return;
-  destroyed_ = nullptr;
 
   CheckAllSequencesCompleted();
 }
@@ -110,19 +106,17 @@ void CallbackLayerAnimationObserver::CheckAllSequencesStarted() {
 void CallbackLayerAnimationObserver::CheckAllSequencesCompleted() {
   if (active_ && GetNumSequencesCompleted() == attached_sequence_count_) {
     active_ = false;
-    bool destroyed = false;
-    destroyed_ = &destroyed;
-
+    base::WeakPtr<CallbackLayerAnimationObserver> weak_this =
+        weak_factory_.GetWeakPtr();
     bool should_delete = animation_ended_callback_.Run(*this);
 
-    if (destroyed) {
+    if (!weak_this) {
       if (should_delete)
         LOG(WARNING) << "CallbackLayerAnimationObserver was explicitly "
                         "destroyed AND was requested to be destroyed via the "
                         "AnimationEndedCallback's return value.";
       return;
     }
-    destroyed_ = nullptr;
 
     if (should_delete)
       delete this;

ui/compositor/callback_layer_animation_observer.h

@@ -167,9 +167,8 @@ class COMPOSITOR_EXPORT CallbackLayerAnimationObserver
   // The callback to invoke once all the animation sequences have finished.
   AnimationEndedCallback animation_ended_callback_;
 
-  // Set to true in the destructor (if non-NULL). Used to detect deletion while
-  // calling out.
-  bool* destroyed_ = nullptr;
+  // Used to detect deletion while calling out.
+  base::WeakPtrFactory<CallbackLayerAnimationObserver> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(CallbackLayerAnimationObserver);
 };

ui/compositor/callback_layer_animation_observer_unittest.cc

@@ -256,6 +256,47 @@ CallbackLayerAnimationObserverTest::CreateLayerAnimationSequence() {
   return sequences_.back().get();
 }
 
+class CallbackLayerAnimationObserverTestOverwrite
+    : public CallbackLayerAnimationObserverTest {
+ public:
+  CallbackLayerAnimationObserverTestOverwrite();
+
+ protected:
+  void AnimationStarted(const CallbackLayerAnimationObserver& observer);
+
+  std::unique_ptr<CallbackLayerAnimationObserver> CreateAnimationObserver();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackLayerAnimationObserverTestOverwrite);
+};
+
+CallbackLayerAnimationObserverTestOverwrite::
+    CallbackLayerAnimationObserverTestOverwrite() {
+  observer_ = CreateAnimationObserver();
+  observer_test_api_ =
+      std::make_unique<LayerAnimationObserverTestApi>(observer_.get());
+}
+
+void CallbackLayerAnimationObserverTestOverwrite::AnimationStarted(
+    const CallbackLayerAnimationObserver& observer) {
+  observer_->OnLayerAnimationAborted(sequences_.front().get());
+  observer_test_api_.reset();
+  // Replace the current observer with a new observer so that the destructor
+  // gets called on the current observer.
+  observer_ = CreateAnimationObserver();
+}
+
+std::unique_ptr<CallbackLayerAnimationObserver>
+CallbackLayerAnimationObserverTestOverwrite::CreateAnimationObserver() {
+  return std::make_unique<CallbackLayerAnimationObserver>(
+      base::BindRepeating(
+          &CallbackLayerAnimationObserverTestOverwrite::AnimationStarted,
+          base::Unretained(this)),
+      base::BindRepeating([](const CallbackLayerAnimationObserver& observer) {
+        return false;
+      }));
+}
+
 TEST(CallbackLayerAnimationObserverDestructionTest, VerifyFalseAutoDelete) {
   TestCallbacks callbacks;
   callbacks.set_should_delete_observer_on_animations_ended(false);
@@ -327,6 +368,23 @@ TEST(CallbackLayerAnimationObserverDestructionTest, AnimationEndedReturnsTrue) {
   EXPECT_TRUE(is_destroyed);
 }
 
+// Verifies that there are not heap-use-after-free errors when an observer has
+// its animation aborted and it gets destroyed due to a
+// unique_ptr<CallbackLayerAnimationObserver> being assigned a new value.
+TEST_F(CallbackLayerAnimationObserverTestOverwrite,
+       VerifyOverwriteOnAnimationStart) {
+  LayerAnimationSequence* sequence_1 = CreateLayerAnimationSequence();
+  LayerAnimationSequence* sequence_2 = CreateLayerAnimationSequence();
+
+  observer_test_api_->AttachedToSequence(sequence_1);
+  observer_test_api_->AttachedToSequence(sequence_2);
+  observer_->OnLayerAnimationStarted(sequence_1);
+  observer_->OnLayerAnimationStarted(sequence_2);
+  observer_->OnLayerAnimationEnded(sequence_1);
+  observer_->SetActive();
+  EXPECT_FALSE(observer_->active());
+}
+
 TEST_F(CallbackLayerAnimationObserverTest, VerifyInitialState) {
   EXPECT_FALSE(observer_->active());
   EXPECT_EQ(0, observer_->aborted_count());