Implement new websocket handshake based on draft-hixie-thewebsocketprotocol-76

Implement new websocket handshake based on draft-hixie-thewebsocketprotocol-76

BUG=none
TEST=net_unittests passes

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42736

[ukai, 2010-03-18 09:49:35]

This implementation will be used for live experiment and/or unit tests before
submitting to webkit.

[tyoshino (SeeGerritForStatus), 2010-03-25 04:51:04]

http://codereview.chromium.org/1108002/diff/3001/4006
File net/websockets/websocket_handshake.h (right):

http://codereview.chromium.org/1108002/diff/3001/4006#newcode86
net/websockets/websocket_handshake.h:86: void GetExpectedResponse(uint8
*expected) const;
stick * to left.

http://codereview.chromium.org/1108002/diff/3001/4006#newcode105
net/websockets/websocket_handshake.h:105: bool ProcessHeaders(const
HttpResponseHeaders& headers);
virtual

http://codereview.chromium.org/1108002/diff/3001/4006#newcode106
net/websockets/websocket_handshake.h:106: bool CheckResponseHeaders() const;
virtual

http://codereview.chromium.org/1108002/diff/3001/4008
File net/websockets/websocket_handshake_draft75.h (right):

http://codereview.chromium.org/1108002/diff/3001/4008#newcode50
net/websockets/websocket_handshake_draft75.h:50: bool ProcessHeaders(const
HttpResponseHeaders& headers);
virtual

http://codereview.chromium.org/1108002/diff/3001/4008#newcode56
net/websockets/websocket_handshake_draft75.h:56: bool CheckResponseHeaders()
const;
virtual

http://codereview.chromium.org/1108002/diff/3001/4010
File net/websockets/websocket_handshake_unittest.cc (right):

http://codereview.chromium.org/1108002/diff/3001/4010#newcode44
net/websockets/websocket_handshake_unittest.cc:44: break;
how about checking i + 2 is now expected_lines.size() and make sure
expected_lines[expected_lines.size() - 1] is also ""?

http://codereview.chromium.org/1108002/diff/3001/4010#newcode52
net/websockets/websocket_handshake_unittest.cc:52: break;
ditto

http://codereview.chromium.org/1108002/diff/3001/4010#newcode68
net/websockets/websocket_handshake_unittest.cc:68:
ExpectHeaderEquals(expected_lines, actual_lines);
Doesn't compile?

http://codereview.chromium.org/1108002/diff/3001/4010#newcode74
net/websockets/websocket_handshake_unittest.cc:74: << ", actual_key3:" <<
DumpKey(actual_key3, 12);
this sounds like that mismatch in \r\n\r\n is also key3 mismatch.

http://codereview.chromium.org/1108002/diff/3001/4010#newcode113
net/websockets/websocket_handshake_unittest.cc:113: ExpectHandshakeMesageEquals(
ExpectHandshakeMessageEquals

[tyoshino (SeeGerritForStatus), 2010-03-25 07:02:20]

http://codereview.chromium.org/1108002/diff/3001/4005
File net/websockets/websocket_handshake.cc (right):

http://codereview.chromium.org/1108002/diff/3001/4005#newcode216
net/websockets/websocket_handshake.cc:216: uint32 result = min +
static_cast<int>(number % range);
don't we need to make this uniform?

http://codereview.chromium.org/1108002/diff/3001/4005#newcode280
net/websockets/websocket_handshake.cc:280: if (s.length() == 1) {
please add todo to revisit here when the decision is made on hybi.

http://codereview.chromium.org/1108002/diff/3001/4005#newcode290
net/websockets/websocket_handshake.cc:290: int chpos = rand_(0,
sizeof(randomCharacterInSecWebSocketKey));
sizeof( ... ) - 1 ?

http://codereview.chromium.org/1108002/diff/3001/4006
File net/websockets/websocket_handshake.h (right):

http://codereview.chromium.org/1108002/diff/3001/4006#newcode91
net/websockets/websocket_handshake.h:91: static void SetRandomNumberGenerator(
Please put some comment to explain the requirement on rand. Especially, the
range of return value.

[tyoshino (SeeGerritForStatus), 2010-03-25 07:19:28]

http://codereview.chromium.org/1108002/diff/3001/4010
File net/websockets/websocket_handshake_unittest.cc (right):

http://codereview.chromium.org/1108002/diff/3001/4010#newcode44
net/websockets/websocket_handshake_unittest.cc:44: break;
> expected_lines[expected_lines.size() - 1] is also ""?

Sorry, ignore this.

[ukai, 2010-03-25 07:22:12]

http://codereview.chromium.org/1108002/diff/3001/4005
File net/websockets/websocket_handshake.cc (right):

http://codereview.chromium.org/1108002/diff/3001/4005#newcode216
net/websockets/websocket_handshake.cc:216: uint32 result = min +
static_cast<int>(number % range);
On 2010/03/25 07:02:20, tyoshino wrote:
> don't we need to make this uniform?

unsigned?

http://codereview.chromium.org/1108002/diff/3001/4005#newcode280
net/websockets/websocket_handshake.cc:280: if (s.length() == 1) {
On 2010/03/25 07:02:20, tyoshino wrote:
> please add todo to revisit here when the decision is made on hybi.

I change UintToString to StringPrintf("%011u", product) to avoid 1 length s case
here.

http://codereview.chromium.org/1108002/diff/3001/4005#newcode290
net/websockets/websocket_handshake.cc:290: int chpos = rand_(0,
sizeof(randomCharacterInSecWebSocketKey));
On 2010/03/25 07:02:20, tyoshino wrote:
> sizeof( ... ) - 1 ?

Done.

http://codereview.chromium.org/1108002/diff/3001/4006
File net/websockets/websocket_handshake.h (right):

http://codereview.chromium.org/1108002/diff/3001/4006#newcode86
net/websockets/websocket_handshake.h:86: void GetExpectedResponse(uint8
*expected) const;
On 2010/03/25 04:51:04, tyoshino wrote:
> stick * to left.

Done.

http://codereview.chromium.org/1108002/diff/3001/4006#newcode105
net/websockets/websocket_handshake.h:105: bool ProcessHeaders(const
HttpResponseHeaders& headers);
On 2010/03/25 04:51:04, tyoshino wrote:
> virtual

Done.

http://codereview.chromium.org/1108002/diff/3001/4006#newcode106
net/websockets/websocket_handshake.h:106: bool CheckResponseHeaders() const;
On 2010/03/25 04:51:04, tyoshino wrote:
> virtual

Done.

http://codereview.chromium.org/1108002/diff/3001/4008
File net/websockets/websocket_handshake_draft75.h (right):

http://codereview.chromium.org/1108002/diff/3001/4008#newcode50
net/websockets/websocket_handshake_draft75.h:50: bool ProcessHeaders(const
HttpResponseHeaders& headers);
On 2010/03/25 04:51:04, tyoshino wrote:
> virtual

Done.

http://codereview.chromium.org/1108002/diff/3001/4008#newcode56
net/websockets/websocket_handshake_draft75.h:56: bool CheckResponseHeaders()
const;
On 2010/03/25 04:51:04, tyoshino wrote:
> virtual

Done.

http://codereview.chromium.org/1108002/diff/3001/4010
File net/websockets/websocket_handshake_unittest.cc (right):

http://codereview.chromium.org/1108002/diff/3001/4010#newcode44
net/websockets/websocket_handshake_unittest.cc:44: break;
On 2010/03/25 04:51:04, tyoshino wrote:
> how about checking i + 2 is now expected_lines.size() and make sure
> expected_lines[expected_lines.size() - 1] is also ""?

I don't think so. key_3 might include "\r\n".

http://codereview.chromium.org/1108002/diff/3001/4010#newcode68
net/websockets/websocket_handshake_unittest.cc:68:
ExpectHeaderEquals(expected_lines, actual_lines);
On 2010/03/25 04:51:04, tyoshino wrote:
> Doesn't compile?

Done.

http://codereview.chromium.org/1108002/diff/3001/4010#newcode74
net/websockets/websocket_handshake_unittest.cc:74: << ", actual_key3:" <<
DumpKey(actual_key3, 12);
On 2010/03/25 04:51:04, tyoshino wrote:
> this sounds like that mismatch in \r\n\r\n is also key3 mismatch.

Yes. the last 12 bytes must exactly \r\n\r\n<key3>.
We should not allow \r\n\r\n<garbage><key3> or so.

http://codereview.chromium.org/1108002/diff/3001/4010#newcode113
net/websockets/websocket_handshake_unittest.cc:113: ExpectHandshakeMesageEquals(
On 2010/03/25 04:51:04, tyoshino wrote:
> ExpectHandshakeMessageEquals

Done.

[Yuta Kitamura, 2010-03-25 08:21:37]

LGTM if the following issues are addressed.

http://codereview.chromium.org/1108002/diff/15001/16005
File net/websockets/websocket_handshake.cc (right):

http://codereview.chromium.org/1108002/diff/15001/16005#newcode71
net/websockets/websocket_handshake.cc:71: random_shuffle(fields.begin(),
fields.end());
nit: std::random_shuffle

http://codereview.chromium.org/1108002/diff/15001/16005#newcode127
net/websockets/websocket_handshake.cc:127: std::string host =
StringToLowerASCII(url_.host());
Just curious, what happens if host contains non-ASCII characters? What should we
do in such a case?

http://codereview.chromium.org/1108002/diff/15001/16010
File net/websockets/websocket_handshake_unittest.cc (right):

http://codereview.chromium.org/1108002/diff/15001/16010#newcode74
net/websockets/websocket_handshake_unittest.cc:74: const char* actual_key3 =
actual.data() + actual.size() - 12;
It's better to check if actual.size() >= 12 (and possibly expected.size() >= 12
as well), otherwise we get an invalid pointer.

[ukai, 2010-03-25 10:33:35]

http://codereview.chromium.org/1108002/diff/15001/16005
File net/websockets/websocket_handshake.cc (right):

http://codereview.chromium.org/1108002/diff/15001/16005#newcode71
net/websockets/websocket_handshake.cc:71: random_shuffle(fields.begin(),
fields.end());
On 2010/03/25 08:21:37, Yuta Kitamura wrote:
> nit: std::random_shuffle

Done.

http://codereview.chromium.org/1108002/diff/15001/16005#newcode127
net/websockets/websocket_handshake.cc:127: std::string host =
StringToLowerASCII(url_.host());
On 2010/03/25 08:21:37, Yuta Kitamura wrote:
> Just curious, what happens if host contains non-ASCII characters? What should
we
> do in such a case?

it is expected to be punny-encoded.
note that this code is used only for live-experiment now, so no such case would
happen.

http://codereview.chromium.org/1108002/diff/15001/16010
File net/websockets/websocket_handshake_unittest.cc (right):

http://codereview.chromium.org/1108002/diff/15001/16010#newcode74
net/websockets/websocket_handshake_unittest.cc:74: const char* actual_key3 =
actual.data() + actual.size() - 12;
On 2010/03/25 08:21:37, Yuta Kitamura wrote:
> It's better to check if actual.size() >= 12 (and possibly expected.size() >=
12
> as well), otherwise we get an invalid pointer.

added asserts

[Yuta Kitamura, 2010-03-26 02:28:09]

LGTM

http://codereview.chromium.org/1108002/diff/23001/9006
File net/websockets/websocket_handshake.cc (left):

http://codereview.chromium.org/1108002/diff/23001/9006#oldcode163
net/websockets/websocket_handshake.cc:163: // Returns false otherwise.
It's nice to keep the comment above.

http://codereview.chromium.org/1108002/diff/23001/9011
File net/websockets/websocket_handshake_unittest.cc (right):

http://codereview.chromium.org/1108002/diff/23001/9011#newcode75
net/websockets/websocket_handshake_unittest.cc:75: ASSERT_GT(actual.size(),
12U);
ASSERT_GE?

And, it is probably not good to stop the entire test here. I suggest:

EXPECT_GE(actual.size(), 12U);
if (actual.size() < 12U)
  return;

I think ASSERTing "expected" is acceptable.

[tyoshino (SeeGerritForStatus), 2010-03-26 02:53:59]

LGTM

http://codereview.chromium.org/1108002/diff/3001/4005
File net/websockets/websocket_handshake.cc (right):

http://codereview.chromium.org/1108002/diff/3001/4005#newcode216
net/websockets/websocket_handshake.cc:216: uint32 result = min +
static_cast<int>(number % range);
On 2010/03/25 07:22:25, ukai wrote:
> On 2010/03/25 07:02:20, tyoshino wrote:
> > don't we need to make this uniform?
> 
> unsigned?

I meant that the distribution of the result of modulo operation will be biased.
But OK. RandUint64 is using rand(3). Please add some note to revisit here for
checking if there's any security concern on this process.

http://codereview.chromium.org/1108002/diff/23001/9006#newcode279
net/websockets/websocket_handshake.cc:279: for (uint32 i = 0; i < space; i++) {
Good. But why 11 digits?