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

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: bholley, Assigned: boris)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

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.
Boris, could you or Hiro look at this?
Flags: needinfo?(boris.chiou)
I guess transition property also needs Xidorn's work:
https://github.com/servo/servo/pull/15793#issuecomment-283577709
Flags: needinfo?(boris.chiou)
Xidorn: ^
Flags: needinfo?(xidorn+moz)
I guess this is because of inconsistency between this and parser.

transition is tricky, because it is kind of different with animation...
I think xidorn is pretty swamped, so assigning to Boris. Feel free to reassign if this isn't the right fit.
Assignee: nobody → boris.chiou
Flags: needinfo?(xidorn+moz)
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.
Blocks: 1302946
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Status: NEW → ASSIGNED
Attachment #8847536 - Flags: review?(cam)
Attachment #8847537 - Flags: review?(cam)
clear review requests because I'd like to try another way.
Comment hidden (mozreview-request)
Attachment #8847537 - Attachment is obsolete: true
Attachment #8847536 - Flags: review?(xidorn+moz)

Comment 11

3 months ago
mozreview-review
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.
Attachment #8847536 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 12

3 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Created attachment 8848007 [details] [review]
Servo PR, #15978
merged into m-c.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.