508325 - "ABORT: negative scaling factors must be handled manually"

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase (causes abort in debug firefox)

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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jeff Walden [:Waldo]]

Hm; simple to fix, I'm sure, just not sure at a glance how we presumably overflowed the scaling space here.  (Or something.)

Comment 3

[Jeff Walden [:Waldo]]

Garbage in garbage out, somehow the background-size width that's calculated is nscoord(0xc0000000), which when converted to double becomes negative:

>eval (double)aDimension.mCoord
-1073741824.0000000

Into the parser now...

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Clamp fixed-to-twips conversion

Parser doesn't overflow, it's when you hit the inches-to-twips conversion that overflow occurs, in CalcLengthWith in nsRuleNode.cpp and nsCSSValue::GetLengthTwips.  Simple fix in gfx-land for this...

Comment 5

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/8e1a59451aaa

I also added a bunch of reftests (reftests, not crashtests, note that the background doesn't display at all in an opt build before this fix) for this at the same time, covering all possible length units.  (Only the fixed ones should matter, theoretically, but I figure best to cover all the bases.)

Comment 6

[Jeff Walden [:Waldo]]

I had to mark the fixed-unit reftest additions as random, because the first couple tinderbox runs were coming back orange.  On later investigation it becomes clear that this line, in which nsCSSValue::GetLengthTwips is called:

    return aPresContext->TwipsToAppUnits(aValue.GetLengthTwips());

contains more than just the one nscoord multiplication I thought it contained.  The previous patch fixed the one in the first method call above -- but the second one is still hit.  (Never trust an eyeballed patch!)  Of course, testing with that extra change now before posting a patch to fix that and mark the tests as passing again...

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Followup patch (built and tested)

With that change the tests run without aborting again.  Tree just closed to branch, I think, so I can't check this in immediately (not to mention it's a bit late), but I'll try to get it in when the trees reopen -- seems small enough that it's reasonable as a followup fix sans review.

I should write a patch to make nscoord a class (DEBUG-only, perhaps) so that it can't be directly multiplied in this manner sometime...

Comment 8

[Bill Gianopoulos [:WG9s]]

(In reply to comment #7)
> Created an attachment (id=392892) [details]
> Followup patch (built and tested)

With this patch applied, bugzilla does not work.  The comment headers are absent.  I also use SquirrelMail for my webmail access and it does not work at all with this patch applied either.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Hopefully final patch

Okay, tryservered this and it only had one orange on one platform, so I think this might be good to go (ugh).  (My local builds are somewhat useless because a clean build doesn't actually pass reftests for me; I have no idea why.)  I assume we're okay with using templates in nsCoord.h?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This template is too magical IMHO. Can't we just duplicate the code, or use a static inline function with a real boolean parameter and trust the compiler to inline and optimize?

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Hmm, good point

Comment 12

[Bill Gianopoulos [:WG9s]]

(In reply to comment #9)
> Created an attachment (id=393251) [details]
> Hopefully final patch

But, the good news is, both bugzilla and SquirelMail work just fine with this patch.

Comment 13

[Bill Gianopoulos [:WG9s]]

(In reply to comment #11)
> Created an attachment (id=393281) [details]
> Hmm, good point

And Bugzilla and SquirrelMail work fine with this patch as well.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Reftest failures on Windows

Sigh.  The orange on try wasn't intermittent or a lie, I see it if I build locally, with the tests and failures in this reftest log to show for it.  They're not off-by-one failures, either, they're huge -- which doesn't make a whole of sense, but there you have it.  :-\

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think it's well past time to change this from NS_ABORT_IF_FALSE to NS_ASSERTION.

Comment 16

[Jeff Walden [:Waldo]]

Sigh.  Hacked to an assertion in branch and trunk.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Huge thanks to bz for help debugging this

The problem, after much hand-wringing and debugging through unit-conversion mess as well as into nsColumnSetFrame::Reflow (where we determined the large difference in rendering is from an intentional, desirable, visibly-massive truncation due to floor(23999tw / 8000tw) != floor(24000tw / 8000tw)), is that the PRInt32 cast of PR_MAX(nscoord_MIN, aCoord*aScale)) (mutatis mutandis for the positive possibility) *truncates* rather than rounding to nearest.  Performing a round-with-clamp nicely addresses the problem.

A full reftest run successfully passes with this patch without aborts.

Huge thanks to bz for helping me with this; I never considered the possibility that cast semantics weren't correct here.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 397322 [details] [diff] [review]
Huge thanks to bz for help debugging this

+                                          bool requireNotNegative) {

PRBool

+  return _nscoordSaturatingMultiply(aCoord, aScale, true);

PR_TRUE

+  return _nscoordSaturatingMultiply(aCoord, aScale, false);

PR_FALSE

Comment 19

[Jeff Walden [:Waldo]]

With the bool changes (shades of argument yesterday about modernization, wink wink, nudge nudge):

http://hg.mozilla.org/mozilla-central/rev/eb19d94d35ef

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm all for modernization, but it should be based on explicit decisions and updates to the style guide, not by people seeing what they can get past reviewers of individual patches.

Comment 21

[Jeff Walden [:Waldo]]

Until the discussion to which I referred, I wasn't aware that not everyone thought bool was kosher in non-XPCOM usage.