GNOME Bugzilla – Bug 656521
Broadway backend doesn't work with Firefox 6
Last modified: 2012-01-31 10:47:37 UTC
Apparently we have to change WebSocket with MozWebSocket according to https://bugzilla.mozilla.org/show_bug.cgi?id=664692 .
The change is not so simple because the WebSocket protocol version changed. This is a request from FF5: GET /socket HTTP/1.1 Sec-WebSocket-Key2: Z.50748C ,3724 Sec-WebSocket-Protocol: broadway Connection: Upgrade Origin: http://localhost:8080 Sec-WebSocket-Key1: 1 9 2 8 43 9 11K 7 Host: localhost:8080 Upgrade: WebSocket This is one from FF6: GET /socket HTTP/1.1 Host: 192.168.1.80:8080 User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Accept-Language: de-de,de;q=0.8,en-us;q=0.5,en;q=0.3 Accept-Encoding: gzip, deflate Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7 Connection: keep-alive, Upgrade Sec-WebSocket-Version: 7 Sec-WebSocket-Origin: http://192.168.1.80:8080 Sec-WebSocket-Key: RRDsV6GFpadXWHZ1QPphtA== Cookie: __utma=1.1349684304.1313333714.1313333714.1313338493.2; __utmc=1; __utmz=1.1313333714.1.1.utmcsr=(direct)|utmccn=(... Pragma: no-cache Cache-Control: no-cache Upgrade: websocket
Created attachment 193834 [details] [review] WIP patch
Created attachment 193836 [details] [review] WIP patch v2 Oops, last patch was incorrect.
Created attachment 193881 [details] [review] WIP patch v3 This patch fixes warnings, coding style nitpicks, has been compiled/tested (but currently does not succeed, I'm trying to get help in https://bugzilla.mozilla.org/show_bug.cgi?id=640003#c94 ). This patch also tries to avoid identation fixes so it is clearer what is changed (maybe we can fix the identation in a commit after this).
Comment on attachment 193881 [details] [review] WIP patch v3 Chatted with Alex via IRC: 16:15 knocte_ well, to get it done by parts... so then later I can refactor it to include a new file, as you told me 16:16 knocte_ ah yeah the hex_string_to_ascii_string , weird I didn't find that in glib 16:16 knocte_ was going to propose its inclusion 16:16 alexl there are some helpers 16:16 knocte_ I didn't find the helper for this when I was looking for it.. 16:19 alexl g_ascii_xdigit_value 16:19 alexl helps clean it up 16:20 knocte_ ok, I'll take a look at that 16:22 alexl + gchar* sha1_result = g_compute_checksum_for_data (G_CHECKSUM_SHA1, 16:22 alexl + (const guchar *)full_key, 16:22 alexl + strlen (full_key)); 16:22 alexl + const guchar* decoded = _hex_string_to_ascii_string (sha1_result); 16:23 alexl why are you using that, which converts to hex and then manually convert it back? 16:23 alexl g_checksum_get_digest 16:23 knocte_ well, the only SHA1 operation I found in glib, returns stuff in hex 16:23 alexl use that rather than the helper 16:23 knocte_ ah, ok, will try that 16:24 alexl Look at the g_compute_checksum_for_data implementation, then switch g_checksum_get_string to g_checksum_get_digest
*** Bug 660200 has been marked as a duplicate of this bug. ***
Created attachment 200057 [details] [review] cleaned up patch, still not functioning ... So - this cleans up the code a tad. I've verified that we do indeed produce the correct handshake in reply (at least for the published sample). Unfortunately Firefix <recent> still doesn't like what it sees. I get no error, and no UI - just endless white cats in snow :-)
After some considerable poking at the NSPR_LOG_MODULES=all:4 debug output from firefox I noticed: -1331086480[823cbe8]: WebSocketChannel::OnInputStreamReady: read 165 rv 0 -1331086480[823cbe8]: WebSocketChannel::ProcessInput 955eb58 [165 2] -1331086480[823cbe8]: WebSocketChannel::UpdateReadBuffer() 955eb58 [b0a928e0 165] -1331086480[823cbe8]: WebSocketChannel: update read buffer absorbed 165 -1331086480[823cbe8]: WebSocketChannel::ProcessInput: payload 10 avail 165 -1331086480[823cbe8]: WebSocketChannel::ProcessInput: Frame accumulated - opcode 13 -1331086480[823cbe8]: WebSocketChannel:: fragmented control frame code 13 -1331086480[823cbe8]: WebSocketChannel::AbortSession() 955eb58 [reason 80070057] stopped = 0 It seems that Mozilla's WebSocket.cpp wants: // Control codes are required to have the fin bit set if (!finBit && (opcode & kControlFrameMask)) { LOG(("WebSocketChannel:: fragmented control frame code %d\n", opcode)); AbortSession(NS_ERROR_ILLEGAL_VALUE); return NS_ERROR_ILLEGAL_VALUE; } set - and this is where we bomb out ...
Reading the code, it makes me wonder how section 5.2 - the base framing protocol is implemented cf. http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-17. This seems to be what that code is validating - it seems we just shovel a custom raw-text protocol directly down the pipe :-)
(In reply to comment #9) > Reading the code, it makes me wonder how section 5.2 - the base framing > protocol is implemented cf. > http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-17. This seems > to be what that code is validating - it seems we just shovel a custom raw-text > protocol directly down the pipe :-) Oh, yes! Sorry, forgot to share here my latest findings. Handshake rework is not enough, we need to adapt the whole data transfer to the version of websocket which has changed a bit (at the beginning when I created this bug, I thought the main changes were just on handshake, because FF didn't enable by default WS until FF6). So I had a small look and what we would need to do in broadway, and ended up thinking that we maybe should just reuse a C library (the only C library I know for Server impl of WS): http://git.warmcat.com/cgi-bin/cgit/libwebsockets/ This way we would only need to raise the version dep whenever protocol changes again (assuming libwebsockets hacker are as fast as nowadays). Michael, do you have time to integrate libwebsockets one of this days? Not sure I'll be able to find time myself. Thanks for your contributions.
I'd like to avoid more dependencies. Websockets isn't rocket science, and once stable is unlikely to change much.
Created attachment 201022 [details] [review] more bits ... Still not functioning, generating a rather curious: -1330705552[823ed20]: WebSocketChannel::ProcessInput: payload 46 avail 2083 -1330705552[823ed20]: WebSocketChannel::ProcessInput: Frame accumulated - opcode 1 -1330705552[823ed20]: WebSocketChannel:: unexpected reserved bits 20 when the header sent is: Gdk-WARNING **: header 0x21 0x2e always but ... hey ho, will dig more later.
it seems BE means that bit 0 is the MSB from the spec ;-) I fixed that, and then hit this: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsWebSocket.cpp#257 note the (non) implementation of OnBinaryMessageAvailable - which is the path that we trigger ... Looks to me like we'd need to re-work the protocol to use text if we want to run in recent firefoxen at least; must check to see if Chrome handles the binary messages.
Created attachment 201082 [details] [review] implement simple websockets support. Review appreciated for the above, it implements websockets for me - and works with both new and old FireFoxes 7.0.1 and 4.something tested.
Needs this too really: index 9cdab58..432d303 100644 --- a/gdk/broadway/gdkdisplay-broadway.c +++ b/gdk/broadway/gdkdisplay-broadway.c @@ -387,7 +387,7 @@ parse_input (BroadwayInput *input) payload_len = GUINT64_FROM_BE( *(guint64 *) data ); data += 8; } - if (payload_len > len) + if (data - buf + payload_len > len) return; /* wait to accumulate more */ if (is_mask)
Review of attachment 201082 [details] [review]: I didn't look in detail at the protocol stuff, suffice to say that by actually working its a clear improvement, and any issues can be fixed later. ::: gdk/broadway/gdkdisplay-broadway.c @@ +353,2 @@ { + hex_dump (input->buffer->data, input->buffer->len); Seems kinda overkill to always print this if built with debug support. We could add a runtime check, but i think at this point it would be better to make it an in-file #define check instead. @@ +670,3 @@ + g_checksum_update (checksum, full_key, strlen ((char *)full_key)); + + digest_len = g_checksum_type_get_length (G_CHECKSUM_SHA1) + 1; SHA1 are a fixed size, so i prefer a normal on-heap allocation rather than alloca.
I pushed this patch with some minor cleanup to the checksumming code and with a local debug define. It seems to somewhat work, but I can easily get it to hang both in chrome and in firefox by clicking on the "button box" button in testgtk+. Not sure what is causing this, but we should keep this bug open until that is fixed.
Comment on attachment 201082 [details] [review] implement simple websockets support. Update status to get off review queue
Tried this out today, works great but needs a two-line fix (attached).
Created attachment 201257 [details] [review] fix uninitialized digest_len
Comment on attachment 201257 [details] [review] fix uninitialized digest_len pushed, thanks
(In reply to comment #15) > Needs this too really: > > index 9cdab58..432d303 100644 > --- a/gdk/broadway/gdkdisplay-broadway.c > +++ b/gdk/broadway/gdkdisplay-broadway.c > @@ -387,7 +387,7 @@ parse_input (BroadwayInput *input) > payload_len = GUINT64_FROM_BE( *(guint64 *) data ); > data += 8; > } > - if (payload_len > len) > + if (data - buf + payload_len > len) > return; /* wait to accumulate more */ > > if (is_mask) Sorry, I am too busy now to prepare the correct patch but this is just one line change. This line (gdkdisplay-broadway.c:394 in the latest source): if (data - buf + payload_len > len) should be replaced with if (data - buf + payload_len + (is_mask ? 4 : 0) > len) or anything equivalent. I know this expression becomes obfuscated but is correct, maybe somebody will have a better idea. Please note that immediately after this line and after "if (is_mask)" there is "data += 4" (gdkdisplay-broadway.c:402) and this test result becomes invalid. As a consequence, the line gdkdisplay-broadway.c:432: g_byte_array_remove_range (input->buffer, 0, data - buf + payload_len); causes an infinite series of g_asserts because now data - buf + payload_len > len, and the application window seems to hang in the browser either after the first mouse move or after few clicks. With this fix applied runs fine, at least does not hang. I have tested this with the standard gnome-calculator displayed in Firefox 8.0 on Windows XP. Please test and apply, sorry again for no patch.
I pushed a fix that is a bit cleaner: http://git.gnome.org/browse/gtk+/commit/?id=fa6ad2ca047925a3ebf7a94750b283e1a36925d9 Thanks a lot for the debugging, this seems to work for me now. Marking this as resolved.
There is one more commit you need for a good experience here - just pushed it: http://git.gnome.org/browse/gtk+/commit/?id=0481fbf7ce5b9a505577e2fbf8570c6c0abc2f86 that kills the problems Alex noted in comment #17, and the key control/alt state mangling that we had too. It's now rather ~useable IMHO.