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

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

Attachment: 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

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: Windows Opt Log

Comment 2

[Bob Clary [:bc] (inactive)]

bp-a6d10f04-fbdf-464e-a88b-2de440170918
[@ mozalloc_abort | abort | style::stylist::CascadeData::add_stylesheet<T> ]

Comment 3

[Brad Werth [:bradwerth]]

Did a quick reproduction and saw that this is related to keyframes animation. Hiro or Brian will likely be fastest on fixing it.

Comment 4

[Bobby Holley (:bholley)]

Also NI Boris. Whoever gets to it first.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Test case that causes the crash

We don't allow important flag in keyframes rule parser, but we *do* allow it by setting CSSOM unfortunately.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I take this. Given that CSSOM directly modifies property declaration blocks in keyframes rules, we should filter our !important rule there.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Hmm... it's probably worth a spec issue. Filed w3c/csswg-drafts#1824.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

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...

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 11

[Simon Sapin (:SimonSapin)]

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.)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 13

[Brian Birtles (:birtles)]

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!)

Comment 14

[Hiroyuki Ikezoe (:hiro)]

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 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1400926 - Filter out !important property in keyframes for stylo.

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

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

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

Comment 17

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(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 19

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

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

Comment 22

[Emilio Cobos Álvarez (:emilio)]

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`).

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

(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`?

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1400926 - Filter out !important property in keyframes for servo.

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

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181174/diff/3-4/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

(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 28

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181174/diff/4-5/

Comment 32

[Hiroyuki Ikezoe (:hiro)]

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

Comment 33

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/autoland/rev/89b1fac5342d

NI Hiro to land the testcase.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Landed the test case.
Thank you, Emilio and Bobby!

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26f3a378ff0e
Filter out !important property in keyframes for stylo. r=emilio

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/26f3a378ff0e