Last Comment Bug 836173 - Leak involving Cairo under nsSVGGlyphFrame::SetupInheritablePaint
: Leak involving Cairo under nsSVGGlyphFrame::SetupInheritablePaint
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla21
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-29 19:57 PST by Nicholas Nethercote [:njn]
Modified: 2013-02-06 03:41 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
DMD output (9.98 KB, text/plain)
2013-01-29 19:57 PST, Nicholas Nethercote [:njn]
no flags Details
Likely fix. (1.51 KB, patch)
2013-01-29 23:48 PST, Johnny Stenback (:jst, jst@mozilla.com)
jwatt: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2013-01-29 19:57:42 PST
Created attachment 707974 [details]
DMD output

jst used DMD on a long-running session.  It crashed while producing the output (see bug 836054) but spat out the top three unreported records before doing so, which together accounted for 12% of his heap.  The full output is attached.

The first and third record accounted for 66 MB and looked like this:

>   replace_malloc (/home/jst/fast/work/tip/mozilla/memory/replace/dmd/DMD.cpp:1222) 0x7f9e6360c1a8
>   _cairo_pattern_create_solid (/home/jst/fast/work/tip/mozilla/gfx/cairo/cairo/src/cairo-pattern.c:490) 0x7f9e5f24b6ec
>   *INT__moz_cairo_pattern_create_rgba (/home/jst/fast/work/tip/mozilla/gfx/cairo/cairo/src/cairo-pattern.c:593) 0x7f9e5f24b857
>   gfxPattern (/home/jst/fast/work/tip/mozilla/gfx/thebes/gfxPattern.cpp:26) 0x7f9e5f188747
>   nsSVGGlyphFrame::SetupInheritablePaint(gfxContext*, float&, gfxTextObjectPaint*, nsSVGGlyphFrame::SVGTextObjectPaint::Paint&, nsStyleSVGPaint nsStyle
SVG::*, mozilla::FramePropertyDescriptor const*) (asn1cmn.c:0) 0x7f9e5eb8855d
>   nsSVGGlyphFrame::SetupCairoFill(gfxContext*, gfxTextObjectPaint*, nsSVGGlyphFrame::SVGTextObjectPaint*) (asn1cmn.c:0) 0x7f9e5eb88615
>   nsSVGGlyphFrame::SetupCairoState(gfxContext*, gfxTextObjectPaint*, gfxTextObjectPaint**) (asn1cmn.c:0) 0x7f9e5eb89935
>   nsSVGGlyphFrame::PaintSVG(nsRenderingContext*, nsIntRect const*) (asn1cmn.c:0) 0x7f9e5eb8b562

(I think the filenames and line numbers aren't right here, but hopefully the function names are.)  

Unfortunately, the memory is allocated within Cairo, and we don't currently have any memory reporting for Cairo.

The second record accounted for 45 MB and looked like this:

>   replace_malloc (/home/jst/fast/work/tip/mozilla/memory/replace/dmd/DMD.cpp:1222) 0x7f9e6360c1a8
>   moz_xmalloc (/home/jst/fast/work/tip/mozilla/memory/mozalloc/mozalloc.cpp:55) 0x7f9e60c94e5a
>   nsSVGGlyphFrame::SetupInheritablePaint(gfxContext*, float&, gfxTextObjectPaint*, nsSVGGlyphFrame::SVGTextObjectPaint::Paint&, nsStyleSVGPaint nsStyle
SVG::*, mozilla::FramePropertyDescriptor const*) (asn1cmn.c:0) 0x7f9e5eb8854e
>   nsSVGGlyphFrame::SetupCairoFill(gfxContext*, gfxTextObjectPaint*, nsSVGGlyphFrame::SVGTextObjectPaint*) (asn1cmn.c:0) 0x7f9e5eb88615
>   nsSVGGlyphFrame::SetupCairoState(gfxContext*, gfxTextObjectPaint*, gfxTextObjectPaint**) (asn1cmn.c:0) 0x7f9e5eb89935
>   nsSVGGlyphFrame::PaintSVG(nsRenderingContext*, nsIntRect const*) (asn1cmn.c:0) 0x7f9e5eb8b562
>   nsDisplaySVGGlyphs::Paint(nsDisplayListBuilder*, nsRenderingContext*) (asn1cmn.c:0) 0x7f9e5eb887bd

I looks rather similar, and may be basically the same thing.

jst said his workload involves a page that uses SVG:  "it's an internal interface to my router, so not something that is publicly accessible. I always have a firefox window open that shows bandwidth graph on the router, and that graph is done with a live SVG image. That's more than likely the culprit here if my above assumptions are correct, and that graph updates once every 5 seconds or so, so lots of changes over time."  This was a very long-running session, days or even weeks.
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2013-01-29 23:33:38 PST
FWIW, the router I use is a netgear thingie with an installation of Tomato firmware, version 1.28.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2013-01-29 23:48:46 PST
Created attachment 708015 [details] [diff] [review]
Likely fix.

If my quick look here based on what Nick found is correct then this might not be a memory reporter problem at all, but an actual leak.

If cairo is used, as I suspect is the case for me (because vnc), then this code passes in an unowned  gfxPattern pointer to gfxContext::SetPattern(), which in that case just extracts the cairo pattern from it and returns, w/o anything actually holding on to or ever releasing the given gfxPattern. That leaves the gfxPattern hanging w/o anything referencing it.

This compiles and seems to run, but I have not verified that this fixes this problem.
Comment 3 Jonathan Watt [:jwatt] 2013-01-30 01:09:47 PST
Comment on attachment 708015 [details] [diff] [review]
Likely fix.

Nice catch. r=me.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2013-01-30 01:35:15 PST
Tested that patch, and it does indeed fix a leak! W/o that patch, the number of allocated gfxPattern's is ever growing, with the patch, we create ~14 and stay there. As a side note, we also appear to leak those 14 or so :(
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2013-01-30 09:36:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cd7a9bc6406
Comment 6 Nicholas Nethercote [:njn] 2013-01-30 19:16:01 PST
\o/
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-01-31 13:38:11 PST
https://hg.mozilla.org/mozilla-central/rev/7cd7a9bc6406

Note You need to log in before you can comment on or make changes to this bug.