Revert "desks: Fix crash when touchpad swipe with external monitors."

This reverts commit 9771027b.

Reason for revert: Breaks ash_unittests on ASAN bots (use after free)
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/39004/overview

Original change's description:
> desks: Fix crash when touchpad swipe with external monitors.
>
> The crash was because a new screenshot is requested when the current
> screenshot is about to get scrolled out of bounds. It was using a fixed
> number threshold, meaning displays that are not the same size could
> get out of sync easily, leading to a CHECK failure.
>
> This CL addresses that by requesting screenshot on all displays if even
> one of them needs one. Another approach would be change the spacing and
> threshold to be based on the display width instead of fixed number, but
> that could be prone to roundoff error which may cause a similar issue.
>
> Also noticed visible desk metric was double counting when there are
> multiple displays. Added a TODO for that one as it is lower priority.
>
> Bug: 1154868
> Test: manual, added regression test
> Change-Id: I48f5ef62d3284b710f44a2808f75c95686bc9b3d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2570551
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#833993}

TBR=afakhry@chromium.org,sammiequon@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1154868
Change-Id: I23cf043d8bc4ae5c9ed8531429b9ffcee43224e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575778Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834100}

ash/wm/desks/autotest_desks_api.cc

@@ -112,7 +112,7 @@ class ChainedDeskAnimationObserver : public ui::LayerAnimationObserver,
 
     auto* animation = DesksController::Get()->animation();
     DCHECK(animation);
-    animation_layer_ = animation->GetDeskSwitchAnimatorAtIndexForTesting(0)
+    animation_layer_ = animation->GetFirstDeskSwitchAnimatorForTesting()
                            ->GetAnimationLayerForTesting();
     animation_layer_->GetAnimator()->AddObserver(this);
   }

ash/wm/desks/desk_animation_base.cc

@@ -143,9 +143,9 @@ void DeskAnimationBase::OnVisibleDeskChanged() {
 }
 
 RootWindowDeskSwitchAnimator*
-DeskAnimationBase::GetDeskSwitchAnimatorAtIndexForTesting(size_t index) const {
-  DCHECK_LT(index, desk_switch_animators_.size());
-  return desk_switch_animators_[index].get();
+DeskAnimationBase::GetFirstDeskSwitchAnimatorForTesting() const {
+  DCHECK(!desk_switch_animators_.empty());
+  return desk_switch_animators_.front().get();
 }
 
 }  // namespace ash

ash/wm/desks/desk_animation_base.h

@@ -62,8 +62,7 @@ class ASH_EXPORT DeskAnimationBase
     skip_notify_controller_on_animation_finished_for_testing_ = val;
   }
 
-  RootWindowDeskSwitchAnimator* GetDeskSwitchAnimatorAtIndexForTesting(
-      size_t index) const;
+  RootWindowDeskSwitchAnimator* GetFirstDeskSwitchAnimatorForTesting() const;
 
  protected:
   // Abstract functions that can be overridden by child classes to do different

ash/wm/desks/desk_animation_impl.cc

@@ -129,8 +129,7 @@ bool DeskActivationAnimation::Replace(bool moving_left,
   }
 
   // Activate the target desk and take a screenshot.
-  // TODO(crbug.com/1134390): Convert back to DCHECK when the issue is fixed.
-  CHECK_EQ(pending_animators.size(), desk_switch_animators_.size());
+  DCHECK_EQ(pending_animators.size(), desk_switch_animators_.size());
   PrepareDeskForScreenshot(new_ending_desk_index);
   for (auto* animator : pending_animators)
     animator->TakeEndingDeskScreenshot();
@@ -143,27 +142,28 @@ bool DeskActivationAnimation::UpdateSwipeAnimation(float scroll_delta_x) {
 
   presentation_time_recorder_->RequestNext();
 
-  // If any of the displays need a new screenshot while scrolling, take the
-  // ending desk screenshot for all of them to keep them in sync.
-  base::Optional<int> ending_desk_index;
+  // List of animators that need a screenshot. It should be either empty or
+  // match the size of |desk_switch_animators_| as all the animations should be
+  // in sync.
+  std::vector<RootWindowDeskSwitchAnimator*> pending_animators;
   for (const auto& animator : desk_switch_animators_) {
-    if (!ending_desk_index)
-      ending_desk_index = animator->UpdateSwipeAnimation(scroll_delta_x);
-    else
-      animator->UpdateSwipeAnimation(scroll_delta_x);
+    if (animator->UpdateSwipeAnimation(scroll_delta_x))
+      pending_animators.push_back(animator.get());
   }
 
   // No screenshot needed.
-  if (!ending_desk_index)
+  if (pending_animators.empty()) {
+    OnEndingDeskScreenshotTaken();
     return true;
+  }
 
   // Activate the target desk and take a screenshot.
-  ending_desk_index_ = *ending_desk_index;
+  // TODO(crbug.com/1134390): Convert back to DCHECK when the issue is fixed.
+  CHECK_EQ(pending_animators.size(), desk_switch_animators_.size());
+  ending_desk_index_ = desk_switch_animators_[0]->ending_desk_index();
   PrepareDeskForScreenshot(ending_desk_index_);
-  for (const auto& animator : desk_switch_animators_) {
-    animator->PrepareForEndingDeskScreenshot(ending_desk_index_);
+  for (auto* animator : pending_animators)
     animator->TakeEndingDeskScreenshot();
-  }
   return true;
 }
 

ash/wm/desks/desk_animation_impl_unittest.cc

@@ -8,8 +8,6 @@
 #include "ash/test/ash_test_base.h"
 #include "ash/wm/desks/desks_controller.h"
 #include "ash/wm/desks/desks_histogram_enums.h"
-#include "ash/wm/desks/root_window_desk_switch_animator_test_api.h"
-#include "base/barrier_closure.h"
 #include "base/test/scoped_feature_list.h"
 
 namespace ash {
@@ -35,45 +33,4 @@ TEST_F(DeskActivationAnimationTest, EndSwipeBeforeStartingScreenshot) {
   animation.EndSwipeAnimation();
 }
 
-// Tests that there is no crash when swiping with external displays. Regression
-// test for https://crbug.com/1154868.
-TEST_F(DeskActivationAnimationTest, UpdateSwipeNewScreenshotCrash) {
-  // Crash is only reproducible on different resolution widths and easier to
-  // repro when the widths differ by a lot.
-  UpdateDisplay("600x600,601+0-2000x600");
-
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(features::kEnhancedDeskAnimations);
-
-  // Crash repro requires three desks.
-  auto* desks_controller = DesksController::Get();
-  desks_controller->NewDesk(DesksCreationRemovalSource::kButton);
-  desks_controller->NewDesk(DesksCreationRemovalSource::kButton);
-
-  DeskActivationAnimation animation(desks_controller, 1, 2,
-                                    DesksSwitchSource::kDeskSwitchTouchpad,
-                                    /*update_window_activation=*/false);
-  animation.set_skip_notify_controller_on_animation_finished_for_testing(true);
-  animation.Launch();
-
-  // Wait until all ending screenshots have been taken before swiping.
-  size_t num_animators = 2u;
-  base::RunLoop run_loop;
-  base::RepeatingClosure end_screenshot_callback =
-      base::BarrierClosure(num_animators, run_loop.QuitClosure());
-  for (size_t i = 0; i < num_animators; ++i) {
-    auto* desk_switch_animator =
-        animation.GetDeskSwitchAnimatorAtIndexForTesting(i);
-    RootWindowDeskSwitchAnimatorTestApi(desk_switch_animator)
-        .SetOnEndingScreenshotTakenCallback(end_screenshot_callback);
-  }
-  run_loop.Run();
-
-  // Swipe in a way which would have caused a crash using the old algorithm. See
-  // bug for more details.
-  animation.UpdateSwipeAnimation(-20);
-  animation.UpdateSwipeAnimation(10);
-  animation.EndSwipeAnimation();
-}
-
 }  // namespace ash

ash/wm/desks/root_window_desk_switch_animator.cc

@@ -248,10 +248,9 @@ bool RootWindowDeskSwitchAnimator::ReplaceAnimation(int new_ending_desk_index) {
   return true;
 }
 
-base::Optional<int> RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(
-    float scroll_delta_x) {
+bool RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(float scroll_delta_x) {
   if (!starting_desk_screenshot_taken_ || !ending_desk_screenshot_taken_)
-    return base::nullopt;
+    return false;
 
   const float translation_delta_x =
       TouchpadToXTranslation(scroll_delta_x, x_translation_offset_);
@@ -305,16 +304,13 @@ base::Optional<int> RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(
           : transformed_animation_layer_bounds.x() >
                 -kMinDistanceBeforeScreenshotDp;
 
-  // TODO(sammiequon): Make GetIndexOfMostVisibleDeskScreenshot() public and
-  // have DeskActivationAnimation keep track of |visible_desk_index_|. Right now
-  // OnVisibleDeskChanged will get called once for each display.
   const int old_visible_desk_index = visible_desk_index_;
   visible_desk_index_ = GetIndexOfMostVisibleDeskScreenshot();
   if (old_visible_desk_index != visible_desk_index_)
     delegate_->OnVisibleDeskChanged();
 
   if (!going_out_of_bounds)
-    return base::nullopt;
+    return false;
 
   // The upcoming desk we need to show will be an adjacent desk to the desk at
   // |visible_desk_index_| based on |moving_left|.
@@ -322,17 +318,13 @@ base::Optional<int> RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(
 
   if (new_desk_index < 0 ||
       new_desk_index >= int{DesksController::Get()->desks().size()}) {
-    return base::nullopt;
+    return false;
   }
 
-  return new_desk_index;
-}
-
-void RootWindowDeskSwitchAnimator::PrepareForEndingDeskScreenshot(
-    int new_ending_desk_index) {
-  ending_desk_index_ = new_ending_desk_index;
+  ending_desk_index_ = new_desk_index;
   ending_desk_screenshot_retries_ = 0;
   ending_desk_screenshot_taken_ = false;
+  return true;
 }
 
 int RootWindowDeskSwitchAnimator::EndSwipeAnimation() {

ash/wm/desks/root_window_desk_switch_animator.h

@@ -263,17 +263,8 @@ class ASH_EXPORT RootWindowDeskSwitchAnimator
   // Called as a user is performing a touchpad swipe. Requests a new screenshot
   // if necessary based on the last direction as specified in |scroll_delta_x|.
   // |scroll_delta_x| is in touchpad units, it will be converted to display
-  // units and then used to shift the animation layer. If the animation layer is
-  // near its boundaries, this will return an index for the desk we should take
-  // a screenshot for. If we are not near the boundaries, or if there is no next
-  // adjacent desk in the direction we are heading, return base::nullopt. The
-  // delegate is responsible for requesting the screenshot.
-  base::Optional<int> UpdateSwipeAnimation(float scroll_delta_x);
-
-  // Maybe called after UpdateSwipeAnimation() if we need a new screenshot.
-  // Updates |ending_desk_index_| and resets some other internal state related
-  // to the ending desk screenshot.
-  void PrepareForEndingDeskScreenshot(int new_ending_desk_index);
+  // units and then used to shift the animation layer.
+  bool UpdateSwipeAnimation(float scroll_delta_x);
 
   // Called when a user ends a touchpad swipe. This will animate to the most
   // visible desk, whose index is also returned.

ash/wm/desks/root_window_desk_switch_animator_unittest.cc

@@ -486,18 +486,12 @@ TEST_F(RootWindowDeskSwitchAnimatorTest, VisibleDeskChangeCount) {
   // Swipe enough so that our third and fourth desk screenshots are taken, and
   // then swipe so that the fourth desk is fully shown. There should be 3
   // visible desk changes in total.
-  base::Optional<int> new_index =
-      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
-  ASSERT_TRUE(new_index.has_value());
-  animator()->PrepareForEndingDeskScreenshot(*new_index);
+  ASSERT_TRUE(
+      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change));
   TakeEndingDeskScreenshotAndWait();
-
-  new_index =
-      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
-  ASSERT_TRUE(new_index.has_value());
-  animator()->PrepareForEndingDeskScreenshot(*new_index);
+  ASSERT_TRUE(
+      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change));
   TakeEndingDeskScreenshotAndWait();
-
   animator()->UpdateSwipeAnimation(-3 * touchpad_swipe_length_for_desk_change);
   EXPECT_EQ(3, visible_desk_changed_count());
 

ash/wm/gestures/wm_gesture_handler_unittest.cc

@@ -129,7 +129,7 @@ class WmGestureHandlerTest : public AshTestBase {
       auto* animation = DesksController::Get()->animation();
       DCHECK(animation);
       auto* desk_switch_animator =
-          animation->GetDeskSwitchAnimatorAtIndexForTesting(0);
+          animation->GetFirstDeskSwitchAnimatorForTesting();
       base::RunLoop run_loop;
       RootWindowDeskSwitchAnimatorTestApi(desk_switch_animator)
           .SetOnEndingScreenshotTakenCallback(run_loop.QuitClosure());