Bug 13090 - curl (linux/gdk) networking improvements
Summary: curl (linux/gdk) networking improvements
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC OS X 10.4
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2007-03-15 19:30 PDT by Krzysztof Kowalczyk
Modified: 2007-05-29 23:22 PDT (History)
0 users

See Also:

curl networking improvements (14.05 KB, patch)
2007-03-15 19:31 PDT, Krzysztof Kowalczyk
mrowe: review-
Details | Formatted Diff | Diff
curl cleanup, take two (13.73 KB, patch)
2007-04-29 11:22 PDT, Alp Toker
mjs: review+
Details | Formatted Diff | Diff
Curl re-entrancy problem: tries to reuse semi-active handle (9.34 KB, text/plain)
2007-05-07 15:16 PDT, Kimmo Kinnunen
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Kowalczyk 2007-03-15 19:30:30 PDT
curl (linux/gdk) networking improvements
Comment 1 Krzysztof Kowalczyk 2007-03-15 19:31:38 PDT
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
Comment 2 Mark Rowe (bdash) 2007-04-18 09:42:15 PDT
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.
Comment 3 Alp Toker 2007-04-29 11:22:03 PDT
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.
Comment 4 Kimmo Kinnunen 2007-05-07 15:16:10 PDT
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, "");
Comment 5 Maciej Stachowiak 2007-05-29 00:35:52 PDT
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.
Comment 6 Maciej Stachowiak 2007-05-29 00:36:45 PDT
Comment on attachment 14268 [details]
curl cleanup, take two

Comment 7 Mark Rowe (bdash) 2007-05-29 23:22:58 PDT
Landed in r21885.