Closed Bug 1399941 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed

People

(Reporter: bc, Assigned: bradwerth)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Windows Debug Log
122.31 KB, text/plain
Details
Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Attached file 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.
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
(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.
Attachment #8908365 - Flags: review?(emilio)
Attachment #8908366 - Flags: review?(emilio)
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 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 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+
Attachment #8908365 - Attachment is obsolete: true
Brad, did this land?
Flags: needinfo?(bwerth)
(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)
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
https://hg.mozilla.org/mozilla-central/rev/c89b96997d04
Status: NEW → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: