Correctly handle cancelling compositor animations initiated from main thread.

third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp

 /*
  * Copyright (c) 2011, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 90 matching lines @@
             resetAnimationState();
         }
         return ScrollAnimatorBase::userScroll(orientation, granularity, step, delta);
     }
 
     float usedPixelDelta = computeDeltaToConsume(orientation, step * delta);
     FloatPoint pixelDelta = (orientation == VerticalScrollbar
         ? FloatPoint(0, usedPixelDelta) : FloatPoint(usedPixelDelta, 0));
 
     FloatPoint targetPos = desiredTargetPosition();
     targetPos.moveBy(pixelDelta);

[ymalik, 2016/02/07 21:23:37]

@ajuma

This is speculatively fix up the assert failure on L128. Presumably what's
happening is that it's in state WaitingToCancelOnCompositor. I suspect that a
user scroll is cancelled and another user scroll happens before
UpdateCompositorState can clear the animation. This is possible right?

[ajuma, 2016/02/08 15:05:35]

Assuming that userScroll can get called at arbitrary times, yes.

 
+    // Reset animation state in case we get a scroll between when an animation
+    // was asked to be cancelled and updateCompositorAnimations is called.
+    if (m_runState == RunState::WaitingToCancelOnCompositor) {
+        abortAnimation();

[ajuma, 2016/02/08 15:05:35]

It looks like this could touch compositing state in a situation where the
document lifecycle state isn't CompositingClean. There's probably an assert
disabler somewhere that's preventing an ASSERT from triggering for this. (And
there's another abortAnimation call above that's similarly problematic.)
Anything involving updating things on the compositor should happen inside of
::updateCompositorAnimations.

[ymalik, 2016/02/08 18:41:23]

Ah right. That's why we have the whole state machine to begin with. Handling
this and the above case from ::updateCompositorAnimations.

+        resetAnimationState();
+    }
+
     if (m_animationCurve) {
         if ((targetPos - m_targetOffset).isZero()) {
             // Report unused delta only if there is no animation running. See
             // comment below regarding scroll latching.
             return ScrollResultOneDimensional(/* didScroll */ true, /* unusedScrollDelta */ 0);
         }
 
         m_targetOffset = targetPos;
         ASSERT(m_runState == RunState::RunningOnMainThread
             || m_runState == RunState::RunningOnCompositor
@@ skipping 58 matching lines @@
     if (isFinished)
         resetAnimationState();
     else
         scrollableArea()->scheduleAnimation();
 
     TRACE_EVENT0("blink", "ScrollAnimator::notifyPositionChanged");
     notifyPositionChanged();
 }
 
 void ScrollAnimator::updateCompositorAnimations()
 {

[ymalik, 2016/02/07 21:23:37]

I don't think we should ever be in a state where we have
m_compositorAnimationId, and are not running on the compositor. I copied this
over from programmatic scroll animator at the time. In the case of a
programmatic scroll, we could be in WaitingToSendToCompositor if another
programmatic scroll happens while we're running on the compositor. In this case,
we either update the target right from SA::UserScroll or go into state
RunningOnCompositorButNeedsUpdate.

WDYT?

[ajuma, 2016/02/08 15:05:35]

Sounds reasonable.

[ymalik, 2016/02/08 18:41:23]

Based on your feedback above, I moved the call to abortAnimation into
::updateComposiorAnimations. Now we can be in a state when
m_compositorAnimationId exists, and the state is WaitingToSendToCompositor
(We're in WaitingToCancelOnCompositor and another user scroll arrives).

Thus, this change should be undone. 

-    if (m_compositorAnimationId && m_runState != RunState::RunningOnCompositor
-        && m_runState != RunState::RunningOnCompositorButNeedsUpdate) {
-        // If the current run state is WaitingToSendToCompositor but we have a
-        // non-zero compositor animation id, there's a currently running
-        // compositor animation that needs to be removed here before the new
-        // animation is added below.
-        ASSERT(m_runState == RunState::WaitingToCancelOnCompositor
-            || m_runState == RunState::WaitingToSendToCompositor);
-
+    if (m_runState == RunState::WaitingToCancelOnCompositor) {
+        ASSERT(m_compositorAnimationId);
         abortAnimation();
-
-        m_compositorAnimationId = 0;
-        m_compositorAnimationGroupId = 0;
-        if (m_runState == RunState::WaitingToCancelOnCompositor) {
-            resetAnimationState();
-            return;
-        }
+        resetAnimationState();
+        return;
     }
 
     if (m_runState == RunState::WaitingToSendToCompositor
         || m_runState == RunState::RunningOnCompositorButNeedsUpdate) {
         if (m_runState == RunState::RunningOnCompositorButNeedsUpdate) {
             // Abort the running animation before a new one with an updated
             // target is added.
             abortAnimation();
 
             m_compositorAnimationId = 0;
@@ skipping 80 matching lines @@
     }
     return true;
 }
 
 DEFINE_TRACE(ScrollAnimator)
 {
     ScrollAnimatorBase::trace(visitor);
 }
 
 } // namespace blink