Bug 199151 – [SOUP] Use libsoup WebSockets API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Instead of the WebKit one.

[Carlos Garcia Campos]

Created attachment 372750 [details]
WIP

This is a WIP patch. Old code is used unless WEBKIT_USE_SOUP_WEBSOCKETS env var is present. This is just temporary until the implementation is complete and we make layout tests pass. Most of them are failing with the patch, but in a lot of cases it's because of missing console messages that new code path doesn't generate. I'll add a comment with more information about the layout tests.

[Carlos Garcia Campos]

As I said most of the tests are failing because of missing console messages, so they are actually working and I won't mention them here.

http/tests/websocket/tests/hybi/bad-handshake-crash.html

This needs investigation, "WebSocket is open" message is unexpectedly shown in the output.

http/tests/websocket/tests/hybi/client-close.html

I've debugged this and I don't understand why it fails, the diff is:
-PASS closeEvent.reason is "close_frame[:2]='\\x88\\x80'"
+FAIL closeEvent.reason should be close_frame[:2]='\x88\x80'. Was close_frame[:2]='\x88\x82'.

http/tests/websocket/tests/hybi/close-code-and-reason.html

This one expects error codes that the spec says shouldn't be used like 1006. But close reason is also failing, so it needs some more investigation.

http/tests/websocket/tests/hybi/close.html

It seems onclose is not called in a couple of cases.

http/tests/websocket/tests/hybi/deflate-frame-parameter.html
http/tests/websocket/tests/hybi/extensions.html

x-webkit-deflate-frame extension not supported

http/tests/websocket/tests/hybi/handshake-fail-by-no-cr.html

Libsoup doesn't fail when status line finishes with \n since that's what the http spec suggests.

http/tests/websocket/tests/hybi/handshake-ok-with-http-version-beyond-1_1.html

There are a couple of cases where libsoup doesn't fail either.

http/tests/websocket/tests/hybi/inspector/*

It seems we are not notifying the inspector in some cases about websockets with the new code path.

http/tests/websocket/tests/hybi/null-character.html

libsoup API always expect a null terminated string for text frames. I haven't found anything in the spec about it, it just says that text frames should contains valid utf8, and null character si perfectly valid, so maybe we need to add new API in libsoup to allow passing char* + length.

http/tests/websocket/tests/hybi/send-object-tostring-check.html

It expects to be closed cleanly, but apparently it isn't.

http/tests/websocket/tests/hybi/simple-wss.html

We don't have a way to accept the invalid certificate only for tests.

http/tests/websocket/tests/hybi/inspector/binary.html
http/tests/websocket/tests/hybi/send-blob-onmessage-origin.html
http/tests/websocket/tests/hybi/send-blob.html

Blobs are not implemented in new code path.

[Michael Catanzaro]

Comment on attachment 372750 [details]
WIP

Amazing!

[youenn fablet]

Nice to see that.
We have similar test issues with the NSURLSession code path that we will need to handle one way or the other.

I run some of these tests with Chrome and Firefox and some of them fail with either one or both, which indicates that these tests might be WebKit implementation specific.

(In reply to Carlos Garcia Campos from comment #2)
> As I said most of the tests are failing because of missing console messages,
> so they are actually working and I won't mention them here.

Right, I am not sure we will be able to get back the exact same missing console messages given that part of the messages are generated by processing done by libsoup/nsurlsession now.

> 
> http/tests/websocket/tests/hybi/bad-handshake-crash.html
> 
> This needs investigation, "WebSocket is open" message is unexpectedly shown
> in the output.

I think we also have an issue there with NSURLSession.

> http/tests/websocket/tests/hybi/client-close.html
> 
> I've debugged this and I don't understand why it fails, the diff is:
> -PASS closeEvent.reason is "close_frame[:2]='\\x88\\x80'"
> +FAIL closeEvent.reason should be close_frame[:2]='\x88\x80'. Was
> close_frame[:2]='\x88\x82'.

Same issue here with NSURLSession.

> http/tests/websocket/tests/hybi/close-code-and-reason.html
> 
> This one expects error codes that the spec says shouldn't be used like 1006.
> But close reason is also failing, so it needs some more investigation.
> 
> http/tests/websocket/tests/hybi/close.html
> 
> It seems onclose is not called in a couple of cases.
> 
> http/tests/websocket/tests/hybi/deflate-frame-parameter.html
> http/tests/websocket/tests/hybi/extensions.html
> 
> x-webkit-deflate-frame extension not supported

The idea would be to move from x-webkit-deflate-frame to the standard version permessage-deflate.

> 
> http/tests/websocket/tests/hybi/handshake-fail-by-no-cr.html
> 
> Libsoup doesn't fail when status line finishes with \n since that's what the
> http spec suggests.
> 
> http/tests/websocket/tests/hybi/handshake-ok-with-http-version-beyond-1_1.
> html
> 
> There are a couple of cases where libsoup doesn't fail either.
> 
> http/tests/websocket/tests/hybi/inspector/*
> 
> It seems we are not notifying the inspector in some cases about websockets
> with the new code path.

I am not sure we will be ever able to get back the same level of inspector control with the new code path.

> http/tests/websocket/tests/hybi/null-character.html
> 
> libsoup API always expect a null terminated string for text frames. I
> haven't found anything in the spec about it, it just says that text frames
> should contains valid utf8, and null character si perfectly valid, so maybe
> we need to add new API in libsoup to allow passing char* + length.
> 
> http/tests/websocket/tests/hybi/send-object-tostring-check.html
> 
> It expects to be closed cleanly, but apparently it isn't.
> 
> http/tests/websocket/tests/hybi/simple-wss.html
> 
> We don't have a way to accept the invalid certificate only for tests.

The plan is to reuse the existing NSURLSession code path for these.
I would hope you could do the same for libsoup.

> http/tests/websocket/tests/hybi/inspector/binary.html
> http/tests/websocket/tests/hybi/send-blob-onmessage-origin.html
> http/tests/websocket/tests/hybi/send-blob.html
> 
> Blobs are not implemented in new code path.

Yep, this will be needed to implement.
This will require to queue additional outgoing messages until the blob is read/put in memory and sent.

[youenn fablet]

(In reply to Carlos Garcia Campos from comment #1)
> Created attachment 372750 [details]
> WIP
> 
> This is a WIP patch. Old code is used unless WEBKIT_USE_SOUP_WEBSOCKETS env
> var is present. This is just temporary until the implementation is complete
> and we make layout tests pass. Most of them are failing with the patch, but
> in a lot of cases it's because of missing console messages that new code
> path doesn't generate. I'll add a comment with more information about the
> layout tests.

Depending on how you want to do the switch, it might be feasible to rename/reuse NSURLSessionWebSocket runtime flag.

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #4)
> Nice to see that.
> We have similar test issues with the NSURLSession code path that we will
> need to handle one way or the other.

So, the NSURLSession impl is not used yet in production, and not tested by the bots either?

> I run some of these tests with Chrome and Firefox and some of them fail with
> either one or both, which indicates that these tests might be WebKit
> implementation specific.

I'll prioritize the ones that we knoe are bugs or missing features.

> (In reply to Carlos Garcia Campos from comment #2)
> > As I said most of the tests are failing because of missing console messages,
> > so they are actually working and I won't mention them here.
> 
> Right, I am not sure we will be able to get back the exact same missing
> console messages given that part of the messages are generated by processing
> done by libsoup/nsurlsession now.

Indeed, if we keep using console messages for those events, we will need platform specific test results. I don't know if it's possible to ignore those messages (if we end up adding them back) in layout tests to not show them in the results.

> > 
> > http/tests/websocket/tests/hybi/bad-handshake-crash.html
> > 
> > This needs investigation, "WebSocket is open" message is unexpectedly shown
> > in the output.
> 
> I think we also have an issue there with NSURLSession.
> 
> > http/tests/websocket/tests/hybi/client-close.html
> > 
> > I've debugged this and I don't understand why it fails, the diff is:
> > -PASS closeEvent.reason is "close_frame[:2]='\\x88\\x80'"
> > +FAIL closeEvent.reason should be close_frame[:2]='\x88\x80'. Was
> > close_frame[:2]='\x88\x82'.
> 
> Same issue here with NSURLSession.

Interesting, I debugged it for a while, but couldn't understand what's going on there.
 
> > http/tests/websocket/tests/hybi/close-code-and-reason.html
> > 
> > This one expects error codes that the spec says shouldn't be used like 1006.
> > But close reason is also failing, so it needs some more investigation.
> > 
> > http/tests/websocket/tests/hybi/close.html
> > 
> > It seems onclose is not called in a couple of cases.
> > 
> > http/tests/websocket/tests/hybi/deflate-frame-parameter.html
> > http/tests/websocket/tests/hybi/extensions.html
> > 
> > x-webkit-deflate-frame extension not supported
> 
> The idea would be to move from x-webkit-deflate-frame to the standard
> version permessage-deflate.

Since it's in a spec now I think we could implement it directly in libsoup, otherwise I'll add new API to libsoup to implement extensions. I guess this doesn't cause any regression in functionality, only that frames are not compressed.

> > 
> > http/tests/websocket/tests/hybi/handshake-fail-by-no-cr.html
> > 
> > Libsoup doesn't fail when status line finishes with \n since that's what the
> > http spec suggests.
> > 
> > http/tests/websocket/tests/hybi/handshake-ok-with-http-version-beyond-1_1.
> > html
> > 
> > There are a couple of cases where libsoup doesn't fail either.
> > 
> > http/tests/websocket/tests/hybi/inspector/*
> > 
> > It seems we are not notifying the inspector in some cases about websockets
> > with the new code path.
> 
> I am not sure we will be ever able to get back the same level of inspector
> control with the new code path.

I'll take a look.

> > http/tests/websocket/tests/hybi/null-character.html
> > 
> > libsoup API always expect a null terminated string for text frames. I
> > haven't found anything in the spec about it, it just says that text frames
> > should contains valid utf8, and null character si perfectly valid, so maybe
> > we need to add new API in libsoup to allow passing char* + length.
> > 
> > http/tests/websocket/tests/hybi/send-object-tostring-check.html
> > 
> > It expects to be closed cleanly, but apparently it isn't.
> > 
> > http/tests/websocket/tests/hybi/simple-wss.html
> > 
> > We don't have a way to accept the invalid certificate only for tests.
> 
> The plan is to reuse the existing NSURLSession code path for these.
> I would hope you could do the same for libsoup.

I'll check it too. We should be able, since the handshake happens in the session like a normal load.

> > http/tests/websocket/tests/hybi/inspector/binary.html
> > http/tests/websocket/tests/hybi/send-blob-onmessage-origin.html
> > http/tests/websocket/tests/hybi/send-blob.html
> > 
> > Blobs are not implemented in new code path.
> 
> Yep, this will be needed to implement.
> This will require to queue additional outgoing messages until the blob is
> read/put in memory and sent.

I'll take a look too.

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #5)
> (In reply to Carlos Garcia Campos from comment #1)
> > Created attachment 372750 [details]
> > WIP
> > 
> > This is a WIP patch. Old code is used unless WEBKIT_USE_SOUP_WEBSOCKETS env
> > var is present. This is just temporary until the implementation is complete
> > and we make layout tests pass. Most of them are failing with the patch, but
> > in a lot of cases it's because of missing console messages that new code
> > path doesn't generate. I'll add a comment with more information about the
> > layout tests.
> 
> Depending on how you want to do the switch, it might be feasible to
> rename/reuse NSURLSessionWebSocket runtime flag.

We already depend on a lisboup version having the websockets API (although I've fixed several bugs these days while working on it, but mostly edge cases covered by the tests). So, as soon as we have all the basic functionality working we can do the switch unconditionally. I can use platform specific test results until apple switches to use NSURLSession for the tests too. We can probably land this patch using the env var and continue working on the remaining issues in follow up patches.

[youenn fablet]

(In reply to Carlos Garcia Campos from comment #6)
> (In reply to youenn fablet from comment #4)
> > Nice to see that.
> > We have similar test issues with the NSURLSession code path that we will
> > need to handle one way or the other.
> 
> So, the NSURLSession impl is not used yet in production, and not tested by
> the bots either?

Right, it is off by default in WebKitTestRunner.

> > I run some of these tests with Chrome and Firefox and some of them fail with
> > either one or both, which indicates that these tests might be WebKit
> > implementation specific.
> 
> I'll prioritize the ones that we knoe are bugs or missing features.
> 
> > (In reply to Carlos Garcia Campos from comment #2)
> > > As I said most of the tests are failing because of missing console messages,
> > > so they are actually working and I won't mention them here.
> > 
> > Right, I am not sure we will be able to get back the exact same missing
> > console messages given that part of the messages are generated by processing
> > done by libsoup/nsurlsession now.
> 
> Indeed, if we keep using console messages for those events, we will need
> platform specific test results. I don't know if it's possible to ignore
> those messages (if we end up adding them back) in layout tests to not show
> them in the results.

It is possible using DumpJSConsoleLogInStdErr but I am not sure we want to use that option. Platform specific changes might be better.

> > > 
> > > http/tests/websocket/tests/hybi/bad-handshake-crash.html
> > > 
> > > This needs investigation, "WebSocket is open" message is unexpectedly shown
> > > in the output.
> > 
> > I think we also have an issue there with NSURLSession.
> > 
> > > http/tests/websocket/tests/hybi/client-close.html
> > > 
> > > I've debugged this and I don't understand why it fails, the diff is:
> > > -PASS closeEvent.reason is "close_frame[:2]='\\x88\\x80'"
> > > +FAIL closeEvent.reason should be close_frame[:2]='\x88\x80'. Was
> > > close_frame[:2]='\x88\x82'.
> > 
> > Same issue here with NSURLSession.
> 
> Interesting, I debugged it for a while, but couldn't understand what's going
> on there.
>  
> > > http/tests/websocket/tests/hybi/close-code-and-reason.html
> > > 
> > > This one expects error codes that the spec says shouldn't be used like 1006.
> > > But close reason is also failing, so it needs some more investigation.
> > > 
> > > http/tests/websocket/tests/hybi/close.html
> > > 
> > > It seems onclose is not called in a couple of cases.
> > > 
> > > http/tests/websocket/tests/hybi/deflate-frame-parameter.html
> > > http/tests/websocket/tests/hybi/extensions.html
> > > 
> > > x-webkit-deflate-frame extension not supported
> > 
> > The idea would be to move from x-webkit-deflate-frame to the standard
> > version permessage-deflate.
> 
> Since it's in a spec now I think we could implement it directly in libsoup,
> otherwise I'll add new API to libsoup to implement extensions. I guess this
> doesn't cause any regression in functionality, only that frames are not
> compressed.

The handshake request would be different so servers could react differently and for instance fail the handshake.
That said, Chrome and Firefox are using the standard extension now, so compatibility risk might be low.

> 
> > > 
> > > http/tests/websocket/tests/hybi/handshake-fail-by-no-cr.html
> > > 
> > > Libsoup doesn't fail when status line finishes with \n since that's what the
> > > http spec suggests.
> > > 
> > > http/tests/websocket/tests/hybi/handshake-ok-with-http-version-beyond-1_1.
> > > html
> > > 
> > > There are a couple of cases where libsoup doesn't fail either.
> > > 
> > > http/tests/websocket/tests/hybi/inspector/*
> > > 
> > > It seems we are not notifying the inspector in some cases about websockets
> > > with the new code path.
> > 
> > I am not sure we will be ever able to get back the same level of inspector
> > control with the new code path.
> 
> I'll take a look.
> 
> > > http/tests/websocket/tests/hybi/null-character.html
> > > 
> > > libsoup API always expect a null terminated string for text frames. I
> > > haven't found anything in the spec about it, it just says that text frames
> > > should contains valid utf8, and null character si perfectly valid, so maybe
> > > we need to add new API in libsoup to allow passing char* + length.
> > > 
> > > http/tests/websocket/tests/hybi/send-object-tostring-check.html
> > > 
> > > It expects to be closed cleanly, but apparently it isn't.
> > > 
> > > http/tests/websocket/tests/hybi/simple-wss.html
> > > 
> > > We don't have a way to accept the invalid certificate only for tests.
> > 
> > The plan is to reuse the existing NSURLSession code path for these.
> > I would hope you could do the same for libsoup.
> 
> I'll check it too. We should be able, since the handshake happens in the
> session like a normal load.
> 
> > > http/tests/websocket/tests/hybi/inspector/binary.html
> > > http/tests/websocket/tests/hybi/send-blob-onmessage-origin.html
> > > http/tests/websocket/tests/hybi/send-blob.html
> > > 
> > > Blobs are not implemented in new code path.
> > 
> > Yep, this will be needed to implement.
> > This will require to queue additional outgoing messages until the blob is
> > read/put in memory and sent.
> 
> I'll take a look too.

Cool, thanks!

[Carlos Garcia Campos]

Created attachment 372916 [details]
Patch

[Michael Catanzaro]

Comment on attachment 372916 [details]
Patch

OK the soup changes LGTM.

There are some bits in here that require owner approval from Youenn before landing.

[Michael Catanzaro]

Would be worth double-checking to make sure proxies still work, since we had a CVE there in the past.

[youenn fablet]

Comment on attachment 372916 [details]
Patch

r=me as well.
Some comments below.

View in context: https://bugs.webkit.org/attachment.cgi?id=372916&action=review

> Source/WebKit/NetworkProcess/soup/WebSocketTaskSoup.h:38
> +class WebSocketTask {

Some small code is shared between the soup and cocoa versions.
Maybe code should be shared.

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:164
> +void WebSocketChannel::didConnect(const String& subprotocol)

String&&

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:173
> +        enqueueTask([this, subprotocol] {

WTFMove() and mutable

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:179
> +    m_subprotocol = subprotocol;

WTFMove()

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:237
> +void WebSocketChannel::didFail(const String& reason)

String&&

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:243
> +        enqueueTask([this, reason] {

WTFMove and mutable

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:249
> +    // FIXME: do something with reason.

reason might not be a great name since it relates to didClose.
Maybe errorMessage might be better.

There is a preexisting issue as well in the sense we might usually need to call didClose after didFail to trigger the onclose event.
This is not really consistent with other code where didFail is final.
Maybe we should rename didFail to didReceiveMessageError.

[Carlos Garcia Campos]

Comment on attachment 372916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372916&action=review

>> Source/WebKit/NetworkProcess/soup/WebSocketTaskSoup.h:38
>> +class WebSocketTask {
> 
> Some small code is shared between the soup and cocoa versions.
> Maybe code should be shared.

Yes, we could move to a shared header instead of the current one just including the platform ones.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:164
>> +void WebSocketChannel::didConnect(const String& subprotocol)
> 
> String&&

I'm following what other IPC message chandlers do. I think we should change the others too, so better to do it in a follow up. I'll submit a new patch.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:249
>> +    // FIXME: do something with reason.
> 
> reason might not be a great name since it relates to didClose.
> Maybe errorMessage might be better.
> 
> There is a preexisting issue as well in the sense we might usually need to call didClose after didFail to trigger the onclose event.
> This is not really consistent with other code where didFail is final.
> Maybe we should rename didFail to didReceiveMessageError.

It's the same nomenclature used in WebCore/Modules/websockets/WebSocketChannel.cpp. In the old code WebSocketChannel::fail() calls m_handle->disconnect() after m_client->didReceiveMessageError() which closes the socket stream causing the close.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #11)
> Would be worth double-checking to make sure proxies still work, since we had
> a CVE there in the past.

That shouldn't be a problem. The bug was that we were not using the soup session to start the TCP connection, so anything condifugred in the session was lost. The new code path uses soup_session_websocket_connect_async().

[Carlos Garcia Campos]

Committed r246877: <https://trac.webkit.org/changeset/246877>

[Konstantin Tokarev]

Out of curiosity: what benefits does use of platform-specific implementation of WebSockets instead of internal impementation provide?

[Carlos Garcia Campos]

At least it's less code to maintain in WebKit :-)

[Konstantin Tokarev]

But AFAIU WebKit implementation will have to stay anyway because of Windows ports and PlayStation.

[youenn fablet]

WebSocket handshake is based on HTTP so it is best to let the underlying HTTP library handle it (cookies, certification validation, proxies...).
It would be nice if other WK2 implementations could follow the same path.
The other WebSocket library will remain for WK1.

[youenn fablet]

> >> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:164
> >> +void WebSocketChannel::didConnect(const String& subprotocol)
> > 
> > String&&
> 
> I'm following what other IPC message chandlers do. I think we should change
> the others too, so better to do it in a follow up. I'll submit a new patch.

It is only needed to change the ones that can benefit r values.