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

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: heycam, Assigned: hiro)

Tracking

(Blocks: 1 bug, {crash})

unspecified
mozilla56
crash
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1380897
(Assignee)

Updated

2 years ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
(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
(Assignee)

Comment 3

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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 8

2 years ago
mozreview-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+
(Assignee)

Comment 9

2 years ago
(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!
(Assignee)

Comment 10

2 years ago
Posted file Servo PR (obsolete) —
(Assignee)

Comment 11

2 years ago
Posted file Servo PR
Did I paste wrong text?
Attachment #8890156 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8890148 - Attachment is obsolete: true

Comment 13

2 years ago
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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45b3b1292029
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.