Bug 131478 – Move early return out of dispatch_async() block so we can return from willSendRequest quickly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Pratik Solanki]

Move early return out of dispatch_async() block so we can return from willSendRequest quickly

[Pratik Solanki]

Created attachment 229025 [details]
Patch

[Pratik Solanki]

<rdar://problem/16575535>

[Alexey Proskuryakov]

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.

[Pratik Solanki]

Created attachment 229084 [details]
Patch

[Pratik Solanki]

(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.

[Alexey Proskuryakov]

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.

[Pratik Solanki]

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.

[Pratik Solanki]

(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.

[Pratik Solanki]

(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.

[Pratik Solanki]

Committed r167200: <http://trac.webkit.org/changeset/167200>