animations: fix possible use-after-free in rotate animations

The use after free would happen if the animation duration was 0.
The reason is RotateHidingWindowAnimationObserver would end up being
destroyed when the animation was started. This is because if the
animation completes immediately, then
RotateHidingWindowAnimationObserver deletes itself, and
AddLayerAnimationsForRotate tries to use
RotateHidingWindowAnimationObserver after scheduling the aniation.

The fix is to only create the observer if the animation hasn't
completed.

I had originally tried to disable animations if the duration is 0,
as there is no point in doing any work. Unfortunately this proves
quite problematic. In addition to animating, the animations may update
other state, such as the bounds. I settled for adding a comment as to
being careful in disabling animations. My suspicion is there are likely
problems with disabling animations, but that's for another day.

BUG=1154677
TEST=covered by tests

Change-Id: I9f63d5c65a86c59d5ad82a8805e66a9a51ccc4b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2570101
Commit-Queue: Scott Violet <sky@chromium.org>
Auto-Submit: Scott Violet <sky@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836189}

chrome/browser/ui/views/desktop_capture/get_current_browsing_context_media_dialog_unittest.cc

@@ -102,11 +102,6 @@ class GetCurrentBrowsingContextMediaDialogTest
         base::BindOnce(&GetCurrentBrowsingContextMediaDialogTest::OnDialogDone,
                        base::Unretained(this)));
 
-    // TODO(crbug.com/1154677): Remove this work-around.
-    dialog_.GetHostForTesting()
-        ->GetWidget()
-        ->SetVisibilityChangedAnimationsEnabled(false);
-
     render_process_id_ = web_contents_->GetMainFrame()->GetProcess()->GetID();
     render_frame_id_ = web_contents_->GetMainFrame()->GetRoutingID();
   }

ui/wm/core/window_animations.cc

@@ -443,10 +443,7 @@ void AddLayerAnimationsForRotate(aura::Window* window, bool show) {
   base::TimeDelta duration = base::TimeDelta::FromMilliseconds(
       kWindowAnimation_Rotate_DurationMS);
 
-  RotateHidingWindowAnimationObserver* observer = nullptr;
-
   if (!show) {
-    observer = new RotateHidingWindowAnimationObserver(window);
     window->layer()->GetAnimator()->SchedulePauseForProperties(
         duration * (100 - kWindowAnimation_Rotate_OpacityDurationPercent) / 100,
         ui::LayerAnimationElement::OPACITY);
@@ -494,10 +491,17 @@ void AddLayerAnimationsForRotate(aura::Window* window, bool show) {
           std::move(rotation), duration);
   ui::LayerAnimationSequence* last_sequence =
       new ui::LayerAnimationSequence(std::move(transition));
+  auto weak_last_sequence = last_sequence->AsWeakPtr();
   window->layer()->GetAnimator()->ScheduleAnimation(last_sequence);
-
-  if (observer) {
-    observer->SetLastSequence(last_sequence);
+  // If the animation is immediate, then |last_sequence| will have been
+  // deleted.
+  last_sequence = nullptr;
+
+  if (!show && weak_last_sequence) {
+    // RotateHidingWindowAnimationObserver deletes itself when no longer
+    // needed.
+    auto* observer = new RotateHidingWindowAnimationObserver(window);
+    observer->SetLastSequence(weak_last_sequence.get());
     observer->DetachAndRecreateLayers();
   }
 
@@ -663,6 +667,15 @@ bool AnimateWindow(aura::Window* window, WindowAnimationType type) {
 }
 
 bool WindowAnimationsDisabled(aura::Window* window) {
+  // WARNING: this function is called from VisibilityController to determine
+  // if an animation should happen when the Window's visibility changes.
+  // Returning false results in VisibilityController applying default
+  // handling of the transition. This can result in dramatically different
+  // results than if an animation occurs. For example, VisibilityController
+  // doesn't change the opacity, yet many of the animations do. Similarly,
+  // ash's animations may change the bounds, which VisibilityController won't
+  // do. Take care when adding a new path that returns false.
+
   // Individual windows can choose to skip animations.
   if (window && window->GetProperty(aura::client::kAnimationsDisabledKey))
     return true;

ui/wm/core/window_animations_unittest.cc

@@ -285,7 +285,7 @@ TEST_F(WindowAnimationsTest, RotateHideNoLeak) {
       ui::ScopedAnimationDurationScaleMode::FAST_DURATION);
 
   std::unique_ptr<aura::Window> window(
-      aura::test::CreateTestWindowWithId(0, NULL));
+      aura::test::CreateTestWindowWithId(0, nullptr));
   ui::Layer* animating_layer = window->layer();
   wm::SetWindowVisibilityAnimationType(window.get(),
                                        WINDOW_VISIBILITY_ANIMATION_TYPE_ROTATE);
@@ -296,6 +296,37 @@ TEST_F(WindowAnimationsTest, RotateHideNoLeak) {
   animating_layer->GetAnimator()->StopAnimating();
 }
 
+// The rotation animation for hiding a window should not crash with a zero
+// duration.
+TEST_F(WindowAnimationsTest, RotateHideNoCrashZeroDuration) {
+  std::unique_ptr<aura::Window> window(
+      aura::test::CreateTestWindowWithId(0, nullptr));
+  wm::SetWindowVisibilityAnimationType(window.get(),
+                                       WINDOW_VISIBILITY_ANIMATION_TYPE_ROTATE);
+
+  AnimateOnChildWindowVisibilityChanged(window.get(), true);
+  AnimateOnChildWindowVisibilityChanged(window.get(), false);
+}
+
+TEST_F(WindowAnimationsTest, RotateHideCreatesNewLayer) {
+  ui::ScopedAnimationDurationScaleMode scale_mode(
+      ui::ScopedAnimationDurationScaleMode::FAST_DURATION);
+
+  std::unique_ptr<aura::Window> window(
+      aura::test::CreateTestWindowWithId(0, nullptr));
+  wm::SetWindowVisibilityAnimationType(window.get(),
+                                       WINDOW_VISIBILITY_ANIMATION_TYPE_ROTATE);
+  AnimateOnChildWindowVisibilityChanged(window.get(), true);
+  window->layer()->GetAnimator()->StopAnimating();
+
+  auto* original_layer = window->layer();
+  AnimateOnChildWindowVisibilityChanged(window.get(), false);
+  // The layer should have changed, as the Layer is cloned and detached.
+  EXPECT_NE(original_layer, window->layer());
+  // Need to stop the animation, otherwise there is a leak.
+  original_layer->GetAnimator()->StopAnimating();
+}
+
 // The rotation animation for hiding a window should not crash when terminated
 // by LayerAnimator::StopAnimating().
 TEST_F(WindowAnimationsTest, RotateHideNoCrash) {