792831 - Implement WebSocket compression extension

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[davidgaleano]

Either "deflate-frame":

    http://tools.ietf.org/html/draft-tyoshino-hybi-websocket-perframe-deflate-06

Or the newer "perframe-compress":

    http://tools.ietf.org/html/draft-ietf-hybi-websocket-perframe-compression-04

ATTOW "deflate-frame" is supported by Google Chrome (as "x-webkit-deflate-frame"), pywebsocket and others.

Comment 1

[davidgaleano]

It seems that "perframe" has been superseded by "permessage":

    http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-04

Comment 2

[Michal Novotny [:michal]]

Attachment: patch v1 - permessage compression extension

For now there is no specific test for this new code, but the current websockets tests succeed when PMCE is disabled or enabled in the preferences. I'm going to write some PMCE specific tests and I'll include them in a separate patch.

Comment 3

[Andreas Røsdal]

If anyone needs to test WebSocket compression, then you can try http://play.freeciv.org
It supports WebSocket compression on Google Chrome. The WebSocket server is using the Tornado Web Server which supports compression.

Comment 4

[Michal Novotny [:michal]]

Attachment: test

Comment 5

[Michal Novotny [:michal]]

Attachment: patch v2

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee315f5433a1

The new patch fixes:
- 2 memory leaks (OutboundMessage::mMsg.pString, nsPMCECompression::mInflater)
- compiler failures (explicit nsPMCECompression constructor, signed/unsigned comparison in nsPMCECompression::Inflate/Deflate)
- nsIWebSocketListener::OnAcknowledge is now called correctly with an uncompressed size instead of the compressed size

Comment 6

[Jason Duell]

I'm still reviewing the patch.  One question: the spec says

  A server MAY attach the "c2s_max_window_bits" method parameter to
limit the LZ77 sliding window size that the client uses to build
messages.  A client that received this parameter MUST NOT use LZ77
sliding window size greater than the size specified by this
parameter to build messages.

That makes it sound like we MUST shrink our window size when building messages if we get 'c2s_max_window_bits'.  But I don't see any code that handles the 'c2s_max_window_bits' parameter.

The spec language here is similar to the "c2s_no_context_takeover" (which I see we do handle).

Comment 7

[Michal Novotny [:michal]]

(In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from comment #6)
> I'm still reviewing the patch.  One question: the spec says
> 
>   A server MAY attach the "c2s_max_window_bits" method parameter to
> limit the LZ77 sliding window size that the client uses to build
> messages.  A client that received this parameter MUST NOT use LZ77
> sliding window size greater than the size specified by this
> parameter to build messages.
> 
> That makes it sound like we MUST shrink our window size when building
> messages if we get 'c2s_max_window_bits'.  But I don't see any code that
> handles the 'c2s_max_window_bits' parameter.
> 
> The spec language here is similar to the "c2s_no_context_takeover" (which I
> see we do handle).

It seems that you're reading some older specification, c2s_ and s2c_ prefixes are no longer used. The latest spec says:

If a received extension negotiation offer has the "client_max_window_bits" extension parameter, the server MAY include the "client_max_window_bits" extension parameter in the corresponding extension negotiation response to the offer.

See http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-19 section 8.1.2.2.

Comment 8

[Michal Novotny [:michal]]

(In reply to Michal Novotny (:michal) from comment #7)
> See http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-19
> section 8.1.2.2.

It also directly says:

If a received extension negotiation offer doesn't have the "client_max_window_bits" extension parameter, the corresponding extension negotiation response to the offer MUST NOT include the "client_max_window_bits" extension parameter.

Comment 9

[Jason Duell]

Comment on attachment 8539844 [details] [diff] [review]
patch v2

Review of attachment 8539844 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +1358,5 @@
>  pref("network.websocket.extensions.stream-deflate", false);
>  
> +// Defines whether or not to try to negotiate the permessage compression
> +// extension with the websocket server.
> +// Note: enabling this extension disables stream-deflate compression.

So the old stream-deflate compression has been disabled since Firefox 8:

  https://developer.mozilla.org/en-US/docs/WebSockets

  http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#1358

I'm guessing we should probably just rip out the code for it at this point?  I'll ask Patrick.  Fine with that being a followup.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +716,5 @@
>  //-----------------------------------------------------------------------------
> +// nsPMCECompression
> +//-----------------------------------------------------------------------------
> +
> +class nsPMCECompression

Nit: we generally don't name stuff with the 'ns' prefix any more (except IDLs). So I think just 'PMCECompression' is a better name (yes, I know that there's already the existing nsWebsocketCompression class, but let's not add any more such old-style naming to file).

@@ +1709,5 @@
>      } else if (mStopped) {
>        LOG(("WebSocketChannel:: ignoring read frame code %d after completion\n",
>             opcode));
>      } else if (opcode == kText) {
>        LOG(("WebSocketChannel:: text frame received\n"));

Would be nice to log whether text frame received was compressed or not.

@@ +1814,5 @@
>            mBuffered -= framingLength + payloadLength;
>          payloadLength = 0;
>        }
>      } else if (opcode == kBinary) {
>        LOG(("WebSocketChannel:: binary frame received\n"));

same

Comment 10

[Jason Duell]

Pat: is it time to remove the old deflate-stream compression code from websockets? (See previous comment)

Comment 11

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from comment #10)
> Pat: is it time to remove the old deflate-stream compression code from
> websockets? (See previous comment)

yes - that can go (but optionally for the purposes of this bug)

Comment 12

[Michal Novotny [:michal]]

Attachment: patch v3

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=062d4c01e0f9

- removed stream-deflate support
- renamed nsPMCECompression to PMCECompression
- more information about extensions handshake and incoming deflated messages is logged

Comment 13

[Jason Duell]

Comment on attachment 8539273 [details] [diff] [review]
test

Review of attachment 8539273 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thanks Michal!

::: dom/base/test/test_websocket_permessage_deflate.html
@@ +30,5 @@
> +function ab2str(buf) {
> +  return String.fromCharCode.apply(null, new Uint16Array(buf));
> +}
> +
> +function str2ab(str) {

Funny there's no easier way to convert from strings to arraybuffers.  I just looked on the internet and I think I found the same StackOverflow page :)

@@ +65,5 @@
> +  }
> +
> +  ws.onclose = function(e) {
> +    if (!e.wasClean) {
> +      ok(fasle, "Connection should be closed cleanly!");

s/fasle/false/

Comment 14

[Michal Novotny [:michal]]

Attachment: patch v4 - final patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/724f0a71d621

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/724f0a71d621