1359658 - stylo: Animation-only dirty bit not cleared on elements in display:none subtree

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Test case

While working on SMIL I ran into an issue with display:none subtrees.

The issue is that if we have an element being animated it may cause its parent to have the animation dirty bit set on it. If we then make an ancestor display:none, during the next restyle we will skip it since in traverse_dom (or top_down_dom) we call `traversal.traverse_children` but `should_traverse_children` will return false for elements with a display:none parent. As a result we never call process_preorder/recalc_style_at on the element and the dirty bit remains. When we go to assert the tree is clean at the end of the traversal we fail because the dirty bit is still set.

The attached test case causes an assertion failure for me although I haven't applied it it to a completely clean check-out so it's possible there is something in my local working copy that triggers this.

To work around this I'm thinking of adding a check to recalc_style_at to change the part where we preprocess children to be something like:

  if traversal.should_traverse_children {
    ...(preprocess as usual)
  } else if we're in an animation-only restyle and the animation-only
    descendants dirty bit is set {
    go through descendants and clear animation-only dirty bit
  }

Hiro, does that make sense? Am I missing something?

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I think we should clear animation dirty bit as well as normal dirty bit below places;

https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/style/traversal.rs#l664
https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/style/traversal.rs#l779

Does this change fix it?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

One more;
https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/style/traversal.rs#l540

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Great, thank you!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I think we should clear animation dirty bit as well as normal dirty bit
> below places;
> 
> https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/
> style/traversal.rs#l664

I'm not sure changing this part makes sense. We already unconditionally clear the animation dirty bit just above if we are in an animation restyle. What is needed is to clear it for descendants too. However it would be odd to change this to clear the animation bit on descendants but only clear the other dirty bit for the current element.

However, this part...

> https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/
> style/traversal.rs#l779

... clear_descendant_data, is sometimes called earlier in restyle_style_at and does traverse children. Fixing this (to also clear the animation-only dirty descendants bit) is enough to fix this particular test case.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> One more;
> https://hg.mozilla.org/mozilla-central/file/3f0c8da53c5c/servo/components/
> style/traversal.rs#l540

I don't have a test case that fails without this change but it makes sense to me to also extend this handling to the animation-only dirty descendants bit.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1359658 - Crashtest for clearing animation-only dirty descendants bit on display:none descendants;

When an element has a display:none ancestor we don't traverse it during restyle.
However, at the end of restyling we expect the tree to be free of dirty bits.
We currently take special care to clear the regular (non-animation) dirty bit
on nodes in display:none subtrees in order to preserve this invariant. This
patch applies the same handling to the animation-only dirty descendants bit.

I have verified that the included crashtest fails without the code changes
included in this patch.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8861742 [details]
Bug 1359658 - Crashtest for clearing animation-only dirty descendants bit on display:none descendants;

https://reviewboard.mozilla.org/r/133728/#review136594

Thank you for finding this and quick fix!

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Servo PR #16613

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8861742 [details]
Bug 1359658 - Crashtest for clearing animation-only dirty descendants bit on display:none descendants;

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

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Servo: https://hg.mozilla.org/integration/autoland/rev/29cead1a3a5f864f8f429a9b9cc0f961f7c38837

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a56bcd82878
Crashtest for clearing animation-only dirty descendants bit on display:none descendants; r=hiro

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/3a56bcd82878