Bug 13090 – curl (linux/gdk) networking improvements

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Krzysztof Kowalczyk]

curl (linux/gdk) networking improvements

[Krzysztof Kowalczyk]

Created attachment 13653 [details]
curl networking improvements

Curl (linux/gdk) networking improvements: 
a) POST support
b) don't use CURL in a re-entrant way.
c) for easier debugging, in debug build if env var DEBUG_CURL is set,
   turn on curl's internal debbuging

[Mark Rowe (bdash)]

Comment on attachment 13653 [details]
curl networking improvements

+    if ((CURLE_OK == err) && ((301 == httpCode) || (302 == httpCode)))
+        return totalSize;
+

You have unneeded parenthesis here, and in several other places.


+        CString cstring =  elements[i].m_filename.utf8();

An extra space after the =.

+        // Fill in the file upload field
+        curl_formadd(&post,
+                      &lastItem,
+                      CURLFORM_COPYNAME, "sendfile",
+                      CURLFORM_FILE, d->m_fileName,
+                      CURLFORM_END);
+ 
+        // Fill in the filename field
+        curl_formadd(&post,
+                      &lastItem,
+                      CURLFORM_COPYNAME, "filename",
+                      CURLFORM_COPYCONTENTS, d->m_fileName,
+                      CURLFORM_END);
+ 
+        // Fill in the submit field too, even if this is rarely needed
+        curl_formadd(&post,
+                      &lastItem,
+                      CURLFORM_COPYNAME, "submit",
+                      CURLFORM_COPYCONTENTS, "send",
+                      CURLFORM_END);

This looks to be based on curl example code.  It seems incorrect to be adding a form field named "submit" for each file that is uploaded.  I think the indentation style of this code block could be cleaned up a great deal too.


+        if (job == node->job()) {
+            node->setRemoved(true);
+            return true;
+            node = next;
+        }

I think "node = next;" should be outside the braces.


+class ResourceHandleList
+{

Brace should be on same line as class statement.

[Alp Toker]

Created attachment 14268 [details]
curl cleanup, take two

This patch attempts to address some of the issues in the review. I'm not entirely familiar with what it's doing, but I have pending patches for this area of code and would prefer to get Krzysztof's patch merged while it still applies.

Having this patch open in bugzilla is a blocker for new work being done on the curl backend, so we should try to get this patch applied or WONTFIX if necessary.

[Kimmo Kinnunen]

Created attachment 14401 [details]
Curl re-entrancy problem: tries to reuse semi-active handle

Segfault when libcurl tries to share handle which is not finished yet.

man curl_easy_handle:
"If the curl handles are used simultaneously, you MUST use  the  locking  methods  in  the  share  handle."

When libcurl-gnutls (ubuntu package) is concerned, "simultaneously" means not calling add when you have received callback from that originates from curl_multi_perform.


Temp fix:
--
Index: platform/network/gdk/ResourceHandleManager.cpp
===================================================================
--- platform/network/gdk/ResourceHandleManager.cpp      (revision 21288)
+++ platform/network/gdk/ResourceHandleManager.cpp      (working copy)
@@ -188,7 +188,6 @@ void ResourceHandleManager::add(Resource
     curl_easy_setopt(d->m_handle, CURLOPT_FOLLOWLOCATION, 1);
     curl_easy_setopt(d->m_handle, CURLOPT_MAXREDIRS, 10);
     curl_easy_setopt(d->m_handle, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
-    curl_easy_setopt(d->m_handle, CURLOPT_SHARE, m_curlShareHandle);
     // enable gzip and deflate through Accept-Encoding:
     curl_easy_setopt(d->m_handle, CURLOPT_ENCODING, "");

[Maciej Stachowiak]

Code changes look ok to me, though I am not a curl expert. It's been reported these make curl networking more stable, so let us get them landed for now. They'll be more thoroughly validated once we get http layout tests running for gdk and other ports that use curl.

[Maciej Stachowiak]

Comment on attachment 14268 [details]
curl cleanup, take two

r=me

[Mark Rowe (bdash)]

Landed in r21885.