Bug 56672

Summary: Leaks beneath RenderSVGShadowTreeRootContainer::updateFromElement seen on SnowLeopard Intel Leaks
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, leo.yang, sam, zimmermann
Priority: P2 Keywords: InRadar, MakingBotsRed
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.6   
URL: http://build.webkit.org/LeaksViewer/?url=http%3A%2F%2Fbuild.webkit.org%2F%2Fresults%2FSnowLeopard%20Intel%20Leaks%2Fr81496%20(15670)%2F
Attachments:
Description Flags
Patch
none
Revised patch
krit: review+, commit-queue: commit-queue-
Rebased patch none

Description Adam Roben (:aroben) 2011-03-18 13:42:55 PDT
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.
Comment 1 Jessie Berlin 2011-03-18 14:06:01 PDT
<rdar://problem/9156332>
Comment 2 Sam Weinig 2011-03-18 19:58:26 PDT
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.
Comment 3 Sam Weinig 2011-03-18 19:59:43 PDT
Adding on to what Adam was noting, RenderSVGShadowTreeRootContainers are leaking, so all the children of that element are also leaking.
Comment 4 Sam Weinig 2011-03-18 20:00:08 PDT
(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.
Comment 5 Leo Yang 2011-04-17 22:50:46 PDT
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
Comment 6 Eric Seidel (no email) 2011-04-18 09:29:27 PDT
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?
Comment 7 Leo Yang 2011-04-18 18:59:30 PDT
(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.
Comment 8 Leo Yang 2011-04-18 19:01:42 PDT
Created attachment 90128 [details]
Revised patch
Comment 9 Dirk Schulze 2011-04-18 23:48:02 PDT
Comment on attachment 90128 [details]
Revised patch

Fix looks good to me. r=me
Comment 10 WebKit Commit Bot 2011-04-18 23:51:38 PDT
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
Comment 11 Leo Yang 2011-04-19 21:12:03 PDT
Created attachment 90300 [details]
Rebased patch
Comment 12 Eric Seidel (no email) 2011-04-19 21:42:34 PDT
Comment on attachment 90300 [details]
Rebased patch

OK.  Why was the previous code only clear it when attached?  This seems fine though.
Comment 13 Dirk Schulze 2011-04-19 23:52:09 PDT
Comment on attachment 90300 [details]
Rebased patch

Setting cq+.
Comment 14 WebKit Commit Bot 2011-04-20 01:16:15 PDT
Comment on attachment 90300 [details]
Rebased patch

Clearing flags on attachment: 90300

Committed r84350: <http://trac.webkit.org/changeset/84350>
Comment 15 WebKit Commit Bot 2011-04-20 01:16:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Adam Roben (:aroben) 2011-04-20 06:34:56 PDT
Thanks for looking into this!

It's going to be very hard to tell if this is fixed until bug 58889 is fixed, too.
Comment 17 Adam Roben (:aroben) 2011-11-18 11:04:39 PST
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.