1381420 - stylo: panic "Restyle damage should be empty if the element was not restyled" on FastMail

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

Switching between mail folders on FastMail, with Hiro's patches from bug 1378064 applied, I sometimes run into this assertion:

thread '<unnamed>' panicked at 'Restyle damage should be empty if the element was not restyled', /z/moz/d/servo/ports/geckolib/glue.rs:2745

in Servo_TakeChangeHint.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I noticed that the damage comes from ServoRestyleManager::AttributeChanged() (thanks Cameron for the confirmation).
I don't yet have any good ideas to preserve these restyle hints and change hints during animation-only restyle for throttled animations.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I noticed that the damage comes from ServoRestyleManager::AttributeChanged()
> (thanks Cameron for the confirmation).
> I don't yet have any good ideas to preserve these restyle hints and change
> hints during animation-only restyle for throttled animations.

Err, we don't need to care about those restyle hints, we have already preserved restyle hints other than animation hints.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I had been trying to write a reftest for this, it turns out we can't write any reftest for this since

1) We can't use SpecialPowers to use DOMWindowUtils.sendMouseEvent() in reftest (bug 655622)
2) Element.dispatchEvent() did call FlushThrottledStyles() which is a trigger of this issue

So, I am going to add a crash test for now.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1381420 - Preserve restyle damage if the element was not restyled during throttled animation flush.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1381420 - A crash test that has an element is not restyled but has reconstruct frame damage during flushing throttled animations.

The details element, which is a sibling of an animating element, is checked
whether the element needs to be processed in a post traversal for throttled
animation flush because the parent element has animation-only dirty bit (i.e.
the parent element is a common ancestor of the animating element and the
details element). Also at the moment of the check the details element has
reconstruct frame damage because 'open' attribute is changed. Thus in the check
the details element was not restyled but had reconstruct frame damage.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890148 [details]
Bug 1381420 - Preserve restyle damage if the element was not restyled during throttled animation flush.

https://reviewboard.mozilla.org/r/161228/#review166572

::: servo/ports/geckolib/glue.rs:2773
(Diff revision 1)
>          Some(mut data) => {
>              *was_restyled = data.restyle.is_restyle();
>  
>              let damage = data.restyle.damage;
>              if restyle_behavior == structs::TraversalRestyleBehavior::ForThrottledAnimationFlush {
> -                debug_assert!(data.restyle.is_restyle() || damage.is_empty(),
> +                if !*was_restyled && !damage.is_empty() {

I wonder if this can just be `if !*was_restyled`, right?

And it doesn't really need to be in this block, it could be right after computing `was_restyled`, right?.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890149 [details]
Bug 1381420 - A crash test that has an element is not restyled but has reconstruct frame damage during flushing throttled animations.

https://reviewboard.mozilla.org/r/161230/#review166574

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> Comment on attachment 8890148 [details]
> Bug 1381420 - Preserve restyle damage if the element was not restyled during
> throttled animation flush.
> 
> https://reviewboard.mozilla.org/r/161228/#review166572
> 
> ::: servo/ports/geckolib/glue.rs:2773
> (Diff revision 1)
> >          Some(mut data) => {
> >              *was_restyled = data.restyle.is_restyle();
> >  
> >              let damage = data.restyle.damage;
> >              if restyle_behavior == structs::TraversalRestyleBehavior::ForThrottledAnimationFlush {
> > -                debug_assert!(data.restyle.is_restyle() || damage.is_empty(),
> > +                if !*was_restyled && !damage.is_empty() {
> 
> I wonder if this can just be `if !*was_restyled`, right?
> 
> And it doesn't really need to be in this block, it could be right after
> computing `was_restyled`, right?.

Oh right, exactly!

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Did I paste wrong text?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8890149 [details]
Bug 1381420 - A crash test that has an element is not restyled but has reconstruct frame damage during flushing throttled animations.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45b3b1292029
A crash test that has an element is not restyled but has reconstruct frame damage during flushing throttled animations. r=emilio

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45b3b1292029