When we get a silent TCP RST, SPDY connections need to retry.

Fixing this involved a couple of minor changes.
* We were not tracking whether a SPDY session should be retried.
  The HTTP logic uses "is_socket_idle()" to determine if the socket
  was once good and is worth retrying.  Because SPDY is not serialized
  added methods through the SpdySession and SpdyHttpStream for this.
  (See ShouldResendFailedRequest)
* The spdy_http_stream was not notifying the caller when
  OnSendHeadersComplete occurred when there is no upload body.
  This isn't strictly necessary, but keeps the HttpNetworkTransaction
  state more consistent.
 
BUG=50510
TEST=SpdyNetworkTransactionTest


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

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

8 files changed, 183 insertions, 15 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 060b9e1..aa557db 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -1363,7 +1363,7 @@ int HttpNetworkTransaction::DoSpdySendRequest() {
 
 int HttpNetworkTransaction::DoSpdySendRequestComplete(int result) {
   if (result < 0)
-    return result;
+    return HandleIOError(result);
 
   next_state_ = STATE_SPDY_READ_HEADERS;
   return OK;
@@ -1376,11 +1376,13 @@ int HttpNetworkTransaction::DoSpdyReadHeaders() {
 
 int HttpNetworkTransaction::DoSpdyReadHeadersComplete(int result) {
   // TODO(willchan): Flesh out the support for HTTP authentication here.
+  if (result < 0)
+    return HandleIOError(result);
+
   if (result == OK)
     headers_valid_ = true;
 
   LogTransactionConnectedMetrics();
-
   return result;
 }
 
@@ -1664,7 +1666,8 @@ int HttpNetworkTransaction::HandleIOError(int error) {
     case ERR_CONNECTION_RESET:
     case ERR_CONNECTION_CLOSED:
     case ERR_CONNECTION_ABORTED:
-      LogIOErrorMetrics(*connection_);
+      if (!using_spdy_)
+        LogIOErrorMetrics(*connection_);
       if (ShouldResendRequest(error)) {
         ResetConnectionAndRequestForResend();
         error = OK;
@@ -1689,6 +1692,9 @@ HttpResponseHeaders* HttpNetworkTransaction::GetResponseHeaders() const {
 }
 
 bool HttpNetworkTransaction::ShouldResendRequest(int error) const {
+  if (using_spdy_ && spdy_http_stream_ != NULL)
+    return spdy_http_stream_->ShouldResendFailedRequest(error);
+
   // NOTE: we resend a request only if we reused a keep-alive connection.
   // This automatically prevents an infinite resend loop because we'll run
   // out of the cached keep-alive connections eventually.
@@ -1700,13 +1706,22 @@ bool HttpNetworkTransaction::ShouldResendRequest(int error) const {
 }
 
 void HttpNetworkTransaction::ResetConnectionAndRequestForResend() {
-  if (connection_->socket())
-    connection_->socket()->Disconnect();
-  connection_->Reset();
+  // Note:  When using SPDY we may not own a connection.
+  if (connection_.get()) {
+    if (connection_->socket())
+      connection_->socket()->Disconnect();
+    connection_->Reset();
+  } else {
+    DCHECK(using_spdy_);
+    connection_.reset(new ClientSocketHandle);
+  }
+
   // We need to clear request_headers_ because it contains the real request
   // headers, but we may need to resend the CONNECT request first to recreate
   // the SSL tunnel.
 
+  spdy_http_stream_.reset(NULL);
+
   request_headers_.clear();
   next_state_ = STATE_INIT_CONNECTION;  // Resend the request.
 }
diff --git a/net/socket/socket_test_util.cc b/net/socket/socket_test_util.cc
index 8319563..ce7f3fc06 100644
--- a/net/socket/socket_test_util.cc
+++ b/net/socket/socket_test_util.cc
@@ -522,7 +522,7 @@ DelayedSocketData::DelayedSocketData(
 }
 
 MockRead DelayedSocketData::GetNextRead() {
-  if (write_delay_)
+  if (write_delay_ > 0)
     return MockRead(true, ERR_IO_PENDING);
   return StaticSocketDataProvider::GetNextRead();
 }
diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index 6a328cf..bb64021 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -327,6 +327,8 @@ void SpdyHttpStream::Cancel() {
 }
 
 bool SpdyHttpStream::OnSendHeadersComplete(int status) {
+  if (user_callback_)
+    DoCallback(status);
   return request_body_stream_.get() == NULL;
 }
 
@@ -405,6 +407,10 @@ void SpdyHttpStream::OnClose(int status) {
     DoCallback(status);
 }
 
+bool SpdyHttpStream::ShouldResendFailedRequest(int error) const {
+  return spdy_session_->ShouldResendFailedRequest(error);
+}
+
 void SpdyHttpStream::ScheduleBufferedReadCallback() {
   // If there is already a scheduled DoBufferedReadCallback, don't issue
   // another one.  Mark that we have received more data and return.
diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h
index 939b929..faf1388 100644
--- a/net/spdy/spdy_http_stream.h
+++ b/net/spdy/spdy_http_stream.h
@@ -113,6 +113,8 @@ class SpdyHttpStream : public SpdyStream::Delegate, public HttpStream {
   // ReadResponseHeaders or ReadResponseBody.
   virtual void OnClose(int status);
 
+  bool ShouldResendFailedRequest(int error) const;
+
  private:
   // Call the user callback.
   void DoCallback(int rv);
diff --git a/net/spdy/spdy_http_stream_unittest.cc b/net/spdy/spdy_http_stream_unittest.cc
index 9faa795..ee2ef94 100644
--- a/net/spdy/spdy_http_stream_unittest.cc
+++ b/net/spdy/spdy_http_stream_unittest.cc
@@ -54,8 +54,7 @@ TEST_F(SpdyHttpStreamTest, SendRequest) {
   scoped_ptr<spdy::SpdyFrame> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
   MockRead reads[] = {
     CreateMockRead(*resp, 2),
-    MockRead(true, ERR_IO_PENDING, 3),  // Force a pause
-    MockRead(false, 0, 4)  // EOF
+    MockRead(false, 0, 3)  // EOF
   };
 
   HostPortPair host_port_pair("www.google.com", 80);
@@ -77,12 +76,14 @@ TEST_F(SpdyHttpStreamTest, SendRequest) {
             http_stream->SendRequest("", NULL, &response, &callback));
   EXPECT_TRUE(http_session_->spdy_session_pool()->HasSession(host_port_pair));
 
-  // This triggers the MockWrite and reads 2 & 3
+  // This triggers the MockWrite and read 2
   callback.WaitForResult();
 
-  // This triggers read 4. The empty read causes the session to shut down.
+  // This triggers read 3. The empty read causes the session to shut down.
   data()->CompleteRead();
 
+  // Because we abandoned the stream, we don't expect to find a session in the
+  // pool anymore.
   EXPECT_TRUE(!http_session_->spdy_session_pool()->HasSession(host_port_pair));
   EXPECT_TRUE(data()->at_read_eof());
   EXPECT_TRUE(data()->at_write_eof());
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index 1aefc39..5460aae 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -987,7 +987,7 @@ TEST_P(SpdyNetworkTransactionTest, PostWithEarlySynReply) {
   scoped_ptr<spdy::SpdyFrame>
       req(ConstructSpdyPost(request.upload_data->GetContentLength(), NULL, 0));
   scoped_ptr<spdy::SpdyFrame> body(ConstructSpdyBodyFrame(1, true));
-    MockWrite writes[] = {
+  MockWrite writes[] = {
     CreateMockWrite(*req.get(), 2),
     CreateMockWrite(*body.get(), 3),  // POST upload frame
   };
@@ -1007,7 +1007,6 @@ TEST_P(SpdyNetworkTransactionTest, PostWithEarlySynReply) {
   helper.AddData(data.get());
   helper.RunPreTestSetup();
   helper.RunDefaultTest();
-  helper.VerifyDataNotConsumed();
   TransactionHelperResult out = helper.output();
   EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
 }
@@ -1216,7 +1215,7 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateOverflow) {
   ASSERT_TRUE(session->spdy_session_pool() != NULL);
   session->spdy_session_pool()->ClearSessions();
 
-  EXPECT_FALSE(data->at_read_eof());
+  EXPECT_TRUE(data->at_read_eof());
   EXPECT_TRUE(data->at_write_eof());
 
   SpdySession::SetFlowControl(false);
@@ -1881,6 +1880,7 @@ TEST_P(SpdyNetworkTransactionTest, DecompressFailureOnSynReply) {
   MockRead reads[] = {
     CreateMockRead(*resp),
     CreateMockRead(*body),
+    MockRead(true, 0, 0)
   };
 
   scoped_refptr<DelayedSocketData> data(
@@ -2685,14 +2685,61 @@ TEST_P(SpdyNetworkTransactionTest, GoAwayWithActiveStream) {
   scoped_ptr<spdy::SpdyFrame> go_away(ConstructSpdyGoAway());
   MockRead reads[] = {
     CreateMockRead(*go_away),
-    MockRead(true, 0, 0)  // EOF
+    MockRead(true, 0, 0),  // EOF
+  };
+
+  // Because the server is telling us to GOAWAY without finishing, the browser
+  // will attempt to re-establish.
+  scoped_ptr<spdy::SpdyFrame> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
+  scoped_ptr<spdy::SpdyFrame> body(ConstructSpdyBodyFrame(1, true));
+  MockRead reads2[] = {
+    CreateMockRead(*resp),
+    CreateMockRead(*body),
+    MockRead(true, 0, 0),  // EOF
   };
 
   scoped_refptr<DelayedSocketData> data(
       new DelayedSocketData(1, reads, arraysize(reads),
                             writes, arraysize(writes)));
+  scoped_refptr<DelayedSocketData> data2(
+      new DelayedSocketData(1, reads2, arraysize(reads2),
+                            writes, arraysize(writes)));
   NormalSpdyTransactionHelper helper(CreateGetRequest(),
                                      BoundNetLog(), GetParam());
+  helper.AddData(data);
+  helper.AddData(data2);
+  helper.RunToCompletion(data.get());
+  TransactionHelperResult out = helper.output();
+  EXPECT_EQ(OK, out.rv);
+}
+
+TEST_P(SpdyNetworkTransactionTest, GoAwayWithActiveStreamFail) {
+  scoped_ptr<spdy::SpdyFrame> req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
+  MockWrite writes[] = { CreateMockWrite(*req) };
+
+  scoped_ptr<spdy::SpdyFrame> go_away(ConstructSpdyGoAway());
+  MockRead reads[] = {
+    CreateMockRead(*go_away),
+    MockRead(true, 0, 0),  // EOF
+  };
+
+  // Because the server is telling us to GOAWAY without finishing, the browser
+  // will attempt to re-establish.  On the second connection, just close.  This
+  // should trigger the ERR_CONNECTION_CLOSED status.
+  MockRead reads2[] = {
+    MockRead(true, 0, 0),  // EOF
+  };
+
+  scoped_refptr<DelayedSocketData> data(
+      new DelayedSocketData(1, reads, arraysize(reads),
+                            writes, arraysize(writes)));
+  scoped_refptr<DelayedSocketData> data2(
+      new DelayedSocketData(1, reads2, arraysize(reads2),
+                            writes, arraysize(writes)));
+  NormalSpdyTransactionHelper helper(CreateGetRequest(),
+                                     BoundNetLog(), GetParam());
+  helper.AddData(data);
+  helper.AddData(data2);
   helper.RunToCompletion(data.get());
   TransactionHelperResult out = helper.output();
   EXPECT_EQ(ERR_CONNECTION_CLOSED, out.rv);
@@ -2735,4 +2782,88 @@ TEST_P(SpdyNetworkTransactionTest, CloseWithActiveStream) {
   // Verify that we consumed all test data.
   helper.VerifyDataConsumed();
 }
+
+// When we get a TCP-level RST, we need to retry a HttpNetworkTransaction
+// on a new connection, if the connection was previously known to be good.
+// This can happen when a server reboots without saying goodbye, or when
+// we're behind a NAT that masked the RST.
+TEST_P(SpdyNetworkTransactionTest, VerifyRetryOnConnectionReset) {
+  scoped_ptr<spdy::SpdyFrame> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
+  scoped_ptr<spdy::SpdyFrame> body(ConstructSpdyBodyFrame(1, true));
+  MockRead reads[] = {
+    CreateMockRead(*resp),
+    CreateMockRead(*body),
+    MockRead(true, ERR_IO_PENDING),
+    MockRead(true, ERR_CONNECTION_RESET),
+  };
+
+  MockRead reads2[] = {
+    CreateMockRead(*resp),
+    CreateMockRead(*body),
+    MockRead(true, 0, 0)  // EOF
+  };
+
+  // This test has a couple of variants.
+  enum {
+    // Induce the RST while waiting for our transaction to send.
+    VARIANT_RST_DURING_SEND_COMPLETION,
+    // Induce the RST while waiting for our transaction to read.
+    // In this case, the send completed - everything copied into the SNDBUF.
+    VARIANT_RST_DURING_READ_COMPLETION
+  };
+
+  for (int variant = VARIANT_RST_DURING_SEND_COMPLETION;
+       variant <= VARIANT_RST_DURING_READ_COMPLETION;
+       ++variant) {
+    scoped_refptr<DelayedSocketData> data1(
+        new DelayedSocketData(1, reads, arraysize(reads),
+                              NULL, 0));
+
+    scoped_refptr<DelayedSocketData> data2(
+        new DelayedSocketData(1, reads2, arraysize(reads2),
+                               NULL, 0));
+
+    NormalSpdyTransactionHelper helper(CreateGetRequest(),
+                                       BoundNetLog(), GetParam());
+    helper.AddData(data1.get());
+    helper.AddData(data2.get());
+    helper.RunPreTestSetup();
+
+    for (int i = 0; i < 2; ++i) {
+      scoped_ptr<HttpNetworkTransaction> trans(
+          new HttpNetworkTransaction(helper.session()));
+
+      TestCompletionCallback callback;
+      int rv = trans->Start(&helper.request(), &callback, BoundNetLog());
+      EXPECT_EQ(ERR_IO_PENDING, rv);
+      // On the second transaction, we trigger the RST.
+      if (i == 1) {
+        if (variant == VARIANT_RST_DURING_READ_COMPLETION) {
+          // Writes to the socket complete asynchronously on SPDY by running
+          // through the message loop.  Complete the write here.
+          MessageLoop::current()->RunAllPending();
+        }
+
+        // Now schedule the ERR_CONNECTION_RESET.
+        EXPECT_EQ(3u, data1->read_index());
+        data1->CompleteRead();
+        EXPECT_EQ(4u, data1->read_index());
+      }
+      rv = callback.WaitForResult();
+      EXPECT_EQ(OK, rv);
+
+      const HttpResponseInfo* response = trans->GetResponseInfo();
+      EXPECT_TRUE(response->headers != NULL);
+      EXPECT_TRUE(response->was_fetched_via_spdy);
+      std::string response_data;
+      rv = ReadTransaction(trans.get(), &response_data);
+      EXPECT_EQ(OK, rv);
+      EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine());
+      EXPECT_EQ("hello!", response_data);
+    }
+
+    helper.VerifyDataConsumed();
+  }
+}
+
 }  // namespace net
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index d195fb9..2aa2289 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -166,6 +166,7 @@ SpdySession::SpdySession(const HostPortPair& host_port_pair,
       streams_pushed_count_(0),
       streams_pushed_and_claimed_count_(0),
       streams_abandoned_count_(0),
+      frames_received_(0),
       sent_settings_(false),
       received_settings_(false),
       in_session_pool_(true),
@@ -1139,6 +1140,8 @@ void SpdySession::OnControl(const spdy::SpdyControlFrame* frame) {
     }
   }
 
+  frames_received_++;
+
   switch (type) {
     case spdy::GOAWAY:
       OnGoAway(*reinterpret_cast<const spdy::SpdyGoAwayControlFrame*>(frame));
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index 812aab8..22facba 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -141,6 +141,15 @@ class SpdySession : public base::RefCounted<SpdySession>,
   // error.
   void CloseSessionOnError(net::Error err);
 
+  // Indicates whether we should retry failed requets on a session.
+  bool ShouldResendFailedRequest(int error) const {
+    // NOTE: we resend a request only if this connection has successfully
+    // been used for some data receiving.  Otherwise, we assume the error
+    // is not transient.
+    // This is primarily for use with recovery from a TCP RESET.
+    return frames_received_ > 0;
+  }
+
  private:
   friend class base::RefCounted<SpdySession>;
   FRIEND_TEST_ALL_PREFIXES(SpdySessionTest, GetActivePushStream);
@@ -335,6 +344,7 @@ class SpdySession : public base::RefCounted<SpdySession>,
   int streams_pushed_count_;
   int streams_pushed_and_claimed_count_;
   int streams_abandoned_count_;
+  int frames_received_;
   bool sent_settings_;      // Did this session send settings when it started.
   bool received_settings_;  // Did this session receive at least one settings
                             // frame.