Bug 146242 – WinLauncher fails to download files.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

When trying to download files, WinLauncher crashes.

Created attachment 255420 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

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.

[WebKit Commit Bot]

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.

[Alex Christensen]

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?

[Alex Christensen]

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

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

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.

[Alex Christensen]

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.

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 255634 [details]
Patch

Clearing flags on attachment: 255634

Committed r186004: <http://trac.webkit.org/changeset/186004>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

Thanks for reviewing :)