Closed Bug 1381420 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: heycam, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Attachments

(2 files, 2 obsolete files)

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.
Keywords: crash
Priority: -- → P1
Blocks: 1380897
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
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.
(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.
No longer blocks: 1380897
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 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?.
Attachment #8890148 - Flags: review?(emilio+bugs) → review+
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
Attachment #8890149 - Flags: review?(emilio+bugs) → review+
(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!
Attached file Servo PR (obsolete) —
Attached file Servo PR
Did I paste wrong text?
Attachment #8890156 - Attachment is obsolete: true
Attachment #8890148 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/45b3b1292029
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.