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).
Related bug: https://bugs.webkit.org/show_bug.cgi?id=174845
Created attachment 317390 [details] patch
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
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
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?
(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.
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.
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.
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.
Created attachment 317654 [details] patch
Are there any regressions in the http layout tests with this patch?
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.
(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.
(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!
Ah, did you submit your comments? Unless that, they're gone unfortunately...
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.
Ran http layout tests. base: 1637 passes patched: 1676 passes It seems slightly got better.
Created attachment 317969 [details] patch
(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?
Created attachment 318215 [details] patch part 1
(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.
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?
Created attachment 318435 [details] patch part 1
(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.
Comment on attachment 318435 [details] patch part 1 Clearing flags on attachment: 318435 Committed r220897: <http://trac.webkit.org/changeset/220897>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33955494>