1323114 - Assertion failure: TransformFunctionsMatch(nsStyleTransformMatrix::TransformFunctionOf(a1), nsStyleTransformMatrix::TransformFunctionOf(a2)) (transform function mismatch)

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

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

Assertion failure: TransformFunctionsMatch(nsStyleTransformMatrix::TransformFunctionOf(a1), nsStyleTransformMatrix::TransformFunctionOf(a2)) (transform function mismatch), at src/layout/style/StyleAnimationValue.cpp:2482

    #0 0x7f7bd1f5f6b3 in AddTransformLists(double, nsCSSValueList const*, double, nsCSSValueList const*, nsCSSKeyword) src/layout/style/StyleAnimationValue.cpp:2641:9
    #1 0x7f7bd1f5c90b in mozilla::StyleAnimationValue::AddWeighted(nsCSSPropertyID, double, mozilla::StyleAnimationValue const&, double, mozilla::StyleAnimationValue const&, mozilla::StyleAnimationValue&) src/layout/style/StyleAnimationValue.cpp:3143:20
    #2 0x7f7bcebed5cb in mozilla::dom::KeyframeEffectReadOnly::ComposeStyle(RefPtr<mozilla::AnimValuesStyleRule>&, nsCSSPropertyIDSet const&) src/dom/animation/KeyframeEffectReadOnly.cpp:493:9
    #3 0x7f7bcebeca6f in mozilla::dom::Animation::ComposeStyle(RefPtr<mozilla::AnimValuesStyleRule>&, nsCSSPropertyIDSet const&) src/dom/animation/Animation.cpp:960:7
    #4 0x7f7bcebfd875 in mozilla::EffectCompositor::ComposeAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, mozilla::TimeStamp) src/dom/animation/EffectCompositor.cpp:700:5
    #5 0x7f7bcebfd4eb in mozilla::EffectCompositor::MaybeUpdateAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, nsStyleContext*) src/dom/animation/EffectCompositor.cpp:370:3
    #6 0x7f7bcebfdbdd in mozilla::EffectCompositor::GetAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, nsStyleContext*) src/dom/animation/EffectCompositor.cpp:406:3
    #7 0x7f7bd20c045e in nsStyleSet::RuleNodeWithReplacement(mozilla::dom::Element*, mozilla::dom::Element*, nsRuleNode*, mozilla::CSSPseudoElementType, nsRestyleHint) src/layout/style/nsStyleSet.cpp:1566:34
    #8 0x7f7bd20c0b8f in nsStyleSet::ResolveStyleWithReplacement(mozilla::dom::Element*, mozilla::dom::Element*, nsStyleContext*, nsStyleContext*, nsRestyleHint, unsigned int) src/layout/style/nsStyleSet.cpp:1676:5
    #9 0x7f7bd21b8c7b in mozilla::ElementRestyler::RestyleSelf(nsIFrame*, nsRestyleHint, unsigned int*, nsTArray<mozilla::ElementRestyler::SwapInstruction>&) src/layout/base/RestyleManager.cpp:2816:11
    #10 0x7f7bd21b63af in mozilla::ElementRestyler::Restyle(nsRestyleHint) src/layout/base/RestyleManager.cpp:2139:7
    #11 0x7f7bd21c1768 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
    #12 0x7f7bd21ab189 in mozilla::RestyleManager::ComputeAndProcessStyleChange(nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) src/layout/base/RestyleManager.cpp:3803:3
    #13 0x7f7bd21aa61c in mozilla::RestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) src/layout/base/RestyleManager.cpp:152:5
    #14 0x7f7bd21c9bba in mozilla::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint, mozilla::RestyleHintData const&) src/layout/base/RestyleTracker.cpp:97:5
    #15 0x7f7bd21c7bed in mozilla::RestyleTracker::DoProcessRestyles() src/layout/base/RestyleTracker.cpp:266:9
    #16 0x7f7bd21ae1ba in mozilla::RestyleManager::ProcessPendingRestyles() src/layout/base/RestyleManager.cpp:834:3
    #17 0x7f7bd218170b in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4115:9

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Thanks, Jesse!  This is a case of interpolation between 'transform: none' and 'eCSSKeyword_accumulatematrix'.  We should handle this case as interpolation of different transform list.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cb4d0789b378dbd29f201f43236ae661a748ab2

Comment 3

[Hiroyuki Ikezoe (:hiro)]

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1323114 - Handle interpolation between 'none' and 'accumulate matrix' as different transform list.

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

Comment 5

[Boris Chiou [:boris]]

Comment on attachment 8818193 [details]
Bug 1323114 - Handle interpolation between 'none' and 'accumulate matrix' as different transform list.

https://reviewboard.mozilla.org/r/98320/#review100074

::: layout/style/StyleAnimationValue.cpp:3165
(Diff revision 1)
> -        if (list2->mValue.GetUnit() == eCSSUnit_None) {
> +        if (list2->mValue.GetUnit() == eCSSUnit_None &&
> +            !HasAccumulateMatrix(list1)) {
>            result = AddTransformLists(0, list1, aCoeff1, list1);
>          } else if (TransformFunctionListsMatch(list1, list2)) {
>            result = AddTransformLists(aCoeff1, list1, aCoeff2, list2,
>                                       eCSSKeyword_interpolatematrix);
>          } else {
>            result = AddDifferentTransformLists(aCoeff1, list1,
>                                                aCoeff2, list2,
>                                                eCSSKeyword_interpolatematrix);

If list1 is accumulateMatrix and list2 is none, we call

`AddDifferentTransformLists(aCoeff1, list1, aCoeff2, list2, ...);`

instead of

`AddDifferentTransformLists(0, list1, aCoeff1, list1, ...);`

Therefore, I think we need to add one more crashtest for this case.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8818193 [details]
Bug 1323114 - Handle interpolation between 'none' and 'accumulate matrix' as different transform list.

https://reviewboard.mozilla.org/r/98320/#review100074

> If list1 is accumulateMatrix and list2 is none, we call
> 
> `AddDifferentTransformLists(aCoeff1, list1, aCoeff2, list2, ...);`
> 
> instead of
> 
> `AddDifferentTransformLists(0, list1, aCoeff1, list1, ...);`
> 
> Therefore, I think we need to add one more crashtest for this case.

I thought initially we can't such test case (with iterationComposite:accumulate), but yes, we can write a test case with *composite:accumulate*!  Thanks! I will add the the case.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Comment on attachment 8818193 [details]
> Bug 1323114 - Handle interpolation between 'none' and 'accumulate matrix' as
> different transform list.
> 
> https://reviewboard.mozilla.org/r/98320/#review100074
> 
> > If list1 is accumulateMatrix and list2 is none, we call
> > 
> > `AddDifferentTransformLists(aCoeff1, list1, aCoeff2, list2, ...);`
> > 
> > instead of
> > 
> > `AddDifferentTransformLists(0, list1, aCoeff1, list1, ...);`
> > 
> > Therefore, I think we need to add one more crashtest for this case.
> 
> I thought initially we can't such test case (with
> iterationComposite:accumulate), but yes, we can write a test case with
> *composite:accumulate*!  Thanks! I will add the the case.

And, as you may already notice, the fix was incorrect too.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8818193 [details]
Bug 1323114 - Handle interpolation between 'none' and 'accumulate matrix' as different transform list.

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

Comment 9

[Boris Chiou [:boris]]

Comment on attachment 8818193 [details]
Bug 1323114 - Handle interpolation between 'none' and 'accumulate matrix' as different transform list.

https://reviewboard.mozilla.org/r/98320/#review100100

LGTM, thanks.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8818193 [details]
Bug 1323114 - Handle interpolation between 'none' and 'accumulate matrix' as different transform list.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/629564813616
Handle interpolation between 'none' and 'accumulate matrix' as different transform list. r=boris

Comment 12

[Phil Ringnalda (:philor)]

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