WebSocket over SPDY: WebSocketJob handling SpdyWebSocketStream

Add WebSocket over SPDY experimental implementation.
(Re-land with memory leak fix)

- Realize WebSocketJob's internal protocol switch to SPDY using SpdyWebSocketStream
- Add simple test to verify connection over SPDY

BUG=42320
TEST=net_unittests --gtest_filter=WebSocketJobTest\*

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

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

[Takashi Toyoshima, 2011-07-07 07:38:31]

Hi, Yuta,

This is the last change to achieve an experimental implementation for WSoS.
After this change, we could use WSoS on existent SPDY session with enable flag.

Setting up for SPDY connection test is a pretty difficult.
So I publish this change for request only with simple spdy connection test.
I'd like to add other ERR_IO_PENDING tests later.

Also, I'm starting to consider WebSocket's protocol versions to test opening and
closing handshakes over SPDY.

Thanks

[Takashi Toyoshima, 2011-07-07 07:40:05]

s/request/review/

[Takashi Toyoshima, 2011-07-07 08:39:00]

Willchan reviewed 7312005, so I rebase this change to make it simple.
Now, this change includes only four files!

[Yuta Kitamura, 2011-07-08 07:40:21]

Overall looks good.

http://codereview.chromium.org/7185032/diff/19005/net/socket_stream/socket_st...
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/7185032/diff/19005/net/socket_stream/socket_st...
net/socket_stream/socket_stream.cc:298: delegate->OnError(this, result);
delegate->OnError() is called after delegate_ is set to null. Is this fine? (I
think it's probably okay, but just asking.)

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
net/websockets/websocket_job.cc:551: if (!socket_->is_secure() || use_ssl) {
This seems right, but an identical expression "!(socket_->is_secure() &&
!use_ssl)" may be a bit more straightforward.

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
net/websockets/websocket_job.cc:566: }
(Optional) These blocks seem nested too deeply.

Generally speaking, it is preferred to use "early exits" style rather than "late
exits".

Preferred:
  if (...)
    return status;
  ...
  if (...)
    return status;
  ...
  if (...)
    return status;
  ...

Not preferred:
  if (...) {
    if (...) {
      if (...) {
        ...
      }
    }
  }
  return status;

This helps readability a lot. However, this is not required by our style guide,
so this is optional.

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
net/websockets/websocket_job.cc:595: CompleteIO(TrySpdyStream());
TrySpdyStream() can return ERR_IO_PENDING. Is it okay to call
CompleteIO(ERR_IO_PENDING)? (because ERR_IO_PENDING means the I/O in question
has not finished yet)

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
File net/websockets/websocket_job_unittest.cc (right):

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
net/websockets/websocket_job_unittest.cc:331: }
The three blocks above sound a bit confusing...

How about the following:

  if (stream_type == STREAM_MOCK_SOCKET) {
    socket_ = ...;
  } else if (stream_type == STREAM_SOCKET ||
             stream_type == STREAM_SPDY_WEBSOCKET) {
    if (stream_type == STREAM_SPDY_WEBSOCKET) {
      ...
    }
    ...
    socket_ = ...;
    ...
  }

It's also good to add an assertion "DCHECK_NE(STREAM_INVALID, stream_type)". If
you can make sure that stream_type != STREAM_INVALID, the conditional statement
above will become much simpler.

[Takashi Toyoshima, 2011-07-08 08:35:14]

Thank you for nice review!

http://codereview.chromium.org/7185032/diff/19005/net/socket_stream/socket_st...
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/7185032/diff/19005/net/socket_stream/socket_st...
net/socket_stream/socket_stream.cc:298: delegate->OnError(this, result);
Yes.
It will be better than calling it without clearance of reference from this
object.
Here, all we have to do for this delegate is call OnError and OnClose.
If we didn't clear its reference, other events might call delegates via its
reference.

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
net/websockets/websocket_job.cc:551: if (!socket_->is_secure() || use_ssl) {
On 2011/07/08 07:40:21, Yuta Kitamura wrote:
> This seems right, but an identical expression "!(socket_->is_secure() &&
> !use_ssl)" may be a bit more straightforward.

Done.

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
net/websockets/websocket_job.cc:566: }
I love it.
Done!

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
net/websockets/websocket_job.cc:595: CompleteIO(TrySpdyStream());
Oh!
It's a bug to cause DCHECK failure !!
In that case, we should call CompleteIO() from OnCreatedSpdyStream().
Thanks.

I have a plan to add tests which can check this case.
But, I'd like to split it into another change.
The bug is fixed in the next change set.

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
File net/websockets/websocket_job_unittest.cc (right):

http://codereview.chromium.org/7185032/diff/19005/net/websockets/websocket_jo...
net/websockets/websocket_job_unittest.cc:331: }
Thanks.
New one looks fine!

[Yuta Kitamura, 2011-07-11 07:03:27]

OK, LGTM.

[Takashi Toyoshima, 2011-07-11 12:20:12]

Sorry, this test has a leak problem.
I reopen it.

http://build.chromium.org/p/chromium.memory/builders/Linux%20Tests%20%28valgr...

03:00:01 memcheck_analyze.py [ERROR] FAIL! There were 2 errors: 
03:00:01 memcheck_analyze.py [ERROR] Command:
src/sconsbuild/Release/net_unittests <snip>
Leak_DefinitelyLost
68 bytes in 1 blocks are definitely lost in loss record 2,123 of 4,064
  operator new(unsigned int) (m_replacemalloc/vg_replace_malloc.c:905)
  net::WebSocketJobTest::TestConnectBySpdy(bool)
(net/websockets/websocket_job_unittest.cc:816)
  net::WebSocketJobTest_ConnectBySpdySpdyEnabled_Test::TestBody()
(net/websockets/websocket_job_unittest.cc:919)
  void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*)
(testing/gtest/src/gtest.cc:2090)
  void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*)
(testing/gtest/src/gtest.cc:2142)
  testing::Test::Run() (testing/gtest/src/gtest.cc:2162)
Suppression (error hash=#9EBE489948A078A1#):
  For more info on using suppressions see
http://dev.chromium.org/developers/how-tos/using-valgrind#TOC-Suppressing-Errors
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:_Znw*
   fun:_ZN3net16WebSocketJobTest17TestConnectBySpdyEb
   fun:_ZN3net46WebSocketJobTest_ConnectBySpdySpdyEnabled_Test8TestBodyEv
  
fun:_ZN7testing8internal38HandleSehExceptionsInMethodIfSupportedINS_4TestEvEET0_PT_MS4_FS3_vEPKc
  
fun:_ZN7testing8internal35HandleExceptionsInMethodIfSupportedINS_4TestEvEET0_PT_MS4_FS3_vEPKc
}

03:00:01 memcheck_analyze.py [ERROR] Command:
src/sconsbuild/Release/net_unittests <snip>
Leak_DefinitelyLost
68 bytes in 1 blocks are definitely lost in loss record 2,122 of 4,064
  operator new(unsigned int) (m_replacemalloc/vg_replace_malloc.c:905)
  net::WebSocketJobTest::TestConnectBySpdy(bool)
(net/websockets/websocket_job_unittest.cc:820)
  net::WebSocketJobTest_ConnectBySpdy_Test::TestBody()
(net/websockets/websocket_job_unittest.cc:914)
  void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*)
(testing/gtest/src/gtest.cc:2090)
  void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*)
(testing/gtest/src/gtest.cc:2142)
  testing::Test::Run() (testing/gtest/src/gtest.cc:2162)
Suppression (error hash=#3B4731268A3B74EC#):
  For more info on using suppressions see
http://dev.chromium.org/developers/how-tos/using-valgrind#TOC-Suppressing-Errors
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:_Znw*
   fun:_ZN3net16WebSocketJobTest17TestConnectBySpdyEb
   fun:_ZN3net35WebSocketJobTest_ConnectBySpdy_Test8TestBodyEv
  
fun:_ZN7testing8internal38HandleSehExceptionsInMethodIfSupportedINS_4TestEvEET0_PT_MS4_FS3_vEPKc
  
fun:_ZN7testing8internal35HandleExceptionsInMethodIfSupportedINS_4TestEvEET0_PT_MS4_FS3_vEPKc
}

03:00:01 valgrind_test.py [INFO] Please see
http://dev.chromium.org/developers/how-tos/using-valgrind for the info on
Memcheck/Valgrind
03:00:01 valgrind_test.py [ERROR] Analyze failed.
03:00:01 valgrind_test.py [INFO] Search the log for '[ERROR]' to see the error
reports.
03:00:01 valgrind_test.py [INFO] elapsed time: 00:09:02

[Takashi Toyoshima, 2011-07-11 14:32:47]

Set 18 reproduce these memory leaks.
Set 19 fixed them.
Try on valgrind still failed, but it is not related this change.

[Yuta Kitamura, 2011-07-11 15:16:24]

LGTM, you can reland the patch anytime. Please keep an eye on the memory bots
after you land this.