Closed Bug 585185 Opened 14 years ago Closed 11 years ago

ABORT: negative lengths and percents should be rejected by parser: 'sizeValue->IsCalcUnit()' setting text-indent, negative font size and huge line-height

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: posidron, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [fuzzblocker])

Attachments

(3 files)

Attached file callstack
<html>
<body>
<span style="text-indent:21cm;font:700 -1%/4294967295pt Helvetica Arial monospace">
</body>
</html>
See Also: → 576927
Summary: *((int *) 0) [@TouchBadMemory] → "ABORT: negative lengths and percents should be rejected by parser" setting text-indent, negative font size and huge line-height
QA Contact: general → style-system
Confirmed on a Win 7 PC as well. Setting just a huge line-height (>40000000px works) causes a hang.
Component: DOM: CSS Object Model → Style System (CSS)
Keywords: assertion, testcase
<a style="font: -2px Verdana;"> is sufficient to get 

 ABORT: negative lengths and percents should be rejected by parser: 'sizeValue->IsCalcUnit()', file /work/mozilla/builds/nightly/mozilla/layout/style/nsRuleNode.cpp, line 2559

I think the original abort ABORT: negative lengths and percents should be rejected by parser: 'aFontData.mSize.IsCalcUnit()' has been replaced with this abort.
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: "ABORT: negative lengths and percents should be rejected by parser" setting text-indent, negative font size and huge line-height → ABORT: negative lengths and percents should be rejected by parser: 'sizeValue->IsCalcUnit()' setting text-indent, negative font size and huge line-height
Attached file testcase
ABORT: negative lengths and percents should be rejected by parser: 'sizeValue->IsCalcUnit()'
still reproducible on Beta/11, Aurora/12, Nightly/13 On Linux, Mac, Windows.
Whiteboard: [fuzzblocker]
Attached patch fix v1Splinter Review
Looks like we're inconsistent on whether font-size has to be non-negative or not.

In nsCSSPropList, we say it's non-negative:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h?rev=ae6f8ea61f33&mark=1739-1739#1734

...but in the font shorthand parsing code, we don't currently have that restriction for font-size (we call ParseVariant instead of ParseNonNegativeVariant):
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp?rev=ae6f8ea61f33&mark=8613-8613#8611

I think that last part is the bug here.  Here's a patch to fix it -- let's see how Try likes it:  https://tbpl.mozilla.org/?tree=Try&rev=9e6ab6065aba
Comment on attachment 753123 [details] [diff] [review]
fix v1

Hooray, try likes it!

The code has been this way since forever, which makes me slightly wonder if this change will be web-compatible.  I suppose we can optimistically hope that it will be, for consistency / correctness.

Just as more backup for this being correct -- note that the CSS 2.1 spec does explicitly say "Negative values are not allowed" for the font-size property:
  http://www.w3.org/TR/CSS21/fonts.html#font-size-props
Attachment #753123 - Flags: review?(bzbarsky)
(In reply to Daniel Holbert [:dholbert] from comment #6)
> The code has been this way since forever, which makes me slightly wonder if
> this change will be web-compatible.  I suppose we can optimistically hope
> that it will be, for consistency / correctness.

(...and if we end up finding that it's not web-compatible, it's a trivial enough patch that we can easily back it out in aurora/beta without too much worry)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Comment on attachment 753123 [details] [diff] [review]
fix v1

Good catch.  Thanks for fixing this.  r=dbaron
Attachment #753123 - Flags: review?(bzbarsky) → review+
Thanks!

(I just noticed I'd used slightly wonky indentation in the c++ code for some reason.  I fixed it before landing.)

Pushed:  https://hg.mozilla.org/integration/mozilla-inbound/rev/f857b6882f4b
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/f857b6882f4b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.