desks: Speculative fix for crash.

The previous attempt [1] added checks for whether the starting or ending
screenshot was taken. In the case of the starting screenshot missing, it
would signal the animation to remove some RootWindowDeskSwitchAnimators,
leading to a vector out of bounds.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2506356

Bug: 1148607
Test: added regression test
Change-Id: I7f14df877cb93619a0ca8a85f5d0ad434dda3ad2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2536934
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828034}

ash/BUILD.gn

@@ -2256,6 +2256,7 @@ test("ash_unittests") {
     "wm/container_finder_unittest.cc",
     "wm/default_window_resizer_unittest.cc",
     "wm/desks/autotest_desks_api_unittests.cc",
+    "wm/desks/desk_animation_impl_unittest.cc",
     "wm/desks/desks_unittests.cc",
     "wm/desks/root_window_desk_switch_animator_unittest.cc",
     "wm/drag_window_resizer_unittest.cc",

ash/wm/desks/desk_animation_base.cc

@@ -128,6 +128,9 @@ void DeskAnimationBase::OnDeskSwitchAnimationFinished() {
   for (auto& observer : controller_->observers_)
     observer.OnDeskSwitchAnimationFinished();
 
+  if (skip_notify_controller_on_animation_finished_for_testing_)
+    return;
+
   controller_->OnAnimationFinished(this);
   // `this` is now deleted.
 }

ash/wm/desks/desk_animation_base.h

@@ -5,6 +5,7 @@
 #ifndef ASH_WM_DESKS_DESK_ANIMATION_BASE_H_
 #define ASH_WM_DESKS_DESK_ANIMATION_BASE_H_
 
+#include "ash/ash_export.h"
 #include "ash/public/cpp/metrics_util.h"
 #include "ash/wm/desks/desks_histogram_enums.h"
 #include "ash/wm/desks/root_window_desk_switch_animator.h"
@@ -19,7 +20,8 @@ class DesksController;
 // such as DeskActivationAnimation and DeskRemovalAnimation implement the
 // abstract interface of this class to handle the unique operations specific to
 // each animation type.
-class DeskAnimationBase : public RootWindowDeskSwitchAnimator::Delegate {
+class ASH_EXPORT DeskAnimationBase
+    : public RootWindowDeskSwitchAnimator::Delegate {
  public:
   DeskAnimationBase(DesksController* controller,
                     int ending_desk_index,
@@ -56,6 +58,10 @@ class DeskAnimationBase : public RootWindowDeskSwitchAnimator::Delegate {
   void OnDeskSwitchAnimationFinished() override;
   void OnVisibleDeskChanged() override;
 
+  void set_skip_notify_controller_on_animation_finished_for_testing(bool val) {
+    skip_notify_controller_on_animation_finished_for_testing_ = val;
+  }
+
   RootWindowDeskSwitchAnimator* GetFirstDeskSwitchAnimatorForTesting() const;
 
  protected:
@@ -97,6 +103,12 @@ class DeskAnimationBase : public RootWindowDeskSwitchAnimator::Delegate {
 
   // ThroughputTracker used for measuring this animation smoothness.
   ui::ThroughputTracker throughput_tracker_;
+
+  // If true, do not notify |controller_| when
+  // OnDeskSwitchAnimationFinished() is called. This class and
+  // DeskController are tied together in production code, but may not be in a
+  // test scenario.
+  bool skip_notify_controller_on_animation_finished_for_testing_ = false;
 };
 
 }  // namespace ash

ash/wm/desks/desk_animation_impl.cc

@@ -182,9 +182,8 @@ bool DeskActivationAnimation::EndSwipeAnimation() {
   // and update their ending desk index. When the animation is finished we will
   // activate that desk.
   for (const auto& animator : desk_switch_animators_)
-    animator->EndSwipeAnimation();
+    ending_desk_index_ = animator->EndSwipeAnimation();
 
-  ending_desk_index_ = desk_switch_animators_[0]->ending_desk_index();
   return true;
 }
 

ash/wm/desks/desk_animation_impl.h

@@ -5,6 +5,7 @@
 #ifndef ASH_WM_DESKS_DESK_ANIMATION_IMPL_H_
 #define ASH_WM_DESKS_DESK_ANIMATION_IMPL_H_
 
+#include "ash/ash_export.h"
 #include "ash/public/cpp/metrics_util.h"
 #include "ash/wm/desks/desk_animation_base.h"
 #include "ash/wm/desks/desks_histogram_enums.h"
@@ -14,7 +15,7 @@ namespace ash {
 class DesksController;
 class PresentationTimeRecorder;
 
-class DeskActivationAnimation : public DeskAnimationBase {
+class ASH_EXPORT DeskActivationAnimation : public DeskAnimationBase {
  public:
   DeskActivationAnimation(DesksController* controller,
                           int starting_desk_index,

ash/wm/desks/desk_animation_impl_unittest.cc

+// Copyright 2020 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 "ash/wm/desks/desk_animation_impl.h"
+
+#include "ash/public/cpp/ash_features.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/wm/desks/desks_controller.h"
+#include "ash/wm/desks/desks_histogram_enums.h"
+#include "base/test/scoped_feature_list.h"
+
+namespace ash {
+
+using DeskActivationAnimationTest = AshTestBase;
+
+// Tests that there is no crash when ending a swipe animation before the
+// starting screenshot has been taken. Regression test for
+// https://crbug.com/1148607.
+TEST_F(DeskActivationAnimationTest, EndSwipeBeforeStartingScreenshot) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kEnhancedDeskAnimations);
+
+  auto* desks_controller = DesksController::Get();
+  desks_controller->NewDesk(DesksCreationRemovalSource::kButton);
+
+  DeskActivationAnimation animation(desks_controller, 0, 1,
+                                    DesksSwitchSource::kDeskSwitchTouchpad,
+                                    /*update_window_activation=*/false);
+  animation.set_skip_notify_controller_on_animation_finished_for_testing(true);
+  animation.Launch();
+  animation.UpdateSwipeAnimation(10);
+  animation.EndSwipeAnimation();
+}
+
+}  // namespace ash

ash/wm/desks/root_window_desk_switch_animator.cc

@@ -75,6 +75,8 @@ void TakeScreenshot(
       viz::CopyOutputRequest::ResultFormat::RGBA_TEXTURE,
       std::move(on_screenshot_taken));
   screenshot_request->set_area(request_bounds);
+  screenshot_request->set_result_task_runner(
+      base::SequencedTaskRunnerHandle::Get());
   screenshot_layer->RequestCopyOfOutput(std::move(screenshot_request));
 }
 
@@ -325,7 +327,7 @@ bool RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(float scroll_delta_x) {
   return true;
 }
 
-void RootWindowDeskSwitchAnimator::EndSwipeAnimation() {
+int RootWindowDeskSwitchAnimator::EndSwipeAnimation() {
   // If the starting screenshot has not finished, just let our delegate know
   // that the desk animation is finished (and |this| will soon be deleted), and
   // go back to the starting desk.
@@ -333,7 +335,7 @@ void RootWindowDeskSwitchAnimator::EndSwipeAnimation() {
     animation_finished_ = true;
     ending_desk_index_ = starting_desk_index_;
     delegate_->OnDeskSwitchAnimationFinished();
-    return;
+    return ending_desk_index_;
   }
 
   // If the ending desk screenshot has not finished, |visible_desk_index_| will
@@ -344,6 +346,7 @@ void RootWindowDeskSwitchAnimator::EndSwipeAnimation() {
 
   ending_desk_index_ = visible_desk_index_;
   StartAnimation();
+  return ending_desk_index_;
 }
 
 void RootWindowDeskSwitchAnimator::OnImplicitAnimationsCompleted() {

ash/wm/desks/root_window_desk_switch_animator.h

@@ -266,8 +266,8 @@ class ASH_EXPORT RootWindowDeskSwitchAnimator
   bool UpdateSwipeAnimation(float scroll_delta_x);
 
   // Called when a user ends a touchpad swipe. This will animate to the most
-  // visible desk.
-  void EndSwipeAnimation();
+  // visible desk, whose index is also returned.
+  int EndSwipeAnimation();
 
   // ui::ImplicitAnimationObserver:
   void OnImplicitAnimationsCompleted() override;