Closed Bug 792831 Opened 7 years ago Closed 5 years ago

Implement WebSocket compression extension

Categories

(Core :: Networking: WebSockets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: davidgaleano, Assigned: michal)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

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.
Component: General → Networking: WebSockets
Product: Firefox → Core
It seems that "perframe" has been superseded by "permessage":

    http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-04
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → michal.novotny
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.
Attachment #8537222 - Flags: review?(jduell.mcbugs)
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.
Attached patch test (obsolete) — Splinter Review
Attachment #8539273 - Flags: review?(jduell.mcbugs)
Attached patch patch v2 (obsolete) — Splinter Review
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
Attachment #8537222 - Attachment is obsolete: true
Attachment #8537222 - Flags: review?(jduell.mcbugs)
Attachment #8539844 - Flags: review?(jduell.mcbugs)
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).
Flags: needinfo?(michal.novotny)
(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.
Flags: needinfo?(michal.novotny)
(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 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
Attachment #8539844 - Flags: review?(jduell.mcbugs) → review+
Pat: is it time to remove the old deflate-stream compression code from websockets? (See previous comment)
Flags: needinfo?(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)
Flags: needinfo?(mcmanus)
Attached patch patch v3 (obsolete) — Splinter Review
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
Attachment #8539844 - Attachment is obsolete: true
Attachment #8541217 - Flags: review+
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/
Attachment #8539273 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/724f0a71d621
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1137008
Depends on: 1466577
You need to log in before you can comment on or make changes to this bug.