Bug 86603 – Memory leak in NetworkInfoClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Sudarsana Nagineni (babu)]

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)

[Gyuyoung Kim]

Thank you for your bug report. Will you submit a patch for this ?

[Sudarsana Nagineni (babu)]

(In reply to comment #1)
> Thank you for your bug report. Will you submit a patch for this ?
Yes, Gyuyoung. Fixing the leak now.

[Sudarsana Nagineni (babu)]

Created attachment 142213 [details]
Patch

Fix memory leak.

[Gyuyoung Kim]

Comment on attachment 142213 [details]
Patch

LGTM.

[Martin Robinson]

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.

[Sudarsana Nagineni (babu)]

(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!

[Martin Robinson]

(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. :)

[Adam Barth]

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.

[Gyuyoung Kim]

(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.

[Raphael Kubo da Costa (:rakuco)]

(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?

[Adam Barth]

> 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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Adam Barth]

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.

[Sudarsana Nagineni (babu)]

Thanks for the feedback and letting me know this nice approach for clients destruction. I'm going to update the patch now.

[Sudarsana Nagineni (babu)]

Created attachment 142569 [details]
Fix memory leak

Updated patch taking Adam Barth's feedback into consideration.

[Adam Barth]

Comment on attachment 142569 [details]
Fix memory leak

Perfect.  Thanks!

[WebKit Review Bot]

Comment on attachment 142569 [details]
Fix memory leak

Clearing flags on attachment: 142569

Committed r117515: <http://trac.webkit.org/changeset/117515>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.