If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

"ABORT: negative scaling factors must be handled manually"

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla1.9.2a1
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 392538 [details]
testcase (causes abort in debug firefox)

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

Comment 1

8 years ago
Created attachment 392539 [details]
stack trace
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...
Created attachment 392584 [details] [diff] [review]
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...
Assignee: nobody → jwalden+bmo
Attachment #392584 - Flags: review?(roc)
Attachment #392584 - Flags: review?(roc) → review+
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
Last Resolved: 8 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 → ---
Created attachment 392892 [details] [diff] [review]
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...
(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.
Created attachment 393251 [details] [diff] [review]
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?
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?
Created attachment 393281 [details] [diff] [review]
Hmm, good point
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.
(Reporter)

Updated

8 years ago
Blocks: 372581
Created attachment 393671 [details]
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.  :-\
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
Created attachment 397322 [details] [diff] [review]
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.
Attachment #393281 - Attachment is obsolete: true
Attachment #397322 - Flags: review?(roc)
Attachment #393281 - Flags: review?(roc)
Attachment #397322 - Flags: review?(roc) → review+
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
Last Resolved: 8 years ago8 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.