secure proxy support in websocket

secure proxy support in websocket

If an error is returned synchronously, next_state_ is not STATE_CLOSE.
In such case, we should just close the socket stream with the error.

BUG=83950
TEST=net_unittest --gtest_filter=SocketStreamTest.* pass

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

[ukai, 2011-07-25 09:43:41]

I reverted the previous one because buildbot failed
e.g.
http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28Views%20dbg%...

[ RUN      ] SocketStreamTest.SecureProxyConnectError
[1553:1553:0725/022223:90851953:WARNING:proxy_service.cc(456)] PAC support
disabled because there is no system implementation
[1553:1553:0725/022223:90852456:FATAL:socket_stream.cc(522)] Check failed:
next_state_ == STATE_CLOSE (16 vs. 21)
Backtrace:
	base::debug::StackTrace::StackTrace() [0xca8c12]
	logging::LogMessage::~LogMessage() [0xcb97c2]
	net::SocketStream::DoLoop() [0xbc6cda]
	net::SocketStream::OnIOCompleted() [0xbc63d4]

state 16 is STATE_SECURE_PROXY_CONNECT_COMPLETE, so it is because
DoSecureProxyConnect() returns error.

So, we should not check next_state_ is STATE_CLOSE at 522.
Instead, we make next_state_ to STATE_CLOSE, so it closes with the error soon.

[Yuta Kitamura, 2011-07-25 10:24:44]

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream.cc
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream...
net/socket_stream/socket_stream.cc:920: return socket_->Connect(&io_callback_);
Can't you set STATE_CLOSE to next_state_ if Connect() has failed?

[ukai, 2011-07-25 10:29:59]

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream.cc
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream...
net/socket_stream/socket_stream.cc:920: return socket_->Connect(&io_callback_);
On 2011/07/25 10:24:44, Yuta Kitamura wrote:
> Can't you set STATE_CLOSE to next_state_ if Connect() has failed?

It's possible, but other methods (Read, Write, ...) might return the error too.
So it would be better to handle in DoLoop, I think.

[Yuta Kitamura, 2011-07-25 10:44:48]

I'm not convinced about the change yet, but declaring "LGTM" because at least
the code should work.

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream.cc
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream...
net/socket_stream/socket_stream.cc:920: return socket_->Connect(&io_callback_);
Um... if I read DoReadWrite() correctly, it always changes the state to
STATE_CLOSE if |result| indicates an error (i.e. neither OK nor ERR_IO_PENDING).

I'm still thinking it's better to change the state here.

[ukai, 2011-07-25 11:49:04]

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream.cc
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream...
net/socket_stream/socket_stream.cc:920: return socket_->Connect(&io_callback_);
On 2011/07/25 10:44:48, Yuta Kitamura wrote:
> Um... if I read DoReadWrite() correctly, it always changes the state to
> STATE_CLOSE if |result| indicates an error (i.e. neither OK nor
ERR_IO_PENDING).

not only DoReadWrite().
We had a potential bug when other Do* method other than Do*Complete() returns
error other than OK or ERR_IO_PENDING.

For example, DoResolveProxy() returns ResolveProxy() without checking. So if
ResolveProxy() returns an error synchronously, next_state_ is
STATE_RESOLVE_PROXY_COMPLETE and error is not ERR_IO_PENDING, so it fails
DCHECK_EQ in DoLoop().  We can say the same thing in other Do* method.

We can add error check in every Do* method, but I think it's ok to set
next_state_ to STATE_CLOSE in DoLoop if some error is returned.

> 
> I'm still thinking it's better to change the state here.

[ukai, 2011-07-25 12:33:14]

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream.cc
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream...
net/socket_stream/socket_stream.cc:522: next_state_ = STATE_CLOSE;
hmm, we might not need this.
If Do* method returns error, then Do*Complete() method will check the error and
set next_state_ to STATE_CLOSE.

[Yuta Kitamura, 2011-07-25 12:39:45]

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream.cc
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream...
net/socket_stream/socket_stream.cc:522: next_state_ = STATE_CLOSE;
I think that's better.

[ukai, 2011-07-26 01:14:19]

On 2011/07/25 12:39:45, Yuta Kitamura wrote:
>
http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream.cc
> File net/socket_stream/socket_stream.cc (right):
> 
>
http://codereview.chromium.org/7484065/diff/1/net/socket_stream/socket_stream...
> net/socket_stream/socket_stream.cc:522: next_state_ = STATE_CLOSE;
> I think that's better.

PTAL

[Yuta Kitamura, 2011-07-26 03:18:21]

LGTM.

[Takashi Toyoshima, 2011-07-26 03:21:52]

Sorry after LGTM.
Actually, statue transition in DoLoop was redundant.
But, I feel previous implementation was strong to human errors.

I think it's better to add DCHECK if you remove that transition.