1347053 - stylo: division by zero in set_transition_delay when share profile from gecko profiler

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

Just hit a reliable crash in an opt build. This is primarily noteworthy because it happens when trying to share a performance profile, so I can't generate stylo profiles to link to in bugs. As such, it'd be good to get this fixed up to avoid hindering performance work.

STR:
(1) Install the gecko profiler addon from https://perf-html.io/
(2) Capture a profile
(3) Click the green "share" button

Result:

thread '<unnamed>' panicked at 'attempt to calculate the remainder with a divisor of zero', /files/mozilla/mc/z/obj-x86_64-apple-darwin16.4.0/toolkit/library/x86_64-apple-darwin/release/build/style-7180206542e5ea8b/out/gecko_properties.rs:6132

Here's the function that crashes:

    #[allow(non_snake_case)]
    pub fn set_transition_delay(&mut self, v: longhands::transition_delay::computed_value::T) {
        debug_assert!(!v.0.is_empty());
        let input_len = v.0.len();
        unsafe { self.gecko.mTransitions.ensure_len(input_len) };

        self.gecko.mTransitionDelayCount = input_len as u32;
        for (i, gecko) in self.gecko.mTransitions.iter_mut().enumerate() {
            gecko.mDelay = v.0[i % input_len].seconds() * 1000.;
        }
    }

The modulus of zero line is:

> gecko.mDelay = v.0[i % input_len].seconds() * 1000.;

So presumably the debug_assert! would fire in a debug build.

Comment 1

[Bobby Holley (:bholley)]

Boris, could you or Hiro look at this?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I guess transition property also needs Xidorn's work:
https://github.com/servo/servo/pull/15793#issuecomment-283577709

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Xidorn: ^

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I guess this is because of inconsistency between this and parser.

transition is tricky, because it is kind of different with animation...

Comment 5

[Bobby Holley (:bholley)]

I think xidorn is pretty swamped, so assigning to Boris. Feel free to reassign if this isn't the right fit.

Comment 6

[Boris Chiou [:boris]]

According to Xidorn's hints today, I can use another example to reproduce:

  #target {
    transition: none;        // use `none` property in transition shorthand value.
  }
  <div id='target'></div>

Then stylo panics immediately.

In set_transition_delay(Gecko style struct: GeckoBox, v: T([])):

thread '<unnamed>' panicked at 'attempt to calculate the remainder with a divisor of zero', /Users/boris/projects/firefox/gecko-dev/objdirs/obj-browser-stylo-debug/toolkit/library/x86_64-apple-darwin/debug/build/style-ffe1341867262ac3/out/gecko_properties.rs:6135

This makes me easier to reproduce.

Comment 7

[Boris Chiou [:boris]]

Attachment: Bug 1347053 - Fix shorthand parser for transition none.

We split impl_animation_or_transition_time_value() into two functions,
set_transition_{delay|duration} and set_animation_{delay|duration|...},
and then keep animation functions unchanged. We only handle none transition in
set_transition_{delay|duration}.

The reason is simple: in gecko, nsRuleNode::ComputeDisplayData() also
fills in a none transition with eCSSPropertyExtra_no_properties property,
initial time values, and an initial timing function. Let's keep them
consistent.

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

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1347053 - Part 2: Handle none transition in set_transition_timing_function.

We split impl_animation_or_transition_timing_function() into two functions,
set_transition_timing_function and set_animation_timing_function, and
then do the same things as those of time values.

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

Comment 9

[Boris Chiou [:boris]]

clear review requests because I'd like to try another way.

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8847536 [details]
Bug 1347053 - Fix shorthand parser for transition none.

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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8847536 [details]
Bug 1347053 - Fix shorthand parser for transition none.

https://reviewboard.mozilla.org/r/120520/#review122880

::: servo/components/style/properties/shorthand/box.mako.rs:112
(Diff revision 2)
> +            durations.push(transition_duration::single_value::get_initial_specified_value());
> +            timing_functions.push(transition_timing_function::single_value::get_initial_specified_value());
> +            delays.push(transition_delay::single_value::get_initial_specified_value());

Maybe it can be less redundant to do something like
```rust
% for prop in "duration timing_function delay".split():
${prop}s.push(transition_${prop}::single_value::get_initial_specified_value());
% endfor
```

Same above.

Comment 12

[Boris Chiou [:boris]]

Comment on attachment 8847536 [details]
Bug 1347053 - Fix shorthand parser for transition none.

https://reviewboard.mozilla.org/r/120520/#review122880

> Maybe it can be less redundant to do something like
> ```rust
> % for prop in "duration timing_function delay".split():
> ${prop}s.push(transition_${prop}::single_value::get_initial_specified_value());
> % endfor
> ```
> 
> Same above.

Sounds great! Thanks for the suggestion.

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8847536 [details]
Bug 1347053 - Fix shorthand parser for transition none.

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

Comment 14

[Boris Chiou [:boris]]

Attachment: Servo PR, #15978

Comment 15

[Boris Chiou [:boris]]

merged into m-c.