Bug 10835 – REGRESSION: LOTS of SVG-related leaks

• Component: SVG
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

Buildbot indicates that the leaks originated with <http://build.webkit.org/post-commit-leaks-powerpc-mac-os-x/builds/2515>, which corresponds to revision 16308.  Revision 16308 is the fix for bug 10750, "SVG needs SVGAnimatedTypeNamePropertyName<ClassName> classes"

[Nikolas Zimmermann]

Created attachment 10529 [details]
Initial patch

This should fix the mem leaks we've been seeing recently.
Though I have no idea wheter all of them are fixed, because
if I run run-webkit-tests --leaks, _no_ SVG related memory
leaks are shown, before and after this patch. No idea why.

But it's quite clear that there are memory leaks in SVGList's
which reference ptr objects. This patch fixes that.

[Nikolas Zimmermann]

If anyone takes a look at the patch, please read this first :-)

I orignally had clearVector() a non-const method, and it didn't
take the Vector<Item> param. I planned to make m_vector protected
and access it from SVGList::clearVector, though that never compiled.
Anyone can enlighten me why?

[Eric Seidel (no email)]

Comment on attachment 10529 [details]
Initial patch

This could be done in a cleaner fashion using a Vector of RefPtrs and pointer-specialization in the SVGList template (similar to how hashtraits is constructed).

But this is OK as is, and it's good to get this landed to get the leaks fixed.

r=me

[Mark Rowe (bdash)]

This patch did not appear to fix the memory leaks.  <http://build.webkit.org/results/post-commit-leaks-powerpc-mac-os-x/2541/DumpRenderTree5-leaks.txt> contains the leaks output from the most recent buildbot build, and indicates leaks under SVGPathElement::createSVGPathSegCurvetoCubicAbs, SVGTransformable::parseTransformAttribute and SVGLengthList::parse.

Nikolas, are you testing for leaks with a debug build?  In release builds I believe we use a custom memory allocator which will circumvent the `leaks' tools check for memory leaks.

[Nikolas Zimmermann]

Hi Mark,

> leaks under SVGPathElement::createSVGPathSegCurvetoCubicAbs,
> SVGTransformable::parseTransformAttribute and SVGLengthList::parse.
> 
> Nikolas, are you testing for leaks with a debug build?  In release builds I
> believe we use a custom memory allocator which will circumvent the `leaks'
> tools check for memory leaks.

Hah! That's it! I used release builds on osx, as I do actual debugging
within Linux - well, now that's really stupid... Will try with a debug
build later on today...

Thanks a lot, Niko

[Nikolas Zimmermann]

Created attachment 10578 [details]
Updated patch

Now that I can finally check for leaks locally on osx, I could really fix them.
It's done in a much better way now - new SVGListTraits class which is modelled
after WTF::VectorTraits which makes virtual functions unneccessary and removes
the SVGListBase base class. Down to 2 leaks again. No regressions.

[Eric Seidel (no email)]

Comment on attachment 10578 [details]
Updated patch

This could be done cleaner, without having to modify the generator, or having getFirst() etc, return a RefPtr (instead of a raw pointer).

Given how many leaks the existing code causes, I think it's worth landing this now, and filling a follow-up bug about cleanup.

[Mark Rowe (bdash)]

Landed as r16378.