Check for invalid use of data frame opcodes.

Check for invalid use of data frame opcodes.

Text and Binary opcodes cannot appear in the middle of a message, and
the Continuation opcode cannot appear at the start.

Fail the channel if violations of this rule are found.

BUG=343105
TEST=net_unittests, layout tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252001

[yhirano, 2014-02-13 05:36:41]

https://codereview.chromium.org/157033013/diff/1/net/websockets/websocket_cha...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/157033013/diff/1/net/websockets/websocket_cha...
net/websockets/websocket_channel.cc:670: if (expecting_to_read_continuation_) {
Shouldn't the received message be ignored when the state_ is not CONNECTED?

https://codereview.chromium.org/157033013/diff/1/net/websockets/websocket_cha...
net/websockets/websocket_channel.cc:679: if (!expecting_to_read_continuation_) {
ditto

[Adam Rice, 2014-02-13 07:29:49]

https://codereview.chromium.org/157033013/diff/1/net/websockets/websocket_cha...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/157033013/diff/1/net/websockets/websocket_cha...
net/websockets/websocket_channel.cc:670: if (expecting_to_read_continuation_) {
On 2014/02/13 05:36:42, yhirano wrote:
> Shouldn't the received message be ignored when the state_ is not CONNECTED?

Yes, you are right. Done.

https://codereview.chromium.org/157033013/diff/1/net/websockets/websocket_cha...
net/websockets/websocket_channel.cc:679: if (!expecting_to_read_continuation_) {
On 2014/02/13 05:36:42, yhirano wrote:
> ditto

Done.

[yhirano, 2014-02-13 08:51:28]

lgtm

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
net/websockets/websocket_channel.cc:744: if (expecting_continuation !=
expecting_to_read_continuation_) {
[opt] Just using |opcode| instead of |expecting_continuation| here and unifying
the switch branches seems cleaner for me.

[Adam Rice, 2014-02-18 05:22:36]

+tyoshino

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
net/websockets/websocket_channel.cc:744: if (expecting_continuation !=
expecting_to_read_continuation_) {
On 2014/02/13 08:51:28, yhirano wrote:
> [opt] Just using |opcode| instead of |expecting_continuation| here and
unifying
> the switch branches seems cleaner for me.

Yeah, this code is ugly. But I don't want to check |opcode| again when I just
switched on it immediately before calling this method.

I'm going to send to tyoshino to see if he has any ideas for how to do this
nicely.

[tyoshino (SeeGerritForStatus), 2014-02-18 06:24:12]

lgtm

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
net/websockets/websocket_channel.cc:744: if (expecting_continuation !=
expecting_to_read_continuation_) {
On 2014/02/18 05:22:36, Adam Rice wrote:
> On 2014/02/13 08:51:28, yhirano wrote:
> > [opt] Just using |opcode| instead of |expecting_continuation| here and
> unifying
> > the switch branches seems cleaner for me.
> 
> Yeah, this code is ugly. But I don't want to check |opcode| again when I just
> switched on it immediately before calling this method.
> 
> I'm going to send to tyoshino to see if he has any ideas for how to do this
> nicely.

In general, it's good to have exhaustive switch-case for handling enum. But
here, I think you can choose to compare opcode with OpcodeContinuation using ==
by also adding DCHECKs to ensure that we're here only when opcode is one of
text, binary and cont. There's no worry to forget handling a new enum (opcode).
Then, we fail with DCHECK.

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
net/websockets/websocket_channel.h:332: bool expecting_to_read_continuation_;
In this class, "Read" is used for receiving data from the socket. Let's use
handle since this corresponds to HandleFrame() code.

[Adam Rice, 2014-02-18 11:12:42]

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
net/websockets/websocket_channel.cc:744: if (expecting_continuation !=
expecting_to_read_continuation_) {
On 2014/02/18 06:24:12, tyoshino wrote:
> On 2014/02/18 05:22:36, Adam Rice wrote:
> > On 2014/02/13 08:51:28, yhirano wrote:
> > > [opt] Just using |opcode| instead of |expecting_continuation| here and
> > unifying
> > > the switch branches seems cleaner for me.
> > 
> > Yeah, this code is ugly. But I don't want to check |opcode| again when I
just
> > switched on it immediately before calling this method.
> > 
> > I'm going to send to tyoshino to see if he has any ideas for how to do this
> > nicely.
> 
> In general, it's good to have exhaustive switch-case for handling enum. But
> here, I think you can choose to compare opcode with OpcodeContinuation using
==
> by also adding DCHECKs to ensure that we're here only when opcode is one of
> text, binary and cont. There's no worry to forget handling a new enum
(opcode).
> Then, we fail with DCHECK.

Is this what you mean?

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
net/websockets/websocket_channel.h:332: bool expecting_to_read_continuation_;
On 2014/02/18 06:24:12, tyoshino wrote:
> In this class, "Read" is used for receiving data from the socket. Let's use
> handle since this corresponds to HandleFrame() code.

Done.

[tyoshino (SeeGerritForStatus), 2014-02-19 04:32:14]

On 2014/02/18 11:12:42, Adam Rice wrote:
>
https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
> File net/websockets/websocket_channel.cc (right):
> 
>
https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
> net/websockets/websocket_channel.cc:744: if (expecting_continuation !=
> expecting_to_read_continuation_) {
> On 2014/02/18 06:24:12, tyoshino wrote:
> > On 2014/02/18 05:22:36, Adam Rice wrote:
> > > On 2014/02/13 08:51:28, yhirano wrote:
> > > > [opt] Just using |opcode| instead of |expecting_continuation| here and
> > > unifying
> > > > the switch branches seems cleaner for me.
> > > 
> > > Yeah, this code is ugly. But I don't want to check |opcode| again when I
> just
> > > switched on it immediately before calling this method.
> > > 
> > > I'm going to send to tyoshino to see if he has any ideas for how to do
this
> > > nicely.
> > 
> > In general, it's good to have exhaustive switch-case for handling enum. But
> > here, I think you can choose to compare opcode with OpcodeContinuation using
> ==
> > by also adding DCHECKs to ensure that we're here only when opcode is one of
> > text, binary and cont. There's no worry to forget handling a new enum
> (opcode).
> > Then, we fail with DCHECK.
> 
> Is this what you mean?

Yes!

> 
>
https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
> File net/websockets/websocket_channel.h (right):
> 
>
https://codereview.chromium.org/157033013/diff/50001/net/websockets/websocket...
> net/websockets/websocket_channel.h:332: bool expecting_to_read_continuation_;
> On 2014/02/18 06:24:12, tyoshino wrote:
> > In this class, "Read" is used for receiving data from the socket. Let's use
> > handle since this corresponds to HandleFrame() code.
> 
> Done.

LGTM

[Adam Rice, 2014-02-19 05:27:25]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2014-02-19 05:27:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ricea@chromium.org/157033013/160001

[commit-bot: I haz the power, 2014-02-19 08:28:55]

Change committed as 252001