Bug 154298 - Add checks before redirecting with NetworkSession
Summary: Add checks before redirecting with NetworkSession
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-16 11:11 PST by Alex Christensen
Modified: 2016-02-16 13:28 PST (History)
1 user (show)

See Also:


Attachments
Patch (11.00 KB, patch)
2016-02-16 11:17 PST, Alex Christensen
aestes: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-02-16 11:11:35 PST
Add checks before redirecting with NetworkSession
Comment 1 Alex Christensen 2016-02-16 11:17:39 PST
Created attachment 271449 [details]
Patch
Comment 2 Brian Weinstein 2016-02-16 12:57:50 PST
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.
Comment 3 Andy Estes 2016-02-16 13:14:38 PST
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?
Comment 4 Alex Christensen 2016-02-16 13:17:15 PST
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.
Comment 5 Andy Estes 2016-02-16 13:27:27 PST
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).
Comment 6 Alex Christensen 2016-02-16 13:28:22 PST
Done.  It's a reference now.
http://trac.webkit.org/changeset/196656