Closed Bug 1324554 Opened 9 years ago Closed 9 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 1 open bug)

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

Attachment

General

Created:
Updated:
Size: