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

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

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...

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...

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.)

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...

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...

(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.

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?

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?

(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.

(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.

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. :-\

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

Sigh. Hacked to an assertion in branch and trunk.

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 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

With the bool changes (shades of argument yesterday about modernization, wink wink, nudge nudge): http://hg.mozilla.org/mozilla-central/rev/eb19d94d35ef

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.

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