Closed
Bug 792831
Opened 11 years ago
Closed 9 years ago
Implement WebSocket compression extension
Categories
(Core :: Networking: WebSockets, defect)
Core
Networking: WebSockets
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: davidgaleano, Assigned: michal)
References
()
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 4 obsolete files)
324.79 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Component: General → Networking: WebSockets
Product: Firefox → Core
Reporter | ||
Comment 1•11 years ago
|
||
It seems that "perframe" has been superseded by "permessage": http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-04
Updated•10 years ago
|
Updated•9 years ago
|
Assignee: nobody → michal.novotny
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8539273 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
(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)
Assignee | ||
Comment 8•9 years ago
|
||
(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•9 years ago
|
||
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+
Comment 10•9 years ago
|
||
Pat: is it time to remove the old deflate-stream compression code from websockets? (See previous comment)
Flags: needinfo?(mcmanus)
Comment 11•9 years ago
|
||
(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)
Assignee | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/724f0a71d621
Attachment #8539273 -
Attachment is obsolete: true
Attachment #8541217 -
Attachment is obsolete: true
Attachment #8542124 -
Flags: review+
https://hg.mozilla.org/mozilla-central/rev/724f0a71d621
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•9 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•