| Summary: | Add checks before redirecting with NetworkSession | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | beidson | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Alex Christensen
2016-02-16 11:11:35 PST
Created attachment 271449 [details]
Patch
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 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 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 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). Done. It's a reference now. http://trac.webkit.org/changeset/196656 |