Closed Bug 1383493 Opened 2 years ago Closed 2 years ago

stylo: panicked at 'add_weighted should only be used for interpolating or accumulating transforms'

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: truber, Assigned: hiro)

References

(Blocks 3 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 1 obsolete file)

Attached file testcase.html
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?
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
Priority: -- → P1
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 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 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+
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
The wpt fails on gecko... We need to fix gecko side too. sigh.
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 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&regexp=false&path=
And I guess that should be aCoeff1 == 0.0
(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!
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+
(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
Attachment #8889653 - Attachment is obsolete: true
Attached file Servo PR
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
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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.