Fixing race conditions in ui::content::WindowSlider which could cause the overscroll overlay to nev…

content/browser/web_contents/aura/window_slider.cc

 // Copyright (c) 2013 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 "content/browser/web_contents/aura/window_slider.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "content/browser/web_contents/aura/shadow_layer_delegate.h"
 #include "content/public/browser/overscroll_configuration.h"
 #include "ui/aura/window.h"
 #include "ui/compositor/layer_animation_observer.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/events/event.h"
 
 namespace content {
 
 namespace {
 
-void DeleteLayerAndShadow(ui::Layer* layer,
-                          ShadowLayerDelegate* shadow) {
-  delete shadow;
-  delete layer;
-}
-
 // An animation observer that runs a callback at the end of the animation, and
 // destroys itself.
 class CallbackAnimationObserver : public ui::ImplicitAnimationObserver {
  public:
   CallbackAnimationObserver(const base::Closure& closure)
       : closure_(closure) {
   }
 
   virtual ~CallbackAnimationObserver() {}
 
@@ skipping 11 matching lines @@
 };
 
 }  // namespace
 
 WindowSlider::WindowSlider(Delegate* delegate,
                            aura::Window* event_window,
                            aura::Window* owner)
     : delegate_(delegate),
       event_window_(event_window),
       owner_(owner),
+      active_animator_(NULL),
       delta_x_(0.f),
       weak_factory_(this),
       active_start_threshold_(0.f),
       start_threshold_touchscreen_(content::GetOverscrollConfig(
           content::OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHSCREEN)),
       start_threshold_touchpad_(content::GetOverscrollConfig(
           content::OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHPAD)),
       complete_threshold_(content::GetOverscrollConfig(
           content::OVERSCROLL_CONFIG_HORIZ_THRESHOLD_COMPLETE)) {
   event_window_->AddPreTargetHandler(this);
@@ skipping 18 matching lines @@
   owner_ = new_owner;
   if (owner_) {
     owner_->AddObserver(this);
     UpdateForScroll(0.f, 0.f);
   }
 }
 
 bool WindowSlider::IsSlideInProgress() const {
   // if active_start_threshold_ is 0, it means that sliding hasn't been started
   return active_start_threshold_ != 0 &&
-      (fabs(delta_x_) >= active_start_threshold_ || slider_.get() ||
-      weak_factory_.HasWeakPtrs());
+      ((fabs(delta_x_) >= active_start_threshold_ && slider_.get()) ||

[sadrul, 2014/03/19 20:21:18]

This seems wrong? What if the slide starts, and then ends with delta_x_ <
active_start_threshold_?

[mfomitchev, 2014/03/20 15:10:06]

Good point! I think we can just get rid of the delta_x comparison. You can have
delta_x > active_start_threshold and slider == NULL if you are trying to
navigate past the last page, so || was wrong. I don't see any reason to look at
delta_x here...

[sadrul, 2014/03/24 20:20:46]

sounds good

+          active_animator_);
 }
 
 void WindowSlider::SetupSliderLayer() {
   ui::Layer* parent = owner_->layer()->parent();
   parent->Add(slider_.get());
   if (delta_x_ < 0)
     parent->StackAbove(slider_.get(), owner_->layer());
   else
     parent->StackBelow(slider_.get(), owner_->layer());
   slider_->SetBounds(owner_->layer()->bounds());
   slider_->SetVisible(true);
 }
 
 void WindowSlider::UpdateForScroll(float x_offset, float y_offset) {
+  if (active_animator_) {
+    // If there is an active animation, complete it before processing the scroll
+    // so that the callbacks that are invoked on the Delegate are consistent.
+    // Completing the animation may destroy WindowSlider through the animation
+    // callback, so we can't continue processing the scroll event here.
+    delta_x_ += x_offset;
+    CompleteActiveAnimations();
+    return;
+  }
+
   float old_delta = delta_x_;
   delta_x_ += x_offset;
   if (fabs(delta_x_) < active_start_threshold_ && !slider_.get())
     return;
 
   if ((old_delta < 0 && delta_x_ > 0) ||
       (old_delta > 0 && delta_x_ < 0)) {
     slider_.reset();
     shadow_.reset();
   }
 
   float translate = 0.f;
   ui::Layer* translate_layer = NULL;
 
   if (!slider_.get()) {
-    slider_.reset(delta_x_ < 0 ? delegate_->CreateFrontLayer() :
-                                 delegate_->CreateBackLayer());
+    slider_.reset(delta_x_ < 0 ? delegate_->OnSlideForwardStartedCreateLayer() :
+                                 delegate_->OnSlideBackStartedCreateLayer());
     if (!slider_.get())
       return;
     SetupSliderLayer();
   }
 
   if (delta_x_ <= -active_start_threshold_) {
     translate = owner_->bounds().width() +
         std::max(delta_x_ + active_start_threshold_,
                  static_cast<float>(-owner_->bounds().width()));
     translate_layer = slider_.get();
@@ skipping 13 matching lines @@
   translate_layer->SetTransform(transform);
 }
 
 void WindowSlider::UpdateForFling(float x_velocity, float y_velocity) {
   if (!slider_.get())
     return;
 
   int width = owner_->bounds().width();
   float ratio = (fabs(delta_x_) - active_start_threshold_) / width;
   if (ratio < complete_threshold_) {
-    ResetScroll();
+    ResetSlide();
     return;
   }
 
   ui::Layer* sliding = delta_x_ < 0 ? slider_.get() : owner_->layer();
-  ui::ScopedLayerAnimationSettings settings(sliding->GetAnimator());
+  active_animator_ = sliding->GetAnimator();
+
+  ui::ScopedLayerAnimationSettings settings(active_animator_);
   settings.SetPreemptionStrategy(
       ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
   settings.SetTweenType(gfx::Tween::EASE_OUT);
   settings.AddObserver(new CallbackAnimationObserver(
-      base::Bind(&WindowSlider::CompleteWindowSlideAfterAnimation,
-                 weak_factory_.GetWeakPtr())));
+      base::Bind(&WindowSlider::FlingAnimationCompleted,
+                 weak_factory_.GetWeakPtr(),
+                 base::Unretained(slider_.release()),
+                 base::Unretained(shadow_.release()))));
 
   gfx::Transform transform;
   transform.Translate(delta_x_ < 0 ? 0 : width, 0);
+  delta_x_ = 0;
+  delegate_->OnWindowFlingStarted();
   sliding->SetTransform(transform);
 }
 
-void WindowSlider::ResetScroll() {
+void WindowSlider::CompleteActiveAnimations() {
+  if (active_animator_)
+    active_animator_->StopAnimating();
+}
+
+void WindowSlider::ResetSlide() {
   if (!slider_.get())
     return;
 
-  // Do not trigger any callbacks if this animation replaces any in-progress
-  // animation.
-  weak_factory_.InvalidateWeakPtrs();
-
   // Reset the state of the sliding layer.
   if (slider_.get()) {
-    ui::Layer* layer = slider_.release();
-    ui::ScopedLayerAnimationSettings settings(layer->GetAnimator());
-    settings.SetPreemptionStrategy(
-        ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
-    settings.SetTweenType(gfx::Tween::EASE_OUT);
+    ui::Layer* translate_layer;
+    gfx::Transform transform;
+    if (delta_x_ < 0) {
+      translate_layer = slider_.get();
+      transform.Translate(translate_layer->bounds().width(), 0);
+    } else {
+      translate_layer = owner_->layer();
+    }
 
-    // Delete the layer and the shadow at the end of the animation.
-    settings.AddObserver(new CallbackAnimationObserver(
-        base::Bind(&DeleteLayerAndShadow,
-                   base::Unretained(layer),
-                   base::Unretained(shadow_.release()))));
-
-    gfx::Transform transform;
-    transform.Translate(delta_x_ < 0 ? layer->bounds().width() : 0, 0);
-    layer->SetTransform(transform);
-  }
-
-  // Reset the state of the main layer.
-  {
-    ui::ScopedLayerAnimationSettings settings(owner_->layer()->GetAnimator());
+    active_animator_ = translate_layer->GetAnimator();
+    ui::ScopedLayerAnimationSettings settings(active_animator_);
     settings.SetPreemptionStrategy(
         ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
     settings.SetTweenType(gfx::Tween::EASE_OUT);
     settings.AddObserver(new CallbackAnimationObserver(
-        base::Bind(&WindowSlider::AbortWindowSlideAfterAnimation,
-                   weak_factory_.GetWeakPtr())));
-    owner_->layer()->SetTransform(gfx::Transform());
-    owner_->layer()->SetLayerBrightness(0.f);
+        base::Bind(&WindowSlider::ResetSlideAnimationCompleted,
+                   weak_factory_.GetWeakPtr(),
+                   base::Unretained(slider_.release()),
+                   base::Unretained(shadow_.release()))));
+    translate_layer->SetTransform(transform);
   }
 
   delta_x_ = 0.f;
 }
 
-void WindowSlider::CancelScroll() {
-  ResetScroll();
+void WindowSlider::FlingAnimationCompleted(
+    ui::Layer* layer, ShadowLayerDelegate* shadow) {
+  active_animator_ = NULL;
+  delete shadow;
+  delete layer;
+  delegate_->OnWindowFlingCompleted();
 }
 
-void WindowSlider::CompleteWindowSlideAfterAnimation() {
-  weak_factory_.InvalidateWeakPtrs();
-  shadow_.reset();
-  slider_.reset();
-  delta_x_ = 0.f;
-
-  delegate_->OnWindowSlideComplete();
-}
-
-void WindowSlider::AbortWindowSlideAfterAnimation() {
-  weak_factory_.InvalidateWeakPtrs();
-
+void WindowSlider::ResetSlideAnimationCompleted(
+    ui::Layer* layer, ShadowLayerDelegate* shadow) {
+  active_animator_ = NULL;
+  delete shadow;
+  delete layer;
   delegate_->OnWindowSlideAborted();
 }
 
 void WindowSlider::OnKeyEvent(ui::KeyEvent* event) {
-  CancelScroll();
+  ResetSlide();
 }
 
 void WindowSlider::OnMouseEvent(ui::MouseEvent* event) {
   if (!(event->flags() & ui::EF_IS_SYNTHESIZED))
-    CancelScroll();
+    ResetSlide();
 }
 
 void WindowSlider::OnScrollEvent(ui::ScrollEvent* event) {
   active_start_threshold_ = start_threshold_touchpad_;
   if (event->type() == ui::ET_SCROLL)
     UpdateForScroll(event->x_offset_ordinal(), event->y_offset_ordinal());
   else if (event->type() == ui::ET_SCROLL_FLING_START)
     UpdateForFling(event->x_offset_ordinal(), event->y_offset_ordinal());
   else
-    CancelScroll();
+    ResetSlide();
   event->SetHandled();
 }
 
 void WindowSlider::OnGestureEvent(ui::GestureEvent* event) {
   active_start_threshold_ = start_threshold_touchscreen_;
   const ui::GestureEventDetails& details = event->details();
   switch (event->type()) {
     case ui::ET_GESTURE_SCROLL_BEGIN:
-      ResetScroll();
+      CompleteActiveAnimations();
       break;
 
     case ui::ET_GESTURE_SCROLL_UPDATE:
       UpdateForScroll(details.scroll_x(), details.scroll_y());
       break;
 
     case ui::ET_GESTURE_SCROLL_END:
       UpdateForFling(0.f, 0.f);
       break;
 
     case ui::ET_SCROLL_FLING_START:
       UpdateForFling(details.velocity_x(), details.velocity_y());
       break;
 
     case ui::ET_GESTURE_PINCH_BEGIN:
     case ui::ET_GESTURE_PINCH_UPDATE:
     case ui::ET_GESTURE_PINCH_END:
-      CancelScroll();
+      ResetSlide();
       break;
 
     default:
       break;
   }
 
   event->SetHandled();
 }
 
 void WindowSlider::OnWindowRemovingFromRootWindow(aura::Window* window) {
   if (window == event_window_) {
     window->RemoveObserver(this);
     window->RemovePreTargetHandler(this);
     event_window_ = NULL;
   } else if (window == owner_) {
     window->RemoveObserver(this);
     owner_ = NULL;
     delete this;
   } else {
     NOTREACHED();
   }
 }
 
 }  // namespace content