Bug 146832 – [Curl] Crash in CurlDownload::didReceiveHeader when downloading file.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

I just got a crash in CurlDownload::didReceiveHeader in the call to m_response.setMimeType.

We should only call ResourceResponse::setMimeType from the main thread.

Created attachment 256575 [details]
Patch

[Darin Adler]

Comment on attachment 256575 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlDownload.cpp:402
> +            callOnMainThread([this, strUrl] {

What guarantees the CurlDownload won’t be deleted between now and when the function is called on the main thread?

It’s not thread-safe to capture a string on one thread and use it on another. We need to use StringCapture instead to pass strings across threads.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:412
> +        callOnMainThread([this, header] {

What guarantees the CurlDownload won’t be deleted between now and when the function is called on the main thread?

It’s not thread-safe to capture a string on one thread and use it on another. We need to use StringCapture instead to pass strings across threads.

Thanks for reviewing :) I will update the patch.

Created attachment 257466 [details]
Patch

[Darin Adler]

Comment on attachment 257466 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlDownload.cpp:309
> +    ref(); // The download manager will call deref when the download has finished.

Might be better to say "CurlDownloadManager::downloadThread" will call deref when the download has finished.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:407
> +            ref();

You can capture a RefPtr to do this without an explicit ref/deref; RefPtr<CurlDownload> protectedDownload(this) or capturedDownload and then capture that. Doing that has a slight clarity advantage of running the deref if the lambda is ever deleted without being called, not an issue in these cases. Sometimes people capture both the RefPtr and "this".

(In reply to comment #5)
> Comment on attachment 257466 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257466&action=review
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:309
> > +    ref(); // The download manager will call deref when the download has finished.
> 
> Might be better to say "CurlDownloadManager::downloadThread" will call deref
> when the download has finished.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:407
> > +            ref();
> 
> You can capture a RefPtr to do this without an explicit ref/deref;
> RefPtr<CurlDownload> protectedDownload(this) or capturedDownload and then
> capture that. Doing that has a slight clarity advantage of running the deref
> if the lambda is ever deleted without being called, not an issue in these
> cases. Sometimes people capture both the RefPtr and "this".

Thanks for reviewing :) I will update the patch.

Committed r187436: <http://trac.webkit.org/changeset/187436>