Closed Bug 1383492 Opened 2 years ago Closed 2 years ago

stylo: panicked at 'attempt to add with overflow'

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: truber, Assigned: manishearth)

References

(Blocks 3 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file testcase.html
The attached testcase causes a panic in m-c rev c22502562670 with stylo enabled by pref.

thread '<unnamed>' panicked at 'attempt to add with overflow', /home/worker/workspace/build/src/third_party/rust/app_units/src/app_unit.rs:84
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::_print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::begin_panic
   6: std::panicking::begin_panic_fmt
   7: rust_begin_unwind
   8: core::panicking::panic_fmt
   9: core::panicking::panic
  10: <app_units::app_unit::Au as core::ops::Add>::add
  11: <app_units::app_unit::Au as core::ops::AddAssign>::add_assign
  12: style::values::computed::length::<impl style::values::computed::ToComputedValue for style::values::specified::calc::CalcLengthOrPercentage>::to_computed_value
  13: style::values::computed::length::<impl style::values::computed::ToComputedValue for style::values::specified::length::Length>::to_computed_value
  14: style::properties::longhands::_webkit_text_stroke_width::cascade_property
  15: style::properties::apply_declarations
  16: style::properties::cascade
  17: <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_style
  18: <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::resolve_primary_style
  19: <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::resolve_style
  20: style::traversal::resolve_style
  21: Servo_ResolveStyleLazily
Flags: in-testsuite?
This will be a crash in debug builds, but will work fine in release afaict... I don't think this overflow is problematic, but I'm not sure we should change all our additions to the special rust method to add without overflow...
If web content can trigger a panic in debug builds we need to fix it, otherwise the fuzzers won't be able to do their job.

In general, my guess is that the set of integer overflows that can be reliably triggered is pretty small, and we should audit/fix them when we find them.

IIUC, this crash is triggering the additions in [1]. What does the spec say to do there?

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/servo/components/style/values/computed/length.rs#231
Priority: -- → P1
Gecko seems to do saturating add in calc ops. We should do the same.
Comment on attachment 8890078 [details]
Bug 1383492: stylo: Bump app units version;

https://reviewboard.mozilla.org/r/161148/#review166486

::: servo/components/style/values/computed/length.rs:236
(Diff revision 1)
>  impl ToComputedValue for specified::CalcLengthOrPercentage {
>      type ComputedValue = CalcLengthOrPercentage;
>  
>      fn to_computed_value(&self, context: &Context) -> CalcLengthOrPercentage {
>          let mut length = Au(0);
> +        println!("l {:?} {:?} {:?}", length, ::app_units::MAX_AU, ::app_units::MIN_AU);

Remove all the printfs please :)
Attachment #8890078 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8890080 [details]
Bug 1383492: stylo: Change nscoord_MAX to 1<<30 - 1 ;

https://reviewboard.mozilla.org/r/161152/#review166512

This needs a try run, and I'd rather get the signoff from dbaron too (though this was discussed with bz and he was ok with this, so it may be ok).
Attachment #8890080 - Flags: review?(emilio+bugs) → review+
Attachment #8890080 - Flags: review?(dbaron)
Assignee: nobody → manishearth
Try passes except for one assertion going away (marked as expected)
Looks like this causes some new assertions in the crashtests.

They seem to just be uncovering existing bugs since the saturated value is now different.
Comment on attachment 8890080 [details]
Bug 1383492: stylo: Change nscoord_MAX to 1<<30 - 1 ;

https://reviewboard.mozilla.org/r/161152/#review166594
Attachment #8890080 - Flags: review?(dbaron) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b4dfcf4f795e
stylo: Bump app units version; r=emilio
https://hg.mozilla.org/integration/autoland/rev/11cadad891a4
stylo: Change nscoord_MAX to 1<<30 - 1 ; r=emilio,dbaron
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f89500a307f6
stylo: Bump app units version; r=emilio
https://hg.mozilla.org/integration/autoland/rev/7fbc3c5786d1
stylo: Change nscoord_MAX to 1<<30 - 1 ; r=emilio,dbaron
https://hg.mozilla.org/mozilla-central/rev/f89500a307f6
https://hg.mozilla.org/mozilla-central/rev/7fbc3c5786d1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(manishearth)
Depends on: 1396117
You need to log in before you can comment on or make changes to this bug.