Summary: | Consider exposing or hiding knowledge of a redirect from clients of WebCoreNSURLSession | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||
Component: | WebCore Misc. | Assignee: | Jer Noble <jer.noble> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, ap, bfulgham, commit-queue, dbates, eric.carlson, mkwst, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.11 | ||||||||||
Attachments: |
|
Description
Daniel Bates
2016-04-18 16:37:18 PDT
Created attachment 281760 [details]
Patch
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. 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
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 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. (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. Created attachment 282003 [details]
Patch
We should update the title of this bug to reflect the decision that we made. Comment on attachment 282003 [details] Patch Clearing flags on attachment: 282003 Committed r202466: <http://trac.webkit.org/changeset/202466> All reviewed patches have been landed. Closing bug. |