Closed Bug 1377053 Opened 7 years ago Closed 7 years ago

stylo: Animation division by zero crash.

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: birtles)

References

Details

Attachments

(4 files)

Bug 1377053 - Crash test for interpolating zero-length lists in add_weighted;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1377053 - Add test for discrete interpolation of stroke-dasharray with 'none';
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1377053 - Update test expectations based in fixed interpolation fallback behavior;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR #17613
41 bytes, text/x-github-pull-request
Details | Review 
The following test-case:

<!doctype html>
<style>
@keyframes t {
  to {
    all: initial;
  }
}
body {
  animation: t 3s infinite alternate;
  color: red;
  margin: 10px;
}
</style>
test

Crashes stylo with a division by zero panic in animation code.
This is happening in the call to lcm in repeated_vec_impl's implementation of add_weighted here:

  let len = lcm(self.len(), other.len());

http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/servo/components/style/properties/helpers/animated_properties.mako.rs#817

Specifically, we have the following stack:

> xul.dll!num_integer::{{impl}}::lcm(unsigned __int64 * self, unsigned __int64 * other) Line 539	Unknown
> xul.dll!style::properties::animated_properties::{{impl}}::add_weighted<style::values::generics::position::Position<style::values::computed::length::LengthOrPercentage, style::values::computed::length::LengthOrPercentage>>(smallvec::SmallVec<[style::values::generics::position::Position<style::values::computed::length::LengthOrPercentage, style::values::computed::length::LengthOrPercentage>; 1]> * self, smallvec::SmallVec<[style::values::generics::position::Position<style::values::computed::length::LengthOrPercentage, style::values::computed::length::LengthOrPercentage>; 1]> * other, double self_portion, double other_portion) Line 108422	Unknown
> xul.dll!style::properties::longhands::scroll_snap_coordinate::computed_value::{{impl}}::add_weighted(style::properties::longhands::scroll_snap_coordinate::computed_value::T * self, style::properties::longhands::scroll_snap_coordinate::computed_value::T * other, double self_portion, double other_portion) Line 17344	Unknown
> xul.dll!style::properties::animated_properties::{{impl}}::add_weighted(style::properties::animated_properties::AnimationValue * self, style::properties::animated_properties::AnimationValue * other, double self_portion, double other_portion) Line 103952	Unknown
> xul.dll!style::properties::animated_properties::Animatable::interpolate<style::properties::animated_properties::AnimationValue>(style::properties::animated_properties::AnimationValue * self, style::properties::animated_properties::AnimationValue * other, double progress) Line 108370	Unknown
> xul.dll!geckoservo::glue::Servo_AnimationCompose(style::gecko_bindings::bindings::RawServoAnimationValueMap * raw_value_map, std::os::raw::c_void * base_values, style::gecko_bindings::structs::root::nsCSSPropertyID css_property, style::gecko_bindings::structs::root::mozilla::AnimationPropertySegment * segment, style::gecko_bindings::structs::root::mozilla::AnimationPropertySegment * last_segment, style::gecko_bindings::structs::root::mozilla::ComputedTiming * computed_timing, style::gecko_bindings::structs::root::mozilla::dom::IterationCompositeOperation iteration_composite) Line 546	Unknown

The definition of lcm is:

  *self * (*other / self.gcd(other))

And in this case both self and other are zero.

Mathematically, LCM is not defined when either argument is zero.[1] I *think* the behavior we want in this case is that if both arguments are zero, we return zero, but of only one argument is zero, then we want the other argument.

[1] https://en.wikipedia.org/wiki/Least_common_multiple
(In reply to Brian Birtles (:birtles) from comment #2)
> I *think* the behavior we want in this case is that if both arguments are
> zero, we return zero, but of only one argument is zero, then we want the
> other argument.

Actually, that's not right. We can't cycle() an empty list. So I'm not sure if we want to treat the resulting length list as zero, try to interpolate with some sort of none value, or simply fail to interpolate in this case.

If we were to animate scroll-snap-coordinate between 'none' and some list I wonder if we should just fail? Probably I should check what other implementations do.
I can also produce this with:

div {
  animation: t 1s;
}
@keyframes t {
  to {
    scroll-snap-coordinate: none;
  }
}

Or likewise:

  stroke-dasharray: none;

Basically, any to value that computes to an empty list.

Testing how stroke-dasharray interpolates in different browsers[1]:

Gecko:

  0px, 0px -> 10px, 20px at ~50%: 5.07799, 10.156
  0px -> 10px, 20px at ~50%: 5.07799, 10.156
  (none) -> 10px, 20px at ~50%: 10, 20
  'none' -> 10px, 20px at ~50%: 10, 20
  10px, 20px -> (none) at ~50%: none
  10px, 20px -> 'none' at ~50%: none
  'none' -> 'none' at ~50%: none
  (none) -> 'none' at ~50%: none

Blink:

  0px, 0px -> 10px, 20px at ~50%: 5.09167px, 10.1833px
  0px -> 10px, 20px at ~50%: 5.09167px, 10.1833px
  (none) -> 10px, 20px at ~50%: 10px, 20px
  'none' -> 10px, 20px at ~50%: 10px, 20px
  10px, 20px -> (none) at ~50%: none
  10px, 20px -> 'none' at ~50%: none
  'none' -> 'none' at ~50%: none
  (none) -> 'none' at ~50%: none

Edge:

  0px, 0px -> 10px, 20px at ~50%: 5.1px, 10.2px
  0px -> 10px, 20px at ~50%: 5.1px, 10.2px
  (none) -> 10px, 20px at ~50%: none
  'none' -> 10px, 20px at ~50%: 5.1px, 10.2px
  10px, 20px -> (none) at ~50%: none
  10px, 20px -> 'none' at ~50%: 4.9px, 9.8px
  'none' -> 'none' at ~50%: 0px
  (none) -> 'none' at ~50%: none

Stylo:

  0px, 0px -> 10px, 20px at ~50%: 5.11667px, 10.2333px
  0px -> 10px, 20px at ~50%: 5.11667px, 10.2333px
  (none) -> 10px, 20px at ~50%: none
  'none' -> 10px, 20px at ~50%: none
  10px, 20px -> (none) at ~50%: none
  10px, 20px -> 'none' at ~50%: none
  'none' -> 'none' at ~50%: <crash>
  (none) -> 'none' at ~50%: <crash>

[1] https://jsfiddle.net/ft4k92Ln/6/
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
I'm putting this up for review even though the try run hasn't finished since I need to head out the door soon. Hopefully anything try turns up is a fairly straightforward fix.
Comment on attachment 8883475 [details]
Bug 1377053 - Crash test for interpolating zero-length lists in add_weighted;

Clearing review request for this now since I think we actually want min() here.
Attachment #8883475 - Flags: review?(hikezoe)
Comment on attachment 8883475 [details]
Bug 1377053 - Crash test for interpolating zero-length lists in add_weighted;

https://reviewboard.mozilla.org/r/154376/#review159512
Attachment #8883475 - Flags: review+
(In reply to Brian Birtles (:birtles, away 6~10 July) from comment #9)
> Comment on attachment 8883475 [details]
> Bug 1377053 - Handle zero-length lists in add_weighted;
> 
> Clearing review request for this now since I think we actually want min()
> here.

Doh! I have dull eyes..
Comment on attachment 8883476 [details]
Bug 1377053 - Add test for discrete interpolation of stroke-dasharray with 'none';

https://reviewboard.mozilla.org/r/154378/#review159514
Attachment #8883476 - Flags: review?(hikezoe) → review+
After updating the patch, nearly all the wpt tests pass except the one with keyframe easing. Which is, weird. I might just fix that in bug 1378288 unless I get a chance to debug it before then.
(In reply to Brian Birtles (:birtles, away 6~10 July) from comment #15)
> After updating the patch, nearly all the wpt tests pass except the one with
> keyframe easing. Which is, weird.

Actually, looking at Servo_AnimationCompose, I'm pretty sure following should say, "else if position < 0.5" (i.e. *not* progress),

    let position = unsafe {
        Gecko_GetPositionInSegment(segment, progress, computed_timing.mBeforeFlag)
    };
    if let Ok(value) = from_value.interpolate(to_value, position) {
        value_map.insert(property, value);
    } else if progress < 0.5 {
        value_map.insert(property, from_value.clone());
    } else {
        value_map.insert(property, to_value.clone());
    }
Comment on attachment 8883545 [details]
Bug 1377053 - Update test expectations based in fixed interpolation fallback behavior;

https://reviewboard.mozilla.org/r/154478/#review159680
Attachment #8883545 - Flags: review?(hikezoe) → review+
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7372ca2ea18
Crash test for interpolating zero-length lists in add_weighted; r=hiro
https://hg.mozilla.org/integration/autoland/rev/dfdfec6b02d2
Update test expectations based in fixed interpolation fallback behavior; r=hiro
https://hg.mozilla.org/integration/autoland/rev/df39ebe9235b
Add test for discrete interpolation of stroke-dasharray with 'none'; r=hiro
https://hg.mozilla.org/mozilla-central/rev/e7372ca2ea18
https://hg.mozilla.org/mozilla-central/rev/dfdfec6b02d2
https://hg.mozilla.org/mozilla-central/rev/df39ebe9235b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: