stylo: thread '<unnamed>' panicked at 'attempt to multiply with overflow', servo/components/style/gecko/media_queries.rs:732:17

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: bc, Assigned: bradwerth)

Tracking

(Blocks: 1 bug, {regression})

57 Branch
mozilla57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ fixed)

Details

(URL)

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 months ago
Created attachment 8908218 [details]
Windows Debug Log

[Tracking Requested - why for this release]: stylo debug panic

1. https://chitranj.standardchess.com/play.html
2. thread '<unnamed>' panicked at 'attempt to multiply with overflow', /mozilla/builds/nightly/mozilla/servo/components/style/gecko/media_queries.rs:732:17

Nightly 57 Windows and Linux.
(Assignee)

Comment 1

7 months ago
The big numbers in the integer ratio media queries are causing a multiplication overflow in our optimized ratio comparison. I'll change it to attempt the multiplication with overflow detection on, and if overflow occurs, instead do a slower test that calculates the ratios and compares them directly.
Assignee: nobody → bwerth
Thanks Brad!
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

7 months ago
(In reply to Brad Werth [:bradwerth] from comment #1)
> The big numbers in the integer ratio media queries are causing a
> multiplication overflow in our optimized ratio comparison. I'll change it to
> attempt the multiplication with overflow detection on, and if overflow
> occurs, instead do a slower test that calculates the ratios and compares
> them directly.

I worked up a solution that did this (checking for div by zero, etc.) and it was super-ugly Rust. Then I realized that the u32 values could be cast to u64s, making the existing multiplication safe. Attachment 8908365 [details] follows this approach. Also created some tests.
(Assignee)

Updated

7 months ago
Attachment #8908365 - Flags: review?(emilio)
Attachment #8908366 - Flags: review?(emilio)

Comment 7

7 months ago
mozreview-review
Comment on attachment 8908365 [details]
Bug 1399941 Part 1: Prevent aspect-ratio media queries from causing multiplication overflow by extending values to u64.

https://reviewboard.mozilla.org/r/179986/#review185226

r=me with the nits addressed.

::: servo/components/style/gecko/media_queries.rs:734
(Diff revision 1)
>              (&BoolInteger(one), &BoolInteger(ref other)) => one.cmp(other),
>              (&Float(one), &Float(ref other)) => one.partial_cmp(other).unwrap(),
>              (&IntRatio(one_num, one_den), &IntRatio(other_num, other_den)) => {
> -                (one_num * other_den).partial_cmp(&(other_num * one_den)).unwrap()
> +                // Large u32s may overflow in multiplication, so we up them
> +                // to u64s. This is safe because u32 max ^ 2 < u64 max.
> +                debug_assert!((<u32>::max_value() as u64) ^ 2 < <u64>::max_value());

There's no need to waste cycles in debug builds to know this is true, please remove.

A comment saying something like:

```
// Extend to avoid overflow.
```

May be enough.

::: servo/components/style/gecko/media_queries.rs:735
(Diff revision 1)
>              (&Float(one), &Float(ref other)) => one.partial_cmp(other).unwrap(),
>              (&IntRatio(one_num, one_den), &IntRatio(other_num, other_den)) => {
> -                (one_num * other_den).partial_cmp(&(other_num * one_den)).unwrap()
> +                // Large u32s may overflow in multiplication, so we up them
> +                // to u64s. This is safe because u32 max ^ 2 < u64 max.
> +                debug_assert!((<u32>::max_value() as u64) ^ 2 < <u64>::max_value());
> +                (one_num as u64 * other_den as u64).partial_cmp(

While you're here, looks weird that this is using `partial_cmp(..).unwrap()`

This can definitely use:

(one_num as u64 * other_den as u64).cmp(&(other_num as u64 * one_den as u64))
Attachment #8908365 - Flags: review?(emilio) → review+

Comment 8

7 months ago
mozreview-review
Comment on attachment 8908366 [details]
Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.

https://reviewboard.mozilla.org/r/179988/#review185228

These are unnecessary. They're testing servo code, to begin with (because that's what test-unit tests with), and Servo and Gecko code for media-query parsing and evaluation is radically different.

Also, servo doesn't support aspect-ratio media queries, so you're effectively just testing the behavior of parsing an invalid MediaList.

Please add either a crashtest in Gecko, or a test to a style test like `test_media_queries.html`.
Attachment #8908366 - Flags: review?(emilio) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

7 months ago
mozreview-review
Comment on attachment 8908366 [details]
Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.

https://reviewboard.mozilla.org/r/179988/#review185626
Attachment #8908366 - Flags: review?(emilio) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8908365 - Attachment is obsolete: true
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
tracking-firefox57: ? → +
Brad, did this land?
Flags: needinfo?(bwerth)
(Assignee)

Comment 15

7 months ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #14)
> Brad, did this land?

Servo fix has landed; Gecko tests have not landed yet.
Flags: needinfo?(bwerth)

Comment 16

7 months ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c89b96997d04
Part 2: Add more aspect-ratios to a test of media queries, testing for overflow. r=emilio

Comment 17

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c89b96997d04
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.