Bug 164662

Created attachment 294563 [details] Patch and layout test I am not a loader/ResourceHandle/NetworkLoad expert. I am open to suggestions on the approach of this patch.

Comment on attachment 294563 [details] Patch and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=294563&action=review Why do we want to move this to the NetworkProcess? Do we care about WebSockets here? > Source/WebCore/platform/network/ResourceHandle.cpp:153 > +void ResourceHandle::didReceiveResponse(ResourceResponse&& response) If we do something with ResourceHandle, we should do it with NetworkDataTask, too. I'm not sure we should do this here, though. > LayoutTests/platform/wk2/TestExpectations:551 > +# Internals.registerDefaultPortForProtocol() does not affect NetworkProcess We shouldn't expand this.

(In reply to comment #2) > Comment on attachment 294563 [details] > Patch and layout test > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294563&action=review > > Why do we want to move this to the NetworkProcess? Do we care about > WebSockets here? I believe we already have been blocking WebSockets that use HTTP 0.9 (at least in soup ports). I have seen several websites broken as a result, but I don't remember any off the top of my head. But I might be remembering wrongly; maybe they used HTTP 1.0. I wish I could remember which websites were affected.

(In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 294563 [details] > > Patch and layout test > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=294563&action=review > > > > Why do we want to move this to the NetworkProcess? Do we care about > > WebSockets here? > > I believe we already have been blocking WebSockets that use HTTP 0.9 (at > least in soup ports). I have seen several websites broken as a result, but I > don't remember any off the top of my head. But I might be remembering > wrongly; maybe they used HTTP 1.0. I wish I could remember which websites > were affected. It is not only okay to break legacy websites that use WebSockets over obsolete comm channels - It is encouraged!

Comment on attachment 294563 [details] Patch and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=294563&action=review >> Source/WebCore/platform/network/ResourceHandle.cpp:153 >> +void ResourceHandle::didReceiveResponse(ResourceResponse&& response) > > If we do something with ResourceHandle, we should do it with NetworkDataTask, too. I'm not sure we should do this here, though. Yes: The NetworkLoad parts of this should be NetworkDataTask. Sorry for telling you the wrong one in person, Dan. But as for Alex's other point; Yes, doing this at the lowest level of the networking abstractions *is* where we should do it.

(In reply to comment #2) > Why do we want to move this to the NetworkProcess? We want to do this blocking of HTTP 0.9 at the lowest network abstraction level to to prevent circumvention. > Do we care about WebSockets here? > We do not allow an HTTP 0.9 response for the WebSocket handshake by <http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/websockets/WebSocketHandshake.cpp?rev=208601#L481> and <http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/websockets/WebSocketHandshake.cpp?rev=208601#L389>. > > Source/WebCore/platform/network/ResourceHandle.cpp:153 > > +void ResourceHandle::didReceiveResponse(ResourceResponse&& response) > > If we do something with ResourceHandle, we should do it with > NetworkDataTask, too. [...] > You're right! > > LayoutTests/platform/wk2/TestExpectations:551 > > +# Internals.registerDefaultPortForProtocol() does not affect NetworkProcess > > We shouldn't expand this. I will update the comment to emphasize that we should remove this function.

Created attachment 294706 [details] Patch and layout tests Updated patch based on feedback from Alex Christensen and Brady Eidson.

Web Sockets are restricted to HTTP 1.1 and above as of https://bugs.webkit.org/show_bug.cgi?id=82714.

Comment on attachment 294706 [details] Patch and layout tests LGTM, I think Alex should take a look too

Comment on attachment 294706 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=294706&action=review elegant > Source/WebKit2/ChangeLog:36 > +2016-11-11 Daniel Bates <dabates@apple.com> This has two ChangeLog entries.

<rdar://problem/29268514>

Committed r208732: <http://trac.webkit.org/changeset/208732>