Closed
Bug 1383493
Opened 7 years ago
Closed 7 years ago
stylo: panicked at 'add_weighted should only be used for interpolating or accumulating transforms'
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
People
(Reporter: truber, Assigned: hiro)
References
(Blocks 2 open bugs)
Details
(Keywords: assertion, testcase)
Attachments
(5 files, 1 obsolete file)
The attached testcase causes a panic in m-c rev c22502562670 with stylo enabled by pref. thread '<unnamed>' panicked at 'add_weighted should only be used for interpolating or accumulating transforms', /home/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/d ebug/build/style-a3e69c1fe576a835/out/properties.rs:91493 stack backtrace: 0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace 1: std::sys_common::backtrace::_print 2: std::panicking::default_hook::{{closure}} 3: std::panicking::default_hook 4: std::panicking::rust_panic_with_hook 5: std::panicking::begin_panic 6: <style::properties::animated_properties::MatrixDecomposed3D as style::properties::animated_properties::Animatable>::add_weighted 7: <style::properties::longhands::transform::computed_value::ComputedMatrix as style::properties::animated_properties::Animatable>::add_weighted 8: style::properties::animated_properties::add_weighted_transform_lists 9: <style::properties::longhands::transform::computed_value::T as style::properties::animated_properties::Animatable>::accumulate 10: <style::properties::animated_properties::AnimationValue as style::properties::animated_properties::Animatable>::accumulate 11: geckoservo::glue::Servo_AnimationCompose::{{closure}} 12: Servo_AnimationCompose
Flags: in-testsuite?
Assignee | ||
Comment 1•7 years ago
|
||
I missed that we pass 0.0 to add_weighted() in case of accumulation with transform: none; https://hg.mozilla.org/mozilla-central/file/5928d905c0bc/servo/components/style/properties/helpers/animated_properties.mako.rs#l2609
Updated•7 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8889653 [details] Bug 1383493 - MatrixDecomposed3D.add_weighted() is called with zero value for other_portion in case of iteration composite. https://reviewboard.mozilla.org/r/160692/#review165994
Attachment #8889653 -
Flags: review?(bbirtles) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8889654 [details] Bug 1383493 - A crash test that accumulates transform:none onto decomposed transform matrix for iteration composite. https://reviewboard.mozilla.org/r/160694/#review165996
Attachment #8889654 -
Flags: review?(bbirtles) → review+
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8889655 [details] Bug 1383493 - A test case that accumulates transform:none onto decomposed transform matrix for composite. https://reviewboard.mozilla.org/r/160696/#review165998 ::: testing/web-platform/meta/MANIFEST.json:620972 (Diff revision 1) > "web-animations/animation-model/animation-types/property-list.js": [ > "31ad7b4aa12e4485f95545b087779cabb56c696c", > "support" > ], > "web-animations/animation-model/animation-types/property-types.js": [ > - "6b6ff5601039561c1fb9c185d8451a29150a4a0f", > + "ed8c0c3f297ec6ad93c63d784f2a393659d96d40", Wow, you even updated the manifest checksum! You are my hero! ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1229 (Diff revision 1) > + target.style[idlName] = createMatrixFromArray(matrixArray); > + var animation = > + target.animate({ [idlName]: [ 'none', 'none' ] }, > + { duration: 1000, fill: 'both', composite: 'accumulate' }); > + > + testAnimationSampleMatrices(animation, idlName, > + [{ time: 0, expected: matrixArray }, > + { time: 1000, expected: matrixArray }]); (I wondered if there's a chance that some implementations might optimize out a transform animation whose only values are 'none' and which has a non-replace composite mode. And I thought that if that's the case we could do an animation from 'none' to 'translate(100px)' with a timing function of 'step-end' and sample at 0 and 999. But on further thought I think it's unlikely that UAs will optimize that far.)
Attachment #8889655 -
Flags: review?(bbirtles) → review+
Updated•7 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 years ago
|
||
The wpt fails on gecko... We need to fix gecko side too. sigh.
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a128f627d11e051bf7af44c0b2689e3498f5135b
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8889760 [details] Bug 1383493 - Make AddDifferentTransformLists() add |aCoeff1| instead of just adding 1 to |aCoeff2| for the same list case. https://reviewboard.mozilla.org/r/160836/#review166110 ::: layout/style/StyleAnimationValue.cpp:2155 (Diff revision 1) > - // For accumulation, we need to increase accumulation count for |aList1|. > + // In case of accumulation, generally |aCoeff1| + |aCoeff2| != 1.0 so we > + // need to take into |aCoeff1| value instead of just multiplication |aList2| > + // by |aCoeff2|. This comment doesn't make sense to me. I tried for a while to write a better one but haven't succeeded yet. I'll try again tomorrow.
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8889760 [details] Bug 1383493 - Make AddDifferentTransformLists() add |aCoeff1| instead of just adding 1 to |aCoeff2| for the same list case. https://reviewboard.mozilla.org/r/160836/#review166558 ::: layout/style/StyleAnimationValue.cpp:2155 (Diff revision 1) > - // For accumulation, we need to increase accumulation count for |aList1|. > + // In case of accumulation, generally |aCoeff1| + |aCoeff2| != 1.0 so we > + // need to take into |aCoeff1| value instead of just multiplication |aList2| > + // by |aCoeff2|. I've gone over and over this code and I think I finally understand it. Looking at that original changeset[1], that code makes the following change: - *resultTail = - AddDifferentTransformLists(&tempList1, aCoeff1, &tempList2, aCoeff2); + if (aList1 == aList2) { + *resultTail = + AddDifferentTransformLists(&tempList1, aCoeff1, &tempList1, aCoeff2); + } else { + *resultTail = + AddDifferentTransformLists(&tempList1, aCoeff1, &tempList2, aCoeff2); + } This seems reasonable (although it would have been great to have a comment explaining *why* we do this). However in AddDifferentTransformLists we do the following: + // FIXME: We should change the other transform code to also only + // take a single progress value, as having values that don't + // sum to 1 doesn't make sense for these. + if (aList1 == aList2) { + arr->Item(1).Reset(); + } else { + aList1->CloneInto(arr->Item(1).SetListValue()); + } - arr->Item(1).SetPercentValue(aCoeff1); - aList1->CloneInto(arr->Item(2).SetListValue()); + aList2->CloneInto(arr->Item(2).SetListValue()); arr->Item(3).SetPercentValue(aCoeff2); - aList2->CloneInto(arr->Item(4).SetListValue()); And it's not immediately obvious why this is ok. As it turns out, however, the only time `aList1 == aList2` is true, |aCoeff1| is equal to zero. This same changeset makes sure that is the case by rearranging the co-efficients in various places: if (list1->mValue.GetUnit() == eCSSUnit_None) { if (list2->mValue.GetUnit() == eCSSUnit_None) { result = new nsCSSValueList; if (result) { result->mValue.SetNoneValue(); } } else { - result = AddTransformLists(list2, aCoeff2, list2, 0); + result = AddTransformLists(list2, 0, list2, aCoeff2); } } else { if (list2->mValue.GetUnit() == eCSSUnit_None) { - result = AddTransformLists(list1, aCoeff1, list1, 0); + result = AddTransformLists(list1, 0, list1, aCoeff1); It appears this property, that when we pass the same list twice aCoeff1 is zero is still true today.[2] In these cases we produce a result such as the following: [ null, aList2, aCoeff2 ] It seems to me that in this changeset, instead of writing: + if (aList1 == aList2) { + arr->Item(1).Reset(); + } else { + aList1->CloneInto(arr->Item(1).SetListValue()); + } we should have just written: + if (aCoeff1 == 0) { + arr->Item(1).Reset(); + } else { + aList1->CloneInto(arr->Item(1).SetListValue()); + } My guess is that as part of implementing accumulation we introduced another case where `aList1 == aList2` and where aCoeff1 is *not* 0. It turns out that in most cases aCoeff1 is 1 and so we were able to work around this for a long time by just using `aCoeff2 += 1.0` but in this bug we've found other cases where |aCoeff1| is neither 0.0 nor 1.0. So I think the correct fix here is probably something like: if (aCoeff1 == 0) { arr->Item(1).Reset(); } else if (aList1 == aList2) { arr->Item(1).Reset(); aCoeff2 += aCoeff1; } else { aList1->CloneInto(arr->Item(1).SetListValue()); } Does that work? [1] https://hg.mozilla.org/mozilla-central/rev/97983a84f0c3b972986e7d7c992f02b804f9dae6 [2] http://searchfox.org/mozilla-central/search?q=AddTransformLists&case=false®exp=false&path=
Comment 16•7 years ago
|
||
And I guess that should be aCoeff1 == 0.0
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #15) > Comment on attachment 8889760 [details] > Bug 1383493 - AddDifferentTransformLists() takes into |aCoeff1| instead of > just adding 1 to |aCoeff2|. > > https://reviewboard.mozilla.org/r/160836/#review166558 > > ::: layout/style/StyleAnimationValue.cpp:2155 > (Diff revision 1) > > - // For accumulation, we need to increase accumulation count for |aList1|. > > + // In case of accumulation, generally |aCoeff1| + |aCoeff2| != 1.0 so we > > + // need to take into |aCoeff1| value instead of just multiplication |aList2| > > + // by |aCoeff2|. > > I've gone over and over this code and I think I finally understand it. > > Looking at that original changeset[1], that code makes the following change: > > - *resultTail = > - AddDifferentTransformLists(&tempList1, aCoeff1, &tempList2, > aCoeff2); > + if (aList1 == aList2) { > + *resultTail = > + AddDifferentTransformLists(&tempList1, aCoeff1, &tempList1, > aCoeff2); > + } else { > + *resultTail = > + AddDifferentTransformLists(&tempList1, aCoeff1, &tempList2, > aCoeff2); > + } > > This seems reasonable (although it would have been great to have a comment > explaining *why* we do this). > > However in AddDifferentTransformLists we do the following: > > + // FIXME: We should change the other transform code to also only > + // take a single progress value, as having values that don't > + // sum to 1 doesn't make sense for these. > + if (aList1 == aList2) { > + arr->Item(1).Reset(); > + } else { > + aList1->CloneInto(arr->Item(1).SetListValue()); > + } > > - arr->Item(1).SetPercentValue(aCoeff1); > - aList1->CloneInto(arr->Item(2).SetListValue()); > + aList2->CloneInto(arr->Item(2).SetListValue()); > arr->Item(3).SetPercentValue(aCoeff2); > - aList2->CloneInto(arr->Item(4).SetListValue()); > > And it's not immediately obvious why this is ok. As it turns out, however, > the only time `aList1 == aList2` is true, |aCoeff1| is equal to zero. > > This same changeset makes sure that is the case by rearranging the > co-efficients in various places: > > if (list1->mValue.GetUnit() == eCSSUnit_None) { > if (list2->mValue.GetUnit() == eCSSUnit_None) { > result = new nsCSSValueList; > if (result) { > result->mValue.SetNoneValue(); > } > } else { > - result = AddTransformLists(list2, aCoeff2, list2, 0); > + result = AddTransformLists(list2, 0, list2, aCoeff2); > } > } else { > if (list2->mValue.GetUnit() == eCSSUnit_None) { > - result = AddTransformLists(list1, aCoeff1, list1, 0); > + result = AddTransformLists(list1, 0, list1, aCoeff1); > > It appears this property, that when we pass the same list twice aCoeff1 is > zero is still true today.[2] > > In these cases we produce a result such as the following: > > [ null, aList2, aCoeff2 ] > > It seems to me that in this changeset, instead of writing: > > + if (aList1 == aList2) { > + arr->Item(1).Reset(); > + } else { > + aList1->CloneInto(arr->Item(1).SetListValue()); > + } > > we should have just written: > > + if (aCoeff1 == 0) { > + arr->Item(1).Reset(); > + } else { > + aList1->CloneInto(arr->Item(1).SetListValue()); > + } Oh right. This quite makes sense. So, > My guess is that as part of implementing accumulation we introduced another > case where `aList1 == aList2` and where aCoeff1 is *not* 0. It turns out > that in most cases aCoeff1 is 1 and so we were able to work around this for > a long time by just using `aCoeff2 += 1.0` but in this bug we've found other > cases where |aCoeff1| is neither 0.0 nor 1.0. Yes, that's right, that's what I had in mind. > So I think the correct fix here is probably something like: > > if (aCoeff1 == 0) { > arr->Item(1).Reset(); > } else if (aList1 == aList2) { > arr->Item(1).Reset(); > aCoeff2 += aCoeff1; > } else { > aList1->CloneInto(arr->Item(1).SetListValue()); > } > > Does that work? This should work!
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e5095e6f646079fbbc9e8ecda2870642191ed6d The try has not finished yet, but it should work fine.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8889760 [details] Bug 1383493 - Make AddDifferentTransformLists() add |aCoeff1| instead of just adding 1 to |aCoeff2| for the same list case. https://reviewboard.mozilla.org/r/160836/#review166578 ::: commit-message-63b5f:1 (Diff revision 2) > +Bug 1383493 - AddDifferentTransformLists() takes into |aCoeff1| instead of just adding 1 to |aCoeff2| for the same list case. r?birtles Make AddDifferentTransformList add |aCoeff1| instead of... ::: commit-message-63b5f:3 (Diff revision 2) > +Bug 1383493 - AddDifferentTransformLists() takes into |aCoeff1| instead of just adding 1 to |aCoeff2| for the same list case. r?birtles > + > +Also if |aCoeff1| is zero, we can just ignore the first list at all. s/at all/altogether/ ::: layout/style/StyleAnimationValue.cpp:2158 (Diff revision 2) > - if (aOperatorType == eCSSKeyword_accumulatematrix) { > - aCoeff2 += 1.0; > + // If we have the same list, clear the first list, take the first > + // coefficient into the second one so that we can simply multiply the s/take the first coefficient into/add the first coefficient onto/ ::: layout/style/StyleAnimationValue.cpp:2164 (Diff revision 2) > - aCoeff2 += 1.0; > - } > + // coefficient into the second one so that we can simply multiply the > + // second list by the second coefficient value. > + arr->Item(1).Reset(); > + aCoeff2 += aCoeff1; > } else { > aList1->CloneInto(arr->Item(1).SetListValue()); Should we assert that aCoeff1 + aCoeff2 =~ 1.0 here? (Since I think we have dealt with the only two cases where that is not true above? I think we could probably drop the FIXME comment if we do that.)
Attachment #8889760 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 24•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #23) > ::: layout/style/StyleAnimationValue.cpp:2164 > (Diff revision 2) > > - aCoeff2 += 1.0; > > - } > > + // coefficient into the second one so that we can simply multiply the > > + // second list by the second coefficient value. > > + arr->Item(1).Reset(); > > + aCoeff2 += aCoeff1; > > } else { > > aList1->CloneInto(arr->Item(1).SetListValue()); > > Should we assert that aCoeff1 + aCoeff2 =~ 1.0 here? As discussed on IRC, I am going to add an assertion that |aCoeff1| is 1 for accumulation and |aCoeff1| + |aCoeff2| =~ 1.0. See if what happens on try. I believe it should work. https://treeherder.mozilla.org/#/jobs?repo=try&revision=83864c08f544fd9810672772a1d6baa8baa7be7f
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8889653 -
Attachment is obsolete: true
Assignee | ||
Comment 28•7 years ago
|
||
Comment 29•7 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5ef47c46974c A crash test that accumulates transform:none onto decomposed transform matrix for iteration composite. r=birtles https://hg.mozilla.org/integration/autoland/rev/3a5d4b4cc788 Make AddDifferentTransformLists() add |aCoeff1| instead of just adding 1 to |aCoeff2| for the same list case. r=birtles https://hg.mozilla.org/integration/autoland/rev/71f2f36066c3 A test case that accumulates transform:none onto decomposed transform matrix for composite. r=birtles
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5ef47c46974c https://hg.mozilla.org/mozilla-central/rev/3a5d4b4cc788 https://hg.mozilla.org/mozilla-central/rev/71f2f36066c3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•