Avoid updating compositor animations in hitTest path

Avoid updating compositor animations in hitTest path

Before doing hit testing, animations has already been updated.
So update the animation again in hitTest will result in wrong 
animation. This CL moves updatecompositorAnimations out of hitTest
path.

BUG=501297

[qiankun, 2015-07-22 09:08:04]

qiankun.miao@intel.com changed reviewers:
+ chrishtr@chromium.org, leviw@chromium.org

[qiankun, 2015-07-22 09:08:29]

PTAL.

[chrishtr, 2015-07-22 14:14:20]

https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
Source/core/frame/FrameView.cpp:2462:
view->compositor()->updateIfNeededRecursive();
updateIfNeededRecursive() should be a no-op if called when already clean.
Instead you should fix whatever is not a no-op. Do you know which code there is
broken?

[qiankun, 2015-07-22 15:07:08]

On 2015/07/22 14:14:20, chrishtr wrote:
>
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> Source/core/frame/FrameView.cpp:2462:
> view->compositor()->updateIfNeededRecursive();
> updateIfNeededRecursive() should be a no-op if called when already clean.
Can we just return if life cycle in CompositingClean state when calling
updateIfNeededRecursive()? I am not sure about this. State changing from
CompositingClean to InCompositingUpdate is allowed.

> Instead you should fix whatever is not a no-op. Do you know which code there
is
> broken?

The root cause is before doing LayoutView::hitTest(), updateIfNeededRecursive()
is already called, then following call stack will call updateIfNeededRecursive()
again:
LayoutView::hitTest() 
frameView()->updateLifecycleToCompositingCleanPlusScrolling(); 
view->compositor()->updateIfNeededRecursive();
DocumentAnimations::updateCompositorAnimations(m_layoutView.document());


DocumentAnimations::updateCompositorAnimations(m_layoutView.document()) will
reschedule the animation. Then the bug happens.

[chrishtr, 2015-07-22 15:15:51]

On 2015/07/22 at 15:07:08, qiankun.miao wrote:
> On 2015/07/22 14:14:20, chrishtr wrote:
> >
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> > File Source/core/frame/FrameView.cpp (right):
> > 
> >
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> > Source/core/frame/FrameView.cpp:2462:
> > view->compositor()->updateIfNeededRecursive();
> > updateIfNeededRecursive() should be a no-op if called when already clean.
> Can we just return if life cycle in CompositingClean state when calling
updateIfNeededRecursive()? I am not sure about this. State changing from
CompositingClean to InCompositingUpdate is allowed.

No, that's not a good idea. The lifecycle states are just for asserting
correctness in debug builds, not for recording dirtiness.

> 
> > Instead you should fix whatever is not a no-op. Do you know which code there
is
> > broken?
> 
> The root cause is before doing LayoutView::hitTest(),
updateIfNeededRecursive() is already called, then following call stack will call
updateIfNeededRecursive() again:
> LayoutView::hitTest() 
> frameView()->updateLifecycleToCompositingCleanPlusScrolling(); 
> view->compositor()->updateIfNeededRecursive();
> DocumentAnimations::updateCompositorAnimations(m_layoutView.document());
> 
> 
> DocumentAnimations::updateCompositorAnimations(m_layoutView.document()) will
reschedule the animation. Then the bug happens.

Why will updatecompositorAnimations reschedule the animation? That seems like
the bug that should be fixed.

[qiankun, 2015-07-22 15:23:07]

On 2015/07/22 15:15:51, chrishtr wrote:
> On 2015/07/22 at 15:07:08, qiankun.miao wrote:
> > On 2015/07/22 14:14:20, chrishtr wrote:
> > >
>
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> > > File Source/core/frame/FrameView.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> > > Source/core/frame/FrameView.cpp:2462:
> > > view->compositor()->updateIfNeededRecursive();
> > > updateIfNeededRecursive() should be a no-op if called when already clean.
> > Can we just return if life cycle in CompositingClean state when calling
> updateIfNeededRecursive()? I am not sure about this. State changing from
> CompositingClean to InCompositingUpdate is allowed.
> 
> No, that's not a good idea. The lifecycle states are just for asserting
> correctness in debug builds, not for recording dirtiness.
> 
> > 
> > > Instead you should fix whatever is not a no-op. Do you know which code
there
> is
> > > broken?
> > 
> > The root cause is before doing LayoutView::hitTest(),
> updateIfNeededRecursive() is already called, then following call stack will
call
> updateIfNeededRecursive() again:
> > LayoutView::hitTest() 
> > frameView()->updateLifecycleToCompositingCleanPlusScrolling(); 
> > view->compositor()->updateIfNeededRecursive();
> > DocumentAnimations::updateCompositorAnimations(m_layoutView.document());
> > 
> > 
> > DocumentAnimations::updateCompositorAnimations(m_layoutView.document()) will
> reschedule the animation. Then the bug happens.
> 
> Why will updatecompositorAnimations reschedule the animation? That seems like
> the bug that should be fixed.

That's what updatecompositorAnimations do. Then updatecompositorAnimations
should not be called in hitTest path.

[chrishtr, 2015-07-22 15:28:29]

On 2015/07/22 at 15:23:07, qiankun.miao wrote:
> On 2015/07/22 15:15:51, chrishtr wrote:
> > On 2015/07/22 at 15:07:08, qiankun.miao wrote:
> > > On 2015/07/22 14:14:20, chrishtr wrote:
> > > >
> >
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> > > > File Source/core/frame/FrameView.cpp (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> > > > Source/core/frame/FrameView.cpp:2462:
> > > > view->compositor()->updateIfNeededRecursive();
> > > > updateIfNeededRecursive() should be a no-op if called when already
clean.
> > > Can we just return if life cycle in CompositingClean state when calling
> > updateIfNeededRecursive()? I am not sure about this. State changing from
> > CompositingClean to InCompositingUpdate is allowed.
> > 
> > No, that's not a good idea. The lifecycle states are just for asserting
> > correctness in debug builds, not for recording dirtiness.
> > 
> > > 
> > > > Instead you should fix whatever is not a no-op. Do you know which code
there
> > is
> > > > broken?
> > > 
> > > The root cause is before doing LayoutView::hitTest(),
> > updateIfNeededRecursive() is already called, then following call stack will
call
> > updateIfNeededRecursive() again:
> > > LayoutView::hitTest() 
> > > frameView()->updateLifecycleToCompositingCleanPlusScrolling(); 
> > > view->compositor()->updateIfNeededRecursive();
> > > DocumentAnimations::updateCompositorAnimations(m_layoutView.document());
> > > 
> > > 
> > > DocumentAnimations::updateCompositorAnimations(m_layoutView.document())
will
> > reschedule the animation. Then the bug happens.
> > 
> > Why will updatecompositorAnimations reschedule the animation? That seems
like
> > the bug that should be fixed.
> 
> That's what updatecompositorAnimations do. Then updatecompositorAnimations
should not be called in hitTest path.

I agree that these things don't need to be called in the hitTest path. To fix I
suggest moving lines 235-241 of DeprecatedPaintLayerCompositor out of there and
into FrameView::updatePostLifecycleData. But it should still be the case that
updatecompositorAnimations should behave correctly if no work needs to be done
and it is called.

[chrishtr, 2015-07-22 15:28:41]

chrishtr@chromium.org changed reviewers:
+ dstockwell@chromium.org

[chrishtr, 2015-07-22 15:28:42]

+dstockwell

[qiankun, 2015-07-23 09:10:25]

On 2015/07/22 15:28:29, chrishtr wrote:
> On 2015/07/22 at 15:23:07, qiankun.miao wrote:
> > On 2015/07/22 15:15:51, chrishtr wrote:
> > > On 2015/07/22 at 15:07:08, qiankun.miao wrote:
> > > > On 2015/07/22 14:14:20, chrishtr wrote:
> > > > >
> > >
>
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> > > > > File Source/core/frame/FrameView.cpp (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/1246363002/diff/1/Source/core/frame/FrameView...
> > > > > Source/core/frame/FrameView.cpp:2462:
> > > > > view->compositor()->updateIfNeededRecursive();
> > > > > updateIfNeededRecursive() should be a no-op if called when already
> clean.
> > > > Can we just return if life cycle in CompositingClean state when calling
> > > updateIfNeededRecursive()? I am not sure about this. State changing from
> > > CompositingClean to InCompositingUpdate is allowed.
> > > 
> > > No, that's not a good idea. The lifecycle states are just for asserting
> > > correctness in debug builds, not for recording dirtiness.
> > > 
> > > > 
> > > > > Instead you should fix whatever is not a no-op. Do you know which code
> there
> > > is
> > > > > broken?
> > > > 
> > > > The root cause is before doing LayoutView::hitTest(),
> > > updateIfNeededRecursive() is already called, then following call stack
will
> call
> > > updateIfNeededRecursive() again:
> > > > LayoutView::hitTest() 
> > > > frameView()->updateLifecycleToCompositingCleanPlusScrolling(); 
> > > > view->compositor()->updateIfNeededRecursive();
> > > > DocumentAnimations::updateCompositorAnimations(m_layoutView.document());
> > > > 
> > > > 
> > > > DocumentAnimations::updateCompositorAnimations(m_layoutView.document())
> will
> > > reschedule the animation. Then the bug happens.
> > > 
> > > Why will updatecompositorAnimations reschedule the animation? That seems
> like
> > > the bug that should be fixed.
> > 
> > That's what updatecompositorAnimations do. Then updatecompositorAnimations
> should not be called in hitTest path.
> 
> I agree that these things don't need to be called in the hitTest path. To fix
I
> suggest moving lines 235-241 of DeprecatedPaintLayerCompositor out of there
and
> into FrameView::updatePostLifecycleData. But it should still be the case that
> updatecompositorAnimations should behave correctly if no work needs to be done
> and it is called.

I updated the CL following your suggestion. updatecompositorAnimations won't be
called in hitTest path now.

[chrishtr, 2015-07-23 15:54:45]

Doug, does this change seem reasonable to you?

In addition, whatever makes updateCompositorAnimations not a no-op when called
twice should be fixed. This CL, while I think ok from a performance and
do-only-what-work-is-needed-for-hit-testing perspective is fine, but papers over
a separate underlying animations bug.

[qiankun, 2015-07-24 08:50:12]

Patchset #3 (id:40001) has been deleted

[qiankun, 2015-07-24 10:57:08]

https://codereview.chromium.org/1246363002/diff/20001/Source/core/layout/comp...
File Source/core/layout/compositing/DeprecatedPaintLayerCompositor.cpp (left):

https://codereview.chromium.org/1246363002/diff/20001/Source/core/layout/comp...
Source/core/layout/compositing/DeprecatedPaintLayerCompositor.cpp:242: 
Move these code out of updateIfNeededRecursive will make subFrame don't update
animation. There are some layout tests failure . I added a new function for hit
test path. The new function is same as updateIfNeededRecursive except no
animation update. What do you think about it?

I tried to make updateCompositorAnimations not a no-op when called twice, but
didn't get a good solution. Feel free to take over the bug, if you have a good
solution.

[chrishtr, 2015-07-24 14:12:51]

chrishtr@chromium.org changed reviewers:
+ timloh@chromium.org

[chrishtr, 2015-07-24 14:12:51]

This CL is fine as long as dstockwell or timloh approves of it, and we don't
lose track of fixing the underlying cause of the referenced bug. Waiting for
their feedback.

[qiankun, 2015-07-29 17:27:18]

On 2015/07/24 14:12:51, chrishtr -- OOO til Aug 3 wrote:
> This CL is fine as long as dstockwell or timloh approves of it, and we don't
> lose track of fixing the underlying cause of the referenced bug. Waiting for
> their feedback.

dstockwell and timloh, please help to review this CL when you free, thanks!

[qiankun, 2015-07-29 17:27:19]

On 2015/07/24 14:12:51, chrishtr -- OOO til Aug 3 wrote:
> This CL is fine as long as dstockwell or timloh approves of it, and we don't
> lose track of fixing the underlying cause of the referenced bug. Waiting for
> their feedback.

dstockwell and timloh, please help to review this CL when you free, thanks!

[dstockwell, 2015-08-06 01:31:22]

> > Why will updatecompositorAnimations reschedule the animation? That seems
like
> > the bug that should be fixed.
> 
> That's what updatecompositorAnimations do. Then updatecompositorAnimations
should not be called in hitTest path.

Can you explain what you are seeing there? This should not reschedule. See eg.
this assert:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[qiankun, 2015-08-06 04:22:21]

On 2015/08/06 01:31:22, dstockwell wrote:
> > > Why will updatecompositorAnimations reschedule the animation? That seems
> like
> > > the bug that should be fixed.
> > 
> > That's what updatecompositorAnimations do. Then updatecompositorAnimations
> should not be called in hitTest path.
> 
> Can you explain what you are seeing there? This should not reschedule. See eg.
> this assert:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I didn't describe the problem clearly before.  It should be next animation fire
time is changed after calling updateCompositorAnimations. See the following call
stack:

#0  blink::TimerBase::setNextFireTime (this=0x370a631d07c0,
now=7754507.0951079996, delay=3.3433300001919268) at
../../third_party/WebKit/Source/platform/Timer.
#1  0x00007fe21f2abb52 in blink::TimerBase::start (this=0x370a631d07c0,
nextFireInterval=3.3433300001919268, repeatInterval=0, caller=...) at
../../third_party/
r.cpp:67
#2  0x00007fe21f27e94a in blink::TimerBase::startOneShot (this=0x370a631d07c0,
interval=3.3433300001919268, caller=...) at
../../third_party/WebKit/Source/platf
#3  0x00007fe220607b83 in
blink::AnimationTimeline::AnimationTimelineTiming::wakeAfter
(this=0x370a631d07b0, duration=3.3433300001919268) at ../../third_party/W
/AnimationTimeline.cpp:177
#4  0x00007fe220607b1d in blink::AnimationTimeline::scheduleNextService
(this=0x146fbb2ac170) at
../../third_party/WebKit/Source/core/animation/AnimationTimelin
#5  0x00007fe220610099 in blink::DocumentAnimations::updateCompositorAnimations
(document=...) at ../../third_party/WebKit/Source/core/animation/DocumentAnimati
#6  0x00007fe220d8772c in
blink::DeprecatedPaintLayerCompositor::updateIfNeededRecursive
(this=0x146fbb29d330) at ../../third_party/WebKit/Source/core/layout/co
yerCompositor.cpp:235
#7  0x00007fe220833dea in
blink::FrameView::updateLifecycleToCompositingCleanPlusScrolling
(this=0x146fbb2e0010) at ../../third_party/WebKit/Source/core/frame/F
#8  0x00007fe220d53c28 in blink::LayoutView::hitTest (this=0x322e16c04010,
result=...) at ../../third_party/WebKit/Source/core/layout/LayoutView.cpp:90

[eae, 2015-08-12 20:36:53]

eae@chromium.org changed reviewers:
- leviw@chromium.org

[dstockwell, 2015-08-13 07:46:40]

On 2015/08/06 at 04:22:21, qiankun.miao wrote:
> On 2015/08/06 01:31:22, dstockwell wrote:
> > > > Why will updatecompositorAnimations reschedule the animation? That seems
> > like
> > > > the bug that should be fixed.
> > > 
> > > That's what updatecompositorAnimations do. Then updatecompositorAnimations
> > should not be called in hitTest path.
> > 
> > Can you explain what you are seeing there? This should not reschedule. See
eg.
> > this assert:
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I didn't describe the problem clearly before.  It should be next animation
fire time is changed after calling updateCompositorAnimations. See the following
call stack:
> 
> #0  blink::TimerBase::setNextFireTime (this=0x370a631d07c0,
now=7754507.0951079996, delay=3.3433300001919268) at
../../third_party/WebKit/Source/platform/Timer.
> #1  0x00007fe21f2abb52 in blink::TimerBase::start (this=0x370a631d07c0,
nextFireInterval=3.3433300001919268, repeatInterval=0, caller=...) at
../../third_party/
> r.cpp:67
> #2  0x00007fe21f27e94a in blink::TimerBase::startOneShot (this=0x370a631d07c0,
interval=3.3433300001919268, caller=...) at
../../third_party/WebKit/Source/platf
> #3  0x00007fe220607b83 in
blink::AnimationTimeline::AnimationTimelineTiming::wakeAfter
(this=0x370a631d07b0, duration=3.3433300001919268) at ../../third_party/W
> /AnimationTimeline.cpp:177
> #4  0x00007fe220607b1d in blink::AnimationTimeline::scheduleNextService
(this=0x146fbb2ac170) at
../../third_party/WebKit/Source/core/animation/AnimationTimelin
> #5  0x00007fe220610099 in
blink::DocumentAnimations::updateCompositorAnimations (document=...) at
../../third_party/WebKit/Source/core/animation/DocumentAnimati
> #6  0x00007fe220d8772c in
blink::DeprecatedPaintLayerCompositor::updateIfNeededRecursive
(this=0x146fbb29d330) at ../../third_party/WebKit/Source/core/layout/co
> yerCompositor.cpp:235
> #7  0x00007fe220833dea in
blink::FrameView::updateLifecycleToCompositingCleanPlusScrolling
(this=0x146fbb2e0010) at ../../third_party/WebKit/Source/core/frame/F
> #8  0x00007fe220d53c28 in blink::LayoutView::hitTest (this=0x322e16c04010,
result=...) at ../../third_party/WebKit/Source/core/layout/LayoutView.cpp:90

Thanks, likewise scheduleNextService should be idempotent. So still requires
some further investigation.

But this patch seems OK for now, lgtm.

[esprehn, 2015-08-13 08:53:49]

This is a lot of copy pasta, I'd prefer we didn't do this. The paths will just
diverge further and make maintenance harder in the future.

We shouldn't need a special path just for hit testing.

[esprehn, 2015-08-13 08:54:59]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-08-13 09:03:36]

On 2015/08/13 at 08:53:49, esprehn wrote:
> This is a lot of copy pasta, I'd prefer we didn't do this. The paths will just
diverge further and make maintenance harder in the future.
> 
> We shouldn't need a special path just for hit testing.

Note also that this can't land without a test. This doesn't seem like the right
fix either though.

[qiankun, 2015-08-14 09:40:47]

https://codereview.chromium.org/1246363002/diff/60001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/1246363002/diff/60001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2462:
view->compositor()->updateIfNeededRecursive();
I agree with esprehn that this fix has issues esprehn pointed.
But by now, I didn't find a right method to avoid calling
DocumentAnimations::updateCompositorAnimations() in updateIfNeededRecursive()
for hit test path. 

Or we can remove this life cycle update and only update style and layout here.
This prevents updating life cycle to CompositingClean state which is the
situation before https://codereview.chromium.org/999033002/.

If you have good solution for this bug, I am very appreciated. Thanks in advance
for any ideas and solutions.

About the test, I constructed a simple test to reproduce the bug. But I need to
hover the mouse over the marquee text and check if the text rendering is proper.
How can I translate it to a layout test? Are there any similar examples to
refer? I didn't find a similar one in current tests by myself.

Attach my test:

<html>
<body onload="autoTest()">

<marquee direction="up" id="marquee" onmouseover="marquee.stop();"
onmouseout="marquee.start();">
    Marquee test1. <br><br>
    Marquee test2. <br><br>
    Marquee test3. <br><br>
</marquee>
</body>
</html>

[esprehn, 2015-08-14 15:45:35]

This seems like an animation engine bug, you should be able to run the
pipeline as many times as you want without side effects. The problem is the
timer they're using, not hit testing trying to run the pipeline.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[qiankun, 2015-08-25 07:26:38]

hi reviewers, I updated the patch. The current problem is due to animation
update in hit testing path via document.compositorPendingAnimations().update()
in DocumentAnimations::updateCompositorAnimations(). We should not do animation
update from hit testing path. I reverted the CL
(https://codereview.chromium.org/999033002) which caused the bug in this new
patch. Please take a look.

[esprehn, 2015-08-25 07:37:53]

This still doesn't seem right, please fix the animation engine. The bug is not
in these update methods, it's something wrong with the animation engine.

We update the pipeline in other places too, ex. here with the touch handling
code. Whatever is wrong with animations for marquee is also broken anywhere else
we call into this.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1246363002/diff/80001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/1246363002/diff/80001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2433:
frame().localFrameRoot()->view()->updateLifecyclePhasesInternal(OnlyUpToCompositingCleanPlusScrolling);
This leaves the OnlyUpToCompositingCleanPlusScrolling code path as dead code?

[qiankun, 2015-08-31 09:10:56]

The bug should be fixed in https://codereview.chromium.org/1319653002. Close
this CL.