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}

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() {

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());