Bug 154298 – Add checks before redirecting with NetworkSession

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

[Alex Christensen]

Add checks before redirecting with NetworkSession

[Alex Christensen]

Created attachment 271449 [details]
Patch

[Brian Weinstein]

Comment on attachment 271449 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:102
> +    if (!request.url().protocolIs("https") && WebCore::protocolIs(request.httpReferrer(), "https") && m_shouldClearReferrerOnHTTPSToHTTPRedirect)

You should probably put the fastest part of this check (checking m_shouldClearReferrerOnHTTPSToHTTPRedirect) first.

[Andy Estes]

Comment on attachment 271449 [details]
Patch

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

> Source/WebKit2/ChangeLog:20
> +        1. When using NSURLConnection, the last http method and the current http method are not always equal, and this
> +        check needs to change the current http method to match the last http method when being redirected with an http
> +        status code 307.  When using NSURLSession, the current http method is already updated properly.

Can you assert that it didn't change?

> Source/WebKit2/NetworkProcess/NetworkDataTask.h:114
> +    void willPerformHTTPRedirection(const WebCore::ResourceResponse, WebCore::ResourceRequest&&, RedirectCompletionHandler);

Do you really mean to pass ResourceResponse by value?

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:93
> +        if (!equalLettersIgnoringASCIICase(m_lastHTTPMethod, "get") && body && !body->isEmpty())

If you reordered the operands in the if expression to check body && !body->isEmpty() first, we'd be able to sometimes skip the more expensive string comparison.

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:117
> +    

Could you add a FIXME here for implementing the credential storage bits?

[Alex Christensen]

Comment on attachment 271449 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/NetworkDataTask.h:114
>> +    void willPerformHTTPRedirection(const WebCore::ResourceResponse, WebCore::ResourceRequest&&, RedirectCompletionHandler);
> 
> Do you really mean to pass ResourceResponse by value?

No.  Will fix.

>> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:117
>> +    
> 
> Could you add a FIXME here for implementing the credential storage bits?

We don't want to use a CredentialStorage if we can get away with it.

[Andy Estes]

Comment on attachment 271449 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/NetworkDataTask.h:114
>>> +    void willPerformHTTPRedirection(const WebCore::ResourceResponse, WebCore::ResourceRequest&&, RedirectCompletionHandler);
>> 
>> Do you really mean to pass ResourceResponse by value?
> 
> No.  Will fix.

You should still change it, but I guess in this case it doesn't actually matter. We're passing a NSHTTPURLResponse * into this function, so in either case we'll only construct one ResourceResponse (either a temporary that is bound to a const reference or the argument value).

[Alex Christensen]

Done.  It's a reference now.
http://trac.webkit.org/changeset/196656