1137008 - Implement missing parameters of WebSocket permessage compression extension

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Michal Novotny [:michal]]

Chrome (when acting as websocket server) returns server_max_window_bits and client_max_window_bits parameters in the PMCE negotiation response. It's neither required nor recommended by the spec to understand these parameters and client must fail the connection when it doesn't support the configuration, so we behave "correctly". Anyway it makes sense to add a support for these parameters since section 8.1.2.1. in the spec http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-19 says:

  A server MAY include the "server_max_window_bits" extension parameter
  in an extension negotiation response even if the extension
  negotiation offer being accepted by the response didn't include the
  "server_max_window_bits" extension parameter.

OTOH section 8.1.2.2. 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.

Including this parameter in the response is a bug in Chrome, but I think we could be relaxed about this when parsing the response.

Comment 1

[Patrick McManus [:mcmanus]]

Takeshi - is there a chromium bug here that should be filed too?

Comment 2

[Takeshi Yoshino]

I'll file a bug for you.

Is this about ChromeDriver? Telemetry?

Comment 3

[Michal Novotny [:michal]]

(In reply to Takeshi Yoshino from comment #2)
> I'll file a bug for you.
> 
> Is this about ChromeDriver? Telemetry?

The bug is in server side WebSocket implementation. Chromium must not send "client_max_window_bits" in the response when it was not present in the offer.

Comment 4

[Takeshi Yoshino]

Sorry for delay. Just filed crbug.com/467909 and made a patch for it.

Comment 5

[Michal Novotny [:michal]]

Attachment: fix

Comment 6

[Michal Novotny [:michal]]

(In reply to Takeshi Yoshino from comment #4)
> Sorry for delay. Just filed crbug.com/467909 and made a patch for it.

Thanks Takeshi. I just found another bug. Chrome doesn't handle multiple extension offers. It doesn't choose any offer when I send:

Sec-WebSocket-Extensions: permessage-deflate, permessage-deflate; client_max_window_bits

I'll keep just one offer in our code to make compression working between chrome and firefox.

Comment 7

[Michal Novotny [:michal]]

Attachment: test

There is no easy way to write a test for this functionality without modifying pywebsocket which I would like to avoid. We send the simplest possible offer due to reason described in comment #6. The only parameter that a pywebsocket handler can add to the response for such offer is client_no_context_takeover. And even then pywebsocket actually doesn't reset the inflater's sliding window. So this test only checks parsing of a single parameter.

Comment 8

[Takeshi Yoshino]

(In reply to Michal Novotny (:michal) from comment #6)
> (In reply to Takeshi Yoshino from comment #4)
> > Sorry for delay. Just filed crbug.com/467909 and made a patch for it.
> 
> Thanks Takeshi. I just found another bug. Chrome doesn't handle multiple
> extension offers. It doesn't choose any offer when I send:

Sorry about the bug. Fixing now.
http://crbug.com/471646

Comment 9

[Jason Duell]

Comment on attachment 8578639 [details] [diff] [review]
fix

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

Make sure to remove try server notation from patch comment before landing.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +2462,5 @@
> +            clientMaxWindowBits = paramValue.ToInteger(&errcode);
> +            if (NS_FAILED(errcode) || clientMaxWindowBits < 8 ||
> +                clientMaxWindowBits > 15) {
> +              LOG(("WebSocketChannel::HandleExtensions: found invalid "
> +                   "parameter client_max_window_bits\n"));

might as well log the invalid value?

@@ +2479,5 @@
> +            serverMaxWindowBits = paramValue.ToInteger(&errcode);
> +            if (NS_FAILED(errcode) || serverMaxWindowBits < 8 ||
> +                serverMaxWindowBits > 15) {
> +              LOG(("WebSocketChannel::HandleExtensions: found invalid "
> +                   "parameter server_max_window_bits\n"));

Same

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5a9b0979d9

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eace5363f85

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b5a9b0979d9
https://hg.mozilla.org/mozilla-central/rev/8eace5363f85