516512 - "ABORT: negative scaling factors must be handled manually" take two

Status: RESOLVED WORKSFORME

(Core :: Layout: Text and Fonts, defect)

Description

[Jesse Ruderman]

Attachment: testcase (causes abort in debug firefox)

###!!! ABORT: negative scaling factors must be handled manually: 'aScale >= 0.0f', file ../../dist/include/nsCoord.h, line 127

(Bug 508325 fixed this abort for another testcase.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

There's a non-saturating scale with a bad value of the browser.display.base_font_scaler preference, but since from what I can tell the preference only existed for NS4 backwards-compat (and under a different name completely!) I don't care.  That'll die with bug 516547, and it's not important enough for anyone with better things to do to fix it.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 400650 [details] [diff] [review]
Patch

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp

>@@ -2573,7 +2573,7 @@ nsRuleNode::SetFontSize(nsPresContext* a
>       if (NS_STYLE_FONT_SIZE_LARGER == value) {
>         *aSize = nsStyleUtil::FindNextLargerFontSize(parentSize,
>                          baseSize, scaleFactor, aPresContext, eFontSize_CSS);
>-        NS_ASSERTION(*aSize > parentSize,
>+        NS_ASSERTION(*aSize >= parentSize,
>                      "FindNextLargerFontSize failed");

Scaling up nscoord_MAX doesn't produce a scaled size that's bigger, hit by font-size-monster-2.html.

>@@ -2602,8 +2602,9 @@ nsRuleNode::SetFontSize(nsPresContext* a
>     // Note that % units use the parent's size unadjusted for scriptlevel
>     // changes. A scriptlevel change between us and the parent is simply
>     // ignored.
>-    *aSize = NSToCoordRound(aParentSize *
>-                            aFontData.mSize.GetPercentValue());
>+    *aSize =
>+        NSCoordSaturatingNonnegativeMultiply(aParentSize,
>+                                             aFontData.mSize.GetPercentValue());

This is required by font-size-monster-1.html, that is, the original testcase here.


>diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp

> nsStyleFont::ZoomText(nsPresContext *aPresContext, nscoord aSize)
> {
>-  return nscoord(float(aSize) * aPresContext->TextZoom());
>+  return NSCoordSaturatingNonnegativeMultiply(aSize, aPresContext->TextZoom());
> }

This is required by font-size-monster-3.html.


> nsStyleFont::UnZoomText(nsPresContext *aPresContext, nscoord aSize)
> {
>-  return nscoord(float(aSize) / aPresContext->TextZoom());
>+  return NSCoordSaturatingDivide(aSize, aPresContext->TextZoom());
> }

This is required by font-size-monster-4.html.


>diff --git a/layout/style/nsStyleUtil.cpp b/layout/style/nsStyleUtil.cpp
>--- a/layout/style/nsStyleUtil.cpp
>+++ b/layout/style/nsStyleUtil.cpp
>@@ -366,7 +366,7 @@ nscoord nsStyleUtil::FindNextLargerFontS
>       largerSize = indexFontSize + NSToCoordRound(relativePosition * (largerIndexFontSize - indexFontSize));      
>     }
>     else {  // larger than HTML table, increase by 50%
>-      largerSize = NSToCoordRound(float(aFontSize) * 1.5);
>+      largerSize = NSCoordSaturatingNonnegativeMultiply(aFontSize, 1.5);
>     }
>   }
>   else { // smaller than HTML table, increase by 1px

This is required by font-size-monster-2.html.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Missed a file last time

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here but....  NSCoordSaturatingDivide no longer seems to exist.  And I'm confused by the code there and in _nscoordSaturatingMultiply (which does exist): why do we floor in the float case but round to nearest in the nscoord case?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 400652 [details] [diff] [review]
Missed a file last time

r- based at least on the differential rounding behavior.

Comment 6

[Jesse Ruderman]

WFM

Comment 7

[Jeff Walden [:Waldo]]

I think I saw the changeset that fixed this go past in pushlog semi-recently-ish, with basically or identically the change here.  Need to check up on that and get the tests in in any case...

Comment 8

[Ryan VanderMeulen [:RyanVM]]

No longer happens on trunk.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51df8da6f1e4

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51df8da6f1e4