HttpServer WebSocket: don't crash on data before server handshake

HttpServer WebSocket: don't crash on data before server handshake

(Which includes the case of a server delegate which doesn't do anything
 with WebSocket requests).

Note: this assumes 860f9bd is in, since otherwise the test 
would leak. 

BUG=606428
Review-Url: https://codereview.chromium.org/2640363004
Cr-Commit-Position: refs/heads/master@{#445439}
Committed: https://chromium.googlesource.com/chromium/src/+/788a643320ed52bdaf97892a43c505314678157a

[Maks Orlovich, 2017-01-19 20:36:24]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 20:36:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 22:35:38]

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

[commit-bot: I haz the power, 2017-01-19 22:35:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Maks Orlovich, 2017-01-20 21:40:54]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 21:41:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-01-20 21:43:37]

Description was changed from

==========
HttpServer WebSocket: don't crash on data before server handshake

(Which includes the case of a server delegate which doesn't do anything
 with WebSocket requests).

BUG=606428
==========

to

==========
HttpServer WebSocket: don't crash on data before server handshake

(Which includes the case of a server delegate which doesn't do anything
 with WebSocket requests).

Note: this assumes 860f9bd is in, since otherwise the test 
would leak. 

BUG=606428
==========

[commit-bot: I haz the power, 2017-01-20 22:33:07]

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

[commit-bot: I haz the power, 2017-01-20 22:33:08]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-01-23 14:14:42]

morlovich@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2017-01-23 16:17:21]

Thanks for finding and fixing this.  LGTM, modulo comments.

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.cc
File net/server/web_socket.cc (right):

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.c...
net/server/web_socket.cc:104: // any further data". If encoder_ is null here, we
haven't even sent a
nit (x2):  |encoder_| is the more common pattern in net/, for class and stack
variable names.

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.c...
net/server/web_socket.cc:106: // that, so we can reject them guilt-free,
especially since we can't
nit (x2):  Avoid we in comments, due to ambiguity.

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.c...
net/server/web_socket.cc:108: Fail();
Hrm...The caller already closes the socket when we return FRAME_CLOSE or
FRAME_ERROR.  There's no documented API contract here, but digging into the
encoder_->DecodeFrame call below, it looks like on errors it does not close the
socket, but instead just returns FRAME_ERROR, so I think it's more consistent
not to call Fail() here.

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.c...
net/server/web_socket.cc:109: return FRAME_CLOSE;
FRAME_ERROR seems more accurate.

[Maks Orlovich, 2017-01-23 17:27:50]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 17:28:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-01-23 17:28:29]

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.cc
File net/server/web_socket.cc (right):

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.c...
net/server/web_socket.cc:104: // any further data". If encoder_ is null here, we
haven't even sent a
On 2017/01/23 16:17:21, mmenke wrote:
> nit (x2):  |encoder_| is the more common pattern in net/, for class and stack
> variable names.

Done. I incorrectly assumed this was just for API docs...

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.c...
net/server/web_socket.cc:106: // that, so we can reject them guilt-free,
especially since we can't
On 2017/01/23 16:17:21, mmenke wrote:
> nit (x2):  Avoid we in comments, due to ambiguity.

Rephrased.

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.c...
net/server/web_socket.cc:108: Fail();
On 2017/01/23 16:17:21, mmenke wrote:
> Hrm...The caller already closes the socket when we return FRAME_CLOSE or
> FRAME_ERROR.  There's no documented API contract here, but digging into the
> encoder_->DecodeFrame call below, it looks like on errors it does not close
the
> socket, but instead just returns FRAME_ERROR, so I think it's more consistent
> not to call Fail() here.

Good point, done. This does mean we don't set closed_, but since the connection
is closed no one should be able to call into something like WebSocket::Send()
anyway.

https://codereview.chromium.org/2640363004/diff/20001/net/server/web_socket.c...
net/server/web_socket.cc:109: return FRAME_CLOSE;
On 2017/01/23 16:17:21, mmenke wrote:
> FRAME_ERROR seems more accurate.

Done.

[commit-bot: I haz the power, 2017-01-23 17:30:42]

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

[commit-bot: I haz the power, 2017-01-23 17:30:43]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Maks Orlovich, 2017-01-23 18:38:15]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 18:38:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 19:37:45]

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

[commit-bot: I haz the power, 2017-01-23 19:37:46]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-01-23 19:39:00]

The CQ bit was checked by morlovich@chromium.org

[Maks Orlovich, 2017-01-23 19:39:00]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2640363004/#ps40001
(title: "Apply review feedback.")

[commit-bot: I haz the power, 2017-01-23 19:39:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 19:47:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485200340212840,
"parent_rev": "608d39b1e2e2666c2a37512452be51309709362e", "commit_rev":
"788a643320ed52bdaf97892a43c505314678157a"}

[commit-bot: I haz the power, 2017-01-23 19:47:33]

Description was changed from

==========
HttpServer WebSocket: don't crash on data before server handshake

(Which includes the case of a server delegate which doesn't do anything
 with WebSocket requests).

Note: this assumes 860f9bd is in, since otherwise the test 
would leak. 

BUG=606428
==========

to

==========
HttpServer WebSocket: don't crash on data before server handshake

(Which includes the case of a server delegate which doesn't do anything
 with WebSocket requests).

Note: this assumes 860f9bd is in, since otherwise the test 
would leak. 

BUG=606428

Review-Url: https://codereview.chromium.org/2640363004
Cr-Commit-Position: refs/heads/master@{#445439}
Committed:
https://chromium.googlesource.com/chromium/src/+/788a643320ed52bdaf97892a43c5...
==========

[commit-bot: I haz the power, 2017-01-23 19:47:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/788a643320ed52bdaf97892a43c5...