Closed Bug 1400926 Opened 2 years ago Closed 2 years ago

stylo: thread '<unnamed>' panicked at 'assertion failed: !importance.important()', servo/components/style/stylesheets/keyframes_rule.rs:379

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed

People

(Reporter: bc, Assigned: hiro)

References

()

Details

(Keywords: crash, regression)

Crash Data

Attachments

(5 files, 1 obsolete file)

Attached file Windows Debug Log
[Tracking Requested - why for this release]: stylo regression

1. https://ubishaker.com/fr/38/1503902837704555/game/taptap/screen

2. thread '<unnamed>' panicked at 'assertion failed: !importance.important()', Z:/build/build/src/servo/components/style/stylesheets/keyframes_rule.rs:379

 2  xul.dll!style::stylist::CascadeData::add_stylesheet<style::gecko::data::GeckoStyleSheet> [stylist.rs:ae39864562c6 : 2072 + 0x1d7]
    rbx = 0x00007ffc3bb9cd10   rbp = 0x000000e4623faec0
    rsp = 0x000000e4623faf50   r12 = 0x000000000000017b
    r13 = 0x000002b3d74eb6a0   r14 = 0x0000000000000001
    r15 = 0x00007ffc3bb8f6d0   rip = 0x00007ffc38395661
    Found by: call frame info
 3  xul.dll!geckoservo::glue::Servo_StyleSet_FlushStyleSheets [glue.rs:ae39864562c6 : 1025 + 0x177c]
    rbx = 0x00007ffc3bb9cd10   rbp = 0x000000e4623faec0
    rsp = 0x000000e4623fb5b0   r12 = 0x000000000000017b
    r13 = 0x000002b3d74eb6a0   r14 = 0x0000000000000001
    r15 = 0x00007ffc3bb8f6d0   rip = 0x00007ffc3838e3ac
    Found by: call frame info
 4  xul.dll!mozilla::ServoStyleSet::UpdateStylist() [ServoStyleSet.cpp:ae39864562c6 : 1396 + 0xc]
    rbx = 0x00007ffc3bb9cd10   rbp = 0x000000e4623faec0
    rsp = 0x000000e4623fdfd0   r12 = 0x000000000000017b
    r13 = 0x000002b3d74eb6a0   r14 = 0x0000000000000001
    r15 = 0x00007ffc3bb8f6d0   rip = 0x00007ffc39e7b749
    Found by: call frame info
 5  xul.dll!mozilla::ServoStyleSet::AppendFontFaceRules(nsTArray<nsFontFaceRuleContainer> &) [ServoStyleSet.cpp:ae39864562c6 : 1353 + 0x7]
    rbx = 0x00007ffc3bb9cd10   rbp = 0x000000e4623faec0
    rsp = 0x000000e4623fe000   r12 = 0x000000000000017b
    r13 = 0x000002b3d74eb6a0   r14 = 0x0000000000000001
    r15 = 0x00007ffc3bb8f6d0   rip = 0x00007ffc39e6bf73
    Found by: call frame info
 6  xul.dll!nsIDocument::FlushUserFontSet() [nsDocument.cpp:ae39864562c6 : 13366 + 0x25]
    rbx = 0x00007ffc3bb9cd10   rbp = 0x000000e4623faec0
    rsp = 0x000000e4623fe030   r12 = 0x000000000000017b
    r13 = 0x000002b3d74eb6a0   r14 = 0x0000000000000001
    r15 = 0x00007ffc3bb8f6d0   rip = 0x00007ffc3920240f
    Found by: call frame info
 7  xul.dll!mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [PresShell.cpp:ae39864562c6 : 4152 + 0x9]
    rbx = 0x00007ffc3bb9cd10   rbp = 0x000000e4623faec0
    rsp = 0x000000e4623fe070   r12 = 0x000000000000017b
    r13 = 0x000002b3d74eb6a0   r14 = 0x0000000000000001
    r15 = 0x00007ffc3bb8f6d0   rip = 0x00007ffc39f17590
    Found by: call frame info

Windows, Linux Nightly 57
Attached file Windows Opt Log
bp-a6d10f04-fbdf-464e-a88b-2de440170918
[@ mozalloc_abort | abort | style::stylist::CascadeData::add_stylesheet<T> ]
Crash Signature: [@ mozalloc_abort | abort | style::stylist::CascadeData::add_stylesheet<T> ]
Keywords: crash
Priority: -- → P2
Did a quick reproduction and saw that this is related to keyframes animation. Hiro or Brian will likely be fastest on fixing it.
Flags: needinfo?(hikezoe)
Also NI Boris. Whoever gets to it first.
Flags: needinfo?(boris.chiou)
This looks more like parsing issue rather than animating code. Also I don't think that assertion needs to be a release assertion, we should probably downgrade it to debug_assert.
We don't allow important flag in keyframes rule parser, but we *do* allow it by setting CSSOM unfortunately.
Flags: needinfo?(hikezoe)
I take this. Given that CSSOM directly modifies property declaration blocks in keyframes rules, we should filter our !important rule there.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(boris.chiou)
Hmm... it's probably worth a spec issue. Filed w3c/csswg-drafts#1824.
So I think Gecko is just ignoring it, and doesn't assert on this. Not sure which is better.

Allowing setting priority on declarations in @keyframes means that its serialization may not round-trip, which is conceptually a problem as well.

Probably we should allow !important in @keyframes but just ignore them...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> So I think Gecko is just ignoring it, and doesn't assert on this. Not sure
> which is better.

To be clear, Gecko ignores the important property in CSS animation handling, does not ignore it in setting keyframes rules.

E.g. 
When setProperty('background-clor', 'red', 'important') is called for a keyframe at 0%, it replaces the target keyframe, then we can see the keyframes in devtools something like this;

@keyframes anim {
  0% { background-color: red !important; }
100% { background-color: blue; }
}

Then, CSS animation handles the important value invalid.
Summary: thread '<unnamed>' panicked at 'assertion failed: !importance.important()', servo/components/style/stylesheets/keyframes_rule.rs:379 → stylo: thread '<unnamed>' panicked at 'assertion failed: !importance.important()', servo/components/style/stylesheets/keyframes_rule.rs:379
Until CSSWG decides on this I suggest changing CSSOM setProperty and setPropertyPriority to not set declarations in keyframes rules to important. (Either succeed while pretending the "priority" parameter was the empty string, or fail, I don’t have an opinion on that.)
Brian created an example that the important entry handles as invalid in setProperty(), and checked it for other browsers.

The example is: http://jsbin.com/suxabaxoce/edit?css,js,console,output

The result is that all browser store the important entry in keyframes, so stylo also should store it as it is, and handle it as invalid during generating keyframes.
Based on some cross-browser testing:

* EdgeHTML, WebKit, Gecko (non-stylo) all animate attachment 8909509 [details] from green to red (i.e. they treat the !important declaration as invalid)
* Only Blink fails to ignore it (tested with Chrome unstable on Linux and Chrome stable on Windows)

As for whether they simply drop the existing declaration or retain the invalid !important declaration, based on [1] it appears they all retain the (invalid) !important declaration.

As discussed with hiro, based on this, the safest thing here is probably just to match Gecko's existing behavior and retain the invalid !important declaration as-is, but ignore it when generating keyframes.

[1] http://jsbin.com/suxabaxoce/edit?css,js,console,output (Note: will crash Stylo builds!)
Oh dear, the example causes an assertion on Gecko too.

Assertion failure: !aKeyframeDeclaration->HasImportantData() (Keyframe rule has !important data), at /home/ikezoe/autoland/layout/style/nsAnimationManager.cpp:390
Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

https://reviewboard.mozilla.org/r/181174/#review186328

I think this is not perfect, but I'd like to get feedbacks about this approach.

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

::: servo/components/style/shared_lock.rs:208
(Diff revision 2)
> +impl Locked<PropertyDeclarationBlock> {
> +    /// Create a new Locked<PropertyDeclarationBlock> with the same SharedRwLock.
> +    pub fn new_with(&self, other: PropertyDeclarationBlock) -> Self {
> +        self.shared_lock.wrap(other)
> +    }
> +}

I did add this implementation to create a new Locked<PropertyDeclarationBlock> without important properties.  Should we pass SharedRwLock instead of stealing shared_lock from existent Locked<PropertyDeclarationBlock>?

Also, I did initially put this impl into declaration_block.rs, but it did not work since the private member, shared_lock can not be accessed there.

::: servo/components/style/stylesheets/keyframes_rule.rs:432
(Diff revision 2)
> +                    let block = keyframe.block.read_with(&guard);
> +                    block.clone_only_normal_importance()
> +                };
> +                Arc::new(keyframe.block.new_with(cloned))
> +            } else {
> +                keyframe.block.clone()

I'd actually want to avoid this clone() but I have no idea how to avoid it.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Oh dear, the example causes an assertion on Gecko too.
> 
> Assertion failure: !aKeyframeDeclaration->HasImportantData() (Keyframe rule
> has !important data), at
> /home/ikezoe/autoland/layout/style/nsAnimationManager.cpp:390

Filed bug 1401104 for Gecko.
With attachment 8909658 [details] Stylo does not cause any panic and assertions.
Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

https://reviewboard.mozilla.org/r/181174/#review186518

Yeah, I don't really love this patch.

Why do we want this instead of, let's say, something like, adding a `normal_declarations` iterator that filters out important properties, and using it in the 4 or so relevant places, mentioning the spec issue raised by xidorn?

Seems a bit less heavy-weight.

::: servo/components/style/properties/declaration_block.rs:217
(Diff revision 2)
> +    /// no important properties.
> +    pub fn clone_only_normal_importance(&self) -> PropertyDeclarationBlock {
> +        let mut longhands = self.longhands.clone();
> +        let mut declarations = self.declarations.clone();
> +        let mut i = 0;
> +        for (decl, importance) in self.declarations.iter().zip(self.declarations_importance.iter()) {

There's a `declaration_importance_iter` that does just this and is faster.
Attachment #8909658 - Flags: review?(emilio)
Duplicate of this bug: 1401173
Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

https://reviewboard.mozilla.org/r/181174/#review186798

::: servo/components/style/properties/declaration_block.rs:109
(Diff revision 3)
>          self.iter.size_hint()
>      }
>  }
>  
> +/// Iterator over `PropertyDeclaration` for Importance::Normal.
> +pub struct DeclarationNormalIterator<'a> {

nit: `NormalDeclarationIterator`.

::: servo/components/style/stylesheets/keyframes_rule.rs:430
(Diff revision 3)
>              return result;
>          }
>  
>          for keyframe in keyframes {
>              let keyframe = keyframe.read_with(&guard);
> +            let block = if keyframe.block.read_with(&guard).any_important() {

Why do we need to clone the property declaration block? Let's filter it out instead whenever we go over the properties instead (in `glue.rs`).
Attachment #8909658 - Flags: review?(emilio)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)

> ::: servo/components/style/stylesheets/keyframes_rule.rs:430
> (Diff revision 3)
> >              return result;
> >          }
> >  
> >          for keyframe in keyframes {
> >              let keyframe = keyframe.read_with(&guard);
> > +            let block = if keyframe.block.read_with(&guard).any_important() {
> 
> Why do we need to clone the property declaration block? Let's filter it out
> instead whenever we go over the properties instead (in `glue.rs`).

Because I'd like to fix this bug both for servo and stylo altogether.  OK, I am fine with fixing it in glue.rs.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> 
> > ::: servo/components/style/stylesheets/keyframes_rule.rs:430
> > (Diff revision 3)
> > >              return result;
> > >          }
> > >  
> > >          for keyframe in keyframes {
> > >              let keyframe = keyframe.read_with(&guard);
> > > +            let block = if keyframe.block.read_with(&guard).any_important() {
> > 
> > Why do we need to clone the property declaration block? Let's filter it out
> > instead whenever we go over the properties instead (in `glue.rs`).
> 
> Because I'd like to fix this bug both for servo and stylo altogether.  OK, I
> am fine with fixing it in glue.rs.

Isn't that just a matter of using the new iterator in `compute_style_for_animation_step`?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #24)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> > 
> > > ::: servo/components/style/stylesheets/keyframes_rule.rs:430
> > > (Diff revision 3)
> > > >              return result;
> > > >          }
> > > >  
> > > >          for keyframe in keyframes {
> > > >              let keyframe = keyframe.read_with(&guard);
> > > > +            let block = if keyframe.block.read_with(&guard).any_important() {
> > > 
> > > Why do we need to clone the property declaration block? Let's filter it out
> > > instead whenever we go over the properties instead (in `glue.rs`).
> > 
> > Because I'd like to fix this bug both for servo and stylo altogether.  OK, I
> > am fine with fixing it in glue.rs.
> 
> Isn't that just a matter of using the new iterator in
> `compute_style_for_animation_step`?

yeah, right. but we also need double ended iterator since we use rev() there (I don't know why rev() is necessary though).
Comment on attachment 8910066 [details]
Bug 1400926 - Filter out !important property in keyframes for servo.

https://reviewboard.mozilla.org/r/181542/#review186930

::: servo/components/style/animation.rs:491
(Diff revision 1)
> +                // so we have to filter them out.
> +                // See the spec issue https://github.com/w3c/csswg-drafts/issues/1824
> +                // Also we filter our non-animatable properties.
> +                guard.normal_declaration_iter()
> +                     .filter(|declaration| declaration.is_animatable())
> +                     .rev()

Doesn't need `rev()`, let's get rid of this.

::: servo/components/style/properties/declaration_block.rs:151
(Diff revision 1)
>      fn size_hint(&self) -> (usize, Option<usize>) {
>          self.iter.size_hint()
>      }
>  }
>  
> +impl<'a> DoubleEndedIterator for NormalDeclarationIterator<'a> {

Let's get rid of this too.
Attachment #8910066 - Flags: review?(emilio) → review+
Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

https://reviewboard.mozilla.org/r/181174/#review186934

::: layout/style/crashtests/1400926.html:2
(Diff revision 4)
> +<!DOCTYPE html>
> +<html>

nit: No need for `<html>`, specially if there isn't a closing tag.

::: servo/components/style/properties/declaration_block.rs:117
(Diff revision 4)
>      }
>  }
>  
> +/// Iterator over `PropertyDeclaration` for Importance::Normal.
> +pub struct NormalDeclarationIterator<'a> {
> +    iter: Zip<Iter<'a, PropertyDeclaration>, smallbitvec::Iter<'a>>,

I think this would still be nicer just wrapping `DeclarationImportanceIterator`, any reason that's not done?

Anyway, looks ok.
Attachment #8909658 - Flags: review?(emilio) → review+
Attached file Servo PR
Attachment #8910066 - Attachment is obsolete: true
https://hg.mozilla.org/integration/autoland/rev/89b1fac5342d

NI Hiro to land the testcase.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Flags: needinfo?(hikezoe)
Landed the test case.
Thank you, Emilio and Bobby!
Flags: needinfo?(hikezoe)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/26f3a378ff0e
Filter out !important property in keyframes for stylo. r=emilio
You need to log in before you can comment on or make changes to this bug.