1307295 - Do not update cascade result and compose style if the target element has been removed

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

While implementing bug 1305325, I noticed that we do update cascade result and compose style even if the target element has been removed.

 var div = document.getElementById("target");
 var animation = div.animate({ }, 1000);
 div.remove();

In above code, we don't remove the target element from mElementsToRestyle so in a subsequent tick, we call MaybeUpdateCascadeResults and ComposeAnimationRule in EffectCompositor::AddStyleUpdatesTo.

Checking IsInComposedDoc() avoids this,  I am now figuring out how to write a test case for this.

This bug blocks bug 1305325 because we can't resolve the base style for the element which is not associated with any document.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Automation test cases

I came up with test cases to check skipping updating cascade does not cause any side effects when the target element is re-attached to the document.

BUT, unfortunately these test cases currently do not work well because we don't clear isRunningOnCompositor flag when the target element is removed from document.  Actually the animation on removed element is not running on the compositor.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Created attachment 8797848 [details] [diff] [review]
> Automation test cases
> 
> I came up with test cases to check skipping updating cascade does not cause
> any side effects when the target element is re-attached to the document.
> 
> BUT, unfortunately these test cases currently do not work well because we
> don't clear isRunningOnCompositor flag when the target element is removed
> from document.  Actually the animation on removed element is not running on
> the compositor.

I think the place we clear the isRunningOnCompositor flag is Element::UnbindFromTree().  I am not sure we should do it because animations on removed elements does not consume main thread at all just like animations on the compositor.  (Actually they don't consume the compositor thread either though)

So, I'd like to check IsInComposeDoc() in EffectCompositor::AddStyleUpdatesTo() here.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1307295 - Do not request restyle for animations on orphaned element.

If animation reqeusted restyle but the target element has been removed from
document before a subsequent tick, we don't need to update the cascade and
compose style either.

Review commit: https://reviewboard.mozilla.org/r/85026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85026/

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8799995 [details]
Bug 1307295 - Do not request restyle for animations on orphaned element.

https://reviewboard.mozilla.org/r/85026/#review83604

::: dom/animation/EffectCompositor.cpp:424
(Diff revision 1)
> +      // Ignore animations for the element which is not attached to the
> +      // document.
> +      if (!pseudoElem.mElement->IsInComposedDoc()) {
> +        continue;
> +      }

Whatever we do in Element::UnbindFromTree(), checking IsInComposedDoc() is worthwhile just like we do in nsStyle::GetContext()[1].  Also this change is sufficient for bug 1305325, as far as I can tell.

[1] http://searchfox.org/mozilla-central/rev/e8dd21b1673c07307068303ab3e99360c78f4d12/layout/style/nsStyleSet.cpp#972

Comment 5

[Brian Birtles (:birtles)]

I did some code archaeology and turned up bug 1200568 as the place where we added the IsInComposedDoc() check that eventually got moved to nsStyleSet::GetContext().

It made sense there because we definitely don't want to run CSS animations on elements that are not part of the tree. I wonder if script-generated animations should work, however? Maybe not?

I believe that currently getComputedStyle(elem) doesn't work in Firefox for elements that are in a display:none subtree and that bz is currently fixing that. I guess that is only for display:none nodes, however, and not orphaned nodes though so this is probably fine.

I haven't completely thought this through yet, but two things I want to check are:

1) If we drop restyle requests for orphaned nodes, are we sure to perform the necessary restyle and cascade update when the node is rebound to the document?

2) Can we stop adding requesting restyles for orphaned nodes earlier on? (I think we'll still need this check for the case when we add the restyle, then unbind the node, but we could probably save some unnecessary work if we skip adding the restyle in the first place.)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)
> I did some code archaeology and turned up bug 1200568 as the place where we
> added the IsInComposedDoc() check that eventually got moved to
> nsStyleSet::GetContext().
> 
> It made sense there because we definitely don't want to run CSS animations
> on elements that are not part of the tree. I wonder if script-generated
> animations should work, however? Maybe not?
> 
> I believe that currently getComputedStyle(elem) doesn't work in Firefox for
> elements that are in a display:none subtree and that bz is currently fixing
> that. I guess that is only for display:none nodes, however, and not orphaned
> nodes though so this is probably fine.
> 
> I haven't completely thought this through yet, but two things I want to
> check are:
> 
> 1) If we drop restyle requests for orphaned nodes, are we sure to perform
> the necessary restyle and cascade update when the node is rebound to the
> document?

That's attachment 8797848 [details] [diff] [review].  It may not be perfect, but it might cover some cases.  If we clear mIsRunningOnCompositor flag in Element::UnbindFromTree(), the test works.

Regarding the cascade it should be done against the newly attached parent element when re-attaching the new document, so this skip does not matter at all I think.  (I am not sure where we currently do MarkCascadeNeedsUpdate() when re-attaching to the document though. We don't? I will check it later)
Regarding the request restyle we repeatedly call KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() in every tick, so this does not matter I think.  This made me realize that the right place is NotifyAnimationTimingUpdated().

> 2) Can we stop adding requesting restyles for orphaned nodes earlier on? (I
> think we'll still need this check for the case when we add the restyle, then
> unbind the node, but we could probably save some unnecessary work if we skip
> adding the restyle in the first place.)

That's right!  We can check it just before calling RequestRestyle() in KeyframeEffectReadOnly::NotifyAnimationTimingUpdated().

Thanks!  I will update the patch soon.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > 2) Can we stop adding requesting restyles for orphaned nodes earlier on? (I
> > think we'll still need this check for the case when we add the restyle, then
> > unbind the node, but we could probably save some unnecessary work if we skip
> > adding the restyle in the first place.)
> 
> That's right!  We can check it just before calling RequestRestyle() in
> KeyframeEffectReadOnly::NotifyAnimationTimingUpdated().

Ooops! I overlooked the test case in comment 0.  Checking in KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() works for most cases, but in the test case in comment 0, we can't stop request restyle (it's already requested when calling Element.animate) in KeyframeEffectReadOnly::NotifyAnimationTimingUpdated().  To stop the request completely in the test case we need to pull the element out from mElementToRestyle when Element::UnbindFromTree() is called. But I did not want to do that.

Comment 8

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > > 2) Can we stop adding requesting restyles for orphaned nodes earlier on? (I
> > > think we'll still need this check for the case when we add the restyle, then
> > > unbind the node, but we could probably save some unnecessary work if we skip
> > > adding the restyle in the first place.)
> > 
> > That's right!  We can check it just before calling RequestRestyle() in
> > KeyframeEffectReadOnly::NotifyAnimationTimingUpdated().
> 
> Ooops! I overlooked the test case in comment 0.  Checking in
> KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() works for most cases,
> but in the test case in comment 0, we can't stop request restyle (it's
> already requested when calling Element.animate) in
> KeyframeEffectReadOnly::NotifyAnimationTimingUpdated().  To stop the request
> completely in the test case we need to pull the element out from
> mElementToRestyle when Element::UnbindFromTree() is called. But I did not
> want to do that.

Right. That's what I meant by, "I think we'll still need this check for the case when we add the restyle, then unbind the node". I'm wondering if we can *also* stop the request restyle earlier. Otherwise, I think we could continue triggering restyles for an element that is not in the document.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > > > 2) Can we stop adding requesting restyles for orphaned nodes earlier on? (I
> > > > think we'll still need this check for the case when we add the restyle, then
> > > > unbind the node, but we could probably save some unnecessary work if we skip
> > > > adding the restyle in the first place.)
> > > 
> > > That's right!  We can check it just before calling RequestRestyle() in
> > > KeyframeEffectReadOnly::NotifyAnimationTimingUpdated().
> > 
> > Ooops! I overlooked the test case in comment 0.  Checking in
> > KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() works for most cases,
> > but in the test case in comment 0, we can't stop request restyle (it's
> > already requested when calling Element.animate) in
> > KeyframeEffectReadOnly::NotifyAnimationTimingUpdated().  To stop the request
> > completely in the test case we need to pull the element out from
> > mElementToRestyle when Element::UnbindFromTree() is called. But I did not
> > want to do that.
> 
> Right. That's what I meant by, "I think we'll still need this check for the
> case when we add the restyle, then unbind the node". I'm wondering if we can
> *also* stop the request restyle earlier. Otherwise, I think we could
> continue triggering restyles for an element that is not in the document.

Thanks. I did not understand your comment at all.  I will post a new patch with some test cases, but the test case will pass even without the fix.  I may have said before, we don't have any mechanism to check that "*request* restyle" is skipped or not.

Comment 10

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> Thanks. I did not understand your comment at all.  I will post a new patch
> with some test cases, but the test case will pass even without the fix.  I
> may have said before, we don't have any mechanism to check that "*request*
> restyle" is skipped or not.

That's fine. Are you going to add a check to EffectCompositor::RequestRestyle or should I review the patch as-is?

To be clear, what I had in mind was:

1. Add a check to something like EffectCompositor::RequestRestyle for an element that is not in the document.
2. Keep the check in EffectCompositor::AddStyleUpdatesTo to handle cases like comment 0.

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8799995 [details]
Bug 1307295 - Do not request restyle for animations on orphaned element.

https://reviewboard.mozilla.org/r/85026/#review83684

::: dom/animation/EffectCompositor.cpp:419
(Diff revision 1)
>      for (auto iter = elementSet.Iter(); !iter.Done(); iter.Next()) {
>        elementsToRestyle.AppendElement(iter.Key());
>      }
>  
>      for (auto& pseudoElem : elementsToRestyle) {
> +      // Ignore animations for the element which is not attached to the
> +      // document.
> +      if (!pseudoElem.mElement->IsInComposedDoc()) {
> +        continue;
> +      }

Could we add the check to the earlier loop?

e.g.

  for (auto iter = elementSet.Iter(); !iter.Done(); iter.Next()) {
    // Skip orphaned elements
    if (iter.Key().mElement->IsInComposedDoc()) {
      elementsToRestyle.AppendElement(iter.Key());
    }
  }

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > Thanks. I did not understand your comment at all.  I will post a new patch
> > with some test cases, but the test case will pass even without the fix.  I
> > may have said before, we don't have any mechanism to check that "*request*
> > restyle" is skipped or not.
> 
> That's fine. Are you going to add a check to
> EffectCompositor::RequestRestyle or should I review the patch as-is?
> 
> To be clear, what I had in mind was:
> 
> 1. Add a check to something like EffectCompositor::RequestRestyle for an
> element that is not in the document.

I thought I did the check in NotifyAnimationTimingUpdated() to skip CanThrottle() check too. But you are right, EffectCompositor::RequestRestyle() is more proper place.  

> 2. Keep the check in EffectCompositor::AddStyleUpdatesTo to handle cases
> like comment 0.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8799995 [details]
Bug 1307295 - Do not request restyle for animations on orphaned element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85026/diff/1-2/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8799995 [details]
Bug 1307295 - Do not request restyle for animations on orphaned element.

https://reviewboard.mozilla.org/r/85026/#review83688

::: dom/animation/test/chrome/test_restyles.html:762
(Diff revision 2)
> +    markers = yield observeStyling(5);
> +    is(markers.length, 4,
> +       'Animation on re-attached to the document begins to update style');

I am convinced these test cases are valid, but I can't request review yet because I still don't understand why we recevive only 4 restyles here.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8799995 [details]
Bug 1307295 - Do not request restyle for animations on orphaned element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85026/diff/2-3/

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8799995 [details]
Bug 1307295 - Do not request restyle for animations on orphaned element.

https://reviewboard.mozilla.org/r/85026/#review83692

Thanks so much for doing this.

::: dom/animation/EffectCompositor.cpp:425
(Diff revision 3)
> +      // Skip animations on orphaned elements which has been removed from
> +      // the document after requested restyle for the element.

Perhaps we could make this a little shorter:

"Skip animations on elements that have been orphaned since they requested a restyle."

?

::: dom/animation/test/chrome/test_restyles.html:753
(Diff revision 3)
> +    var animation = div.animate({ marginLeft: [ '0px', '100px' ] },
> +                                100 * MS_PER_SEC);
> +
> +    yield animation.ready;
> +
> +    // Disassociate with document.

(I wonder if we need this comment?)

::: dom/animation/test/chrome/test_restyles.html:760
(Diff revision 3)
> +
> +    var markers = yield observeStyling(5);
> +    is(markers.length, 0,
> +       'Animation on orphaned element should not cause restyles');
> +
> +    // Attach to the document again.

(Likewise here?)

::: dom/animation/test/chrome/test_restyles.html:778
(Diff revision 3)
> +  add_task_if_omta_enabled(
> +    function* restyling_for_opacity_animation_on_re_attached_element() {

This could probably do with a comment briefly describing what the purpose of the test here--specifically something like, "Tests that if we remove an element from the document whose animation cascade needs recaculating, that it is correctly updated when it is re-attached to the document"

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8799995 [details]
Bug 1307295 - Do not request restyle for animations on orphaned element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85026/diff/3-4/

Comment 18

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d508665f41

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dd5a50179e5
Do not request restyle for animations on orphaned element. r=birtles

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2dd5a50179e5