cc: Avoid updating animations in the background without an active tree

cc: Avoid updating animations in the background without an active tree

This patch prevents LTHI from background ticking
animations if there is no active tree to
animate since animating with no active tree will
often result in segfaults.

BUG=280706
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220462

[danakj, 2013-08-28 22:47:00]

It seems like maybe ThreadProxy::SetVisibleOnImplThread() is enabling ticking
when it shouldn't because scheduler_on_impl_thread_->WillDrawIfNeeded() is
returning false?

Instead of checking for null at the last second, I thin we should not tick when
we can't animate.

[brianderson, 2013-08-28 22:47:10]

I need to rebase and re-enable the test that this fixes. I've been running for a
very long time with this patch and haven't hit the segfault again.

[danakj, 2013-08-28 22:47:40]

On 2013/08/28 22:47:00, danakj wrote:
> It seems like maybe ThreadProxy::SetVisibleOnImplThread() is enabling ticking

There's a few places in ThreadProxy that do this check, not to pick on
SetVisible only.

[brianderson, 2013-08-28 22:56:35]

On 2013/08/28 22:47:00, danakj wrote:
> It seems like maybe ThreadProxy::SetVisibleOnImplThread() is enabling ticking
> when it shouldn't because scheduler_on_impl_thread_->WillDrawIfNeeded() is
> returning false?
> 
> Instead of checking for null at the last second, I thin we should not tick
when
> we can't animate.

That sounds like a reasonable thing to do. I like the null checks because they
will be needed when we have deadline scheduling anyway, but I'll add them when
they are actually needed and we can debate the use of them there.

Note: I can imagine cases where we force draw without an active tree at ToT, if
a a readback comes in before the first commit after output surface
initialization. That is unlikely though and will be fixed by the readback patch,
so I'm not going to worry about it for now.

[brianderson, 2013-08-28 23:34:22]

PTAL, this version also appears to avoid the segfault.

[danakj, 2013-08-28 23:36:13]

https://codereview.chromium.org/23593003/diff/8001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23593003/diff/8001/cc/trees/thread_proxy.cc#n...
cc/trees/thread_proxy.cc:1008: UpdateBackgroundAnimateTicking();
Why is this change correct, when it used to always be false here?

[brianderson, 2013-08-28 23:42:18]

Rebasing to re-enable the test now.

https://codereview.chromium.org/23593003/diff/8001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23593003/diff/8001/cc/trees/thread_proxy.cc#n...
cc/trees/thread_proxy.cc:1008: UpdateBackgroundAnimateTicking();
On 2013/08/28 23:36:14, danakj wrote:
> Why is this change correct, when it used to always be false here?

At first I did it because it was easier, but then I realized it is more correct
to do it this way anyway. I'm not sure why it was always set to false before.

Reasoning:
It is possible that scheduler_on_impl_thread_->WillDrawIfNeeded() is false here
because of a forced readback draw while we are not visible. In that case, we do
not want to disable background ticking.

[danakj, 2013-08-28 23:49:09]

Ok, I was worried that maybe the animation registrar should not have active
animation controllers if there is no active tree, but it seems to me that as
soon as we push an animation to the pending tree, the animation controller is
added to the active list.

How would you repro this 100% in a test? Would you commit a layer tree with an
animation while also setting visible false, and then delay the activation of
that tree so that the background timer would tick? Could we do that in a test
and verify that the ticking is off?

[enne (OOO), 2013-08-29 00:10:10]

https://codereview.chromium.org/23593003/diff/14001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23593003/diff/14001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:248:
layer_tree_host_impl_->active_tree()->root_layer());
This seems a little redundant.  Scheduler::WillDrawIfNeeded returns
!SchedulerStateMachine::PendingDrawsShouldBeAborted which returns true if
!can_draw.  CanDraw() itself returns false if there's no active root layer. 
Does that just mean that the scheduler state machine hasn't been updated?

[brianderson, 2013-08-29 00:16:03]

https://codereview.chromium.org/23593003/diff/14001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23593003/diff/14001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:248:
layer_tree_host_impl_->active_tree()->root_layer());
On 2013/08/29 00:10:10, enne wrote:
> This seems a little redundant.  Scheduler::WillDrawIfNeeded returns
> !SchedulerStateMachine::PendingDrawsShouldBeAborted which returns true if
> !can_draw.  CanDraw() itself returns false if there's no active root layer. 
> Does that just mean that the scheduler state machine hasn't been updated?

It's confusing with all the negations going on. The issue was we were enabling
background ticking when !can_draw, but we shouldn't be if the reason we
!can_draw is that the active_tree's root layer is null.

[brianderson, 2013-08-29 00:36:27]

On 2013/08/28 23:49:09, danakj wrote:
> Ok, I was worried that maybe the animation registrar should not have active
> animation controllers if there is no active tree, but it seems to me that as
> soon as we push an animation to the pending tree, the animation controller is
> added to the active list.
> 
> How would you repro this 100% in a test? Would you commit a layer tree with an
> animation while also setting visible false, and then delay the activation of
> that tree so that the background timer would tick? Could we do that in a test
> and verify that the ticking is off?

I will add a DCHECKs in LTHI that check if background ticking is ever enabled
when the root layer is null. That will catch the problem at the source 100% of
the time.

I'm already finding a few more places that are enabling background ticking when
the root layer is null just by running the normal unit tests.

[danakj, 2013-08-29 00:44:57]

On Wed, Aug 28, 2013 at 8:36 PM, <brianderson@chromium.org> wrote:

> On 2013/08/28 23:49:09, danakj wrote:
>
>> Ok, I was worried that maybe the animation registrar should not have
>> active
>> animation controllers if there is no active tree, but it seems to me that
>> as
>> soon as we push an animation to the pending tree, the animation
>> controller is
>> added to the active list.
>>
>
>  How would you repro this 100% in a test? Would you commit a layer tree
>> with an
>> animation while also setting visible false, and then delay the activation
>> of
>> that tree so that the background timer would tick? Could we do that in a
>> test
>> and verify that the ticking is off?
>>
>
> I will add a DCHECKs in LTHI that check if background ticking is ever
> enabled
> when the root layer is null. That will catch the problem at the source
> 100% of
> the time.
>
> I'm already finding a few more places that are enabling background ticking
> when
> the root layer is null just by running the normal unit tests.
>

How else would it happen if you're null checking at all these sites?


>
>
>
https://codereview.chromium.**org/23593003/<https://codereview.chromium.org/2...
>

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

[brianderson, 2013-08-29 00:50:10]

On 2013/08/29 00:44:57, danakj wrote:
> On Wed, Aug 28, 2013 at 8:36 PM, <mailto:brianderson@chromium.org> wrote:
> 
> > On 2013/08/28 23:49:09, danakj wrote:
> >
> >> Ok, I was worried that maybe the animation registrar should not have
> >> active
> >> animation controllers if there is no active tree, but it seems to me that
> >> as
> >> soon as we push an animation to the pending tree, the animation
> >> controller is
> >> added to the active list.
> >>
> >
> >  How would you repro this 100% in a test? Would you commit a layer tree
> >> with an
> >> animation while also setting visible false, and then delay the activation
> >> of
> >> that tree so that the background timer would tick? Could we do that in a
> >> test
> >> and verify that the ticking is off?
> >>
> >
> > I will add a DCHECKs in LTHI that check if background ticking is ever
> > enabled
> > when the root layer is null. That will catch the problem at the source
> > 100% of
> > the time.
> >
> > I'm already finding a few more places that are enabling background ticking
> > when
> > the root layer is null just by running the normal unit tests.
> >
> 
> How else would it happen if you're null checking at all these sites?
> 

I didn't have a chance to look at the DCHECKs before, but it turns out the
SingleThreadProxy also uses background ticking, which I wasn't aware of before,
and turns it on when the active_tree is null.

> 
> >
> >
> >
>
https://codereview.chromium.**org/23593003/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2013-08-29 01:03:09]

Hmm. I suppose that, in the SingleThreadProxy case, the animation registrar
would be empty if the active tree was null, which would prevent background
ticking even if SingleThreadProxy wanted it.

I still like having the DCHECKs enabled for the SingleThreadProxy though, since
it forces the SingleThreadProxy to be strict in case LTHI ever decides to
actually background tick.

https://codereview.chromium.org/23593003/diff/22001/cc/trees/single_thread_pr...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/23593003/diff/22001/cc/trees/single_thread_pr...
cc/trees/single_thread_proxy.cc:460: UpdateBackgroundAnimateTicking();
For the same reasons as in ThreadProxy, I do not always enable or disable
background ticking in the DoComposite method.

[danakj, 2013-08-29 01:11:02]

OK, the dchecks looks reasonable. Do you have a test coming that would trigger
them before these changes?

[brianderson, 2013-08-29 01:23:22]

Ok. Working on a test now.

[brianderson, 2013-08-29 04:11:51]

Added the test.

[danakj, 2013-08-29 17:22:17]

https://codereview.chromium.org/23593003/diff/34001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/23593003/diff/34001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:311: // This alternates setting
the root tree null and root tree with animation.
"setting the root tree null" - i think you mean setting an empty tree?

"and root tree with animation" - setting a non-empty tree with an animation?

https://codereview.chromium.org/23593003/diff/34001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:318:
PostAddAnimationToMainThread(layer_tree_host()->root_layer());
This is going to be racey, you're post-tasking the add animation, but the impl
thread could have already post-tasked the beginframe. You should just add the
animation directly.

You could also do the same in BeginTest() and simply PostSNC().

https://codereview.chromium.org/23593003/diff/34001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:345:
&LayerTreeHostAnimationTestNoBackgroundTickingWithoutActiveTree::
80 col. save the Bind() result in a callback before this PostDelayedTask to
avoid this?

Callback cb = base::Bind(
     &LayerTre...

https://codereview.chromium.org/23593003/diff/34001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:352:
UnblockActivations(host_impl);
if (!impl paint) do this and early out?  maybe that solves the 80col problem too

https://codereview.chromium.org/23593003/diff/34001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:384: virtual void
InitiateNextCommit() {
not virtual?

[brianderson, 2013-08-29 19:00:36]

ptal

[danakj, 2013-08-29 19:58:19]

Thanks a lot for your effort, this LGTM. enne@ may need some convincing.

To me this makes sense because in order to have an already-started animation in
the pending tree, but an empty active tree, we would have needed to commit an
empty tree from the main thread but still had an animation active in the
compositor for said empty tree.

(Also, existing tests pass so they aren't relying on a different behaviour.)

https://codereview.chromium.org/23593003/diff/51001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/23593003/diff/51001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:300: public:
nit: protected:

https://codereview.chromium.org/23593003/diff/51001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:348: // so just addvance the test
imeediately.
nit: addvance...imeediately

https://codereview.chromium.org/23593003/diff/51001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:412: EXPECT_EQ(has_active_tree,
active_tree_was_animated_);
Thanks for this, i love tests that test themselves!

[enne (OOO), 2013-08-29 21:25:52]

lgtm2

[commit-bot: I haz the power, 2013-08-29 21:30:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23593003/51002

[commit-bot: I haz the power, 2013-08-30 01:10:03]

Change committed as 220462