Closed Bug 1324554 Opened 3 years ago Closed 3 years ago

Assertion failure: !animationProperty

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- unaffected
firefox53 --- verified
firefox54 --- verified

People

(Reporter: truber, Assigned: boris)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(8 files)

Attached file testcase.html
The attached testcase causes the following assertion in mozilla-central rev d4b3146a5567

Assertion failure: !animationProperty, at /home/worker/workspace/build/src/dom/animation/KeyframeUtils.cpp:1366
    #0 0x7f1f8c1b8d54 in mozilla::BuildSegmentsFromValueEntries(nsTArray<mozilla::KeyframeValueEntry>&, nsTArray<mozilla::AnimationProperty>&) /home/worker/workspace/build/src/dom/animation/KeyframeUtils.cpp:1331:7
    #1 0x7f1f8c1b7abf in mozilla::KeyframeUtils::GetAnimationPropertiesFromKeyframes(nsTArray<mozilla::Keyframe> const&, nsTArray<nsTArray<mozilla::PropertyStyleAnimationValuePair> > const&, mozilla::dom::CompositeOperation, nsStyleContext*) /home/worker/workspace/build/src/dom/animation/KeyframeUtils.cpp:695:3
    #2 0x7f1f8c197fdb in mozilla::dom::KeyframeEffectReadOnly::BuildProperties(nsStyleContext*) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:740:5
    #3 0x7f1f8c18df0e in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties(nsStyleContext*) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:276:44
    #4 0x7f1f8c1979d7 in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(nsTArray<mozilla::Keyframe>&&, nsStyleContext*) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:207:5
    #5 0x7f1f8c1978ed in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(JSContext*, JS::Handle<JSObject*>, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:184:3
    #6 0x7f1f8c1942c8 in already_AddRefed<mozilla::dom::KeyframeEffect> mozilla::dom::KeyframeEffectReadOnly::ConstructKeyframeEffect<mozilla::dom::KeyframeEffect, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions>(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:664:3
    #7 0x7f1f8c193f6d in mozilla::dom::KeyframeEffect::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/animation/KeyframeEffect.cpp:65:10
    #8 0x7f1f8c3687f0 in mozilla::dom::Element::Animate(mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Element.cpp:3405:5
    #9 0x7f1f8c368190 in mozilla::dom::Element::Animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Element.cpp:3362:10
    #10 0x7f1f8d763d65 in mozilla::dom::ElementBinding::animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:3327:55
Attached file log.txt
CCing Boris, since the example includes 'spacing'. It might be related to my 'missing keyframe' work though.
Blocks: 1334591
Boris, is this perhaps because we're using a shorthand property as the paced property?
Flags: needinfo?(boris.chiou)
Priority: -- → P1
Let me check this later. (keep the ni.)(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #3)
> Boris, is this perhaps because we're using a shorthand property as the paced
> property?

Not sure, but I can check this later. (keep the ni.)
Assignee: nobody → boris.chiou
Flags: needinfo?(boris.chiou)
Status: NEW → ASSIGNED
I think this bug is not related to spacing and shorthand property. Let see my log:

* spacing: "paced(flex)" (in testcase.html):
KeyFrame [0]: (offset: 0.000000): [ flex-grow(0), flex-shrink(0), flex-basis(auto) ]
KeyFrame [1]: (offset: 0.666667): [ flex-grow(0), flex-shrink(1), flex-basis(auto) ]
KeyFrame [2]: (offset: 0.666667): [ flex-grow(0), flex-shrink(1), flex-basis(0%) ]    // distance between [1] and [2] is 0.0
KeyFrame [3]: (offset: 1.000000): []   // not paceable

* spacing: "distribute", others are not changed:
KeyFrame [0]: (offset: 0.000000): [ flex-grow(0), flex-shrink(0), flex-basis(auto) ]
KeyFrame [1]: (offset: 0.333333): [ flex-grow(0), flex-shrink(1), flex-basis(auto) ]
KeyFrame [2]: (offset: 0.666667): [ flex-grow(0), flex-shrink(1), flex-basis(0%) ]
KeyFrame [3]: (offset: 1.000000): []

This happened when the offsets of Keyframe[1] and Keyframe[2] have the same value, so I can make some revisions on the test:
  div.animate([ { flexBasis: "auto", marginLeft: "0px" },
                { flexBasis: "auto", marginLeft: "10px", offset: "0.5"},
                { flexBasis: "0.0", marginLeft: "10px", offset: "0.5"},
                {} ]);

which means we have zero segment and an empty final keyframe together, and the assertion still happened.

Let's see the sorted nsTArray<KeyframeValueEntry> in BuildSegmentsFromValueEntries():
  entry[0]: (0.000000) flex-basis(auto)
  entry[1]: (0.500000) flex-basis(auto)
  entry[2]: (0.500000) flex-basis(0px)     // its next entry is an empty final keyframe, but we didn't find it.
  entry[3]: (0.000000) margin-left(0px)
  entry[4]: (0.500000) margin-left(10px)
  entry[5]: (0.500000) margin-left(10px)

While checking entry[1], we incorrectly check if it has an empty final keyframe because entry[2] has the same property and its offset is not 1.0 [ref-1], so we still build the animation segment between entry[2] and entry[3] [ref-2], which is not correct, I think. Therefore, we don't clear animationProperty for the next iteration, so the assertion happened. Maybe we should check the possible zero segment in [ref-1].

[ref-1] http://searchfox.org/mozilla-central/rev/f31f538622d0dfb2d869d4babc4951f6001e9be2/dom/animation/KeyframeUtils.cpp#1333-1341
[ref-2] http://searchfox.org/mozilla-central/rev/f31f538622d0dfb2d869d4babc4951f6001e9be2/dom/animation/KeyframeUtils.cpp#1372-1376
(In reply to Boris Chiou [:boris] (away 1/27 ~ 2/1) from comment #5)
> I think this bug is not related to spacing and shorthand property. Let see
> my log:
> 
> * spacing: "paced(flex)" (in testcase.html):
> KeyFrame [0]: (offset: 0.000000): [ flex-grow(0), flex-shrink(0),
> flex-basis(auto) ]
> KeyFrame [1]: (offset: 0.666667): [ flex-grow(0), flex-shrink(1),
> flex-basis(auto) ]
> KeyFrame [2]: (offset: 0.666667): [ flex-grow(0), flex-shrink(1),
> flex-basis(0%) ]    // distance between [1] and [2] is 0.0
> KeyFrame [3]: (offset: 1.000000): []   // not paceable
> 
> * spacing: "distribute", others are not changed:
> KeyFrame [0]: (offset: 0.000000): [ flex-grow(0), flex-shrink(0),
> flex-basis(auto) ]
> KeyFrame [1]: (offset: 0.333333): [ flex-grow(0), flex-shrink(1),
> flex-basis(auto) ]
> KeyFrame [2]: (offset: 0.666667): [ flex-grow(0), flex-shrink(1),
> flex-basis(0%) ]
> KeyFrame [3]: (offset: 1.000000): []
> 
> This happened when the offsets of Keyframe[1] and Keyframe[2] have the same
> value, so I can make some revisions on the test:
>   div.animate([ { flexBasis: "auto", marginLeft: "0px" },
>                 { flexBasis: "auto", marginLeft: "10px", offset: "0.5"},
>                 { flexBasis: "0.0", marginLeft: "10px", offset: "0.5"},
>                 {} ]);
> 
> which means we have zero segment and an empty final keyframe together, and
> the assertion still happened.
> 
> Let's see the sorted nsTArray<KeyframeValueEntry> in
> BuildSegmentsFromValueEntries():
>   entry[0]: (0.000000) flex-basis(auto)
>   entry[1]: (0.500000) flex-basis(auto)
>   entry[2]: (0.500000) flex-basis(0px)     // its next entry is an empty
> final keyframe, but we didn't find it.

Great! Thank you Boris!
We'd need to break the while loop and insert a 1.0 offset entry here. 

>   entry[3]: (0.000000) margin-left(0px)
>   entry[4]: (0.500000) margin-left(10px)
>   entry[5]: (0.500000) margin-left(10px)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > Let's see the sorted nsTArray<KeyframeValueEntry> in
> > BuildSegmentsFromValueEntries():
> >   entry[0]: (0.000000) flex-basis(auto)
> >   entry[1]: (0.500000) flex-basis(auto)
> >   entry[2]: (0.500000) flex-basis(0px)     // its next entry is an empty
> > final keyframe, but we didn't find it.
> 
> Great! Thank you Boris!
> We'd need to break the while loop and insert a 1.0 offset entry here. 

OK. I will try this way. Thanks.
Attachment #8832338 - Flags: review?(hikezoe)
Attachment #8832339 - Flags: review?(hikezoe)
Comment on attachment 8832339 [details]
Bug 1324554 - Part 2: Add a crash test.

https://reviewboard.mozilla.org/r/108672/#review109822

Thanks! Looks good to me except file name. :-)

::: dom/animation/test/crashtests/1324054-1.html:5
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +  <meta charset="UTF-8">
> +  <title>Bug 1324053 - missing final keyframes and zero-length segments together</title>

Nit: wrong bug number.

::: dom/animation/test/crashtests/crashtests.list:20
(Diff revision 1)
>  pref(dom.animations-api.core.enabled,true) load 1322382-1.html
>  skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1322291-1.html # bug 1311257
>  skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1322291-2.html # bug 1311257 and bug 1311257
>  skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1323114-1.html # bug 1324690 and bug 1311257
>  skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1323114-2.html # bug 1324690
> +pref(dom.animations-api.core.enabled,true) load 1324054-1.html

This needs skip-if(stylo) with '# bug 1311257".
Attachment #8832339 - Flags: review?(hikezoe) → review+
Boris, thanks for fixing this!  I'd need some time to review part 1 carefully.  Would you mind adding some test cases in dom/animation/test/chrome/test_animation_properties.html until waiting for my review?  Thanks!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Boris, thanks for fixing this!  I'd need some time to review part 1
> carefully.  Would you mind adding some test cases in
> dom/animation/test/chrome/test_animation_properties.html until waiting for
> my review?  Thanks!

Sure, I can add more tests into test_animation_properties.html. :)
Comment on attachment 8832338 [details]
Bug 1324554 - Part 1: Filter out zero-length segments earlier.

https://reviewboard.mozilla.org/r/108670/#review110088

This looks really good to me.  One thing I am curious is the reason why you put the check for duplicate offset entries between the check for offset 0 and 1.  It is ordered by offset, right?

::: dom/animation/KeyframeUtils.cpp:1333
(Diff revision 1)
>          ++i;
>          continue;
>        }
>      }
>  
> +    // Filter out zero-length segments except for initial and final ones.

'zero-length sgments' is not easy to understand to me.

// Skip this entry if the next entry has the same offset except for initial
// and final ones.
?

Also I think we should comment about the missing kefyrame case here, actually this bug.

// We will handle missing keyframe in the next loop if the property is
// changed on the next entry.

::: dom/animation/KeyframeUtils.cpp:1351
(Diff revision 1)
>      // Starting from i, determine the next [i, j] interval from which to
>      // generate a segment.

We need to modify this comment too.  Now we starts from i + 1, and I believe interval determination process is only for offset 0 or 1. Right?
Attachment #8832338 - Flags: review?(hikezoe) → review+
Comment on attachment 8832338 [details]
Bug 1324554 - Part 1: Filter out zero-length segments earlier.

https://reviewboard.mozilla.org/r/108670/#review110088

I think it has no influence on offset 0 (only offset 1), so put it only before the check for offset 1 (final missing keyframe). However, I can move it before the check for offset 0, so the order would be more readable.

> 'zero-length sgments' is not easy to understand to me.
> 
> // Skip this entry if the next entry has the same offset except for initial
> // and final ones.
> ?
> 
> Also I think we should comment about the missing kefyrame case here, actually this bug.
> 
> // We will handle missing keyframe in the next loop if the property is
> // changed on the next entry.

OK. I will replace my comment with these you suggested, which is much clearer.

> We need to modify this comment too.  Now we starts from i + 1, and I believe interval determination process is only for offset 0 or 1. Right?

Yes, "the staring from i" is talking about the range of |j|, and now skipped entries are processed earlier, so this code is only for offset 0 and 1. I should fix the comment, too. Thanks.
Comment on attachment 8832743 [details]
Bug 1324554 - Part 3: More tests for test_animation_properties.html.

https://reviewboard.mozilla.org/r/108962/#review110160

Thanks for doing this!
This patch includes the crashtest that has already added in part 2 patch. Please remove it.

::: dom/animation/test/chrome/test_animation_properties.html:720
(Diff revision 1)
> +                { margin: '10px', offset: 0.5 },
> +                { margin: '20px', offset: 0.5 },
> +                { margin: '30px'} ],
> +    expected: [ { property: 'margin-top',
> +                  values: [ value(0, undefined, 'add', 'linear'),
> +                            value(0.5, '10px', 'replace', undefined),

Is this 'undefined' for timing function really necessary?
Attachment #8832743 - Flags: review?(hikezoe) → review+
Comment on attachment 8832743 [details]
Bug 1324554 - Part 3: More tests for test_animation_properties.html.

https://reviewboard.mozilla.org/r/108962/#review110160

Do you mean the missing keyframe with zero-length segment test in this patch? I can remove it.

> Is this 'undefined' for timing function really necessary?

Not necessary, I will remove them.
(In reply to Boris Chiou [:boris] (away 1/27 ~ 2/1) from comment #19)
> Comment on attachment 8832743 [details]
> Bug 1324554 - Part 3: More tests for test_animation_properties.html.
> 
> https://reviewboard.mozilla.org/r/108962/#review110160
> 
> Do you mean the missing keyframe with zero-length segment test in this
> patch? I can remove it.

Oops! Sorry, I was looking squashed patch at that time.  Never mind!
(In reply to Boris Chiou [:boris] (away 1/27 ~ 2/1) from comment #14)
> Comment on attachment 8832338 [details]
> Bug 1324554 - Part 1: Filter out zero-length segments earlier.
> 
> https://reviewboard.mozilla.org/r/108670/#review110088
> 
> I think it has no influence on offset 0 (only offset 1), so put it only
> before the check for offset 1 (final missing keyframe). However, I can move
> it before the check for offset 0, so the order would be more readable.

I found a problem if I move the "skip entries" code before the check for offset 0.
For this case (in part 3):
    frames:   [ { },                              // 1st (missing keyframe)
                { margin: '10px', offset: 0.5 },  // 2nd
                { margin: '20px', offset: 0.5 },  // 3rd
                { margin: '30px'} ]

If we skip entries before the check for offset 0 (i.e. missing initial keyframe):

    // skip entries
    if (aEntries[i].mProperty == aEntries[i + 1].mProperty &&
        aEntries[i].mOffset == aEntries[i + 1].mOffset &&
        aEntries[i].mOffset != 1.0f && aEntries[i].mOffset != 0.0f) { ... }

    // No keyframe for this property at offset 0.
    if (aEntries[i].mProperty != lastProperty &&
        aEntries[i].mOffset != 0.0f) { ... }

The 2nd keyframe will be skipped in this test case because the conditions of "skip entries" don't exclude the initial missing keyframe (i.e. property != lastProperty), so we accidentally skip the 2nd keyframe. Therefore, I think it's better to put "skip entries" code after the check for offset 0; otherwise, we need to add more if conditions in it.

Thanks.
(In reply to Boris Chiou [:boris] (away 1/27 ~ 2/1) from comment #21)
> ... Therefore, I think
> it's better to put "skip entries" code after the check for offset 0;
> otherwise, we need to add more if conditions in it.
> 
> Thanks.

So I prefer to keep the order I use at first. I will update them soon.
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ff224f1e876
Part 1: Filter out zero-length segments earlier. r=hiro
https://hg.mozilla.org/integration/autoland/rev/d2c410e1c919
Part 2: Add a crash test. r=hiro
https://hg.mozilla.org/integration/autoland/rev/4d530258325b
Part 3: More tests for test_animation_properties.html. r=hiro
https://hg.mozilla.org/mozilla-central/rev/1ff224f1e876
https://hg.mozilla.org/mozilla-central/rev/d2c410e1c919
https://hg.mozilla.org/mozilla-central/rev/4d530258325b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Is this worth backporting to 53/ESR52?
Flags: needinfo?(boris.chiou)
Flags: in-testsuite+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)
> Is this worth backporting to 53/ESR52?

Yes, I think this is worth to backport to 53 because this fixed a potential assertion which might be easily reproduced.
Flags: needinfo?(boris.chiou)
Please request Beta/ESR52 approval on this when you get a chance then :)
Flags: needinfo?(boris.chiou)
We need to filter zero-length segments (i.e. entries with the same
offsets) before handle missing final keyframes.

MozReview-Commit-ID: DGJPrNRXlmd

--HG--
extra : rebase_source : e5a56fb9b8d41fedf2458aee8ae2d8bd806451b6
Attachment #8844327 - Flags: review+
MozReview-Commit-ID: GCeqmQm9kIb

--HG--
extra : rebase_source : ee3011cbce2594e0e0ada38b6744c3f354a1aa57
Attachment #8844329 - Flags: review+
MozReview-Commit-ID: 6MkkP8yNGvg

--HG--
extra : rebase_source : 091a1d72747b445a80c255bcc09e33fe23760446
Attachment #8844330 - Flags: review+
Comment on attachment 8844327 [details] [diff] [review]
Part 1: Filter out zero-length segments earlier. (carry hiro's r+)

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1324554
[User impact if declined]: Possible assertions for animations
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: Only these three patches in the bug.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only fix the problem for missing keyframes together with zero-length keyframes. This is a special case for animations.
[String changes made/needed]: Nope.
Flags: needinfo?(boris.chiou)
Attachment #8844327 - Flags: approval-mozilla-beta?
Attachment #8844329 - Flags: approval-mozilla-beta?
Attachment #8844330 - Flags: approval-mozilla-beta?
Comment on attachment 8844327 [details] [diff] [review]
Part 1: Filter out zero-length segments earlier. (carry hiro's r+)

The fix stabilized on Nightly54 for a month, seems safe to uplift to Beta53.
Attachment #8844327 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844329 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844330 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Reproduced on linux64-asan-debug 2017-01-01, Ubuntu 14.04 x64.
Verified fixed Fx 54.0a2 (2017-03-10), 53.0b2 linux64-asan-debug builds.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Mark firefox-esr52 as unaffected because we didn't support missing initial/final keyframes, and I just verified it locally with the crash tests in part 2.
(In reply to Boris Chiou [:boris] from comment #41)
> Mark firefox-esr52 as unaffected because we didn't support missing
> initial/final keyframes, and I just verified it locally with the crash tests
> in part 2.

And Is there anyone who can verify this in esr52?
You need to log in before you can comment on or make changes to this bug.