1363880 - stylo: panicked at 'Animation restyle hints should not appear with non-animation restyle hints'

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

When I run layout/reftests/svg/smil/style/anim-css-color-1-to-ident-hex.svg, I got below assertion:

 panicked at 'Animation restyle hints should not appear with non-animation restyle hints', /home/ikezoe/central/servo/ports/geckolib/glue.rs:1867

Unfortunately we still post eRestyle_StyleAttribute and eRestyle_StyleAttribute_Animations in Element::SetSMILOverrideStyleDeclaration [1].

A call stack:

#6  0x00007fffe89006c9 in std::panicking::begin_panic<&str> (msg=..., file_line=0x7fffed361a80 <geckoservo::glue::Servo_NoteExplicitHints::_FILE_LINE::h5ea713bf17d76bd0>)
    at /checkout/src/libstd/panicking.rs:511
#7  0x00007fffe89b254a in geckoservo::glue::Servo_NoteExplicitHints (element=0x7fffbcff1040, restyle_hint=..., change_hint=...) at /home/ikezoe/stylo/servo/ports/geckolib/glue.rs:1859
#8  0x00007fffe5517fe1 in mozilla::ServoRestyleManager::PostRestyleEvent (this=0x7fffb9f40b20, aElement=0x7fffbcff1040, aRestyleHint=192, aMinChangeHint=nsChangeHint_Empty)
    at /home/ikezoe/stylo/layout/base/ServoRestyleManager.cpp:62
#9  0x00007fffe5473ff9 in mozilla::RestyleManager::PostRestyleEvent (this=0x7fffb9f40b20, aElement=0x7fffbcff1040, aRestyleHint=192, aMinChangeHint=nsChangeHint_Empty)
    at /home/ikezoe/stylo/obj-firefox/dist/include/mozilla/RestyleManagerInlines.h:24
#10 0x00007fffe54f235c in nsIPresShell::RestyleForAnimation (this=0x7fffb5390800, aElement=0x7fffbcff1040, aHint=192) at /home/ikezoe/stylo/layout/base/PresShell.cpp:2975
#11 0x00007fffe2fd2c90 in mozilla::dom::Element::SetSMILOverrideStyleDeclaration (this=0x7fffbcff1040, aDeclaration=0x7fffbaa8dbc0, aNotify=true)
    at /home/ikezoe/stylo/dom/base/Element.cpp:1965
#12 0x00007fffe53edd6b in nsDOMCSSAttributeDeclaration::SetCSSDeclaration (this=0x7fffb9f36540, aDecl=0x7fffbaa8dbc0) at /home/ikezoe/stylo/layout/style/nsDOMCSSAttrDeclaration.cpp:78
#13 0x00007fffe53f1624 in nsDOMCSSDeclaration::ModifyDeclaration<nsDOMCSSDeclaration::ParsePropertyValue(nsCSSPropertyID, const nsAString&, bool)::<lambda(mozilla::css::Declaration*, nsDOMCSSDeclaration::CSSParsingEnvironment&, bool*)>, nsDOMCSSDeclaration::ParsePropertyValue(nsCSSPropertyID, const nsAString&, bool)::<lambda(mozilla::ServoDeclarationBlock*, mozilla::URLExtraData*)> >(nsDOMCSSDeclaration::<lambda(mozilla::css::Declaration*, nsDOMCSSDeclaration::CSSParsingEnvironment&, bool*)>, nsDOMCSSDeclaration::<lambda(mozilla::ServoDeclarationBlock*, mozilla::URLExtraData*)>) (this=0x7fffb9f36540, aGeckoFunc=..., aServoFunc=...) at /home/ikezoe/stylo/layout/style/nsDOMCSSDeclaration.cpp:337
#14 0x00007fffe53ef1bc in nsDOMCSSDeclaration::ParsePropertyValue (this=0x7fffb9f36540, aPropID=eCSSProperty_color, aPropValue=..., aIsImportant=false)
    at /home/ikezoe/stylo/layout/style/nsDOMCSSDeclaration.cpp:356
#15 0x00007fffe53ee660 in nsDOMCSSDeclaration::SetPropertyValue (this=0x7fffb9f36540, aPropID=eCSSProperty_color, aValue=...)
    at /home/ikezoe/stylo/layout/style/nsDOMCSSDeclaration.cpp:92
#16 0x00007fffe53ee352 in nsDOMCSSAttributeDeclaration::SetPropertyValue (this=0x7fffb9f36540, aPropID=eCSSProperty_color, aValue=...)
    at /home/ikezoe/stylo/layout/style/nsDOMCSSAttrDeclaration.cpp:219
#17 0x00007fffe4e09f26 in nsSMILCSSProperty::SetAnimValue (this=0x7fffbaa8db60, aValue=...) at /home/ikezoe/stylo/dom/smil/nsSMILCSSProperty.cpp:131
#18 0x00007fffe4e0c5ee in nsSMILCompositor::ComposeAttribute (this=0x7fffb7c18cc0, aMightHavePendingStyleUpdates=@0x7fffffff9950: true)
    at /home/ikezoe/stylo/dom/smil/nsSMILCompositor.cpp:116
#19 0x00007fffe4e05963 in nsSMILAnimationController::DoSample (this=0x7fffbd4b2880, aSkipUnchangedContainers=false) at /home/ikezoe/stylo/dom/smil/nsSMILAnimationController.cpp:455
#20 0x00007fffe4ac61d3 in nsSMILAnimationController::Resample (this=0x7fffbd4b2880) at /home/ikezoe/stylo/dom/smil/nsSMILAnimationController.h:74
#21 0x00007fffe4ac624a in nsSMILAnimationController::FlushResampleRequests (this=0x7fffbd4b2880) at /home/ikezoe/stylo/dom/smil/nsSMILAnimationController.h:90
#22 0x00007fffe4ab90fa in nsSVGElement::FlushAnimations (this=0x7fffb7cf76a0) at /home/ikezoe/stylo/dom/svg/nsSVGElement.cpp:2705
#23 0x00007fffe4a91766 in mozilla::dom::SVGSVGElement::SetCurrentTime (this=0x7fffb7cf76a0, seconds=3) at /home/ikezoe/stylo/dom/svg/SVGSVGElement.cpp:373
#24 0x00007fffe377a7a3 in mozilla::dom::SVGSVGElementBinding::setCurrentTime (cx=0x7fffd8606000, obj=..., self=0x7fffb7cf76a0, args=...)
    at /home/ikezoe/stylo/obj-firefox/dom/bindings/SVGSVGElementBinding.cpp:64


[1] http://searchfox.org/mozilla-central/source/dom/base/Element.cpp#1964

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I did a try to confirm whether there are test cases that rely on eRestyle_StyleAttribute in Element::SetSMILOverrideStyleDeclaration (just dropped eRestyle_StyleAttribute there). 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d9201bddb86fae84d7c03803ae261983e4b904e
(A failed reftest (R13 on stylo) is a test case that is hit by this panic, I enabled it to check the panic does not happen anymore)

As per this result, there is no test cases relying on the eRestyle_StyleAttribute hint.
Also, as per the stack trace in comment 0, we call FlushPendingNotifications() in nsSMILAnimationController::DoSample() [1] before we reach to Element::SetSMILOverrideStyleDeclaration(). I did confirm that isStyleFlushNeeded is true on gdb.

[1] https://hg.mozilla.org/mozilla-central/file/3e166b683893/dom/smil/nsSMILAnimationController.cpp#l439

Also, RestyleForAnimation() in Element::SetSMILOverrideStyleDeclaration() is only called when nsDOMCSSAttributeDeclaration::mIsSMILOverride is true.  And the mIsSMILOverride is set when we call nsSMILCSSProperty::SetAnimValue() or nsSMILCSSProperty::ClearAnimValue(). 
  nsSMILCSSProperty::SetAnimValue() is only called in nsSMILCompositor::ComposeAttribute, it's after the FlushPendingNotifications call in nsSMILAnimationController::DoSample.
  nsSMILCSSProperty::ClearAnimValue is called from three places.  Two of them are also in nsSMILCompositor::ComposeAttribute. The final one is in nsSMILCompositor::ClearAnimationEffects, it's called from nsSMILAnimationController::DoSample() [2] but just right befor the FlushPendingNotifications call.

So, a question is that there needs to post eRestyle_StyleAttribute hint when we clear SMIL animation values? Are there any cases that we can't correctly clear SMIL animated styles without eRestyle_StyleAttribute hint?

From comment [3] in Element::SetSMILOverrideStyleDeclaration:

        // Pass both eRestyle_StyleAttribute and                                
        // eRestyle_StyleAttribute_Animations since we don't know if            
        // this style represents only the ticking of an existing                
        // animation or whether it's a new or changed animation.

I understand there are certainly cases that we need eRestyle_StyleAttribute for *new* animation. But for removing animations?

Brian, what do you think?

[2] https://hg.mozilla.org/mozilla-central/file/3e166b683893/dom/smil/nsSMILAnimationController.cpp#l429
[3] https://hg.mozilla.org/mozilla-central/file/3e166b683893/dom/base/Element.cpp#l2008

Comment 2

[Brian Birtles (:birtles)]

I'll have a proper look at this later this afternoon, but I suspect the following patch will be of interest:

  https://reviewboard.mozilla.org/r/133736/diff/1

That is, it's possible that restyle hint is in error and we should have removed it in bug 1355348 as part of the above patch but I didn't notice it then.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Oh nice, I did not notice the patch.   I wait for the good news!

Comment 4

[Brian Birtles (:birtles)]

Oh! I didn't realize I failed to land that patch! I guess it must have happened when I split patches for landing on Servo? Or maybe MozReview ignored it? Oh dear. I'll land it later this afternoon.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Wow! The patch has been already stamped r+ by dbaron (bug 1355348 comment 30).

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Oh! I didn't realize I failed to land that patch! I guess it must have
> happened when I split patches for landing on Servo? Or maybe MozReview
> ignored it? Oh dear. I'll land it later this afternoon.

Note that there are a bunch of test cases that had skipped because of this panic. We should enable those skipped test as well.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > Oh! I didn't realize I failed to land that patch! I guess it must have
> > happened when I split patches for landing on Servo? Or maybe MozReview
> > ignored it? Oh dear. I'll land it later this afternoon.
> 
> Note that there are a bunch of test cases that had skipped because of this
> panic. We should enable those skipped test as well.

Here it is.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ab29a509fe026c91b2f567881d6fdc8086f388d

Comment 8

[Brian Birtles (:birtles)]

Thanks. I'm just going to land that patch as-is in bug 1355348. We can re-enable the test cases in this bug or another bug.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

OK, I will care of the tests.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

There must be still unexpected-pass reftests.

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

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1363880 - Don't skip reftests that don't fail anymore on stylo.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1363880 - Enable crashtest for SVG that no longer cause panic.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1363880 - Annotate fails-if instead of skip-if for reftests that no longer cause panic but still fail.

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

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8868004 [details]
Bug 1363880 - Don't skip reftests that don't fail anymore on stylo.

https://reviewboard.mozilla.org/r/139566/#review142900

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8868005 [details]
Bug 1363880 - Enable crashtest for SVG that no longer cause panic.

https://reviewboard.mozilla.org/r/139568/#review142902

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8868006 [details]
Bug 1363880 - Annotate fails-if instead of skip-if for reftests that no longer cause panic but still fail.

https://reviewboard.mozilla.org/r/139570/#review142904

Comment 17

[Brian Birtles (:birtles)]

Thank you for doing this!

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Thanks for the review! I will land these patch after this commit https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8900bc3ec0 gets merged into autoland.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868004 [details]
Bug 1363880 - Don't skip reftests that don't fail anymore on stylo.

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

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868005 [details]
Bug 1363880 - Enable crashtest for SVG that no longer cause panic.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868006 [details]
Bug 1363880 - Annotate fails-if instead of skip-if for reftests that no longer cause panic but still fail.

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868006 [details]
Bug 1363880 - Annotate fails-if instead of skip-if for reftests that no longer cause panic but still fail.

https://reviewboard.mozilla.org/r/139570/#review143236

::: layout/reftests/svg/reftest.list:140
(Diff revision 1)
>  == dynamic-inner-svg-01.svg pass.svg
>  == dynamic-link-style-01.svg pass.svg
>  == dynamic-marker-01.svg pass.svg
>  == dynamic-marker-02.svg dynamic-marker-02-ref.svg
>  == dynamic-marker-03.svg pass.svg
> -skip-if(stylo) == dynamic-mask-01.svg pass.svg
> +fails-if(stylo) == dynamic-mask-01.svg pass.svg

dynamic-mask-01.svg and anim-view-01.svg#view failed on a try even if we annotate as 'fails-if'. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b27e37b151b54c6237a7f6bdb813bee479ea279&selectedJob=99527829

I will leave them as skip-if for now.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3b6085985f3
Don't skip reftests that don't fail anymore on stylo. r=birtles
https://hg.mozilla.org/integration/autoland/rev/db0aa9f5cf22
Enable crashtest for SVG that no longer cause panic. r=birtles
https://hg.mozilla.org/integration/autoland/rev/437489a51017
Annotate fails-if instead of skip-if for reftests that no longer cause panic but still fail. r=birtles

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3b6085985f3
https://hg.mozilla.org/mozilla-central/rev/db0aa9f5cf22
https://hg.mozilla.org/mozilla-central/rev/437489a51017