Bug 17754 – [Curl] Implement ResourceHandle::loadResourceSynchronously

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently we lack this method for synchronous request.

[Julien Chaffraix]

Created attachment 19644 [details]
First version

Implement all the necessary operations using an easy handle and curl_easy_perform for 2 reasons :
- curl_easy_perform blocks until the transfert is finished.
- using the multi handle is tricky as we can be called from the multi Curl API (throught a didFinishLoading for example) and that would cause random crashes in Curl.

[Holger Freyther]

The patch looks pretty good, so this is mostly food for thoughts:
   -startSync..Job well you do more than starting let us find a better name
   -there are at least two test cases for sync loading in loading/ IIRC, it would be cool if we start using them and you could verify that they work.
   - f we at some point have proper glib integration in curl, how would the sync handling change?

The only technical issue I see:
   - I'm not sure your usage of the String() c'tor is the one you want. Using String(m_url) but then again the current code is broken in this respect with this:

  d->m_url = strdup(url.latin1().data()); also ResourceHandleInternal initialize's m_url to 0 and if we do not call startJob m_url will stay zero and then we can get a crash in the d'tor of the ResourceHandleInternal. But these two issues predate this bug.

You might want to fix the encoding of m_url (or convince me why url.latin1() is always correct) and then update your ResourceError line and then land your patch.

[Julien Chaffraix]

(In reply to comment #2)
> The patch looks pretty good, so this is mostly food for thoughts:
>    -startSync..Job well you do more than starting let us find a better name

The name was chosen to match the one of the asynchronous case but you are right here (I will switch to dispatchSynchronousJob).

>    -there are at least two test cases for sync loading in loading/ IIRC, it
> would be cool if we start using them and you could verify that they work.

Actually all synchronous XHR uses that methods so it resolves a lot of test cases in http/tests/xmlhttprequest (I have tested the patch thoroughly on those).

>    - f we at some point have proper glib integration in curl, how would the
> sync handling change?
> 

There should be very little modification :
- we will always have to block WebKit main thread as it is already the case (to prevent for example continuing JavaScript execution during synchronous request)
- As I have understood it, Curl is completely reentrant so having 2 threads polling it should not be an issue (if it is wrong then a small synchronization should be added).

> The only technical issue I see:
>    - I'm not sure your usage of the String() c'tor is the one you want. Using
> String(m_url) but then again the current code is broken in this respect with
> this:
> 
>   d->m_url = strdup(url.latin1().data()); also ResourceHandleInternal

IIRC the URL should be in plain ASCII with special characters escaped with %XX. If it is the case, there does not seem to be any appropriate encoder / decoder. It should be quite easy to add one (that could be shared somewhere else).

> initialize's m_url to 0 and if we do not call startJob m_url will stay zero and
> then we can get a crash in the d'tor of the ResourceHandleInternal. But these
> two issues predate this bug.
> 

No, i have moved the initialization of the handle (which include m_url) in the initiliaze() method. It is called in both cases so we should not have any issue.

> You might want to fix the encoding of m_url (or convince me why url.latin1() is
> always correct) and then update your ResourceError line and then land your
> patch.
> 

[Julien Chaffraix]

Created attachment 19846 [details]
Updated patch

> IIRC the URL should be in plain ASCII with special characters escaped with %XX.
> If it is the case, there does not seem to be any appropriate encoder / decoder.
> It should be quite easy to add one (that could be shared somewhere else).

Forget that part : the KURL constructor already escapes characters.

> You might want to fix the encoding of m_url (or convince me why url.latin1() is
> always correct) and then update your ResourceError line and then land your
> patch.
> 

I did some testing and the encoding seems right mainly due to the KURL constructor's escaping as mentioned above. Thus calling latin1() is only a way of having a char* (no character translation occurs in latin1() as the string is already in ASCII).

[Julien Chaffraix]

Comment on attachment 19644 [details]
First version

Clear first patch review flag (forgot to mark it as obsolete).

[Holger Freyther]

Comment on attachment 19846 [details]
Updated patch

The encoded URL should be ascii/latin1. We will find out sooner or later. We should really try to get the layout regression suite going...

[Holger Freyther]

Yeah, landed in r31109.