Bug 156722 - Consider exposing or hiding knowledge of a redirect from clients of WebCoreNSURLSession
Summary: Consider exposing or hiding knowledge of a redirect from clients of WebCoreNS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh OS X 10.11
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-04-18 16:37 PDT by Daniel Bates
Modified: 2016-06-24 17:46 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.66 KB, patch)
2016-06-21 11:08 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (1.11 MB, application/zip)
2016-06-21 12:05 PDT, Build Bot
no flags Details
Patch (5.54 KB, patch)
2016-06-24 11:33 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2016-04-18 16:37:18 PDT
We may want to consider exposing or hiding knowledge of whether a redirect occurred from clients of WebCoreNSURLSession.
Comment 1 Daniel Bates 2016-04-18 16:37:28 PDT
<rdar://problem/25780035>
Comment 2 Jer Noble 2016-06-21 11:08:10 PDT
Created attachment 281760 [details]
Patch
Comment 3 Build Bot 2016-06-21 12:05:47 PDT
Comment on attachment 281760 [details]
Patch

Attachment 281760 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1542985

Number of test failures exceeded the failure limit.
Comment 4 Build Bot 2016-06-21 12:05:50 PDT
Created attachment 281765 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Alexey Proskuryakov 2016-06-21 12:14:18 PDT
Two failures on mac-debug:

- web-platform-tests failure sshould be fixed by http://trac.webkit.org/changeset/202282

- transitions failures are probably unrelated, although it's suspicious that they happened twice in a row.
Comment 6 Alex Christensen 2016-06-24 00:48:22 PDT
Comment on attachment 281760 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Fixes tests: http/tests/security/contentSecurityPolicy/audio-redirect-allowed2.html
> +                     http/tests/security/contentSecurityPolicy/video-redirect-allowed2.html

I think Alexey upstreamed the sierra test results.  I think this should have some kind of change in the test expectations associated with it.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:538
> +        auto responseData = response.crossThreadData();
> +        responseData.url = URL(self.currentRequest.URL);
> +        strongResponse = ResourceResponseBase::fromCrossThreadData(WTFMove(responseData)).nsURLResponse();

This is a strange way to set the url of a response.  If we really do need to copy the whole thing, do we really need to call crossThreadData?

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:606
> +    // current request during responseReceieved: to work around a CoreMedia bug.

you should put the radar in the comment so we know when we can remove this.  Same above.
Comment 7 Jer Noble 2016-06-24 09:27:04 PDT
(In reply to comment #6)
> Comment on attachment 281760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281760&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Fixes tests: http/tests/security/contentSecurityPolicy/audio-redirect-allowed2.html
> > +                     http/tests/security/contentSecurityPolicy/video-redirect-allowed2.html
> 
> I think Alexey upstreamed the sierra test results.  I think this should have
> some kind of change in the test expectations associated with it.

Aha, at the time I wrote this patch, they had not yet been upstreamed. I'll add them.

> > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:538
> > +        auto responseData = response.crossThreadData();
> > +        responseData.url = URL(self.currentRequest.URL);
> > +        strongResponse = ResourceResponseBase::fromCrossThreadData(WTFMove(responseData)).nsURLResponse();
> 
> This is a strange way to set the url of a response.  If we really do need to
> copy the whole thing, do we really need to call crossThreadData?

There's no other way to set the URL of a response apart from recreating it completely. 

> > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:606
> > +    // current request during responseReceieved: to work around a CoreMedia bug.
> 
> you should put the radar in the comment so we know when we can remove this. 
> Same above.

Ok.
Comment 8 Jer Noble 2016-06-24 11:33:06 PDT
Created attachment 282003 [details]
Patch
Comment 9 Daniel Bates 2016-06-24 14:54:26 PDT
We should update the title of this bug to reflect the decision that we made.
Comment 10 WebKit Commit Bot 2016-06-24 17:46:40 PDT
Comment on attachment 282003 [details]
Patch

Clearing flags on attachment: 282003

Committed r202466: <http://trac.webkit.org/changeset/202466>
Comment 11 WebKit Commit Bot 2016-06-24 17:46:45 PDT
All reviewed patches have been landed.  Closing bug.