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

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

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.

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

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.

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

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

(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

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.

OK, I will care of the tests.

There must be still unexpected-pass reftests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8351c342eb0da05a797d04ac4bc21dd7cc9949f8

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 on attachment 8868005 [details] Bug 1363880 - Enable crashtest for SVG that no longer cause panic. https://reviewboard.mozilla.org/r/139568/#review142902

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

Thank you for doing this!

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

Pushed by hikezoe@mozilla.com: 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

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