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

This is a reland of 9771027b

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}

Bug: 1154868
Change-Id: Ib5a1fdd6b5778d350931663862c3204b1bab34fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575850Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834520}

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->GetFirstDeskSwitchAnimatorForTesting()
+    animation_layer_ = animation->GetDeskSwitchAnimatorAtIndexForTesting(0)
                            ->GetAnimationLayerForTesting();
     animation_layer_->GetAnimator()->AddObserver(this);
   }

ash/wm/desks/desk_animation_base.cc

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

ash/wm/desks/desk_animation_base.h

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

ash/wm/desks/desk_animation_impl.cc

@@ -129,7 +129,8 @@ bool DeskActivationAnimation::Replace(bool moving_left,
   }
 
   // Activate the target desk and take a screenshot.
-  DCHECK_EQ(pending_animators.size(), desk_switch_animators_.size());
+  // TODO(crbug.com/1134390): Convert back to DCHECK when the issue is fixed.
+  CHECK_EQ(pending_animators.size(), desk_switch_animators_.size());
   PrepareDeskForScreenshot(new_ending_desk_index);
   for (auto* animator : pending_animators)
     animator->TakeEndingDeskScreenshot();
@@ -142,28 +143,27 @@ bool DeskActivationAnimation::UpdateSwipeAnimation(float scroll_delta_x) {
 
   presentation_time_recorder_->RequestNext();
 
-  // 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;
+  // 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;
   for (const auto& animator : desk_switch_animators_) {
-    if (animator->UpdateSwipeAnimation(scroll_delta_x))
-      pending_animators.push_back(animator.get());
+    if (!ending_desk_index)
+      ending_desk_index = animator->UpdateSwipeAnimation(scroll_delta_x);
+    else
+      animator->UpdateSwipeAnimation(scroll_delta_x);
   }
 
   // No screenshot needed.
-  if (pending_animators.empty()) {
-    OnEndingDeskScreenshotTaken();
+  if (!ending_desk_index)
     return true;
-  }
 
   // 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());
-  ending_desk_index_ = desk_switch_animators_[0]->ending_desk_index();
+  ending_desk_index_ = *ending_desk_index;
   PrepareDeskForScreenshot(ending_desk_index_);
-  for (auto* animator : pending_animators)
+  for (const auto& animator : desk_switch_animators_) {
+    animator->PrepareForEndingDeskScreenshot(ending_desk_index_);
     animator->TakeEndingDeskScreenshot();
+  }
   return true;
 }
 

ash/wm/desks/desk_animation_impl_unittest.cc

@@ -8,6 +8,8 @@
 #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 {
@@ -33,4 +35,45 @@ 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,9 +248,10 @@ bool RootWindowDeskSwitchAnimator::ReplaceAnimation(int new_ending_desk_index) {
   return true;
 }
 
-bool RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(float scroll_delta_x) {
+base::Optional<int> RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(
+    float scroll_delta_x) {
   if (!starting_desk_screenshot_taken_ || !ending_desk_screenshot_taken_)
-    return false;
+    return base::nullopt;
 
   const float translation_delta_x =
       TouchpadToXTranslation(scroll_delta_x, x_translation_offset_);
@@ -304,13 +305,16 @@ bool RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(float scroll_delta_x) {
           : 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 false;
+    return base::nullopt;
 
   // The upcoming desk we need to show will be an adjacent desk to the desk at
   // |visible_desk_index_| based on |moving_left|.
@@ -318,13 +322,17 @@ bool RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(float scroll_delta_x) {
 
   if (new_desk_index < 0 ||
       new_desk_index >= int{DesksController::Get()->desks().size()}) {
-    return false;
+    return base::nullopt;
   }
 
-  ending_desk_index_ = new_desk_index;
+  return new_desk_index;
+}
+
+void RootWindowDeskSwitchAnimator::PrepareForEndingDeskScreenshot(
+    int new_ending_desk_index) {
+  ending_desk_index_ = new_ending_desk_index;
   ending_desk_screenshot_retries_ = 0;
   ending_desk_screenshot_taken_ = false;
-  return true;
 }
 
 int RootWindowDeskSwitchAnimator::EndSwipeAnimation() {
@@ -346,9 +354,13 @@ int RootWindowDeskSwitchAnimator::EndSwipeAnimation() {
   if (!ending_desk_screenshot_taken_)
     weak_ptr_factory_.InvalidateWeakPtrs();
 
-  ending_desk_index_ = visible_desk_index_;
+  // In tests, StartAnimation() may trigger OnDeskSwitchAnimationFinished()
+  // right away which may delete |this|. Store the target index in a
+  // local so we do not try to access a member of a deleted object.
+  const int ending_desk_index = visible_desk_index_;
+  ending_desk_index_ = ending_desk_index;
   StartAnimation();
-  return ending_desk_index_;
+  return ending_desk_index;
 }
 
 void RootWindowDeskSwitchAnimator::OnImplicitAnimationsCompleted() {

ash/wm/desks/root_window_desk_switch_animator.h

@@ -263,8 +263,17 @@ 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.
-  bool UpdateSwipeAnimation(float scroll_delta_x);
+  // 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);
 
   // 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,12 +486,18 @@ 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.
-  ASSERT_TRUE(
-      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change));
+  base::Optional<int> new_index =
+      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
+  ASSERT_TRUE(new_index.has_value());
+  animator()->PrepareForEndingDeskScreenshot(*new_index);
   TakeEndingDeskScreenshotAndWait();
-  ASSERT_TRUE(
-      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change));
+
+  new_index =
+      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
+  ASSERT_TRUE(new_index.has_value());
+  animator()->PrepareForEndingDeskScreenshot(*new_index);
   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->GetFirstDeskSwitchAnimatorForTesting();
+          animation->GetDeskSwitchAnimatorAtIndexForTesting(0);
       base::RunLoop run_loop;
       RootWindowDeskSwitchAnimatorTestApi(desk_switch_animator)
           .SetOnEndingScreenshotTakenCallback(run_loop.QuitClosure());