[Curl] Crash in CurlDownload::didReceiveHeader when downloading file.
authorpeavo@outlook.com <peavo@outlook.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 27 Jul 2015 18:48:02 +0000 (18:48 +0000)
committerpeavo@outlook.com <peavo@outlook.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 27 Jul 2015 18:48:02 +0000 (18:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=146832

Reviewed by Darin Adler.

Source/WebCore:

Only call ResourceResponse::setMimeType from the main thread.
Also, CurlDownload should be reference counted to make sure it
still lives when a function call is invoked on the main thread
from the download thread.

* platform/network/curl/CurlDownload.cpp:
(WebCore::CurlDownloadManager::downloadThread):
(WebCore::CurlDownload::CurlDownload):
(WebCore::CurlDownload::start):
(WebCore::CurlDownload::didReceiveHeader):
(WebCore::CurlDownload::didReceiveData):
* platform/network/curl/CurlDownload.h:
(WebCore::CurlDownloadListener::didFail):
(WebCore::CurlDownload::setListener):

Source/WebKit/win:

CurlDownload should be reference counted to make sure it still
lives when a function call is invoked on the main thread from
the download thread.

* WebDownload.h:
* WebDownloadCurl.cpp:
(WebDownload::init):
(WebDownload::start):
(WebDownload::cancel):
(WebDownload::deletesFileUponFailure):
(WebDownload::setDeletesFileUponFailure):
(WebDownload::setDestination):
(WebDownload::didReceiveResponse):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@187436 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/network/curl/CurlDownload.cpp
Source/WebCore/platform/network/curl/CurlDownload.h
Source/WebKit/win/ChangeLog
Source/WebKit/win/WebDownload.h
Source/WebKit/win/WebDownloadCurl.cpp

index 933b2e7..906a55a 100644 (file)
@@ -1,3 +1,25 @@
+2015-07-27  Per Arne Vollan  <peavo@outlook.com>
+
+        [Curl] Crash in CurlDownload::didReceiveHeader when downloading file.
+        https://bugs.webkit.org/show_bug.cgi?id=146832
+
+        Reviewed by Darin Adler.
+
+        Only call ResourceResponse::setMimeType from the main thread.
+        Also, CurlDownload should be reference counted to make sure it
+        still lives when a function call is invoked on the main thread
+        from the download thread.
+
+        * platform/network/curl/CurlDownload.cpp:
+        (WebCore::CurlDownloadManager::downloadThread):
+        (WebCore::CurlDownload::CurlDownload):
+        (WebCore::CurlDownload::start):
+        (WebCore::CurlDownload::didReceiveHeader):
+        (WebCore::CurlDownload::didReceiveData):
+        * platform/network/curl/CurlDownload.h:
+        (WebCore::CurlDownloadListener::didFail):
+        (WebCore::CurlDownload::setListener):
+
 2015-07-27  Matthew Daiter  <mdaiter@apple.com>
 
         Remove duplicate vectors inside of UserMediaRequest
index 7ec3f99..e4d9f62 100644 (file)
@@ -209,17 +209,19 @@ void CurlDownloadManager::downloadThread(void* data)
         CURLcode err = curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE, &download);
 
         if (msg->msg == CURLMSG_DONE) {
-            if (msg->data.result == CURLE_OK)
-                callOnMainThread([download] {
-                    if (download)
+            if (download) {
+                if (msg->data.result == CURLE_OK) {
+                    callOnMainThread([download] {
                         download->didFinish();
-                });
-            else
-                callOnMainThread([download] {
-                    if (download)
+                        download->deref(); // This matches the ref() in CurlDownload::start().
+                    });
+                } else {
+                    callOnMainThread([download] {
                         download->didFail();
-                });
-
+                        download->deref(); // This matches the ref() in CurlDownload::start().
+                    });
+                }
+            }
             downloadManager->removeFromCurl(msg->easy_handle);
         }
 
@@ -232,12 +234,12 @@ void CurlDownloadManager::downloadThread(void* data)
 CurlDownloadManager CurlDownload::m_downloadManager;
 
 CurlDownload::CurlDownload()
-: m_curlHandle(0)
-, m_customHeaders(0)
-, m_url(0)
-, m_tempHandle(invalidPlatformFileHandle)
-, m_deletesFileUponFailure(false)
-, m_listener(0)
+    : m_curlHandle(nullptr)
+    , m_customHeaders(nullptr)
+    , m_url(nullptr)
+    , m_tempHandle(invalidPlatformFileHandle)
+    , m_deletesFileUponFailure(false)
+    , m_listener(nullptr)
 {
 }
 
@@ -304,6 +306,7 @@ void CurlDownload::init(CurlDownloadListener* listener, ResourceHandle*, const R
 
 bool CurlDownload::start()
 {
+    ref(); // CurlDownloadManager::downloadThread will call deref when the download has finished.
     return m_downloadManager.add(m_curlHandle);
 }
 
@@ -397,19 +400,31 @@ void CurlDownload::didReceiveHeader(const String& header)
         if (httpCode >= 200 && httpCode < 300) {
             const char* url = 0;
             err = curl_easy_getinfo(m_curlHandle, CURLINFO_EFFECTIVE_URL, &url);
-            m_response.setURL(URL(ParsedURLString, url));
 
-            m_response.setMimeType(extractMIMETypeFromMediaType(m_response.httpHeaderField(HTTPHeaderName::ContentType)));
-            m_response.setTextEncodingName(extractCharsetFromMediaType(m_response.httpHeaderField(HTTPHeaderName::ContentType)));
+            String strUrl(url);
+            StringCapture capturedUrl(strUrl);
+
+            RefPtr<CurlDownload> protectedDownload(this);
+
+            callOnMainThread([this, capturedUrl, protectedDownload] {
+                m_response.setURL(URL(ParsedURLString, capturedUrl.string()));
+
+                m_response.setMimeType(extractMIMETypeFromMediaType(m_response.httpHeaderField(HTTPHeaderName::ContentType)));
+                m_response.setTextEncodingName(extractCharsetFromMediaType(m_response.httpHeaderField(HTTPHeaderName::ContentType)));
 
-            callOnMainThread([this] {
                 didReceiveResponse();
             });
         }
     } else {
-        int splitPos = header.find(":");
-        if (splitPos != -1)
-            m_response.setHTTPHeaderField(header.left(splitPos), header.substring(splitPos+1).stripWhiteSpace());
+        StringCapture capturedHeader(header);
+
+        RefPtr<CurlDownload> protectedDownload(this);
+
+        callOnMainThread([this, capturedHeader, protectedDownload] {
+            int splitPos = capturedHeader.string().find(":");
+            if (splitPos != -1)
+                m_response.setHTTPHeaderField(capturedHeader.string().left(splitPos), capturedHeader.string().substring(splitPos + 1).stripWhiteSpace());
+        });
     }
 }
 
@@ -417,7 +432,9 @@ void CurlDownload::didReceiveData(void* data, int size)
 {
     MutexLocker locker(m_mutex);
 
-    callOnMainThread([this, size] {
+    RefPtr<CurlDownload> protectedDownload(this);
+
+    callOnMainThread([this, size, protectedDownload] {
         didReceiveDataOfLength(size);
     });
 
index 8949d72..64ba8a5 100644 (file)
@@ -85,7 +85,7 @@ public:
     virtual void didFail() { }
 };
 
-class CurlDownload {
+class CurlDownload : public ThreadSafeRefCounted<CurlDownload> {
 public:
     CurlDownload();
     ~CurlDownload();
@@ -93,6 +93,8 @@ public:
     void init(CurlDownloadListener*, const WebCore::URL&);
     void init(CurlDownloadListener*, ResourceHandle*, const ResourceRequest&, const ResourceResponse&);
 
+    void setListener(CurlDownloadListener* listener) { m_listener = listener; }
+
     bool start();
     bool cancel();
 
index d49cb7b..6a61d5f 100644 (file)
@@ -1,3 +1,24 @@
+2015-07-27  Per Arne Vollan  <peavo@outlook.com>
+
+        [Curl] Crash in CurlDownload::didReceiveHeader when downloading file.
+        https://bugs.webkit.org/show_bug.cgi?id=146832
+
+        Reviewed by Darin Adler.
+
+        CurlDownload should be reference counted to make sure it still
+        lives when a function call is invoked on the main thread from
+        the download thread.
+
+        * WebDownload.h:
+        * WebDownloadCurl.cpp:
+        (WebDownload::init):
+        (WebDownload::start):
+        (WebDownload::cancel):
+        (WebDownload::deletesFileUponFailure):
+        (WebDownload::setDeletesFileUponFailure):
+        (WebDownload::setDestination):
+        (WebDownload::didReceiveResponse):
+
 2015-07-24  Brent Fulgham  <bfulgham@apple.com>
 
         [Win] Connect UserScript and UserStyleSheet through WebView.
index d4ccd79..9210423 100644 (file)
@@ -141,7 +141,7 @@ protected:
 #if USE(CFNETWORK)
     RetainPtr<CFURLDownloadRef> m_download;
 #elif USE(CURL)
-    WebCore::CurlDownload m_download;
+    RefPtr<WebCore::CurlDownload> m_download;
 #endif
     COMPtr<IWebMutableURLRequest> m_request;
     COMPtr<IWebDownloadDelegate> m_delegate;
index 2ba4398..5d6479c 100644 (file)
@@ -63,7 +63,8 @@ void WebDownload::init(ResourceHandle* handle, const ResourceRequest& request, c
 
     m_delegate = delegate;
 
-    m_download.init(this, handle, request, response);
+    m_download = adoptRef(new CurlDownload());
+    m_download->init(this, handle, request, response);
 
     start();
 }
@@ -72,7 +73,8 @@ void WebDownload::init(const URL& url, IWebDownloadDelegate* delegate)
 {
     m_delegate = delegate;
 
-    m_download.init(this, url);
+    m_download = adoptRef(new CurlDownload());
+    m_download->init(this, url);
 }
 
 // IWebDownload -------------------------------------------------------------------
@@ -101,7 +103,10 @@ HRESULT STDMETHODCALLTYPE WebDownload::initToResumeWithBundle(
 
 HRESULT STDMETHODCALLTYPE WebDownload::start()
 {
-    if (!m_download.start())
+    if (!m_download)
+        return E_FAIL;
+
+    if (!m_download->start())
         return E_FAIL;
 
     if (m_delegate)
@@ -112,9 +117,15 @@ HRESULT STDMETHODCALLTYPE WebDownload::start()
 
 HRESULT STDMETHODCALLTYPE WebDownload::cancel()
 {
-    if (!m_download.cancel())
+    if (!m_download)
+        return E_FAIL;
+
+    if (!m_download->cancel())
         return E_FAIL;
 
+    m_download->setListener(nullptr);
+    m_download = nullptr;
+
     return S_OK;
 }
 
@@ -127,14 +138,20 @@ HRESULT STDMETHODCALLTYPE WebDownload::cancelForResume()
 HRESULT STDMETHODCALLTYPE WebDownload::deletesFileUponFailure(
         /* [out, retval] */ BOOL* result)
 {
-    *result = m_download.deletesFileUponFailure() ? TRUE : FALSE;
+    if (!m_download)
+        return E_FAIL;
+
+    *result = m_download->deletesFileUponFailure() ? TRUE : FALSE;
     return S_OK;
 }
 
 HRESULT STDMETHODCALLTYPE WebDownload::setDeletesFileUponFailure(
         /* [in] */ BOOL deletesFileUponFailure)
 {
-    m_download.setDeletesFileUponFailure(deletesFileUponFailure);
+    if (!m_download)
+        return E_FAIL;
+
+    m_download->setDeletesFileUponFailure(deletesFileUponFailure);
     return S_OK;
 }
 
@@ -142,9 +159,12 @@ HRESULT STDMETHODCALLTYPE WebDownload::setDestination(
         /* [in] */ BSTR path, 
         /* [in] */ BOOL allowOverwrite)
 {
+    if (!m_download)
+        return E_FAIL;
+
     size_t len = wcslen(path);
     m_destination = String(path, len);
-    m_download.setDestination(m_destination);
+    m_download->setDestination(m_destination);
     return S_OK;
 }
 
@@ -177,7 +197,7 @@ void WebDownload::didReceiveResponse()
     COMPtr<WebDownload> protect = this;
 
     if (m_delegate) {
-        ResourceResponse response = m_download.getResponse();
+        ResourceResponse response = m_download->getResponse();
         COMPtr<WebURLResponse> webResponse(AdoptCOM, WebURLResponse::createInstance(response));
         m_delegate->didReceiveResponse(this, webResponse.get());