Bug 175253 – [Curl] Improve multi-threaded networking

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daewoong Jang]

This patch makes multi-threaded curl network more robust by hiding away curl handles from main thread. The new class ResourceHandleCurlDelegate(named after ResourceHandleCFURLConnectionDelegate) stays between threads and manages communication between them. Important thing is that curl APIs are now no longer called from main thread(except for allocation/deallocation of handle, this also should be removed later).

[Daewoong Jang]

Related bug: https://bugs.webkit.org/show_bug.cgi?id=174845

[Daewoong Jang]

Created attachment 317390 [details]
patch

[Build Bot]

Comment on attachment 317390 [details]
patch

Attachment 317390 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4267347

New failing tests:
fast/block/positioning/auto/vertical-lr/001.html

[Build Bot]

Created attachment 317396 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Basuke Suzuki]

Created attachment 317397 [details]
Another patch

Beside Sony are also improving this implementation to make the port stable, I don't agree with this direction because it ties the system with ResourceHandle too much. As we've proposed to the dev mailing list, we are developing WebKit for WinCairo and that must end up with Curl port for NetworkLoadTask. Sticking around with ResourceHandle means we cannot be away form old architecture. The better way is to make a class who doesn't directly related to ResourceHandle which means can be usable from NetworkLoadTask.

I've attached our current effort which we hesitated to send now because it seems big enough to be rejected by reviewers, but because of the delay, I have to compete with others. I am so sorry about that. Can you take a look at this patch also?

[Daewoong Jang]

(In reply to Basuke Suzuki from comment #5)
> Created attachment 317397 [details]
> Another patch
> 
> Beside Sony are also improving this implementation to make the port stable,
> I don't agree with this direction because it ties the system with
> ResourceHandle too much. As we've proposed to the dev mailing list, we are
> developing WebKit for WinCairo and that must end up with Curl port for
> NetworkLoadTask. Sticking around with ResourceHandle means we cannot be away
> form old architecture. The better way is to make a class who doesn't
> directly related to ResourceHandle which means can be usable from
> NetworkLoadTask.
> 
> I've attached our current effort which we hesitated to send now because it
> seems big enough to be rejected by reviewers, but because of the delay, I
> have to compete with others. I am so sorry about that. Can you take a look
> at this patch also?

What I'm trying to do is keeping current status stable - I don't want to get across your architectural goals. My patch is basically based on your recent work. They're not competing each other, I believe.

I have to fix curl backend if it seems unstable and it doesn't mean to interfere you. If you like to avoid that, I hope you to keep curl backend always stable while you're working on it. I'll try your patch.

[Basuke Suzuki]

Okay, I understand. Stability is one of the most important thing than adding new feature. I agree your patch. We will try to rebase our code on top of your patch.

[Basuke Suzuki]

Also this time, please remind that your patch blocks us to submit our patch. We want you to make this patch reviewed as quickly as possible.

[Basuke Suzuki]

I don't agree removing callOnJobThread() from CurlJobManager. This is required from whole clients of CurlJobManager and it should be in the class.

'update()' may not happen so many times. I cannot see any positive meaning to call update() to each job every time of job's run loop.

[Daewoong Jang]

Created attachment 317654 [details]
patch

[Per Arne Vollan]

Are there any regressions in the http layout tests with this patch?

[Basuke Suzuki]

Comment on attachment 317654 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317654&action=review

This patch makes the curl port stable so that I totally +1 to this patch except those trivial things I mentioned inline.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:85
> +    CurlJobTicket job = m_job;

use auto.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:107
> +}

Why do you define alias of def/deref? I think def/deref is better inside WebCore.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:189
> +            callOnMainThread([protectedThis = makeRef(*this), url = url.isolatedCopy()] {

I've got a review from Alex the other day to capture this explicitly even if protectedThis exists.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:199
> +        callOnMainThread([protectedThis = makeRef(*this), header = header.isolatedCopy()] {

ditto.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:213
> +        protectedThis->didReceiveDataOfLength(size);

ditto.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:-286
> -

These are unused method. Safe to remove.

[Daewoong Jang]

(In reply to Per Arne Vollan from comment #11)
> Are there any regressions in the http layout tests with this patch?

I believe there is none.

[Daewoong Jang]

(In reply to Basuke Suzuki from comment #12)
> Comment on attachment 317654 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317654&action=review
> 
> This patch makes the curl port stable so that I totally +1 to this patch
> except those trivial things I mentioned inline.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:85
> > +    CurlJobTicket job = m_job;
> 
> use auto.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:107
> > +}
> 
> Why do you define alias of def/deref? I think def/deref is better inside
> WebCore.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:189
> > +            callOnMainThread([protectedThis = makeRef(*this), url = url.isolatedCopy()] {
> 
> I've got a review from Alex the other day to capture this explicitly even if
> protectedThis exists.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:199
> > +        callOnMainThread([protectedThis = makeRef(*this), header = header.isolatedCopy()] {
> 
> ditto.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:213
> > +        protectedThis->didReceiveDataOfLength(size);
> 
> ditto.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:-286
> > -
> 
> These are unused method. Safe to remove.

I left comments on review page. Thanks!

[Basuke Suzuki]

Ah, did you submit your comments? Unless that, they're gone unfortunately...

[Daewoong Jang]

Comment on attachment 317654 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317654&action=review

>>> Source/WebCore/platform/network/curl/CurlDownload.cpp:85
>>> +    CurlJobTicket job = m_job;
>> 
>> use auto.
> 

CurlJobTicket is a trivial type. Are there any good reasons to use auto?

>> Source/WebCore/platform/network/curl/CurlDownload.cpp:107
>> +}
> 
> Why do you define alias of def/deref? I think def/deref is better inside WebCore.

retain() and release() are callbacks used for keeping references from worker thread.

>> Source/WebCore/platform/network/curl/CurlDownload.cpp:189
>> +            callOnMainThread([protectedThis = makeRef(*this), url = url.isolatedCopy()] {
> 
> I've got a review from Alex the other day to capture this explicitly even if protectedThis exists.

Please refer to ResourceHandleCFURLConnectionDelegate. It doesn't use this in its lambdas and I think it is better way. I think Alex told you so because <this> was being used.

>> Source/WebCore/platform/network/curl/CurlDownload.cpp:-286
>> -
> 
> These are unused method. Safe to remove.

Thank you for review.

[Daewoong Jang]

Ran http layout tests.

base: 1637 passes
patched: 1676 passes

It seems slightly got better.

[Daewoong Jang]

Created attachment 317969 [details]
patch

[Per Arne Vollan]

(In reply to Daewoong Jang from comment #18)
> Created attachment 317969 [details]
> patch

This seems to be a big patch. Is it possible to break it down into smaller, independent patches?

[Daewoong Jang]

Created attachment 318215 [details]
patch part 1

[Daewoong Jang]

(In reply to Per Arne Vollan from comment #19)
> (In reply to Daewoong Jang from comment #18)
> > Created attachment 317969 [details]
> > patch
> 
> This seems to be a big patch. Is it possible to break it down into smaller,
> independent patches?

The original patch is divided into two pieces as you suggested.

[Alex Christensen]

Comment on attachment 318215 [details]
patch part 1

View in context: https://bugs.webkit.org/attachment.cgi?id=318215&action=review

r=me with a few comments.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:64
> +    m_url = url.isolatedCopy();

We make an isolated copy when we are actually going to another thread.  This seems unnecessary.

> Source/WebCore/platform/network/curl/ResourceError.h:59
> -    unsigned m_sslErrors { 0 };
> +    unsigned m_sslErrors;

Initializer lists are all the new c++11 coolness.  Removing it in favor of initializers in each constructor is a step in the wrong direction.

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:148
> +        m_curlHandle.setSslCertType("P12");

Is anyone actually testing client certificate authentication with curl?

[Daewoong Jang]

Created attachment 318435 [details]
patch part 1

[Daewoong Jang]

(In reply to Alex Christensen from comment #22)
> Comment on attachment 318215 [details]
> patch part 1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318215&action=review
> 
> r=me with a few comments.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:64
> > +    m_url = url.isolatedCopy();
> 
> We make an isolated copy when we are actually going to another thread.  This
> seems unnecessary.
> 
> > Source/WebCore/platform/network/curl/ResourceError.h:59
> > -    unsigned m_sslErrors { 0 };
> > +    unsigned m_sslErrors;
> 
> Initializer lists are all the new c++11 coolness.  Removing it in favor of
> initializers in each constructor is a step in the wrong direction.
> 

Right. Fixed.


> > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:148
> > +        m_curlHandle.setSslCertType("P12");
> 
> Is anyone actually testing client certificate authentication with curl?

I'm not sure. I think we need nightly build and layout test to keep wincairo right. In this patch, this is just a simple code move so I believe testing its functionality is beyond the objective of this patch.

[WebKit Commit Bot]

Comment on attachment 318435 [details]
patch part 1

Clearing flags on attachment: 318435

Committed r220897: <http://trac.webkit.org/changeset/220897>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33955494>