Bug 156722 – Consider exposing or hiding knowledge of a redirect from clients of WebCoreNSURLSession

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

We may want to consider exposing or hiding knowledge of whether a redirect occurred from clients of WebCoreNSURLSession.

[Daniel Bates]

<rdar://problem/25780035>

[Jer Noble]

Created attachment 281760 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Alexey Proskuryakov]

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.

[Alex Christensen]

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.

[Jer Noble]

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

[Jer Noble]

Created attachment 282003 [details]
Patch

[Daniel Bates]

We should update the title of this bug to reflect the decision that we made.

[WebKit Commit Bot]

Comment on attachment 282003 [details]
Patch

Clearing flags on attachment: 282003

Committed r202466: <http://trac.webkit.org/changeset/202466>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.