Bug 215456 – Set Sec-WebSocket-Protocol for WebSocket NSURLSession code path

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Set Sec-WebSocket-Protocol for WebSocket NSURLSession code path

[Radar WebKit Bug Importer]

<rdar://problem/66996369>

[youenn fablet]

Created attachment 406601 [details]
Patch

[Alex Christensen]

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?

[Alex Christensen]

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.

[youenn fablet]

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

[Alex Christensen]

There isn't on Catalina.

[youenn fablet]

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

[youenn fablet]

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

[youenn fablet]

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.

[youenn fablet]

> 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!

[Alex Christensen]

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.

[Alex Christensen]

Comment on attachment 406606 [details]
Patch

ok

[youenn fablet]

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

[EWS]

Committed r265752: <https://trac.webkit.org/changeset/265752>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406606 [details].