Closed Bug 1405544 Opened 3 years ago Closed 3 years ago

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

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: bc, Assigned: hiro)

References

()

Details

Attachments

(2 files)

Attached file 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
Flags: needinfo?(hikezoe)
Priority: -- → P2
Can't reproduce at all on m-c <https://hg.mozilla.org/mozilla-central/rev/c97190c389c4>.
Still trying.
OK, I could reproduce while I was doing other things. So I don't know what the trigger is. :)
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
CCing :wcpan this might be related to bug 1363805.
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.
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. :/
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
(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?
(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. :/
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 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.
Attachment #8916841 - Flags: review?(emilio) → review+
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/f62761c9fd71
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.