Implement missing parameters of WebSocket permessage compression extension

RESOLVED FIXED in Firefox 42

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
Takeshi - is there a chromium bug here that should be filed too?
Flags: needinfo?(tyoshino)

Comment 2

3 years ago
I'll file a bug for you.

Is this about ChromeDriver? Telemetry?
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 3

3 years ago
(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.
Flags: needinfo?(michal.novotny)

Comment 4

3 years ago
Sorry for delay. Just filed crbug.com/467909 and made a patch for it.
Flags: needinfo?(tyoshino)
(Assignee)

Comment 5

3 years ago
Created attachment 8578639 [details] [diff] [review]
fix
Attachment #8578639 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
Created attachment 8582108 [details] [diff] [review]
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.
Attachment #8582108 - Flags: review?(jduell.mcbugs)

Comment 8

3 years ago
(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 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
Attachment #8578639 - Flags: review?(jduell.mcbugs) → review+
Attachment #8582108 - Flags: review?(jduell.mcbugs) → review+

Comment 10

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

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eace5363f85
https://hg.mozilla.org/mozilla-central/rev/7b5a9b0979d9
https://hg.mozilla.org/mozilla-central/rev/8eace5363f85
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.