1350115 - stylo: StyleNewSubtree triggers animation restyle and requests a post-traversal we aren't set up to do

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Bobby Holley (:bholley)]

I have a strawman patch to kick off discussion, but it might not be the right thing.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Don't do a second animation traversal when we're styling a fresh subtree. r?birtles

I'm not very confident that this patch is the right thing, looking for feedback
from Brian here.

This patch exists to avoid a crash in layout/style/test/test_animations.html. We end up
generating some ::before content, which causes us to style the new subtree at [1]. In
StyleNewSubtree, we fail the !postTraversalRequired assertion because
PrepareAndTraverseSubtree decided to traverse the tree twice (once to style it, and again
to restyle it for animations), and return that a post-traversal is needed.

We definitely aren't set up to do a post-traversal in StyleNewSubtree and StyleNewChildren.
This patch can solve the problem with StyleNewSubtree, but I'm worried that we could still
trip this case in StyleNewChildren (where the root is styled, but some children may not be).

The reason this issue happens with my patches and not without is that we were previously
filtering out generated ::before content from the servo traversal, so the servo traversal
wouldn't have reached it and (presumably) the animation restyle wouldn't have happened and
we wouldn't have returned true for needing a post-traversal.

[1] http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/layout/base/nsCSSFrameConstructor.cpp#1918

MozReview-Commit-ID: 8tgzLjV8B3A

Comment 2

[Brian Birtles (:birtles)]

Hiro and I discussed this and I'm not quite sure if this will work.

We were originally concerned about the way EffectCompositor tracks restyles and in particular that we would end up in a situation where animation would fail to update because we thought we had posted a restyle when we had not.

The background to this is that EffectCompositor keeps a hashtable of elements needing a restyle (mElementsToRestyle) and a bool that is set to track if we have posted a restyle for a each element. (We sometimes add an element without setting this to true if it is a compositor animation whose main thread restyles we are throttling). If that bool is set to true then we will ignore subsequent calls to EffectCompositor::RequestRestyle for that element since we figure they're redundant.

In this case is we will have something like the following sequence:

* ServoStyleSet::StyleNewSubtree
  * ServoStyleSet::PreTraverse
    * EffectCompositor::PreTraverse
      (Probably nothing of relevance to do here since we haven't restyled the new generated content yet)
  * ServoStyleSet::PrepareAndTraverseSubtree
    * Servo_TraverseSubtree
      - Detect new animations as part of parallel traversal.
      - In a sequential task, create the animations.
        (This is in: update_animations -> Gecko_UpdateAnimations -> nsAnimationManager::UpdateAnimations)
        As part of this we will call EffectCompositor::RequestRestyle for the new animations/elements.
        This will add the element to the hashtable mentioned above (mElementsToRestyle) and go to post a restyle.
        However, in EffectCompositor::PostRestyleForAnimation we will return early since ServoStyleSet::IsInServoTraversal()
        returns true there.

This last part where we return early and skip posting the restyle is because we know that we're going to do another PreTraverse followed by a Servo_TraverseSubtree immediately afterwards. (Note that the PreTraverse will call Servo_NoteExplicitHints for each element in mElementsToRestyle whose corresponding bool is true.)

However, in the above sequence where we return early because aRoot->HasServoData() was false, we have an entry in mElementsToRestyle that says we have posted a restyle but in fact we have not. That seems somewhat problematic.

On the one hand, we need those elements to be in the hashmap with a 'true' value since it's on that basis that we will call Servo_NoteExplicitHints for each of them on the next call to PreTraverse. However, as far as I can tell, at the end of the sequence there's nothing that tells us we still need to do another restyle before painting?

So, perhaps in EffectCompositor::PostRestyleForAnimation we need to behave differently in the case where we know we're not going to run a subsequent traversal? i.e. instead of just checking ServoStyleSet::IsInServoTraversal() we need to know if we're going to do a subsequent restyle or not?

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8850754 [details] [diff] [review]
Don't do a second animation traversal when we're styling a fresh subtree. r?birtles

I think I know what we need to do here. Will attach a patch.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Squelch post-traversal generated by additional animation traversals when we're styling a fresh subtree. v1

MozReview-Commit-ID: 8tgzLjV8B3A

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8850849 [details] [diff] [review]
Squelch post-traversal generated by additional animation traversals when we're styling a fresh subtree. v1

Review of attachment 8850849 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +228,5 @@
>  
>    // If there are still animation restyles needed, trigger a second traversal to
>    // update CSS animations' styles.
> +  if (mPresContext->EffectCompositor()->PreTraverse()) {
> +      if (Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior)) {

Nit: this is indented too much.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3afa3f3c4b2
Squelch post-traversal generated by additional animation traversals when we're styling a fresh subtree. r=heycam,r=birtles

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/e3afa3f3c4b2