[Webkit-unassigned] [Bug 199151] [SOUP] Use libsoup WebSockets API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=199151

--- Comment #8 from youenn fablet <youennf at gmail.com> ---
(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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190625/b8eeb2c6/attachment-0001.html>