When trying to download files, WinLauncher crashes.
Created attachment 255420 [details] Patch
Attachment 255420 [details] did not pass style-queue: ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:26: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:27: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 255420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255420&action=review Hooray! Downloading things from WinLauncher! A few memory management issues, but otherwise this is great! > Tools/WinLauncher/WebDownloadDelegate.cpp:32 > + : m_refCount(1) I don't think this is correct. Something that adopts this should call AddRef then Release when it is done with it, which should delete it if that was the only reference to it. > Tools/WinLauncher/WinMain.cpp:139 > + hr = gWinLauncher->setDownloadDelegate(new WebDownloadDelegate()); This is a memory leak. Use the same pattern as gWinLauncher if this is global, otherwise use some kind of smart pointer.
Created attachment 255425 [details] Patch
(In reply to comment #3) > Comment on attachment 255420 [details] > Patch > > This is a memory leak. Use the same pattern as gWinLauncher if this is > global, otherwise use some kind of smart pointer. Thanks for catching this :) Updated patch.
Attachment 255425 [details] did not pass style-queue: ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:26: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:27: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 255425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255425&action=review > Tools/WinLauncher/WinMain.cpp:139 > + hr = gWinLauncher->setDownloadDelegate(new WebDownloadDelegate()); This is still a memory leak.
(In reply to comment #7) > Comment on attachment 255425 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255425&action=review > > > Tools/WinLauncher/WinMain.cpp:139 > > + hr = gWinLauncher->setDownloadDelegate(new WebDownloadDelegate()); > > This is still a memory leak. I might be mistaken, but since the download delegate object is constructed with refcount 0, and the smart pointer takes ownership here, will it not be destroyed?
I missed that that parameter is a IWebDownloadDelegatePtr which is a _com_ptr_t. Yes, I think this is not a memory leak, but a use-after-free. The IWebDownloadDelegatePtr goes out of scope, which deletes the WebDownloadDelegate which we are still using as a delegate. I think you'll need some kind of global reference to the WebDownloadDelegate.
Created attachment 255476 [details] Patch
Attachment 255476 [details] did not pass style-queue: ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:26: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:27: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #9) > I missed that that parameter is a IWebDownloadDelegatePtr which is a > _com_ptr_t. Yes, I think this is not a memory leak, but a use-after-free. > The IWebDownloadDelegatePtr goes out of scope, which deletes the > WebDownloadDelegate which we are still using as a delegate. I think you'll > need some kind of global reference to the WebDownloadDelegate. Thanks again for reviewing :) I went back to constructing the download delegate object with refount 1 to keep it more in line with the rest of the code here. To avoid the leak, I then transferred ownership to the smart pointer by calling _com_ptr_t::Attach, which does not call AddRef. I checked that this patch does not leak the download delegate object, and there is no use-after-free, the download delegate object is destroyed on exit.
Created attachment 255512 [details] Patch
(In reply to comment #13) > Created attachment 255512 [details] > Patch Updated patch with a global reference to the download delegate.
Attachment 255512 [details] did not pass style-queue: ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:26: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:27: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 255512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255512&action=review Sorry, I'm not doing a very good job reviewing this. This needs one more iteration, and it's my fault. > Tools/WinLauncher/WebDownloadDelegate.cpp:32 > + : m_refCount(0) Ok, sorry about my earlier comment. This should indeed be 1. And use _com_ptr_t::Attach after making a new WebDownloadDelegate.
Created attachment 255634 [details] Patch
(In reply to comment #16) > Comment on attachment 255512 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255512&action=review > > Sorry, I'm not doing a very good job reviewing this. This needs one more > iteration, and it's my fault. > > > Tools/WinLauncher/WebDownloadDelegate.cpp:32 > > + : m_refCount(0) > > Ok, sorry about my earlier comment. This should indeed be 1. And use > _com_ptr_t::Attach after making a new WebDownloadDelegate. No worries :) Updated patch.
Attachment 255634 [details] did not pass style-queue: ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:26: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/WinLauncher/WebDownloadDelegate.cpp:27: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 255634 [details] Patch Clearing flags on attachment: 255634 Committed r186004: <http://trac.webkit.org/changeset/186004>
All reviewed patches have been landed. Closing bug.
Thanks for reviewing :)