Fix a leak in wm::AnimateOnChildWindowVisibilityChanged().

ui/wm/core/window_animations.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/wm/core/window_animations.h"
 
 #include <math.h>
 
 #include <algorithm>
 #include <vector>
@@ skipping 40 matching lines @@
 // 1) Notifies AnimationHost at the end of hiding animation.
 // 2) Detaches the window's layers for hiding animation and deletes
 // them upon completion of the animation. This is necessary to a)
 // ensure that the animation continues in the event of the window being
 // deleted, and b) to ensure that the animation is visible even if the
 // window gets restacked below other windows when focus or activation
 // changes.
 // The subclass will determine when the animation is completed.
 class HidingWindowAnimationObserverBase : public aura::WindowObserver {
  public:
-  HidingWindowAnimationObserverBase(aura::Window* window) : window_(window) {
+  explicit HidingWindowAnimationObserverBase(aura::Window* window)
+      : window_(window) {
     window_->AddObserver(this);
   }
   virtual ~HidingWindowAnimationObserverBase() {
     if (window_)
       window_->RemoveObserver(this);
   }
 
   // aura::WindowObserver:
   virtual void OnWindowDestroying(aura::Window* window) OVERRIDE {
     DCHECK_EQ(window, window_);
@@ skipping 29 matching lines @@
           topmost_transient_child = *iter;
         }
       }
       if (topmost_transient_child) {
         window_->parent()->layer()->StackAbove(
             layer_owner_->root(), topmost_transient_child->layer());
       }
     }
   }
 
+  // Returns the root of the detached layer tree. Should be called only after
+  // DetachAndRecreateLayers(). The caller does not own the result.
+  ui::Layer* detached_layer_tree_root() { return layer_owner_->root(); }
+
  protected:
   // Invoked when the hiding animation is completed.  It will delete
   // 'this', and no operation should be made on this object after this
   // point.
   void OnAnimationCompleted() {
     // Window may have been destroyed by this point.
     if (window_) {
       aura::client::AnimationHost* animation_host =
           aura::client::GetAnimationHost(window_);
       if (animation_host)
@@ skipping 268 matching lines @@
   sequence->AddElement(CreateGrowShrinkElement(window, false));
   window->layer()->GetAnimator()->StartAnimation(sequence.release());
 }
 
 // A HidingWindowAnimationObserver that deletes observer and detached
 // layers when the last_sequence has been completed or aborted.
 class RotateHidingWindowAnimationObserver
     : public HidingWindowAnimationObserverBase,
       public ui::LayerAnimationObserver {
  public:
-  RotateHidingWindowAnimationObserver(aura::Window* window)
-      : HidingWindowAnimationObserverBase(window), last_sequence_(NULL) {}
+  explicit RotateHidingWindowAnimationObserver(aura::Window* window)
+      : HidingWindowAnimationObserverBase(window) {}
   virtual ~RotateHidingWindowAnimationObserver() {}
 
-  void set_last_sequence(ui::LayerAnimationSequence* last_sequence) {
-    last_sequence_ = last_sequence;
+  // The observer destroys itself after the |last_sequence| ends or is aborted.
+  // Does not take ownership of the |last_sequence| parameter, which should not
+  // be NULL.
+  void SetLastSequence(ui::LayerAnimationSequence* last_sequence) {
+    last_sequence->AddObserver(this);
   }
 
   // ui::LayerAnimationObserver:
   virtual void OnLayerAnimationEnded(
       ui::LayerAnimationSequence* sequence) OVERRIDE {
-    if (last_sequence_ == sequence)
-      OnAnimationCompleted();
+    OnAnimationCompleted();
   }
   virtual void OnLayerAnimationAborted(
       ui::LayerAnimationSequence* sequence) OVERRIDE {
-    if (last_sequence_ == sequence)
-      OnAnimationCompleted();
+    OnAnimationCompleted();
   }
   virtual void OnLayerAnimationScheduled(
       ui::LayerAnimationSequence* sequence) OVERRIDE {}
 
  private:
-  ui::LayerAnimationSequence* last_sequence_;
-
   DISALLOW_COPY_AND_ASSIGN(RotateHidingWindowAnimationObserver);
 };
 
 void AddLayerAnimationsForRotate(aura::Window* window, bool show) {
   if (show)
     window->layer()->SetOpacity(kWindowAnimation_HideOpacity);
 
   base::TimeDelta duration = base::TimeDelta::FromMilliseconds(
       kWindowAnimation_Rotate_DurationMS);
 
@@ skipping 40 matching lines @@
   scale_about_pivot->SetChild(perspective.release());
   translation->SetChild(scale_about_pivot.release());
   rotation->SetChild(translation.release());
   rotation->SetReversed(show);
 
   scoped_ptr<ui::LayerAnimationElement> transition(
       ui::LayerAnimationElement::CreateInterpolatedTransformElement(
           rotation.release(), duration));
   ui::LayerAnimationSequence* last_sequence =
       new ui::LayerAnimationSequence(transition.release());
-  window->layer()->GetAnimator()->ScheduleAnimation(last_sequence);
+
   if (observer) {
-    observer->set_last_sequence(last_sequence);
+    observer->SetLastSequence(last_sequence);
     observer->DetachAndRecreateLayers();
+    observer->detached_layer_tree_root()->GetAnimator()->ScheduleAnimation(

[sky, 2014/07/14 19:18:18]

Isn't the important part here your changes to SetLastSequence? In which case
can't you leave 484 old and nuke 492-495 new?

[please use gerrit instead, 2014/07/14 20:12:17]

Yes.
Yes, but then we would not have a unit test for checking the leak. Is that OK
with you? Here is why:

Calling [ window->layer()->GetAnimator()->ScheduleAnimation(last_sequence) ]
before [ observer->DetachAndRecreateLayers() ] works when running in the
browser, because scheduled animations will complete after exiting [
AddLayerAnimationsForRotate() ] function.

However, unit tests run animations in either (a) zero time or (b) not at all, in
my experience.

a) Zero time animations finish [ last_sequence ] before [ set_last_sequence() ],
so this causes a leak in the new unit test.

b) Any other speed of animations does not finish the animations in unit tests.
Only [ OnLayerAnimationScheduled() ] gets called. [ OnLayerAnimationEnded() ]
and [ OnLayerAnimationAborted() ] are never called. This also causes a leak in
the new unit test.

TL;DR: Simpler change means no unit test.

[please use gerrit instead, 2014/07/17 03:14:00]

Done.

+        last_sequence);
+  } else {
+    window->layer()->GetAnimator()->ScheduleAnimation(last_sequence);
   }
 }
 
 void AnimateShowWindow_Rotate(aura::Window* window) {
   AddLayerAnimationsForRotate(window, true);
 }
 
 void AnimateHideWindow_Rotate(aura::Window* window) {
   AddLayerAnimationsForRotate(window, false);
 }
@@ skipping 151 matching lines @@
 }
 
 bool WindowAnimationsDisabled(aura::Window* window) {
   return (!gfx::Animation::ShouldRenderRichAnimation() || (window &&
           window->GetProperty(aura::client::kAnimationsDisabledKey)) ||
       CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kWindowAnimationsDisabled));
 }
 
 }  // namespace wm