385228 - "ASSERTION: Surface size too large (would overflow)" and huge malloc

Status: REOPENED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase from bug 381804

The testcase triggers:

###!!! ASSERTION: Surface size too large (would overflow)!: 'Error', file /Users/jruderman/trunk/mozilla/gfx/thebes/src/gfxASurface.cpp, line 291

firefox-bin(14100,0xa000cfe0) malloc: *** vm_allocate(size=2085036032) failed (error code=3)
firefox-bin(14100,0xa000cfe0) malloc: *** error: can't allocate region
firefox-bin(14100,0xa000cfe0) malloc: *** set a breakpoint in szone_error to debug

Comment 1

[Robert Longson [:longsonr]]

Attachment: change NS_ERROR to NS_WARNING

Comment 2

[Robert Longson [:longsonr]]

No malloc error on Windows.

So I think there are two issues here:

a) gfxASurface::CheckSurfaceSize has some NS_ERROR calls. I think these should either be removed or changed to NS_WARNING.

b) you get a malloc error on a mac.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I don't understand -- why should these be NS_WARNINGs?  Nothing should be giving thebes too-big surface numbers.

Comment 4

[Robert Longson [:longsonr]]

You can crete arbitrary surface sizes in SVG e.g. using filters you can specify the x and y size of the surface directly with the filterRes attributes. I used this function to validate the size of svg surfaces in nsSVGUtils::ConvertToSurfaceSize.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 269136 [details] [diff] [review]
change NS_ERROR to NS_WARNING

Ah ok, fair enough.. I guess I was thinking of this more as a last-ditch sanity check and not an early check, but you'd want to do the same check in both places anyway.  We may want to just get rid of the NS_WARNINGs in that case at some point, but this is fine for now, then.

Comment 6

[Robert Longson [:longsonr]]

The assertion is now gone. Presumably the Mac allocate issue remains which I can't do anything about.

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #5)
> (From update of attachment 269136 [details] [diff] [review])
> Ah ok, fair enough.. I guess I was thinking of this more as a last-ditch sanity
> check and not an early check, but you'd want to do the same check in both
> places anyway.  We may want to just get rid of the NS_WARNINGs in that case at
> some point, but this is fine for now, then.
> 

I could produce another patch to make CheckSurfaceSize take a PRBool suppressErrors argument, possibly defaulting to PR_FALSE. With that I could change the warnings back to asserts.

Comment 8

[Brian Ewins]

Robert: over in bug 374815, comment 13, I posted some stack trace info for another large alloc, and you've asked if this one is related. It does hit the same stack, and there's similar symptoms: the callerBBox in my trace for the testcase above is 130x275, but the bbox ends up as 2600x275000 (20x and 1000x the callerBBox dimensions, obviously; which are the pattern's width and height in the test case).

This is at nsSVGPatternFrame.cpp:320. It looks like something is screwy with the pattern units. It seems to think that pattern width="x" height="y" means that the pattern is x times the width of the callerBBox and y times its height??

Comment 9

[Brian Ewins]

sorry I meant bug 374815 comment 14, obviously. Looks like both of these are hitting the same bug, other things like crashes and the size-clamping warnings are the result.

Comment 10

[Brian Ewins]

Ok, I read the corresponding bits of the spec after this, I didn't realise you were deliberately creating massive patterns here, since otherwise the behaviour is correct: patternUnits defaults to objectBoundingBox, in this case the pattern is 1000x the height of the element to which the pattern is applied, which is always going to result in an enormous bitmap.

However, if the units are objectBoundingBox and the width, height are > 1, you're always going to get a pattern bigger than the element it's being applied to. I don't see why you have to allocate something that large - can't a smaller surface be created with an appropriate offset? The size could be clamped like this somewhere like nsSVGPatternFrame.cpp line 276.

I'll shut up now as this is clearly not a cairo bug and I've strayed into code I understand even less...

Comment 11

[Robert Longson [:longsonr]]

Brian,

What happens is that the testcase deliberately generates a large surface (so far so good).

The SVG code then clamps the surface size using nsSVGUtils::ConvertToSurfaceSize
to a maximum of 16384 x 16384. So the surface size passed to cairo in the case above should now be 2600x16384.

On non-mac platforms we can create a surface this size without problems, on Mac, even this surface size blows up with a malloc error. Why does Mac cairo do that?

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

2,085,036,032 isn't 2600 * 16384 * 4; even 16384 * 16384 * 4 is only 1,073,741,824 .  Can we get a backtrace of exactly where that big number is coming from?

Comment 13

[Brian Ewins]

(In reply to comment #12)
> 2,085,036,032 isn't 2600 * 16384 * 4; even 16384 * 16384 * 4 is only
> 1,073,741,824 .  Can we get a backtrace of exactly where that big number is
> coming from?
> 

Inside CG as far as I can tell. This bug doesn't actually crash for me, but with the similar bug in 374815, I've got that failing right now with:

firefox-bin(17896,0xa079df60) malloc: *** mmap(size=1488482304) failed (error code=12)

another mystery number, > 16384^2. tracing from the malloc back to cairo I have:

#4  0x958c7f15 in malloc_zone_calloc ()
#5  0x909e29b6 in ripl_Create ()
#6  0x909ecb72 in ripc_GetColor ()
#7  0x909cd832 in ripc_Render ()
#8  0x909d68a0 in ripc_DrawPath ()
#9  0x928c0687 in CGContextDrawPath ()
#10 0x928c05d4 in CGContextFillPath ()
#11 0x1176c6de in _cairo_quartz_surface_fill (abstract_surface=0x18bdea30, op=CAIRO_OPERATOR_OVER, source=0xbfffba98, path=0xad717c, fill_rule=CAIRO_FILL_RULE_WINDING, tolerance=0.10000000000000001, antialias=CAIRO_ANTIALIAS_DEFAULT) at /Users/brianewins/projects/ffox/mozilla/gfx/cairo/cairo/src/cairo-quartz-surface.c:1273

I traced the creation of the large pattern that was used in this fill:

#0  0x1176db8c in _cairo_quartz_surface_create_internal (cgContext=0x18bdf470, content=CAIRO_CONTENT_COLOR_ALPHA, width=53160, height=3500) at /Users/brianewins/projects/ffox/mozilla/gfx/cairo/cairo/src/cairo-quartz-surface.c:1737

the bitmap size here is:
53160 * 3500 * 4 = 744,240,000
2 * 744,240,000 = 1,488,480,000

and the smallest alloc that can hold that is:
363399 * 4096 = 1,488,482,304 ... the mystery number.

So CG is allocating twice the pattern size for some reason. 

Comment 14

[Robert Longson [:longsonr]]

How did we get a width > 16384? Does SVG send that to cairo on Mac? If so why is it not clamped by nsSVGUtils::ConvertToSurfaceSize? 

Comment 15

[Brian Ewins]

(In reply to comment #14)
> How did we get a width > 16384? Does SVG send that to cairo on Mac? If so why
> is it not clamped by nsSVGUtils::ConvertToSurfaceSize? 
> 

As far as I can tell, it doesn't work that way on any platform; it clamps when w x h x 4 overflows a PRInt32, except on mac where height is also clamped to 32k (as it is in this bug). If clamping dimensions to 16k is intended, shouldn't nsSVGUtils.cpp#1352 read: 

if (*aResultOverflows ||
       !gfxASurface::CheckSurfaceSize(surfaceSize, NS_SVG_OFFSCREEN_MAX_DIMENSION)) {

I'm not able to debug this on other platforms, but it looks to me like on windows at least, its not doing clamp-and-scale for similarly large patterns, its just not rendering the pattern at all; eg I put a 100%x100% fill=blue rect in the pattern in attachment#1 [details] [diff] [review]; changing the pattern to 250x115 renders no stroke, and at 100x500 it only renders the stroke when zoomed out 4x (and not any other zoom level). That's in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120915 Minefield/3.0b2pre.

Comment 16

[Robert Longson [:longsonr]]

Yes, you are right Brian. I forgot that was how it was supposed to work.

Comment 17

[Jesse Ruderman]

Attachment: testcase 2

This one ends up allocating about 1.5GB at a time.  This doesn't trigger the NS_WARNING but does trigger the malloc warning.

Comment 18

[Robert Longson [:longsonr]]

You've got a rect width 440, a pattern in objectBoundingBox units (the default) and a viewBox which contributes an approximate x2 factor. The pattern is 20 x 440 x 2 userSpace units wide, so it's a big pattern.

It looks like the Mac OS allocates more memory than other operating systems. Maybe CheckSurfaceSize needs to be even more restrictive on that platform.

Comment 19

[Jesse Ruderman]

I'm not getting assertions or malloc errors any more with these testcases.

Comment 20

[Robert Longson [:longsonr]]

Closing based on comment 19

Comment 21

[Jesse Ruderman]

Crashtest: http://hg.mozilla.org/mozilla-central/rev/43339764467b

Comment 22

[Daniel Holbert [:dholbert]]

A mac tinderbox just failed with a huge (634-megabyte) malloc, in between this bug's crashtests.  I initially filed Bug 523255 on the sporadic failure.  Crash log is:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255992265.1255994821.16757.gz

On the bright side, this shows the value of checking in crashtests! :)  But, on the sad side, this bug is still apparently broken. :(  Reopening.

Comment 24

[Daniel Holbert [:dholbert]]

It sounds like comment 13 says we could *legitimately* need up to 744,240,000 bytes for this bug's testcase -- is that true?  If so, maybe we *are* doing the "right thing" now, and allocating "only" ~700 megs instead of 1.5 gigs like we used to.

Comment 25

[Daniel Holbert [:dholbert]]

I've disabled this test's second crashtest for now, since it aborts the whole crashtest suite when it sporadically fails, and that's bad.

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

I guess subsequent action here depends on the answer to comment 24.  If the answer is "yes, this test really needs 700 megs", then we should need to shrink this unittest so that it's not quite so demanding.  I don't think it's reasonable to expect that our tinderboxen (or, for that matter, "Joe Developer" running crashtests locally) will have a contiguous 700-meg block of virtual memory available at any given time.

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to comment #25)
> I've disabled this test's second crashtest for now
Er, this *bug*'s second crashtest -- 385228-2.svg.  (That appears to be what was loading when we died, in the log in comment 23 & quoted in Bug 523255 comment 0.)