1323119 - Assertion failure: false (GetDiscretelyAnimatedCSSValue is not implemented yet)

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes the assertion in mozilla-central rev f46f85dcfbc2

Assertion failure: false (GetDiscretelyAnimatedCSSValue is not implemented yet), at src/layout/style/nsHTMLStyleSheet.cpp:119

    #0 0x7fbd80db9d91 in nsHTMLStyleSheet::TableTHRule::GetDiscretelyAnimatedCSSValue(nsCSSPropertyID, nsCSSValue*) src/layout/style/nsHTMLStyleSheet.cpp:119:3
    #1 0x7fbd80dfba0e in nsRuleNode::GetDiscretelyAnimatedCSSValue(nsCSSPropertyID, nsCSSValue*) src/layout/style/nsRuleNode.cpp:10401:9
    #2 0x7fbd80cc0681 in mozilla::StyleAnimationValue::ExtractComputedValue(nsCSSPropertyID, nsStyleContext*, mozilla::StyleAnimationValue&) src/layout/style/StyleAnimationValue.cpp:4708:7
    #3 0x7fbd7d958a6d in mozilla::EffectCompositor::GetBaseStyle(nsCSSPropertyID, nsStyleContext*, mozilla::dom::Element&, mozilla::CSSPseudoElementType) src/dom/animation/EffectCompositor.cpp:917:5
    #4 0x7fbd7d95fecd in mozilla::dom::KeyframeEffectReadOnly::CompositeValue(nsCSSPropertyID, RefPtr<mozilla::AnimValuesStyleRule> const&, mozilla::StyleAnimationValue const&, mozilla::dom::CompositeOperation) src/dom/animation/KeyframeEffectReadOnly.cpp:375:14
    #5 0x7fbd7d94528b in mozilla::dom::KeyframeEffectReadOnly::ComposeStyle(RefPtr<mozilla::AnimValuesStyleRule>&, nsCSSPropertyIDSet const&) src/dom/animation/KeyframeEffectReadOnly.cpp:445:7
    #6 0x7fbd7d944a6f in mozilla::dom::Animation::ComposeStyle(RefPtr<mozilla::AnimValuesStyleRule>&, nsCSSPropertyIDSet const&) src/dom/animation/Animation.cpp:960:7
    #7 0x7fbd7d955875 in mozilla::EffectCompositor::ComposeAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, mozilla::TimeStamp) src/dom/animation/EffectCompositor.cpp:700:5
    #8 0x7fbd7d9554eb in mozilla::EffectCompositor::MaybeUpdateAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, nsStyleContext*) src/dom/animation/EffectCompositor.cpp:370:3
    #9 0x7fbd7d955bdd in mozilla::EffectCompositor::GetAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, nsStyleContext*) src/dom/animation/EffectCompositor.cpp:406:3
    #10 0x7fbd80e1845e in nsStyleSet::RuleNodeWithReplacement(mozilla::dom::Element*, mozilla::dom::Element*, nsRuleNode*, mozilla::CSSPseudoElementType, nsRestyleHint) src/layout/style/nsStyleSet.cpp:1566:34
    #11 0x7fbd80e18b8f in nsStyleSet::ResolveStyleWithReplacement(mozilla::dom::Element*, mozilla::dom::Element*, nsStyleContext*, nsStyleContext*, nsRestyleHint, unsigned int) src/layout/style/nsStyleSet.cpp:1676:5
    #12 0x7fbd80f10c7b in mozilla::ElementRestyler::RestyleSelf(nsIFrame*, nsRestyleHint, unsigned int*, nsTArray<mozilla::ElementRestyler::SwapInstruction>&) src/layout/base/RestyleManager.cpp:2816:11
    #13 0x7fbd80f0e3af in mozilla::ElementRestyler::Restyle(nsRestyleHint) src/layout/base/RestyleManager.cpp:2139:7
    #14 0x7fbd80f19768 in mozilla::ElementRestyler::ComputeStyleChangeFor(nsIFrame*, nsStyleChangeList*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&, nsTArray<mozilla::ElementRestyler::ContextToClear>&, nsTArray<RefPtr<nsStyleContext> >&) src/layout/base/RestyleManager.cpp:3393:7
    #15 0x7fbd80f03189 in mozilla::RestyleManager::ComputeAndProcessStyleChange(nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) src/layout/base/RestyleManager.cpp:3803:3
    #16 0x7fbd80f0261c in mozilla::RestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) src/layout/base/RestyleManager.cpp:152:5
    #17 0x7fbd80f21bba in mozilla::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint, mozilla::RestyleHintData const&) src/layout/base/RestyleTracker.cpp:97:5
    #18 0x7fbd80f1fbed in mozilla::RestyleTracker::DoProcessRestyles() src/layout/base/RestyleTracker.cpp:266:9
    #19 0x7fbd80f061ba in mozilla::RestyleManager::ProcessPendingRestyles() src/layout/base/RestyleManager.cpp:834:3
    #20 0x7fbd80ed970b in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4115:9

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I think we should implement TableTHRule::GetDiscretelyAnimatedCSSValue and other GetDiscretelyAnimatedCSSValue to get the base style.  I guess we need to walk up tree rules to find the style.  I wondered how CSS transition handle these cases, but currently we do ignore discrete properties for transitions.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I think we should implement TableTHRule::GetDiscretelyAnimatedCSSValue and
> other GetDiscretelyAnimatedCSSValue to get the base style.  I guess we need
> to walk up tree rules to find the style. 

I'd like to fix this property in stylo, since we need to use each member variable which represents each CSS value respectively, e.g. mMaskType in nsStyleSVGReset, it will be some amount of work.

Comment 3

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Additional testcase which triggers the assertion "GetDiscretelyAnimatedCSSValue is not implemented yet".

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Unsurprisingly it sounds like (given comment 2), this doesn't reproduce when Stylo is enabled.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=447dfaa5ce21b66b6eb6494bcd93143786364bb2

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1323119 - Add a test case that had caused an assertion on the old style system.

Note that on the new style system (a.k.a stylo) the test case hasn't caused
any assertions.

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

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8966090 [details]
Bug 1323119 - Add a test case that had caused an assertion on the old style system.

https://reviewboard.mozilla.org/r/234832/#review240442

::: dom/animation/test/crashtests/1323119-1.html:6
(Diff revision 1)
> +<!DOCTYPE html>
> +<html class="reftest-wait">
> +<body>
> +<script>
> +addEventListener("DOMContentLoaded", function (){
> +  let a=document.createElement("th");

I wonder if we don't need the coding style in here?
(Although there were similar codes in crashtests.)
If we need, please add one space both side of "=".

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8966090 [details]
Bug 1323119 - Add a test case that had caused an assertion on the old style system.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1942e1742f2
Add a test case that had caused an assertion on the old style system. r=daisuke

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c1942e1742f2