Bug 87756 – [GTK] [WK2] Memory leak in webkitWebViewBaseStartDrag

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Sudarsana Nagineni (babu)]

Valgrind reports the following memory leak in webkitWebViewBaseStartDrag. I think this can be fixed by adopting an allocation of DataObjectGtk.

==29268== 838 (152 direct, 686 indirect) bytes in 1 blocks are definitely lost in loss record 7,146 of 7,746
==29268==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29268==    by 0xC1B362E: WTF::fastMalloc(unsigned long) (FastMalloc.cpp:268)
==29268==    by 0x60BC6EF: WTF::RefCounted<WebCore::DataObjectGtk>::operator new(unsigned long) (RefCounted.h:185)
==29268==    by 0x60BC422: WebCore::DataObjectGtk::create() (DataObjectGtk.h:36)
==29268==    by 0x60BB86E: CoreIPC::decodeDataObject(CoreIPC::ArgumentDecoder*, WTF::RefPtr<WebCore::DataObjectGtk>&) (ArgumentCodersGtk.cpp:118)
==29268==    by 0x60BBE43: CoreIPC::ArgumentCoder<WebCore::DragData>::decode(CoreIPC::ArgumentDecoder*, WebCore::DragData&) (ArgumentCodersGtk.cpp:212)
==29268==    by 0x62DE17A: bool CoreIPC::ArgumentDecoder::decode<WebCore::DragData>(WebCore::DragData&) (ArgumentDecoder.h:90)
==29268==    by 0x62DDCAF: CoreIPC::Arguments1<WebCore::DragData>::decode(CoreIPC::ArgumentDecoder*, CoreIPC::Arguments1<WebCore::DragData>&) (Arguments.h:77)
==29268==    by 0x62DD271: CoreIPC::Arguments2<WebCore::DragData, WebKit::ShareableBitmap::Handle>::decode(CoreIPC::ArgumentDecoder*, CoreIPC::Arguments2<WebCore::DragData, WebKit::ShareableBitmap::Handle>
==29268==    by 0x62DC276: CoreIPC::ArgumentCoder<CoreIPC::Arguments2<WebCore::DragData, WebKit::ShareableBitmap::Handle> >::decode(CoreIPC::ArgumentDecoder*, CoreIPC::Arguments2<WebCore::DragData, WebKit:
==29268==    by 0x62DA976: bool CoreIPC::ArgumentDecoder::decode<CoreIPC::Arguments2<WebCore::DragData, WebKit::ShareableBitmap::Handle> >(CoreIPC::Arguments2<WebCore::DragData, WebKit::ShareableBitmap::Ha
==29268==    by 0x62D72F9: void CoreIPC::handleMessage<Messages::WebPageProxy::StartDrag, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::DragData const&, WebKit::ShareableBitmap::Handle cons
==29268==    by 0x62D3997: WebKit::WebPageProxy::didReceiveWebPageProxyMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) (WebPageProxyMessageReceiver.cpp:449)
==29268==    by 0x61A4B22: WebKit::WebPageProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) (WebPageProxy.cpp:1744)
==29268==    by 0x61D9391: WebKit::WebProcessProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) (WebProcessProxy.cpp:333)
==29268==    by 0x616EBC2: WebKit::WebConnectionToWebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) (WebConnectionToWebProcess.cpp:92)
==29268==    by 0x6090D3A: CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::ArgumentDecoder>&) (Connection.cpp:691)
==29268==    by 0x6090ED8: CoreIPC::Connection::dispatchOneMessage() (Connection.cpp:717)
==29268==    by 0x609B075: WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*) (Functional.h:173)
==29268==    by 0x609AE7B: WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void ()(CoreIPC::Connection*)>::operator()() (Functional.h:405)
==29268==    by 0x609FEBD: WTF::Function<void ()()>::operator()() const (Functional.h:613)
==29268==    by 0x6B86D50: WebCore::RunLoop::performWork() (RunLoop.cpp:67)
==29268==    by 0x7589241: WebCore::RunLoop::queueWork(WebCore::RunLoop*) (RunLoopGtk.cpp:102)
==29268==    by 0xB204C99: g_main_context_dispatch (gmain.c:2515)
==29268==    by 0xB20505F: g_main_context_iterate.isra.23 (gmain.c:3123)
==29268==    by 0xB205459: g_main_loop_run (gmain.c:3317)
==29268==    by 0xA71125C: gtk_main (gtkmain.c:1165)
==29268==    by 0x40B136: main (main.c:233)

[Sudarsana Nagineni (babu)]

I will provide a fix for this leak.

[Sudarsana Nagineni (babu)]

Created attachment 144744 [details]
Patch

Fix a memory leak.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 144744 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:678
> -    RefPtr<DataObjectGtk> dataObject(dragData.platformData());
> +    RefPtr<DataObjectGtk> dataObject = adoptRef(dragData.platformData());

hmm, it's a bit weird, because that leaves DragData object with an invalid pointer when webkitWebViewBaseStartDrag finishes. It's not a problem because I think DragData is deleted after this and platformData() is not used anymore. The problem is that DradData is supposed to contain a const reference of platformData, so it's not clear in this case who should release the platform data.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:679
>      GRefPtr<GtkTargetList> targetList(PasteboardHelper::defaultPasteboardHelper()->targetListForDataObject(dataObject.get()));

I think this is a leak too, targetListForDataObject() returns a new GtkTargetList and gtk_drag_begin takes a reference, so we should adopt it here, so that the only reference left when webkitWebViewBaseStartDrag finishes is owned by gtk. This happens in WebKit1 code too.

[Sudarsana Nagineni (babu)]

Comment on attachment 144744 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:678
>> +    RefPtr<DataObjectGtk> dataObject = adoptRef(dragData.platformData());
> 
> hmm, it's a bit weird, because that leaves DragData object with an invalid pointer when webkitWebViewBaseStartDrag finishes. It's not a problem because I think DragData is deleted after this and platformData() is not used anymore. The problem is that DradData is supposed to contain a const reference of platformData, so it's not clear in this case who should release the platform data.

The reason for this leak is that DragData does not delete its platformData. So, we need to take care of it and that's the reason I'm adopting it so that it will be dereferenced and released when GtkDragAndDropHelper::handleDragEnd() is called.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:679
>>      GRefPtr<GtkTargetList> targetList(PasteboardHelper::defaultPasteboardHelper()->targetListForDataObject(dataObject.get()));
> 
> I think this is a leak too, targetListForDataObject() returns a new GtkTargetList and gtk_drag_begin takes a reference, so we should adopt it here, so that the only reference left when webkitWebViewBaseStartDrag finishes is owned by gtk. This happens in WebKit1 code too.

I also thought about it, but for some reason valgrind doesn't report anything related to this. Anyway, it makes sense to adopt it, so I will update the patch.

[Sudarsana Nagineni (babu)]

Created attachment 144858 [details]
Patch

Fixed another possible leak by adopting targetList received from targetListForDataObject().

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 144744 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144744&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:678
> >> +    RefPtr<DataObjectGtk> dataObject = adoptRef(dragData.platformData());
> > 
> > hmm, it's a bit weird, because that leaves DragData object with an invalid pointer when webkitWebViewBaseStartDrag finishes. It's not a problem because I think DragData is deleted after this and platformData() is not used anymore. The problem is that DradData is supposed to contain a const reference of platformData, so it's not clear in this case who should release the platform data.
> 
> The reason for this leak is that DragData does not delete its platformData.

Exactly, because it holds a const reference, so the platform data is supposed to be released when DragData is already freed. Otherwise, DragData will have a pointer to freed memory.

> So, we need to take care of it and that's the reason I'm adopting it so that it will be dereferenced and released when GtkDragAndDropHelper::handleDragEnd() is called.

Ok, I see now, the data object is not released when webkitWebViewBaseStartDrag finished because GtkDragAndDropHelper takes a reference to it, so adopting it here makes the GtkDragAndDropHelper to adopt it, which is correct and expected.

> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:679
> >>      GRefPtr<GtkTargetList> targetList(PasteboardHelper::defaultPasteboardHelper()->targetListForDataObject(dataObject.get()));
> > 
> > I think this is a leak too, targetListForDataObject() returns a new GtkTargetList and gtk_drag_begin takes a reference, so we should adopt it here, so that the only reference left when webkitWebViewBaseStartDrag finishes is owned by gtk. This happens in WebKit1 code too.
> 
> I also thought about it, but for some reason valgrind doesn't report anything related to this. Anyway, it makes sense to adopt it, so I will update the patch.

Thanks!

[WebKit Review Bot]

Comment on attachment 144858 [details]
Patch

Clearing flags on attachment: 144858

Committed r119063: <http://trac.webkit.org/changeset/119063>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sudarsana Nagineni (babu)]

Thanks for your review Carlos!