Closed
Bug 1383492
Opened 7 years ago
Closed 7 years ago
stylo: panicked at 'attempt to add with overflow'
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
People
(Reporter: truber, Assigned: manishearth)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(4 files)
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?
Comment 1•7 years ago
|
||
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...
Comment 2•7 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P1
Assignee | ||
Comment 3•7 years ago
|
||
Gecko seems to do saturating add in calc ops. We should do the same.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efb2f7fbef90727629142715ef23e1b606e760a9
Comment 8•7 years ago
|
||
mozreview-review |
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 9•7 years ago
|
||
mozreview-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+
Updated•7 years ago
|
Attachment #8890080 -
Flags: review?(dbaron)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → manishearth
Assignee | ||
Comment 13•7 years ago
|
||
Try passes except for one assertion going away (marked as expected)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
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+
Comment 18•7 years ago
|
||
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
Backing these and the two followups out for continued Windows build bustage and crashtest assertions like https://treeherder.mozilla.org/logviewer.html#?job_id=118275761&repo=autoland https://treeherder.mozilla.org/logviewer.html#?job_id=118286406&repo=autoland https://hg.mozilla.org/integration/autoland/rev/7f5d8fe1c87062853ed44b9ede27ad24c03e5812
Flags: needinfo?(manishearth)
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f89500a307f6 https://hg.mozilla.org/mozilla-central/rev/7fbc3c5786d1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Updated•7 years ago
|
Flags: needinfo?(manishearth)
You need to log in
before you can comment on or make changes to this bug.
Description
•