| Summary: | [Curl] Crash in CurlDownload::didReceiveHeader when downloading file. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | peavo | ||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, darin, galpeter, ossy | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
peavo
2015-07-10 03:48:14 PDT
Created attachment 256575 [details]
Patch
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
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> |