Bug 86603 - Memory leak in NetworkInfoClient
Summary: Memory leak in NetworkInfoClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sudarsana Nagineni (babu)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-16 03:06 PDT by Sudarsana Nagineni (babu)
Modified: 2012-05-17 16:35 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.83 KB, patch)
2012-05-16 03:45 PDT, Sudarsana Nagineni (babu)
abarth: review-
Details | Formatted Diff | Diff
Fix memory leak (2.90 KB, patch)
2012-05-17 15:32 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sudarsana Nagineni (babu) 2012-05-16 03:06:44 PDT
Valgrind reports a memory leak in NetworkInfoClient. I think NetworkInfoClient needs to be released explicitly when the NetworkInfoController is destroyed. 

==24745== 48 bytes in 1 blocks are definitely lost in loss record 4,353 of 9,337
==24745==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24745==    by 0x9E0D17E: WTF::fastMalloc(unsigned long) (FastMalloc.cpp:268)
==24745==    by 0x4FF4C04: WTF::RefCounted<WebCore::NetworkInfoClient>::operator new(unsigned long) (RefCounted.h:185)
==24745==    by 0x4FCF937: _ewk_view_priv_new(_Ewk_View_Smart_Data*) (ewk_view.cpp:656)
==24745==    by 0x4FD0A1E: _ewk_view_smart_add(_Evas_Object*) (ewk_view.cpp:833)
==24745==    by 0x4FF52C8: _ewk_view_single_smart_add(_Evas_Object*) (ewk_view_single.cpp:45)
==24745==    by 0x4E78E18: evas_object_smart_add (evas_object_smart.c:323)
==24745==    by 0x4FF6542: ewk_view_single_add (ewk_view_single.cpp:419)
==24745==    by 0x4047DB: browserCreate (main.c:637)
==24745==    by 0x405135: main (main.c:830)
Comment 1 Gyuyoung Kim 2012-05-16 03:15:13 PDT
Thank you for your bug report. Will you submit a patch for this ?
Comment 2 Sudarsana Nagineni (babu) 2012-05-16 03:21:15 PDT
(In reply to comment #1)
> Thank you for your bug report. Will you submit a patch for this ?
Yes, Gyuyoung. Fixing the leak now.
Comment 3 Sudarsana Nagineni (babu) 2012-05-16 03:45:03 PDT
Created attachment 142213 [details]
Patch

Fix memory leak.
Comment 4 Gyuyoung Kim 2012-05-16 03:52:07 PDT
Comment on attachment 142213 [details]
Patch

LGTM.
Comment 5 Martin Robinson 2012-05-16 09:10:57 PDT
When modifying platform-independent code careful with the use of the [EFL] tag. It usually is a signal to reviewers who know the WebCore code well to ignore your patch.
Comment 6 Sudarsana Nagineni (babu) 2012-05-16 09:15:00 PDT
(In reply to comment #5)
> When modifying platform-independent code careful with the use of the [EFL] tag. It usually is a signal to reviewers who know the WebCore code well to ignore your patch.

Understood, thanks Martin!
Comment 7 Martin Robinson 2012-05-16 09:20:07 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > When modifying platform-independent code careful with the use of the [EFL] tag. It usually is a signal to reviewers who know the WebCore code well to ignore your patch.
> 
> Understood, thanks Martin!

Nice work, hunting all these leaks too. :)
Comment 8 Adam Barth 2012-05-16 20:15:17 PDT
Comment on attachment 142213 [details]
Patch

Thanks for the patch, but this isn't the normal idiom we use for managing the lifetime of clients.  Take a look at how some of the other clients work, such as the Geolocation clients.  It's better if we're consistent in how we handle these sorts of objects.
Comment 9 Gyuyoung Kim 2012-05-16 22:08:26 PDT
(In reply to comment #8)
> (From update of attachment 142213 [details])
> Thanks for the patch, but this isn't the normal idiom we use for managing the lifetime of clients.  Take a look at how some of the other clients work, such as the Geolocation clients.  It's better if we're consistent in how we handle these sorts of objects.

Adam, I'm sorry. I will take care of reviewing patch informally further.
Comment 10 Raphael Kubo da Costa (:rakuco) 2012-05-17 08:05:30 PDT
(In reply to comment #8)
> (From update of attachment 142213 [details])
> Thanks for the patch, but this isn't the normal idiom we use for managing the lifetime of clients.  Take a look at how some of the other clients work, such as the Geolocation clients.  It's better if we're consistent in how we handle these sorts of objects.

To be honest, I got a bit confused when trying to review this yesterday. While some clients do inherit from RefCounted, most of them do not. GeolocationClient itself follows the pattern of not inheriting from RefCounted and having a fooDestroyed() method that's called from its controller's destructor. One of the exceptions I found was SpeechRecognitionController, which says in a comment: "Call m_client->pageDestroyed(); once we have implemented a client" in the destructor.

What would be the normal idiom here?
Comment 11 Adam Barth 2012-05-17 08:41:02 PDT
> What would be the normal idiom here?

Yeah, we should probably clean these up to be more consistent.  IMHO, we should have these clients be owned by the WebView.  That will cause them to naturally outlive the controller.  Notice the OwnPtrs to various clients here:

http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebViewImpl.h#L783

That approach is nice because it avoids these destruction notification functions.
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-05-17 12:48:14 PDT
That sounds nice. I now wonder whether these client classes in WebCore should inherit from RefCounted or not; some of them do, some don't, and using them requires using RefPtrs instead of OwnPtrs in WebKit.
Comment 13 Adam Barth 2012-05-17 12:50:26 PDT
We'll need to look at them each individually.  The approach in Comment #11 uses OwnPtr, not RefPtr, so they shouldn't need to subclass RefCounted.
Comment 14 Sudarsana Nagineni (babu) 2012-05-17 15:22:04 PDT
Thanks for the feedback and letting me know this nice approach for clients destruction. I'm going to update the patch now.
Comment 15 Sudarsana Nagineni (babu) 2012-05-17 15:32:32 PDT
Created attachment 142569 [details]
Fix memory leak

Updated patch taking Adam Barth's feedback into consideration.
Comment 16 Adam Barth 2012-05-17 15:39:23 PDT
Comment on attachment 142569 [details]
Fix memory leak

Perfect.  Thanks!
Comment 17 WebKit Review Bot 2012-05-17 16:34:55 PDT
Comment on attachment 142569 [details]
Fix memory leak

Clearing flags on attachment: 142569

Committed r117515: <http://trac.webkit.org/changeset/117515>
Comment 18 WebKit Review Bot 2012-05-17 16:35:02 PDT
All reviewed patches have been landed.  Closing bug.