Support WebSocket per-message deflate extension in http server.

Support WebSocket per-message deflate extension in http server.

BUG=438230
Committed: https://crrev.com/a6e7009ffc0af9413eb8dffb6100e90b369978e7
Cr-Commit-Position: refs/heads/master@{#308083}

[dgozman, 2014-12-03 11:26:34]

dgozman@chromium.org changed reviewers:
+ mmenke@chromium.org, pfeldman@chromium.org

[dgozman, 2014-12-03 11:26:34]

Hello,

Could you please take a look?

@mmenke: {Encode,Decode}FrameHybi17 functions just moved to another file. The
only change there is to read/write reserved1 bit used for permessage-deflate
compression. Added deflate/inflate atop of that.

@pfeldman: just in case you have any comments on this. Client usage in
AndroidWebSocket coming next.

Thanks,
Dmitry

[mmenke, 2014-12-03 15:38:06]

I'm completely unfamiliar with server/web_socket, and not knowledgeable about
websockets in general.  Should find a better reviewer for this.

https://codereview.chromium.org/769423005/diff/1/net/server/web_socket_encode...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/1/net/server/web_socket_encode...
net/server/web_socket_encoder.cc:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
nit:  Don't use (c) in new files.

[dgozman, 2014-12-03 16:36:18]

Patchset #2 (id:20001) has been deleted

[vkuzkokov, 2014-12-03 18:34:48]

vkuzkokov@chromium.org changed reviewers:
+ vkuzkokov@chromium.org

[vkuzkokov, 2014-12-03 18:34:48]

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:236: WebSocketExtensionParser parser;
Comment in WebSocketExtensionParser says "This parser assumes some preprocesses
are made", namely that passed string contains only one extension. While this may
work now it won't always.

[dgozman, 2014-12-03 21:34:57]

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:236: WebSocketExtensionParser parser;
On 2014/12/03 18:34:48, vkuzkokov wrote:
> Comment in WebSocketExtensionParser says "This parser assumes some
preprocesses
> are made", namely that passed string contains only one extension. While this
may
> work now it won't always.

There is only one extension now. Let's fix the problem when it arises.

[dgozman, 2014-12-04 11:25:54]

dgozman@chromium.org changed reviewers:
+ yhirano@chromium.org

[dgozman, 2014-12-04 11:25:55]

@yhirano: could you please take a look at net/websockets usage here?

[pfeldman, 2014-12-04 12:54:06]

you should split this into two patch sets: extracting the encoder and
implementing the extension.

[pfeldman, 2014-12-04 14:17:27]

This requires a test.

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:301: EncodeFrameHybi17(frame, masking_key,
false, output);
I wonder if this should fail in case deflate failed.

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:304: WebSocket::ParseResult
WebSocketEncoder::Inflate(std::string* message) {
Should be boolean - it is not about source frame parsing.

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:334: WebSocket::ParseResult
WebSocketEncoder::Deflate(
ditto

[dgozman, 2014-12-04 15:17:45]

PTAL

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:236: WebSocketExtensionParser parser;
On 2014/12/03 21:34:56, dgozman wrote:
> On 2014/12/03 18:34:48, vkuzkokov wrote:
> > Comment in WebSocketExtensionParser says "This parser assumes some
> preprocesses
> > are made", namely that passed string contains only one extension. While this
> may
> > work now it won't always.
> 
> There is only one extension now. Let's fix the problem when it arises.

Added TODO.

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:301: EncodeFrameHybi17(frame, masking_key,
false, output);
On 2014/12/04 14:17:27, pfeldman wrote:
> I wonder if this should fail in case deflate failed.

Not really. We can send non-compressed frame just fine - it's supported in
permessage-deflate standard.

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:304: WebSocket::ParseResult
WebSocketEncoder::Inflate(std::string* message) {
On 2014/12/04 14:17:27, pfeldman wrote:
> Should be boolean - it is not about source frame parsing.

Done.

https://codereview.chromium.org/769423005/diff/40001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:334: WebSocket::ParseResult
WebSocketEncoder::Deflate(
On 2014/12/04 14:17:27, pfeldman wrote:
> ditto

Done.

[yhirano, 2014-12-04 15:58:01]

Sorry I will go to offsite tomorrow, so I will review on next Monday.

[mmenke, 2014-12-04 18:07:44]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[yhirano, 2014-12-08 05:06:29]

net/websockets is not built when enable_websockets is not set. Are you OK with
that?

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:46: WebSocket::ParseResult
DecodeFrameHybi17(const base::StringPiece& frame,
You can use WebSocketFrameParser in net/websockets/websocket_frame_parser.h and
WriteWebSocketFrameHeader in net/websockets/websocket_frame.h.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:267: deflater_.reset(new
WebSocketDeflater(WebSocketDeflater::TAKE_OVER_CONTEXT));
If you don't verify peer's preference on context takeover control (i.e.
*_no_context_takeover parameters), DO_NOT_TAKE_OVER_CONTEXT should be used.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:268: deflater_->Initialize(deflate_bits);
WebSocketDeflater::Initialize may fail.
If you ignore the possibility, it's OK, but adding a comment or DCHECK may be
helpful.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:272: inflater_->Initialize(inflate_bits);
ditto

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:308: while (length > 0) {
I don't understand why this loop is needed: Doesn't inflater_->AddBytes(data,
length) work?

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:328: *message = std::string(&output[0],
output.size());
This is problematic when output is empty.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
File net/server/web_socket_encoder.h (right):

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.h:38: WebSocketEncoder(bool is_server);
+explicit

[dgozman, 2014-12-08 17:53:01]

Thank you for review!

> net/websockets is not built when enable_websockets is not set. Are you OK with
that?
Yes.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:46: WebSocket::ParseResult
DecodeFrameHybi17(const base::StringPiece& frame,
On 2014/12/08 05:06:29, yhirano wrote:
> You can use WebSocketFrameParser in net/websockets/websocket_frame_parser.h
and
> WriteWebSocketFrameHeader in net/websockets/websocket_frame.h.

We can try to completely switch over to net/websockets infrastructure, but
definitely not in this patch.
I'm worried it's too heavyweight for our usecase, but I may be wrong here.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:267: deflater_.reset(new
WebSocketDeflater(WebSocketDeflater::TAKE_OVER_CONTEXT));
On 2014/12/08 05:06:28, yhirano wrote:
> If you don't verify peer's preference on context takeover control (i.e.
> *_no_context_takeover parameters), DO_NOT_TAKE_OVER_CONTEXT should be used.

Added no_context_takeover support.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:268: deflater_->Initialize(deflate_bits);
On 2014/12/08 05:06:29, yhirano wrote:
> WebSocketDeflater::Initialize may fail.
> If you ignore the possibility, it's OK, but adding a comment or DCHECK may be
> helpful.

Fixed.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:308: while (length > 0) {
On 2014/12/08 05:06:29, yhirano wrote:
> I don't understand why this loop is needed: Doesn't inflater_->AddBytes(data,
> length) work?

This is not needed. I just read the interface wrong.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:328: *message = std::string(&output[0],
output.size());
On 2014/12/08 05:06:29, yhirano wrote:
> This is problematic when output is empty.

Nice catch! Done.

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
File net/server/web_socket_encoder.h (right):

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.h:38: WebSocketEncoder(bool is_server);
On 2014/12/08 05:06:29, yhirano wrote:
> +explicit

Done.

[yhirano, 2014-12-09 11:11:24]

https://codereview.chromium.org/769423005/diff/80001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/80001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:200: client_no_context_takeover ?
"client_no_context_takeover" : "",
You don't need to send back client_no_context_takeover.

https://codereview.chromium.org/769423005/diff/80001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:201: server_no_context_takeover ?
"server_no_context_takeover" : "");
';' is needed.

[yhirano, 2014-12-09 11:12:01]

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/60001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:46: WebSocket::ParseResult
DecodeFrameHybi17(const base::StringPiece& frame,
On 2014/12/08 17:53:00, dgozman wrote:
> On 2014/12/08 05:06:29, yhirano wrote:
> > You can use WebSocketFrameParser in net/websockets/websocket_frame_parser.h
> and
> > WriteWebSocketFrameHeader in net/websockets/websocket_frame.h.
> 
> We can try to completely switch over to net/websockets infrastructure, but
> definitely not in this patch.
> I'm worried it's too heavyweight for our usecase, but I may be wrong here.
I see, no problem.

[pfeldman, 2014-12-10 09:59:57]

lgtm

[dgozman, 2014-12-10 10:34:10]

PTAL

https://codereview.chromium.org/769423005/diff/80001/net/server/web_socket_en...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/769423005/diff/80001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:200: client_no_context_takeover ?
"client_no_context_takeover" : "",
On 2014/12/09 11:11:23, yhirano wrote:
> You don't need to send back client_no_context_takeover.

Done.

https://codereview.chromium.org/769423005/diff/80001/net/server/web_socket_en...
net/server/web_socket_encoder.cc:201: server_no_context_takeover ?
"server_no_context_takeover" : "");
On 2014/12/09 11:11:23, yhirano wrote:
> ';' is needed.

Oops. Fixed.

[yhirano, 2014-12-11 02:10:08]

lgtm

https://codereview.chromium.org/769423005/diff/100001/net/server/web_socket_e...
File net/server/web_socket_encoder.h (right):

https://codereview.chromium.org/769423005/diff/100001/net/server/web_socket_e...
net/server/web_socket_encoder.h:26: static const char kClientExtensions[];
Is this variable used?

[dgozman, 2014-12-11 08:06:00]

https://codereview.chromium.org/769423005/diff/100001/net/server/web_socket_e...
File net/server/web_socket_encoder.h (right):

https://codereview.chromium.org/769423005/diff/100001/net/server/web_socket_e...
net/server/web_socket_encoder.h:26: static const char kClientExtensions[];
On 2014/12/11 02:10:08, yhirano wrote:
> Is this variable used?

In a follow-up patch: https://codereview.chromium.org/747903006/

[dgozman, 2014-12-11 08:10:33]

dgozman@chromium.org changed reviewers:
+ eroman@chromium.org

[dgozman, 2014-12-11 08:10:33]

@eroman, could you please approve this as a net OWNER? I've got review from
yhirano (net/websockets owner), and pfeldman (author of  net/server).

[eroman, 2014-12-11 19:56:33]

lgtm

[dgozman, 2014-12-12 10:35:19]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-12-12 10:35:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/769423005/100001

[commit-bot: I haz the power, 2014-12-12 10:42:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 10:42:55]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[dgozman, 2014-12-12 12:46:25]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-12-12 12:47:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/769423005/120001

[commit-bot: I haz the power, 2014-12-12 12:54:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 12:54:31]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[dgozman, 2014-12-12 13:45:19]

Patchset #6 (id:120001) has been deleted

[dgozman, 2014-12-12 13:45:47]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-12-12 13:47:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/769423005/140001

[commit-bot: I haz the power, 2014-12-12 14:46:29]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2014-12-12 14:47:13]

Patchset 6 (id:??) landed as
https://crrev.com/a6e7009ffc0af9413eb8dffb6100e90b369978e7
Cr-Commit-Position: refs/heads/master@{#308083}