Fix AnimationThroughputReporter leak when layer gone

Fix AnimationThroughputReporter leaks its AnimationTracker when
the underlying layer is released before the reporter. In such
case, AnimationTracker holds a ref count of LayerAnimator so
that that the animator and the animation sequences are kept
around even if the underlying layer is released. And the animations
will never run in such case and the tracker is leaked. The CL
fixes the issue by observing LayerDestroyed to clean up.

Also rename IsTrackingAnimation -> HasAnimationsToTrack to better
reflect its meaning that returning a true means there will be
animations to be tracked but does not mean it is tracking them
right now.

Bug: 1158510
Change-Id: I98980522a9b6866b0f635a512d0fbb6987eb2c59
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2590646
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837388}

ui/compositor/animation_throughput_reporter.cc

@@ -12,6 +12,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "base/scoped_observation.h"
 #include "cc/animation/animation.h"
 #include "ui/compositor/callback_layer_animation_observer.h"
 #include "ui/compositor/compositor.h"
@@ -19,27 +20,58 @@
 #include "ui/compositor/layer_animation_delegate.h"
 #include "ui/compositor/layer_animation_sequence.h"
 #include "ui/compositor/layer_animator.h"
+#include "ui/compositor/layer_observer.h"
 #include "ui/compositor/throughput_tracker.h"
 
 namespace ui {
 
+// AnimationTracker tracks the layer animations that are created during the
+// lifetime of its owner AnimationThroughputReporter.
+//
+// Lifetime of this tracker class is a bit complicated. If there are animations
+// to track (i.e. HasAnimationsToTrack() returns true) when the owner reporter
+// is going away, it needs to have the same lifetime of the animations to track
+// the performance. In such case, the owner reporter would drop the ownership
+// and set set_should_delete() to let the tracker manages its own lifetime
+// based on LayerDetroyed and LayerAnimationObserver signals. On the other hand,
+// if there are no animations to track, the tracker is released with its owner
+// reporter.
 class AnimationThroughputReporter::AnimationTracker
-    : public CallbackLayerAnimationObserver {
+    : public CallbackLayerAnimationObserver,
+      public LayerObserver {
  public:
-  AnimationTracker(LayerAnimator* animator, ReportCallback report_callback)
+  AnimationTracker(Layer* layer, ReportCallback report_callback)
       : CallbackLayerAnimationObserver(
             base::BindRepeating(&AnimationTracker::OnAnimationEnded,
                                 base::Unretained(this))),
-        animator_(animator),
+        animator_(layer->GetAnimator()),
         report_callback_(std::move(report_callback)) {
     DCHECK(report_callback_);
+    layer_observation_.Observe(layer);
   }
+
   AnimationTracker(const AnimationTracker& other) = delete;
   AnimationTracker& operator=(const AnimationTracker& other) = delete;
-  ~AnimationTracker() override = default;
 
-  // Whether there are attached animation sequences to track.
-  bool IsTrackingAnimation() const { return !attached_sequences().empty(); }
+  ~AnimationTracker() override {
+    // No auto delete in the observer callbacks since `this` is being
+    // destructed.
+    should_delete_ = false;
+
+    // Cancels existing tracking if any.
+    throughput_tracker_.reset();
+
+    // Stops observing animations so that `animator` destruction does not call
+    // back into half destructed `this` if `this` holds the last reference of
+    // `animator_`.
+    StopObserving();
+  }
+
+  // Whether there are/will be animations to track. That is, there is an
+  // underlying layer and there are attached animation sequences.
+  bool HasAnimationsToTrack() const {
+    return layer_observation_.IsObserving() && !attached_sequences().empty();
+  }
 
   void set_should_delete(bool should_delete) { should_delete_ = should_delete; }
 
@@ -83,6 +115,17 @@ class AnimationThroughputReporter::AnimationTracker
     CallbackLayerAnimationObserver::OnLayerAnimationAborted(sequence);
   }
 
+  // LayerObserver:
+  void LayerDestroyed(Layer* layer) override {
+    DCHECK(layer_observation_.IsObservingSource(layer));
+
+    layer_observation_.Reset();
+
+    // No more tracking needed when underlying layer is gone.
+    if (should_delete_)
+      delete this;
+  }
+
   void MaybeStartTracking() {
     // No tracking if no layer animation sequence is started.
     if (!first_animation_group_id_.has_value())
@@ -119,6 +162,7 @@ class AnimationThroughputReporter::AnimationTracker
   // Whether this class should delete itself on animation ended.
   bool should_delete_ = false;
 
+  base::ScopedObservation<Layer, LayerObserver> layer_observation_{this};
   scoped_refptr<LayerAnimator> animator_;
 
   base::Optional<ThroughputTracker> throughput_tracker_;
@@ -134,7 +178,7 @@ AnimationThroughputReporter::AnimationThroughputReporter(
     ReportCallback report_callback)
     : animator_(animator),
       animation_tracker_(
-          std::make_unique<AnimationTracker>(animator_,
+          std::make_unique<AnimationTracker>(animator_->delegate()->GetLayer(),
                                              std::move(report_callback))) {
   animator_->AddObserver(animation_tracker_.get());
 }
@@ -146,7 +190,7 @@ AnimationThroughputReporter::~AnimationThroughputReporter() {
   animator_->observers_.RemoveObserver(animation_tracker_.get());
 
   // |animation_tracker_| deletes itself when its tracked animations finish.
-  if (animation_tracker_->IsTrackingAnimation())
+  if (animation_tracker_->HasAnimationsToTrack())
     animation_tracker_.release()->set_should_delete(true);
 }
 

ui/compositor/animation_throughput_reporter_unittest.cc

@@ -152,6 +152,32 @@ TEST_F(AnimationThroughputReporterTest, AbortedAnimation) {
   Advance(base::TimeDelta::FromMilliseconds(100));
 }
 
+// Tests no report and no leak when underlying layer is gone before reporter.
+TEST_F(AnimationThroughputReporterTest, LayerDestroyedBeforeReporter) {
+  auto layer = std::make_unique<Layer>();
+  layer->SetOpacity(0.5f);
+  root_layer()->Add(layer.get());
+
+  LayerAnimator* animator = layer->GetAnimator();
+  AnimationThroughputReporter reporter(
+      animator, base::BindLambdaForTesting(
+                    [&](const cc::FrameSequenceMetrics::CustomReportData&) {
+                      ADD_FAILURE() << "No report for aborted animations.";
+                    }));
+
+  {
+    ScopedLayerAnimationSettings settings(animator);
+    settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(48));
+    layer->SetOpacity(1.0f);
+  }
+
+  // Delete |layer| to before the reporter.
+  layer.reset();
+
+  // Wait a bit to ensure that report does not happen.
+  Advance(base::TimeDelta::FromMilliseconds(100));
+}
+
 // Tests animation throughput not reported when detached from timeline.
 TEST_F(AnimationThroughputReporterTest, NoReportOnDetach) {
   auto layer = std::make_unique<Layer>();