Bug 56672 - Leaks beneath RenderSVGShadowTreeRootContainer::updateFromElement seen on SnowLeopard Intel Leaks
Summary: Leaks beneath RenderSVGShadowTreeRootContainer::updateFromElement seen on Sno...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL: http://build.webkit.org/LeaksViewer/?...
Keywords: InRadar, MakingBotsRed
Depends on:
Blocks:
 
Reported: 2011-03-18 13:42 PDT by Adam Roben (:aroben)
Modified: 2011-11-18 11:04 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.35 KB, patch)
2011-04-17 22:50 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Revised patch (2.36 KB, patch)
2011-04-18 19:01 PDT, Leo Yang
krit: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Rebased patch (2.97 KB, patch)
2011-04-19 21:12 PDT, Leo Yang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.