Move early return out of dispatch_async() block so we can return from willSendRequest quickly
Created attachment 229025 [details] Patch
<rdar://problem/16575535>
Comment on attachment 229025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229025&action=review > Source/WebCore/ChangeLog:9 > + We can do a check for redirect response on the dispatch queue and return from willSendRequest synthesizeRedirectResponseIfNecessary uses ResourceRequest class, which is not thread safe. The Foundation version of this function doesn't do that, so it's safe to call on a secondary thread. Good catch though, perhaps we can refactor the code to make this fix work.
Created attachment 229084 [details] Patch
(In reply to comment #4) > Created an attachment (id=229084) [details] > Patch Maybe something like this would work? We save the scheme from the request and do a quick comparison and early return if they match.
Comment on attachment 229084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229084&action=review I'm not sure if we need to follow the Mac pattern with secondary threads at all - it looks like CFURLConnection may have all the async delegates we could wish for. If that's feasible, that should be much better, and would make this patch unnecessary. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:86 > + if (!originalRedirectResponse) { > + RetainPtr<CFStringRef> newScheme = adoptCF(CFURLCopyScheme(CFURLRequestGetURL(cfRequest))); > + if (CFStringCompare(newScheme.get(), m_originalScheme.get(), kCFCompareCaseInsensitive) == kCFCompareEqualTo) { > + CFRetain(cfRequest); > + return cfRequest; > + } > + } Shouldn't this code be removed from synthesizeRedirectResponseIfNecessary() now? Would also be good to move a comment about HSTS here.
Comment on attachment 229084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229084&action=review >> Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:86 >> + } > > Shouldn't this code be removed from synthesizeRedirectResponseIfNecessary() now? > > Would also be good to move a comment about HSTS here. Yes, I can remove the code from synthesizeRedirectResponseIfNecessary() and I'll addd a comment.
(In reply to comment #6) > (From update of attachment 229084 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229084&action=review > > I'm not sure if we need to follow the Mac pattern with secondary threads at all - it looks like CFURLConnection may have all the async delegates we could wish for. > > If that's feasible, that should be much better, and would make this patch unnecessary. I'll take a look at this next.
(In reply to comment #7) > (From update of attachment 229084 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229084&action=review > > >> Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:86 > >> + } > > > > Shouldn't this code be removed from synthesizeRedirectResponseIfNecessary() now? > > > > Would also be good to move a comment about HSTS here. > > Yes, I can remove the code from synthesizeRedirectResponseIfNecessary() and I'll addd a comment. SynchronousResourceHandleCFURLConnectionDelegate::willSendRequest() calls synthesizeRedirectResponseIfNecessary() as well so I can't remove the code. I'll leave it in for now.
Committed r167200: <http://trac.webkit.org/changeset/167200>