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)
I will provide a fix for this leak.
Created attachment 144744 [details] Patch Fix a memory leak.
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
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.
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.
Created attachment 144858 [details] Patch Fixed another possible leak by adopting targetList received from targetListForDataObject().
(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!
Comment on attachment 144858 [details] Patch Clearing flags on attachment: 144858 Committed r119063: <http://trac.webkit.org/changeset/119063>
All reviewed patches have been landed. Closing bug.
Thanks for your review Carlos!