Set Sec-WebSocket-Protocol for WebSocket NSURLSession code path
<rdar://problem/66996369>
Created attachment 406601 [details] Patch
Comment on attachment 406601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406601&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1571 > + [requestWithProtocols addValue: StringView(protocol).createNSStringWithoutCopying().get() forHTTPHeaderField:@"Sec-WebSocket-Protocol"]; This seems unsafe. Why not just make a copy?
Comment on attachment 406601 [details] Patch This seems like a good workaround for the fact that there is webSocketTaskWithURL:protocols: but no webSocketTaskWithRequest:protocols:. Let's introduce that API and do this on operating system versions that don't have it.
(In reply to Alex Christensen from comment #4) > Comment on attachment 406601 [details] > Patch > > This seems like a good workaround for the fact that there is > webSocketTaskWithURL:protocols: but no webSocketTaskWithRequest:protocols:. > Let's introduce that API and do this on operating system versions that don't > have it. Actually there is, let's use that.
There isn't on Catalina.
(In reply to Alex Christensen from comment #6) > There isn't on Catalina. We might actually want to bump HAVE_NSURLSESSION_WEBSOCKET to Big Sur.
(In reply to Alex Christensen from comment #6) > There isn't on Catalina. I looked too quickly, there is no such thing given we also want to set other headers like User-Agent.
Created attachment 406606 [details] Patch
There is no variant of WebSocket task with request and protocols. The only variant with protocols is the one that takes a url. The client can add the protocol headers manually on the request which would achieve the same result. I think in WebKit's case we want to pass along the request since other information about main document etc will also be needed to lookup cookies etc, so moving to the url variant might not work.
> I think in WebKit's case we want to pass along the request since other > information about main document etc will also be needed to lookup cookies > etc, so moving to the url variant might not work. Right, I do not think offering webSocketTaskWithRequest:protocols: provides much really. If you are using webSocketTaskWithRequest, you are already out of WebSocket standard API.
(In reply to youenn fablet from comment #11) > > I think in WebKit's case we want to pass along the request since other > > information about main document etc will also be needed to lookup cookies > > etc, so moving to the url variant might not work. > > Right, I do not think offering webSocketTaskWithRequest:protocols: provides > much really. > If you are using webSocketTaskWithRequest, you are already out of WebSocket > standard API. Yup!
Comment on attachment 406606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406606&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1569 > + auto *nsRequest = request.nsURLRequest(WebCore::HTTPBodyUpdatePolicy::DoNotUpdateHTTPBody); I think this returns an NSMutableURLRequest, so we could avoid a copy. Probably not too important here. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1571 > + NSMutableURLRequest *requestWithProtocols = [[nsRequest mutableCopy] autorelease]; adoptNS could avoid putting a reference in the autorelease pool.
Comment on attachment 406606 [details] Patch ok
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1569 > > + auto *nsRequest = request.nsURLRequest(WebCore::HTTPBodyUpdatePolicy::DoNotUpdateHTTPBody); > > I think this returns an NSMutableURLRequest, so we could avoid a copy. > Probably not too important here. It does not seem so from ResourceRequest::nsURLRequest.
Committed r265752: <https://trac.webkit.org/changeset/265752> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406606 [details].