Implement handshake timeout on WebSocket

Implement handshake timeout on WebSocket

Previously, WebSocket has no handshake timeout, i.e. It waited unlimitedly
when the server accepts the TCP connection but doesn't send any handshake
response. This CL fixes the issue.

BUG=391263
R=ricea@chromium.org

Committed: https://crrev.com/569e09ce5c01419c056b3d557d64c0830e45d7c9
Cr-Commit-Position: refs/heads/master@{#294373}

[yhirano, 2014-09-11 06:29:09]

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

[yhirano, 2014-09-11 06:30:14]

PTAL.

I set 1 minute for the timeout duration.

[Adam Rice, 2014-09-11 07:52:42]

https://codereview.chromium.org/565573002/diff/40001/net/websockets/websocket...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/565573002/diff/40001/net/websockets/websocket...
net/websockets/websocket_stream.cc:118: const TimeDelta timeout =
TimeDelta::FromMinutes(1);
The timeout should be defined as a constant at the top of the file. Also, it is
traditional to specify them in seconds.

1 minute is definitely too short, as the connection timeout is 4 minutes. I am
thinking that 4 minutes might be a good choice, because then it would be
impossible to distinguish between connection timeout and server timeout from
Javascript.

https://codereview.chromium.org/565573002/diff/40001/net/websockets/websocket...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/565573002/diff/40001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:424: scoped_ptr<MockWeakTimer> timer(new
MockWeakTimer(false, false));
You should create a separate test that the timer runs, rather than adding extra
conditions to this test.

[yhirano, 2014-09-11 08:45:37]

https://codereview.chromium.org/565573002/diff/40001/net/websockets/websocket...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/565573002/diff/40001/net/websockets/websocket...
net/websockets/websocket_stream.cc:118: const TimeDelta timeout =
TimeDelta::FromMinutes(1);
On 2014/09/11 07:52:42, Adam Rice wrote:
> The timeout should be defined as a constant at the top of the file. Also, it
is
> traditional to specify them in seconds.
> 
> 1 minute is definitely too short, as the connection timeout is 4 minutes. I am
> thinking that 4 minutes might be a good choice, because then it would be
> impossible to distinguish between connection timeout and server timeout from
> Javascript.

Done.

https://codereview.chromium.org/565573002/diff/40001/net/websockets/websocket...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/565573002/diff/40001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:424: scoped_ptr<MockWeakTimer> timer(new
MockWeakTimer(false, false));
On 2014/09/11 07:52:42, Adam Rice wrote:
> You should create a separate test that the timer runs, rather than adding
extra
> conditions to this test.

Done.

[Adam Rice, 2014-09-11 09:47:39]

https://codereview.chromium.org/565573002/diff/80001/net/websockets/websocket...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/565573002/diff/80001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:1148: EXPECT_FALSE(request_info_);
I think you should remove these two assertions as they are not relevant to the
test (and they are covered by other test cases).

https://codereview.chromium.org/565573002/diff/80001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:1155: EXPECT_TRUE(stream_);
Also please remove these three assertions as above.

[yhirano, 2014-09-11 10:46:49]

https://codereview.chromium.org/565573002/diff/80001/net/websockets/websocket...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/565573002/diff/80001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:1148: EXPECT_FALSE(request_info_);
On 2014/09/11 09:47:39, Adam Rice wrote:
> I think you should remove these two assertions as they are not relevant to the
> test (and they are covered by other test cases).

Done.

https://codereview.chromium.org/565573002/diff/80001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:1155: EXPECT_TRUE(stream_);
On 2014/09/11 09:47:39, Adam Rice wrote:
> Also please remove these three assertions as above.

I want to make sure the operation succeeds.
So I'd leave one assertion.

[Adam Rice, 2014-09-11 11:08:11]

lgtm

[yhirano, 2014-09-11 11:10:28]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-09-11 11:10:51]

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

[commit-bot: I haz the power, 2014-09-11 12:08:31]

Committed patchset #6 (id:100001) as ef792ffcbda5e9e9c4c97e86c33cc71f98fd4bc1

[commit-bot: I haz the power, 2014-09-11 12:10:34]

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