1384014 - Stylo: Crash in mozalloc_abort | abort | core::result::unwrap_failed<T> | style::properties::animated_properties::{{impl}}::compute_distance

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-459571b1-15bb-4159-8bf0-3c1cc0170724.
=============================================================

There are 5 crashes from the same installation with buildid 20170724100311.

Comment 1

[Boris Chiou [:boris]]

Astley, could you see any url or STR in the report?

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

I just filed bug 1384213 with an STR at https://daneden.github.io/animate.css/

Comment 3

[Darkspirit]

Had a (whole) browser crash while I was afk. No STR yet.

1 browser crash, but two reports from the same minute:
bp-5b4d7484-df82-4b20-8f63-23e290170725 (this bug's crash signature)
bp-5d1c865c-7973-4a3e-88b1-826460170725 [@ shutdownhang | libpthread-2.24.so@0xd15f ]

and I got bp-6ed2f4dc-8c60-417b-a516-d1c530170725 (this bug's crash signature) some hours ago, but I didn't notice I think.

Setting OS to All because Linux & MacOS are affected (yet).

Comment 4

[Astley Chen (inactive)]

(In reply to Boris Chiou [:boris] from comment #1)
> Astley, could you see any url or STR in the report?

Yes, I'll share with you in private.
For now, I'ven't had a chance to reproduce it.

Comment 5

[Boris Chiou [:boris]]

Thanks, all. I'm trying to reproduce this now. :)

Comment 6

[Astley Chen (inactive)]

(In reply to Boris Chiou [:boris] from comment #5)
> Thanks, all. I'm trying to reproduce this now. :)

Refer to bug 1384213. STR is simply opening DevTool::Animation panel.

Comment 7

[Boris Chiou [:boris]]

My call stack from the STR in comment 2:

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/libcore/result.rs:859
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
   1: std::panicking::default_hook::{{closure}}
   2: std::panicking::default_hook
   3: std::panicking::rust_panic_with_hook
   4: std::panicking::begin_panic
   5: std::panicking::begin_panic_fmt
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::result::unwrap_failed
   9: <core::result::Result<T, E>>::unwrap
  10: <style::values::animated::effects::FilterList as style::properties::animated_properties::Animatable>::compute_squared_distance
  11: <style::values::animated::effects::FilterList as style::properties::animated_properties::Animatable>::compute_distance
  12: <style::properties::animated_properties::AnimationValue as style::properties::animated_properties::Animatable>::compute_distance
  13: Servo_AnimationValues_ComputeDistance
  14: _ZN16nsDOMWindowUtils24ComputeAnimationDistanceEP13nsIDOMElementRK9nsAStringS4_S4_Pd

Comment 8

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #7)
>   10: <style::values::animated::effects::FilterList as
> style::properties::animated_properties::Animatable>::compute_squared_distance
>   11: <style::values::animated::effects::FilterList as

So I guess this happened while trying to compute the square distance for AnimatedFilterList.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Yeah, it's likely happen when we try to do compute_filter_square_distance with different filter functions.

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1384014 - Implement compute_distance for Angle.

When computing the distance between two filter functions, e.g.
HueRotate(0deg) and HueRotate(-360deg), we got an Err(()) and unwrap it
immediately in compute_filter_square_distance(), which causes a crash.
The root-cause is that we don't implement computed_distance of Angle, and we
assume we always get a valid result from compute_squared_distance() for
each filter function.

Review commit: https://reviewboard.mozilla.org/r/161372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161372/

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8890258 [details]
Bug 1384014 - Implement compute_distance for Angle.

https://reviewboard.mozilla.org/r/161372/#review167058

Thanks for doing this. But as well as doing this we should really fix compute_squared_distance for AnimatedFilterList and drop all those unwraps.

Firstly, we should replace that `if from.is_none() { ... ` block with a `match` (e.g. `match (from, to) { (Some(from), None) => { ... }, } `)

Or better still, actually zip() and cycle() those iterators (to match how we actually interpolate them). It's not clear to me why, when the lists have different lengths, it's correct to continually calculate the distance from the last item in the shorter list?

Anyway, more importantly, lines like:

    current_square_distance =
        compute_filter_square_distance(&none, &(from.unwrap())).unwrap();

Should just become:

    current_square_distance = compute_filter_square_distance(&none, &from)?;

Although if we do the iteration properly we shouldn't need &none, I think.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:932
(Diff revision 1)
> +        // Angle could be used for transform or filter functions, so we use the same definition
> +        // from SVG spec:
> +        //
> +        //     distance(Va, Vb) = sqrt((Va.angle − Vb.angle)^2)
> +        //     where: Vi.angle is the angle component of the Vi rotation or skew transform value.
> +        //
> +        // However, there is no official spec to restrict the unit of angle while computing the
> +        // distance, and Radian may be the most reasonable one, so we use it here.
> +        Ok((self.radians64() - other.radians64()).abs())

This code doesn't seem to match the comment?

Also, I'm not sure we need the comment about radians.

In fact, I don't really know if we need any comment except a link to the spec where we got the formula from.

e.g.

// Use the formula for angles defined in SVG:
// https://www.w3.org/TR/SVG/animate.html#complexDistances

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Looks like we hit this same crash when running a DevTools test:

devtools/client/animationinspector/test/browser_animation_pseudo_elements.js

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8890258 [details]
Bug 1384014 - Implement compute_distance for Angle.

https://reviewboard.mozilla.org/r/161372/#review167058

> This code doesn't seem to match the comment?
> 
> Also, I'm not sure we need the comment about radians.
> 
> In fact, I don't really know if we need any comment except a link to the spec where we got the formula from.
> 
> e.g.
> 
> // Use the formula for angles defined in SVG:
> // https://www.w3.org/TR/SVG/animate.html#complexDistances

Thanks for the review. I will drop the comment, and rewrite the compute_squared_distance for AnimatedFilterList.

Comment 14

[Boris Chiou [:boris]]

Comment on attachment 8890258 [details]
Bug 1384014 - Implement compute_distance for Angle.

https://reviewboard.mozilla.org/r/161372/#review167058

> It's not clear to me why, when the lists have different lengths, it's correct to continually calculate the distance from the last item in the shorter list?

If two lists have different lengths
e.g.:
1. "hue-rotate(3rad) invert(2) blur(1.0)"
2. "hue-rotate(0rad) invert(2)"

We should treat the 2nd list as:
2'. "hue-rotate(0rad) invert(2) blur(0.0)"

so we can compute the distance between 1 and 2'. Therefore, I think we still need to calculate the distance from the last item in the shorter list.

Comment 15

[Brian Birtles (:birtles)]

Oh, so I think we actually want to calculate from a zero-type value, not the last item in the list.

Just a thought: would it be easier to fill the shorter list with None values so that they are equal length (assuming there is an easy way of doing that in Rust, e.g. iter::repeat(None).take(length_list_diff) chain()ed to the shorter list), zip() them together, iterate over the zipped iterator, make compute_filter_square_distance take Option<> arguments, and then in compute_filter_square_distance produce the zero-type value when it gets a None value?

Comment 16

[Boris Chiou [:boris]]

Attachment: Bug 1384014 - Rewrite compute_squared_distance for AnimatedFilterList.

Review commit: https://reviewboard.mozilla.org/r/161872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161872/

Comment 17

[Boris Chiou [:boris]]

Attachment: Bug 1384014 - Vender rust for itertools.

We need itertools to handle filter lists with different lengths
(i.e. zip_longest), so vender itertools for Gecko.

Review commit: https://reviewboard.mozilla.org/r/161874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161874/

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8890258 [details]
Bug 1384014 - Implement compute_distance for Angle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161372/diff/1-2/

Comment 19

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #15)
> Oh, so I think we actually want to calculate from a zero-type value, not the
> last item in the list.
> 
> Just a thought: would it be easier to fill the shorter list with None values
> so that they are equal length (assuming there is an easy way of doing that
> in Rust, e.g. iter::repeat(None).take(length_list_diff) chain()ed to the
> shorter list), zip() them together, iterate over the zipped iterator, make
> compute_filter_square_distance take Option<> arguments, and then in
> compute_filter_square_distance produce the zero-type value when it gets a
> None value?

That's a good idea. However, I just tried itertools to zip two iterators according to the longest one. You can check it now. :)

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8890258 [details]
Bug 1384014 - Implement compute_distance for Angle.

https://reviewboard.mozilla.org/r/161372/#review167202

::: servo/components/style/properties/helpers/animated_properties.mako.rs:931
(Diff revision 2)
> +        // Use the formula for angles defined in SVG:
> +        // https://www.w3.org/TR/SVG/animate.html#complexDistances

Sorry, this is my mistake. We should probably make that comment say, "Use the formula for calculating the distance between angles defined in SVG:"

Sorry.

Comment 21

[Boris Chiou [:boris]]

Hi, Chris,

If I want to add one more crate to Gecko, i.e. itertools, which has been used by Servo already, is there anything I have to do for it? I saw Bug 1375292 add many lines, but that patch hasn't been merged until now.

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 8890689 [details]
Bug 1384014 - Rewrite compute_squared_distance for AnimatedFilterList.

https://reviewboard.mozilla.org/r/161872/#review167204

This is so much better. Thank you!

I don't know what restrictions we have regarding including external crates and whether or not they require license review etc. but I assume whoever reviews patch 3 will check that.

Comment 23

[Manish Goregaokar [:manishearth]]

Comment on attachment 8890690 [details]
Bug 1384014 - Vender rust for itertools.

https://reviewboard.mozilla.org/r/161874/#review167208

you only want to land the lockfile changes here, the vcssync handles vendoring

Comment 24

[Boris Chiou [:boris]]

Comment on attachment 8890690 [details]
Bug 1384014 - Vender rust for itertools.

https://reviewboard.mozilla.org/r/161874/#review167208

OK. Thanks. I will remove the new-added files. i.e. third_party/rust/itertools/*

Comment 25

[Boris Chiou [:boris]]

Comment on attachment 8890258 [details]
Bug 1384014 - Implement compute_distance for Angle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161372/diff/2-3/

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8890689 [details]
Bug 1384014 - Rewrite compute_squared_distance for AnimatedFilterList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161872/diff/1-2/

Comment 27

[Boris Chiou [:boris]]

Comment on attachment 8890690 [details]
Bug 1384014 - Vender rust for itertools.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161874/diff/1-2/

Comment 28

[Chris Peterson [:cpeterson]]

(In reply to Boris Chiou [:boris] from comment #21)
> If I want to add one more crate to Gecko, i.e. itertools, which has been
> used by Servo already, is there anything I have to do for it? I saw Bug
> 1375292 add many lines, but that patch hasn't been merged until now.

itertools is dual-licensed as "MIT/Apache-2.0" so we get to choose which license we use. about:license already has a notice for the Apache-2.0 license, so we don't need to add a new notice for itertools. :)

If a new crate is licensed as only MIT or something else, then we will need to a new notice to about:license. The MIT license requires that we add a new copyright entry for every new library. The Apache-2.0 license does not.

https://crates.io/crates/itertools

Comment 29

[Boris Chiou [:boris]]

(In reply to Chris Peterson [:cpeterson] from comment #28)
> (In reply to Boris Chiou [:boris] from comment #21)
> > If I want to add one more crate to Gecko, i.e. itertools, which has been
> > used by Servo already, is there anything I have to do for it? I saw Bug
> > 1375292 add many lines, but that patch hasn't been merged until now.
> 
> itertools is dual-licensed as "MIT/Apache-2.0" so we get to choose which
> license we use. about:license already has a notice for the Apache-2.0
> license, so we don't need to add a new notice for itertools. :)
> 
> If a new crate is licensed as only MIT or something else, then we will need
> to a new notice to about:license. The MIT license requires that we add a new
> copyright entry for every new library. The Apache-2.0 license does not.
> 
> https://crates.io/crates/itertools

Cool. Thanks for the explanation. I can land this now. :)

Comment 30

[Boris Chiou [:boris]]

Attachment: Servo PR, #17898

Comment 31

[Boris Chiou [:boris]]

try (with manually vendoring rust): https://treeherder.mozilla.org/#/jobs?repo=try&revision=048c55fe992bb709b5616a33741ca00a7179f6ae

Comment 32

[Boris Chiou [:boris]]

Looks like Servo VCS Sync also updated the lock files, so I don't need to merge gecko patch. :)

Comment 34

[Boris Chiou [:boris]]

landed in m-c:
https://hg.mozilla.org/mozilla-central/rev/e65c70c5c833