Fix WebSocket crash bug.

If SocketStream closes while waiting ResolveProxy, it badly
calls DoResolveProxyComplete from DoLoop invoked by SocketStream::Close,
and real callback of ResolveProxy failed to call SocketStream::OnIOCompleted.

So, don't run DoLoop when closed, if SocketStream is calling APIs of
proxy_service or resolver. Check closing_ after the API calls back
and closes the SocketStream.

r54707 broke SocketStreamTest::CloseFlushPendingWrite.
This CL fixes this by not closing socket if it is still writing on the socket.

BUG=50394, 46750
TEST=websocket/tests doesn't crash, and net_unittests passes

Review URL: http://codereview.chromium.org/3076024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@55043 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 60 insertions, 24 deletions

diff --git a/net/socket_stream/socket_stream.cc b/net/socket_stream/socket_stream.cc
index fe3afeb..448ebbb 100644
--- a/net/socket_stream/socket_stream.cc
+++ b/net/socket_stream/socket_stream.cc
@@ -66,6 +66,7 @@ SocketStream::SocketStream(const GURL& url, Delegate* delegate)
       write_buf_offset_(0),
       write_buf_size_(0),
       closing_(false),
+      server_closed_(false),
       metrics_(new SocketStreamMetrics(url)) {
   DCHECK(MessageLoop::current()) <<
       "The current MessageLoop must exist";
@@ -188,12 +189,29 @@ void SocketStream::Close() {
   // of AddRef() and Release() in Connect() and Finish(), respectively.
   if (next_state_ == STATE_NONE)
     return;
-  closing_ = true;
-  // Close asynchronously, so that delegate won't be called
-  // back before returning Close().
   MessageLoop::current()->PostTask(
       FROM_HERE,
-      NewRunnableMethod(this, &SocketStream::DoLoop, OK));
+      NewRunnableMethod(this, &SocketStream::DoClose));
+}
+
+void SocketStream::DoClose() {
+  closing_ = true;
+  // If next_state_ is STATE_TCP_CONNECT, it's waiting other socket establishing
+  // connection.  If next_state_ is STATE_AUTH_REQUIRED, it's waiting for
+  // restarting.  In these states, we'll close the SocketStream now.
+  if (next_state_ == STATE_TCP_CONNECT || next_state_ == STATE_AUTH_REQUIRED) {
+    DoLoop(ERR_ABORTED);
+    return;
+  }
+  // If next_state_ is STATE_READ_WRITE, we'll run DoLoop and close
+  // the SocketStream.
+  // If it's writing now, we should defer the closing after the current
+  // writing is completed.
+  if (next_state_ == STATE_READ_WRITE && !current_write_buf_)
+    DoLoop(ERR_ABORTED);
+
+  // In other next_state_, we'll wait for callback of other APIs, such as
+  // ResolveProxy().
 }
 
 void SocketStream::RestartWithAuth(
@@ -330,7 +348,8 @@ void SocketStream::OnIOCompleted(int result) {
 void SocketStream::OnReadCompleted(int result) {
   if (result == 0) {
     // 0 indicates end-of-file, so socket was closed.
-    next_state_ = STATE_CLOSE;
+    // Don't close the socket if it's still writing.
+    server_closed_ = true;
   } else if (result > 0 && read_buf_) {
     result = DidReceiveData(result);
   }
@@ -408,12 +427,16 @@ void SocketStream::DoLoop(int result) {
       case STATE_READ_WRITE:
         result = DoReadWrite(result);
         break;
+      case STATE_AUTH_REQUIRED:
+        NOTREACHED() << "Should not run DoLoop in STATE_AUTH_REQUIRED state.";
+        Finish(result);
+        return;
       case STATE_CLOSE:
         DCHECK_LE(result, OK);
         Finish(result);
         return;
       default:
-        NOTREACHED() << "bad state";
+        NOTREACHED() << "bad state " << state;
         Finish(result);
         return;
     }
@@ -841,26 +864,32 @@ int SocketStream::DoReadWrite(int result) {
 
   next_state_ = STATE_READ_WRITE;
 
-  if (!read_buf_) {
-    // No read pending.
-    read_buf_ = new IOBuffer(kReadBufferSize);
-    result = socket_->Read(read_buf_, kReadBufferSize, &read_callback_);
-    if (result > 0) {
-      return DidReceiveData(result);
-    } else if (result == 0) {
-      // 0 indicates end-of-file, so socket was closed.
-      next_state_ = STATE_CLOSE;
-      return ERR_CONNECTION_CLOSED;
-    }
-    // If read is pending, try write as well.
-    // Otherwise, return the result and do next loop (to close the connection).
-    if (result != ERR_IO_PENDING) {
-      next_state_ = STATE_CLOSE;
-      return result;
+  // If server already closed the socket, we don't try to read.
+  if (!server_closed_) {
+    if (!read_buf_) {
+      // No read pending and server didn't close the socket.
+      read_buf_ = new IOBuffer(kReadBufferSize);
+      result = socket_->Read(read_buf_, kReadBufferSize, &read_callback_);
+      if (result > 0) {
+        return DidReceiveData(result);
+      } else if (result == 0) {
+        // 0 indicates end-of-file, so socket was closed.
+        next_state_ = STATE_CLOSE;
+        server_closed_ = true;
+        return ERR_CONNECTION_CLOSED;
+      }
+      // If read is pending, try write as well.
+      // Otherwise, return the result and do next loop (to close the
+      // connection).
+      if (result != ERR_IO_PENDING) {
+        next_state_ = STATE_CLOSE;
+        server_closed_ = true;
+        return result;
+      }
     }
+    // Read is pending.
+    DCHECK(read_buf_);
   }
-  // Read is pending.
-  DCHECK(read_buf_);
 
   if (write_buf_ && !current_write_buf_) {
     // No write pending.
diff --git a/net/socket_stream/socket_stream.h b/net/socket_stream/socket_stream.h
index 860f785..ec97276 100644
--- a/net/socket_stream/socket_stream.h
+++ b/net/socket_stream/socket_stream.h
@@ -220,6 +220,8 @@ class SocketStream : public base::RefCountedThreadSafe<SocketStream> {
   // Used for WebSocketThrottleTest.
   void CopyAddrInfo(struct addrinfo* head);
 
+  void DoClose();
+
   // Finishes the job.
   // Calls OnError and OnClose of delegate, and no more
   // notifications will be sent to delegate.
@@ -321,6 +323,7 @@ class SocketStream : public base::RefCountedThreadSafe<SocketStream> {
   PendingDataQueue pending_write_bufs_;
 
   bool closing_;
+  bool server_closed_;
 
   scoped_ptr<SocketStreamMetrics> metrics_;
 
diff --git a/net/socket_stream/socket_stream_unittest.cc b/net/socket_stream/socket_stream_unittest.cc
index 9480d88..db1ac87 100644
--- a/net/socket_stream/socket_stream_unittest.cc
+++ b/net/socket_stream/socket_stream_unittest.cc
@@ -308,6 +308,10 @@ TEST_F(SocketStreamTest, BasicAuthProxy) {
     MockRead("HTTP/1.1 200 Connection Established\r\n"),
     MockRead("Proxy-agent: Apache/2.2.8\r\n"),
     MockRead("\r\n"),
+    // SocketStream::DoClose is run asynchronously.  Socket can be read after
+    // "\r\n".  We have to give ERR_IO_PENDING to SocketStream then to indicate
+    // server doesn't close the connection.
+    MockRead(true, ERR_IO_PENDING)
   };
   StaticSocketDataProvider data2(data_reads2, arraysize(data_reads2),
                                  data_writes2, arraysize(data_writes2));