Bug 56672 – Leaks beneath RenderSVGShadowTreeRootContainer::updateFromElement seen on SnowLeopard Intel Leaks

• Component: SVG
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To see the leaks:

1. Go to http://build.webkit.org/LeaksViewer/?url=http%3A%2F%2Fbuild.webkit.org%2F%2Fresults%2FSnowLeopard%20Intel%20Leaks%2Fr81496%20(15670)%2F
2. Dig into malloc_zone_malloc > malloc > WTF::fastMalloc

Many of the leaks in that node are (indirect) callees of RenderSVGShadowTreeRootContainer::updateFromElement.

[Jessie Berlin]

<rdar://problem/9156332>

[Sam Weinig]

It looks like at least some of this leak is coming from somewhere in this set:

sputnik/Unicode/Unicode_410 .........................................
sputnik/Unicode/Unicode_500 .........................................
sputnik/Unicode/Unicode_510 .........................................
storage ..................................
storage/domstorage ......
storage/domstorage/events .....
storage/domstorage/localstorage ........
storage/domstorage/localstorage/storagetracker .....
storage/domstorage/sessionstorage ........
svg/W3C-I18N .........................................
svg/W3C-SVG-1.1-SE ......................
svg/W3C-SVG-1.1 ....................................................................................................................................................................................................................................................................................
svg/animations .............................
svg/batik/filters ..
svg/batik/masking .
svg/batik/paints .....
svg/batik/text ............................
svg/carto.net ..........
svg/clip-path ..........................................
svg/css .............................
svg/custom .....................................................................................................................................................................................................................................................................................................
 ? checking for leaks in DumpRenderTree

 + 24 leaks (11312 bytes including 2 excluded leaks) were found, details in /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-leaks/build/layout-test-results/DumpRenderTree6-leaks.txt

which can be helpful for find a reproducible  test of this happening.

[Sam Weinig]

Adding on to what Adam was noting, RenderSVGShadowTreeRootContainers are leaking, so all the children of that element are also leaking.

[Sam Weinig]

(In reply to comment #3)
> Adding on to what Adam was noting, RenderSVGShadowTreeRootContainers are leaking, so all the children of that element are also leaking.

Make that the SVGShadowTreeRootElement, not the RenderSVGShadowTreeRootContainer.

[Leo Yang]

Created attachment 89993 [details]
Patch

I can't see the leak results from the above links. But I can see the following leak in recent build, so I'm assume the bug was reporting the same issue.

Leak: 0x1165e1df0  size=464  zone: DefaultMallocZone_0x105f89000        instance of 'WebCore::SVGShadowTreeRootElement', type C++, implemented in WebCore       
        0x02fd30d8 0x00000001 0x02fd36d0 0x00000001     .0.......6......
        0x16600000 0x00000000 0x165a08b0 0x00000001     ..`.......Z.....
        0x00000000 0x00000000 0x00000000 0x00000000     ................
        0x07e34a00 0x00000001 0x00000000 0x00000000     .J..............
        0x00000000 0x00000000 0x00000000 0x00000000     ................
        0x005c085c 0x00000000 0x00000000 0x00000000     \.\.............
        0x00000000 0x00000000 0x0ce6a1d0 0x00000001     ................
        0x00000000 0x00000000 0x00000000 0x00000000     ................
        ... 
        Call stack: [thread 0x7fff70e15ca0]: | 0x2 | start | main | dumpRenderTree(int, char const**) | runTestingServerLoop() | runTest(std::string const&) | -[NSRunLoop(NSRunLoop) runMode:beforeDate:] | CFRunLoopRunSpecific | __CFRunLoopRun | __CFRunLoopDoSources0 | MultiplexerSource::perform() | URLConnectionClient::processEvents() | URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) | URLConnectionClient::_clientDidReceiveData(__CFData const*, URLConnectionClient::ClientConnectionEventQueue*) | _NSURLConnectionDidReceiveData | -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:] | WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) | WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) | WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) | WebCore::MainResourceLoader::addData(char const*, int, bool) | WebCore::DocumentLoader::receivedData(char const*, int) | WebCore::DocumentLoader::commitLoad(char const*, int) | WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) | -[WebDataSource(WebInternal) _receivedData:] | -[WebHTMLRepresentation receivedData:withDataSource:] | -[WebFrame(WebInternal) _commitData:] | WebCore::DocumentLoader::commitData(char const*, int) | WebCore::DocumentWriter::addData(char const*, int, bool) | WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, int, bool) | WebCore::XMLDocumentParser::append(WebCore::SegmentedString const&) | WebCore::XMLDocumentParser::doWrite(WTF::String const&) | xmlParseChunk | xmlParseXMLDecl | WebCore::startElementNsHandler(void*, unsigned char const*, unsigned char const*, unsigned char const*, int, unsigned char const**, int, int, unsigned char const**) | WebCore::XMLDocumentParser::startElementNs(unsigned char const*, unsigned char const*, unsigned char const*, int, unsigned char const**, int, int, unsigned char const**) | WebCore::SVGUseElement::attach() | WebCore::SVGStyledElement::attach() | WebCore::RenderSVGShadowTreeRootContainer::updateFromElement() | WebCore::SVGShadowTreeRootElement::create(WebCore::Document*, WebCore::SVGUseElement*) | WTF::fastMalloc(unsigned long) | malloc | malloc_zone_malloc

[Eric Seidel (no email)]

Comment on attachment 89993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89993&action=review

> Source/WebCore/rendering/svg/RenderSVGShadowTreeRootContainer.cpp:42
>          m_shadowRoot->clearShadowHost();

I don't understand why "clearing" the host doesn't also detach it?  Aren't we releasing a ref-counted DOM Node here?  When it's destroyed, doesn't it also destroy its renderer?

[Leo Yang]

(In reply to comment #6)
> (From update of attachment 89993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89993&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGShadowTreeRootContainer.cpp:42
> >          m_shadowRoot->clearShadowHost();
> 
> I don't understand why "clearing" the host doesn't also detach it?  Aren't we releasing a ref-counted DOM Node here?  When it's destroyed, doesn't it also destroy its renderer?

Good point! I was trying to keep the previous detach. I also agree that it needs not detach if we clear shadow host.

[Leo Yang]

Created attachment 90128 [details]
Revised patch

[Dirk Schulze]

Comment on attachment 90128 [details]
Revised patch

Fix looks good to me. r=me

[WebKit Commit Bot]

Comment on attachment 90128 [details]
Revised patch

Rejecting attachment 90128 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
k Schulze', u'--for..." exit_code: 1

Parsed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/rendering/svg/RenderSVGShadowTreeRootContainer.cpp
Hunk #1 FAILED at 36.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/rendering/svg/RenderSVGShadowTreeRootContainer.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Dirk Schulze', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/8475164

[Leo Yang]

Created attachment 90300 [details]
Rebased patch

[Eric Seidel (no email)]

Comment on attachment 90300 [details]
Rebased patch

OK.  Why was the previous code only clear it when attached?  This seems fine though.

[Dirk Schulze]

Comment on attachment 90300 [details]
Rebased patch

Setting cq+.

[WebKit Commit Bot]

Comment on attachment 90300 [details]
Rebased patch

Clearing flags on attachment: 90300

Committed r84350: <http://trac.webkit.org/changeset/84350>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

Thanks for looking into this!

It's going to be very hard to tell if this is fixed until bug 58889 is fixed, too.

[Adam Roben (:aroben)]

Looks like this was either never fixed or has come back. I filed bug 72741 rather than reopening this one as I thought it would be clearer.