419746 - SVG crash in gfxASurface::SetDeviceOffset() via nsSVGMaskFrame::ComputeMaskAlpha()

Status: VERIFIED FIXED

(Core :: SVG, defect, P2)

Description

[Justin Dolske [:Dolske]]

Attachment: SVG Testcase

FF crashes with http://people.freedesktop.org/~lapo/g-i-t/face-mess.svg

Gecko/2008022604 Minefield/3.0b4pre, OS X 10.5

Breakpad report 3384ec4a-e4cb-11dc-8088-001a4bd43ed6

Signature  	Source
0 	gfxASurface::SetDeviceOffset(gfxPoint const&) 	mozilla/gfx/thebes/src/gfxASurface.cpp:220
1 	nsSVGMaskFrame::ComputeMaskAlpha(nsSVGRenderState*, nsISVGChildFrame*, nsIDOMSVGMatrix*, float) 	mozilla/layout/svg/base/src/nsSVGMaskFrame.cpp:173
2 	nsSVGUtils::PaintChildWithEffects(nsSVGRenderState*, nsRect*, nsIFrame*) 	mozilla/layout/svg/base/src/nsSVGUtils.cpp:1348
3 	nsSVGDisplayContainerFrame::PaintSVG(nsSVGRenderState*, nsRect*) 	mozilla/layout/svg/base/src/nsSVGContainerFrame.cpp:184
4 	nsSVGUtils::PaintChildWithEffects(nsSVGRenderState*, nsRect*, nsIFrame*) 	mozilla/layout/svg/base/src/nsSVGUtils.cpp:1333
5 	nsSVGOuterSVGFrame::Paint(nsIRenderingContext&, nsRect const&, nsPoint) 	mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:584
6 	nsDisplaySVG::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) 	mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:464
7 	nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) const 	mozilla/layout/base/nsDisplayList.cpp:294
8 	nsDisplayClip::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) 	mozilla/layout/base/nsDisplayList.cpp:883
9 	nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) const 	mozilla/layout/base/nsDisplayList.cpp:294
10 	nsLayoutUtils::PaintFrame(nsIRenderingContext*, nsIFrame*, nsRegion const&, unsigned int) 	mozilla/layout/base/nsLayoutUtils.cpp:872
11 	PresShell::Paint(nsIView*, nsIRenderingContext*, nsRegion const&)

Comment 1

[Jesse Ruderman]

Attachment: simpler testcase

Comment 2

[Robert Longson [:longsonr]]

Doesn't crash on Windows. Anyone want to try for a regression range?

Comment 3

[Justin Dolske [:Dolske]]

(In reply to comment #0)

> Breakpad report 3384ec4a-e4cb-11dc-8088-001a4bd43ed6

With magic linkification (I hope): bp-3384ec4a-e4cb-11dc-8088-001a4bd43ed6

Comment 4

[Jonathan Watt [:jwatt]]

So it looks like we pass in |gfxPoint(0,0)| to gfxASurface::SetDeviceOffset then crash accessing the members of the gfxPoint? That would suggest something has screwed with the stack.

I don't have a Mac, so CC'ing Vlad and setting blocking1.9?. Any thoughts Vlad?

Comment 5

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

No big mystery here -- the pattern that's returned from PopGroup is in error (CAIRO_STATUS_NO_MEMORY, which is generally a generic not very useful error), and nothing is checking pattern->CairoStatus() to make sure it's == 0 (success).  pattern->GetSurface() is returning NULL, and we end up calling setDeviceOffset with a NULL surface pointer.

Comment 6

[Robert Longson [:longsonr]]

Attachment: possible patch

Can anybody confirm that this fixes it on a Mac?

All I can say is that masks still work on Windows with this change.

Comment 7

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

Comment on attachment 308576 [details] [diff] [review]
possible patch

Should check pattern->CairoStatus() as well, before even calling GetSurface() on it.

Comment 8

[Robert Longson [:longsonr]]

CairoStatus is a member of gfxASurface. It is not a member of gfxPattern so a call to pattern->CairoStatus() will not compile.

Comment 9

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

http://mxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxPattern.h#90 ?

Comment 10

[Robert Longson [:longsonr]]

Attachment: address review comment

(In reply to comment #9)
> http://mxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxPattern.h#90 ?
> 

You are right of course.

Comment 11

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

Comment on attachment 309599 [details] [diff] [review]
address review comment

Looks good

Comment 12

[Jonathan Watt [:jwatt]]

We should definitely take this, so marking blocking1.9+. Please check it in.

Besides these safety checks it would be good to know why the pattern is going into error, and whether we should be doing some earlier checks of the input to cairo. That can be post 1.9, but perhaps file a follow up bug for investigation if you think that's useful?

Comment 13

[Robert Longson [:longsonr]]

I can't investigate this, it only happens on macs and I don't have one.

Comment 14

[Jonathan Watt [:jwatt]]

I didn't mean that you should personally do the investigating, just that we should have a bug on record for it to happen (assuming you didn't know why the pattern is going into error and feel that it's okay for us to allow that). I'll file a bug shortly.

Comment 15

[Jonathan Watt [:jwatt]]

Bug 423403.

Comment 16

[Robert Longson [:longsonr]]

My guess is that the pattern is going into error because it is too big for a Mac. Macs seem to either require more memory for patterns or have a lower memory limit; bug 385228 was looking into this.

Comment 17

[Robert Longson [:longsonr]]

(In reply to comment #9)
> http://mxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxPattern.h#90 ?
> 

I just worked it out that you added that while I wasn't looking ;-)

Comment 18

[Robert Longson [:longsonr]]

checked in earlier today.

Comment 19

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040920 Firefox/3.0pre

When i surf to the url from comment#0 i see the Assertion from Bug 427325, but no crash

--> Verified fixed