Closed Bug 508325 Opened 15 years ago Closed 15 years ago

"ABORT: negative scaling factors must be handled manually"

Categories

(Core :: Graphics, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: jruderman, Assigned: Waldo)

References

Details

(Keywords: assertion, testcase)

Attachments

(6 files, 2 obsolete files)

###!!! ABORT: negative scaling factors must be handled manually: 'aVal >= 0.0f', file ../../dist/include/nsCoord.h, line 101
Attached file stack trace (deleted) —
Hm; simple to fix, I'm sure, just not sure at a glance how we presumably overflowed the scaling space here. (Or something.)
Status: NEW → ASSIGNED
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...
Attached patch Clamp fixed-to-twips conversion (deleted) — Splinter Review
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...
Assignee: nobody → jwalden+bmo
Attachment #392584 - Flags: review?(roc)
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.)
Component: Style System (CSS) → GFX: Thebes
Flags: in-testsuite+
OS: Mac OS X → All
QA Contact: style-system → thebes
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch Hopefully final patch (obsolete) (deleted) — Splinter Review
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?
Attachment #393251 - Flags: review?(roc)
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?
Attached patch Hmm, good point (obsolete) (deleted) — Splinter Review
Attachment #393251 - Attachment is obsolete: true
Attachment #393281 - Flags: review?(roc)
Attachment #393251 - Flags: review?(roc)
(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.
Blocks: 372581
Attached file Reftest failures on Windows (deleted) —
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.
No longer blocks: 372581
Depends on: 372581
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.
Attachment #393281 - Attachment is obsolete: true
Attachment #397322 - Flags: review?(roc)
Attachment #393281 - Flags: review?(roc)
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
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: