Bug 72558 - [Qt] load(), title(), url(), API functions are leaking memory
Summary: [Qt] load(), title(), url(), API functions are leaking memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-16 15:57 PST by Oleg Romashin (:romaxa)
Modified: 2011-11-24 05:45 PST (History)
5 users (show)

See Also:


Attachments
adoptWK ref pointers in order to release implementation when WK pointer released (3.57 KB, patch)
2011-11-17 15:31 PST, Oleg Romashin (:romaxa)
noam: review-
Details | Formatted Diff | Diff
use adoptWK in order to adopt implementation refPtr and release it when WK pointer released (3.82 KB, patch)
2011-11-18 12:34 PST, Oleg Romashin (:romaxa)
hausmann: review+
hausmann: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Romashin (:romaxa) 2011-11-16 15:57:36 PST
QtWebPageProxy::load, QtWebPageProxy::url, QtWebPageProxy::title, and some other API functions leaking strings.

==4060== 98 (36 direct, 62 indirect) bytes in 1 blocks are definitely lost in loss record 2,932 of 3,759
==4060==    at 0x4025018: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==4060==    by 0x5702890: WTF::fastMalloc(unsigned int) (FastMalloc.cpp:268)
==4060==    by 0x472C622: WTF::RefCounted<WebKit::APIObject>::operator new(unsigned int) (RefCounted.h:178)
==4060==    by 0x472E803: WebKit::WebURL::create(WTF::String const&) (WebURL.h:46)
==4060==    by 0x472E916: WebKit::toCopiedURLAPI(WTF::String const&) (WKSharedAPICast.h:162)
==4060==    by 0x4784CB2: WKURLCreateWithQUrl(QUrl const&) (WKURLQt.cpp:34)
==4060==    by 0x484BB7D: QtWebPageProxy::load(QUrl const&) (QtWebPageProxy.cpp:718)
==4060==    by 0x4788D3C: QQuickWebView::load(QUrl const&) (qquickwebview.cpp:390)

WKRetainPtr<WKURLRef> wkurl(WKURLCreateWithQUrl(url));
WKPageLoadURL(pageRef(), wkurl.get());


WebURL created by WKURLCreateWithQUrl, getting refcount + 1 in '
WKURLRef toCopiedURLAPI
RefPtr<WebURL> webURL = WebURL::create(string);
return toAPI(webURL.release().leakRef());

but it never released after WKRetainPtr<WKURLRef> wkurl destroyed...
Not sure what is right way to fix it... I guess on WKRetainPtr<WKURLRef> wkurl destroy it should destroy adopted toImpl WebURL pointer..

Quick workaround to fix this problem is to add:
PassRefPtr<WebURL/WebString> autoDeletePtr = adoptRef(toImpl(wkrefPtr));
which will destroy WebURL/WebString instance on exit from the function...

Otherwise I need to go deeply into smart-pointers relation ship and figure out how to make it works automatically...


Any ideas?
Comment 1 Sam Weinig 2011-11-16 16:21:12 PST
I believe the issue is this line:

WKRetainPtr<WKURLRef> wkurl(WKURLCreateWithQUrl(url));

that should read:

WKRetainPtr<WKURLRef> wkurl = adoptWK(WKURLCreateWithQUrl(url));

This follow the Create Rule (http://developer.apple.com/library/IOs/#documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html).
Comment 2 Oleg Romashin (:romaxa) 2011-11-17 09:29:00 PST
> WKRetainPtr<WKURLRef> wkurl = adoptWK(WKURLCreateWithQUrl(url));
> 
Ok thanks Sam, I'll create patch soon
Comment 3 Oleg Romashin (:romaxa) 2011-11-17 15:31:28 PST
Created attachment 115700 [details]
adoptWK ref pointers in order to release implementation when WK pointer released
Comment 4 Noam Rosenthal 2011-11-17 16:59:07 PST
Comment on attachment 115700 [details]
adoptWK ref pointers in order to release implementation when WK pointer released

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

Patch is good, please improve the changelog.

> Source/WebKit2/ChangeLog:5
> +

Please explain what the patch actually does.
Comment 5 Oleg Romashin (:romaxa) 2011-11-18 12:34:40 PST
Created attachment 115848 [details]
use adoptWK in order to adopt implementation refPtr and release it when WK pointer released
Comment 6 Simon Hausmann 2011-11-24 04:15:39 PST
Comment on attachment 115848 [details]
use adoptWK in order to adopt implementation refPtr and release it when WK pointer released

r=me, but before landing the ChangeLog text should be improved ("don't" -> "doesn't", "adopt" -> "adopts", etc).
Comment 7 Simon Hausmann 2011-11-24 05:45:57 PST
Landed with updated changelog and rebased against ClientImpl changes

Committed r101136: <http://trac.webkit.org/changeset/101136>