1405544 - stylo: thread '<unnamed>' panicked at 'animation restyle hint should be handled during animation-only restyles', servo/components/style/traversal.rs:531

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

Attachment: Windows Debug Log

1. https://www.aol.com/?icid=aolcomlogorefresh5&dlact=dl1

   May need to scroll a bit.

   Windows, Linux, Nightly/58 

   I couldn't reproduce locally with Beta/57 on Linux though not totally sure yet this is a regression from 57.

2. thread '<unnamed>' panicked at 'animation restyle hint should be handled during animation-only restyles', Z:/build/build/src/servo/components/style/traversal.rs:531

 4  xul.dll!style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure> [<panic macros> : 3 + 0x20]
    eip = 0x5ed2d84d   esp = 0x002f575c   ebp = 0x002f5854
    Found by: previous frame's frame pointer
 5  xul.dll!style::gecko::traversal::{{impl}}::process_preorder<closure> [traversal.rs:c97190c389c4 : 37 + 0x22]
    eip = 0x5ed02538   esp = 0x002f585c   ebp = 0x002f588c
    Found by: previous frame's frame pointer
 6  xul.dll!style::driver::traverse_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> [driver.rs:c97190c389c4 : 71 + 0x43]
    eip = 0x5f15d73a   esp = 0x002f5894   ebp = 0x002f5c84
    Found by: previous frame's frame pointer
 7  xul.dll!geckoservo::glue::traverse_subtree [glue.rs:c97190c389c4 : 271 + 0xb]
    eip = 0x5ed60002   esp = 0x002f5c8c   ebp = 0x002f5d80
    Found by: previous frame's frame pointer

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Can't reproduce at all on m-c <https://hg.mozilla.org/mozilla-central/rev/c97190c389c4>.
Still trying.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

OK, I could reproduce while I was doing other things. So I don't know what the trigger is. :)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

CCing :wcpan this might be related to bug 1363805.

Comment 4

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

I cannot reproduce this locally on m-c, so I don't have many useful information for now.

We will restyle if EffectCompositor has pending style updates for the element:
http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/layout/style/nsComputedDOMStyle.cpp#140
before we do the invalidation in Servo.
So if you think bug 1363805 is the root cause, try to change it to restyle if there are any style updates in EffectCompositor for the whole document, not just the element, and see if there is any difference.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Thank you, :wcpan.  Yep, I did actually revert the commit <https://hg.mozilla.org/mozilla-central/rev/3b0852f8a696>, and I could not reproduce the panic with the reverting so far, so I don't think bug 1363805 is not the culprit.  But yes, the panic is hard to reproduce (I haven't yet found any reliable ways to reproduce), so I might be wrong.  Anyway what I knew so far is;

1) When the panic happened, mServoRestyleRootDirtyBits has animation-dirty bit, but the restyle root node itself does not have animation-dirty bit

Thus, animation-dirty bit on a descendant element remains there because we do animation-only traverse only if the root node has animation-dirty bit or animation restyle hints.

That's all. :/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I think I found which bug caused this, it's bug 1403078.  It explains why the panic does not happen on 57.  The place where dirty bits mismatch happens is in ServoStyleSet::StyleDocument() [1]:

      if (parent->HasDirtyDescendantsForServo()) {
        // If any style invalidation was triggered in our siblings, then we may
        // need to post-traverse them, even if the root wasn't restyled after
        // all.
        doc->SetServoRestyleRoot(
            parent,
            doc->GetServoRestyleRootDirtyBits() |
            ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO);
        postTraversalRequired = true;
      }

If there is an animation-dirty bit and style invalidation happened on a sibling, then mServoRestyleRootDirtyBits has still animation-dirty bit, but the new style root element does not have animation-dirty bit unfortunately.

[1] https://hg.mozilla.org/mozilla-central/file/19b32a138d08/layout/style/ServoStyleSet.cpp#l986

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> I think I found which bug caused this, it's bug 1403078.  It explains why
> the panic does not happen on 57.  The place where dirty bits mismatch
> happens is in ServoStyleSet::StyleDocument() [1]:
> 
>       if (parent->HasDirtyDescendantsForServo()) {
>         // If any style invalidation was triggered in our siblings, then we
> may
>         // need to post-traverse them, even if the root wasn't restyled after
>         // all.
>         doc->SetServoRestyleRoot(
>             parent,
>             doc->GetServoRestyleRootDirtyBits() |
>             ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO);
>         postTraversalRequired = true;
>       }
> 
> If there is an animation-dirty bit and style invalidation happened on a
> sibling, then mServoRestyleRootDirtyBits has still animation-dirty bit, but
> the new style root element does not have animation-dirty bit unfortunately.

Well, that's easy to fix then, we need to set the other descendant bits too on the parent, right?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > I think I found which bug caused this, it's bug 1403078.  It explains why
> > the panic does not happen on 57.  The place where dirty bits mismatch
> > happens is in ServoStyleSet::StyleDocument() [1]:
> > 
> >       if (parent->HasDirtyDescendantsForServo()) {
> >         // If any style invalidation was triggered in our siblings, then we
> > may
> >         // need to post-traverse them, even if the root wasn't restyled after
> >         // all.
> >         doc->SetServoRestyleRoot(
> >             parent,
> >             doc->GetServoRestyleRootDirtyBits() |
> >             ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO);
> >         postTraversalRequired = true;
> >       }
> > 
> > If there is an animation-dirty bit and style invalidation happened on a
> > sibling, then mServoRestyleRootDirtyBits has still animation-dirty bit, but
> > the new style root element does not have animation-dirty bit unfortunately.
> 
> Well, that's easy to fix then, we need to set the other descendant bits too
> on the parent, right?

Right, it fixed, but I can't write test cases for it. :/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I am inclined to give up writing test cases for this.  I need to know what's going on there exactly to write the test cases, so I did try rr but it got really slow before reproducing the panic.  I also tried RUST_LOG=debug and stored the log into a file, the file size exceeded over 5GiB, so I had to kill it.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1405544 - Propagate existing bits to the parent element as well when switch the restyle root for invalidation siblings in post-traversal.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8916841 [details]
Bug 1405544 - Propagate existing bits to the parent element as well when switch the restyle root for invalidation siblings in post-traversal.

https://reviewboard.mozilla.org/r/187904/#review193004

::: layout/style/ServoStyleSet.cpp:989
(Diff revision 1)
>        MOZ_ASSERT(root == doc->GetServoRestyleRoot());
>        if (parent->HasDirtyDescendantsForServo()) {
>          // If any style invalidation was triggered in our siblings, then we may
>          // need to post-traverse them, even if the root wasn't restyled after
>          // all.
> +        uint32_t existingBits = doc->GetServoRestyleRootDirtyBits();

Well, we can also do this where the dirty descendants bit is propagated, but not a big deal I suppose.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f62761c9fd71
Propagate existing bits to the parent element as well when switch the restyle root for invalidation siblings in post-traversal. r=emilio

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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