Bug 50344 – wss (websockets ssl) support for gtk via new gio TLS support

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Dan Winship]

Created attachment 75301 [details]
Update SocketStreamHandleSoup to do wss://

Attached patch uses GTlsConnection to make wss:// work. You can test it against http://websockets.org/echo.html.

(The GPollableOutputStream stuff is basically an abstraction of the previous GSocket-specific API, that works on both GSocketConnection and GTlsConnection.)

Notably missing from this patch is a change to configure.in to require glib >= 2.27.4 since I wasn't sure how you want to do that. (Currently the support for gsettings is just conditional on the glib version... so we could do the same thing here, and continue using the old ws://-only code with older glibs...)

[WebKit Review Bot]

Attachment 75301 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/gobject/GTypedefs.h', u'WebCore/ChangeLog', u'WebCore/platform/network/soup/SocketStreamHandle.h', u'WebCore/platform/network/soup/SocketStreamHandleSoup.cpp']" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:90:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:165:  Extra space for operator !   [whitespace/operators] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 75301 [details]
Update SocketStreamHandleSoup to do wss://

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

Awesome! The addition of SSL socket support in GIO is great news. Will it need to be behind some version guards though, so that we will still work properly with older GIO versions?

> WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:90
> +	g_socket_client_set_tls(socketClient.get(), TRUE);

Indentation is slightly off here.

> WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:165
> +    if (error && ! g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK)) {

One extra space between ! and g_error_matches.

> WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:274
> -    // G_IO_HUP and G_IO_ERR are are always active. See:
> -    // http://library.gnome.org/devel/gio/stable/GSocket.html#g-socket-create-source
> -    if (condition & G_IO_HUP) {
> -        handle->close();
> -        return FALSE;
> -    }
> -    if (condition & G_IO_ERR) {
> -        handle->client()->didFail(handle, SocketStreamError(0)); // FIXME: Provide a sensible error.
> -        return FALSE;
> -    }
> -    if (condition & G_IO_OUT)
> -        handle->writeReady();
> +    handle->writeReady();

Are connection erros (such as HUP) still handled properly?

[Dan Winship]

(In reply to comment #2)
> Awesome! The addition of SSL socket support in GIO is great news. Will it need to be behind some version guards though, so that we will still work properly with older GIO versions?

Yes. (Mentioned in comment 0. I didn't deal with it in the patch because I wasn't sure how we wanted to deal with it.)

> Are connection errors (such as HUP) still handled properly?

Yes; when it eventually calls g_pollable_output_stream_write_nonblocking() again, the error will be returned at that point.

[Dan Winship]

Created attachment 75378 [details]
fix style nits

[Dan Winship]

Comment on attachment 75378 [details]
fix style nits

versioning issues will be taken care of by bug 50675

[WebKit Review Bot]

Attachment 75378 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6970047

[Dan Winship]

Created attachment 80580 [details]
updated patch

updated to top of tree, and ready to go now that the required glib version has been updated

[Gustavo Noronha (kov)]

Comment on attachment 80580 [details]
updated patch

Looks good to me. As discussed with danw on IRC I'll add the bug URL and title, fix a nit (remove a line break) and land.

[Gustavo Noronha (kov)]

Committed r77148: <http://trac.webkit.org/changeset/77148>

[Gustavo Noronha (kov)]

Comment on attachment 80580 [details]
updated patch

Landed!