Simplify the the logic for setting the final response status for a

net/quic/chromium/quic_http_stream.cc

Index: net/quic/chromium/quic_http_stream.cc
diff --git a/net/quic/chromium/quic_http_stream.cc b/net/quic/chromium/quic_http_stream.cc
index e6200fbcb85942fa187e96ad5dbe5f004e078b34..594d67bc6ea46ff6803ab43960c64112d4dba2d8 100644
--- a/net/quic/chromium/quic_http_stream.cc
+++ b/net/quic/chromium/quic_http_stream.cc
@@ -50,14 +50,15 @@ QuicHttpStream::QuicHttpStream(
       next_state_(STATE_NONE),
       session_(session),
       quic_version_(session->GetQuicVersion()),
-      session_error_(OK),
+      session_error_(ERR_UNEXPECTED),
       was_handshake_confirmed_(session->IsCryptoHandshakeConfirmed()),
       stream_(nullptr),
       request_info_(nullptr),
       request_body_stream_(nullptr),
       priority_(MINIMUM_PRIORITY),
       response_info_(nullptr),
-      response_status_(OK),
+      has_response_status_(false),
+      response_status_(ERR_UNEXPECTED),
       response_headers_received_(false),
       headers_bytes_received_(0),
       headers_bytes_sent_(0),
@@ -66,6 +67,7 @@ QuicHttpStream::QuicHttpStream(
       closed_is_first_stream_(false),
       user_buffer_len_(0),
       quic_connection_error_(QUIC_NO_ERROR),
+      quic_stream_error_(QUIC_STREAM_NO_ERROR),
       port_migration_detected_(false),
       found_promise_(false),
       push_handle_(nullptr),
@@ -158,8 +160,7 @@ int QuicHttpStream::InitializeStream(const HttpRequestInfo* request_info,
   CHECK(callback_.is_null());
   DCHECK(!stream_);
   if (!session_)
-    return was_handshake_confirmed_ ? ERR_CONNECTION_CLOSED
-                                    : ERR_QUIC_HANDSHAKE_FAILED;
+    return GetResponseStatus();
 
   stream_net_log.AddEvent(
       NetLogEventType::HTTP_STREAM_REQUEST_BOUND_TO_QUIC_SESSION,
@@ -250,10 +251,9 @@ int QuicHttpStream::SendRequest(const HttpRequestHeaders& request_headers,
     UMA_HISTOGRAM_BOOLEAN("Net.QuicSession.CookieSentToAccountsOverChannelId",
                           ssl_info.channel_id_sent);
   }
-  if ((!found_promise_ && !stream_) || !session_) {
-    return was_handshake_confirmed_ ? ERR_CONNECTION_CLOSED
-                                    : ERR_QUIC_HANDSHAKE_FAILED;
-  }
+
+  if ((!found_promise_ && !stream_) || !session_)
+    return GetResponseStatus();
 
   // Store the serialized request headers.
   CreateSpdyHeadersFromHttpRequest(*request_info_, request_headers,
@@ -310,7 +310,7 @@ int QuicHttpStream::ReadResponseHeaders(const CompletionCallback& callback) {
   CHECK(!callback.is_null());
 
   if (stream_ == nullptr)
-    return response_status_;
+    return GetResponseStatus();
 
   // Check if we already have the response headers. If so, return synchronously.
   if (response_headers_received_)
@@ -338,10 +338,9 @@ int QuicHttpStream::ReadResponseBody(IOBuffer* buf,
   // anymore.
   request_info_ = nullptr;
 
-  if (!stream_) {
-    // If the stream is already closed, there is no body to read.
-    return response_status_;
-  }
+  // If the stream is already closed, there is no body to read.
+  if (!stream_)
+    return GetResponseStatus();
 
   int rv = ReadAvailableData(buf, buf_len);
   if (rv != ERR_IO_PENDING)
@@ -353,13 +352,12 @@ int QuicHttpStream::ReadResponseBody(IOBuffer* buf,
   return ERR_IO_PENDING;
 }
 
-void QuicHttpStream::Close(bool not_reusable) {
-  // Note: the not_reusable flag has no meaning for SPDY streams.
+void QuicHttpStream::Close(bool /*not_reusable*/) {
+  GetResponseStatus();  // Sets response_status_ if not already set.
+  // Note: the not_reusable flag has no meaning for QUIC streams.
   if (stream_) {
     stream_->SetDelegate(nullptr);
     stream_->Reset(QUIC_STREAM_CANCELLED);
-    response_status_ = was_handshake_confirmed_ ? ERR_CONNECTION_CLOSED
-                                                : ERR_QUIC_HANDSHAKE_FAILED;
   }
   ResetStream();
 }
@@ -438,6 +436,7 @@ void QuicHttpStream::OnHeadersAvailable(const SpdyHeaderBlock& headers,
       // Close the read side. If the write side has been closed, this will
       // invoke QuicHttpStream::OnClose to reset the stream.
       stream_->OnFinRead();
+      SetResponseStatus(OK);
     }
     return;
   }
@@ -469,35 +468,30 @@ void QuicHttpStream::OnDataAvailable() {
 }
 
 void QuicHttpStream::OnClose() {
-  if (stream_->connection_error() != QUIC_NO_ERROR ||
-      stream_->stream_error() != QUIC_STREAM_NO_ERROR) {
-    response_status_ = was_handshake_confirmed_ ? ERR_QUIC_PROTOCOL_ERROR
-                                                : ERR_QUIC_HANDSHAKE_FAILED;
-  } else if (!response_headers_received_) {
-    response_status_ = ERR_ABORTED;
-  }
-
   quic_connection_error_ = stream_->connection_error();
+  quic_stream_error_ = stream_->stream_error();
+  GetResponseStatus();  // Sets response_status_ if not already set.
+
   ResetStream();
-  if (in_loop_) {
-    // If already in DoLoop(), |callback_| will be handled when DoLoop() exits.
+  // If already in DoLoop(), |callback_| will be handled when DoLoop() exits.
+  if (in_loop_)
     return;
-  }
+
   if (!callback_.is_null()) {
-    DoCallback(response_status_);
+    DoCallback(GetResponseStatus());

[Buck, 2017/03/28 21:19:57]

Why not use a cached result from the call above?  If there is a reason, perhaps
explain in a comment.

[Ryan Hamilton, 2017/03/28 21:54:03]

Done. Good idea!

(Well, and then it changed as a result of a different suggestion of yours.  but
still done :>)

   }
 }
 
 void QuicHttpStream::OnError(int error) {
   ResetStream();
-  response_status_ =
-      was_handshake_confirmed_ ? error : ERR_QUIC_HANDSHAKE_FAILED;
+  session_error_ = error;
+  GetResponseStatus();  // Sets response_status_ if not already set.
   if (in_loop_) {
     // If already in DoLoop(), |callback_| will be handled when DoLoop() exits.
     return;
   }
   if (!callback_.is_null())
-    DoCallback(response_status_);
+    DoCallback(GetResponseStatus());
 }
 
 bool QuicHttpStream::HasSendHeadersComplete() {
@@ -520,9 +514,11 @@ void QuicHttpStream::OnSuccessfulVersionNegotiation(
 }
 
 void QuicHttpStream::OnSessionClosed(int error, bool port_migration_detected) {
-  Close(false);
   session_error_ = error;
   port_migration_detected_ = port_migration_detected;
+  GetResponseStatus();  // Sets response_status_ if not already set.
+
+  Close(false);
   session_.reset();
 }
 
@@ -620,8 +616,10 @@ int QuicHttpStream::DoRequestStream() {
 
 int QuicHttpStream::DoRequestStreamComplete(int rv) {
   DCHECK(rv == OK || !stream_);
-  if (rv != OK)
-    return was_handshake_confirmed_ ? rv : ERR_QUIC_HANDSHAKE_FAILED;
+  if (rv != OK) {
+    session_error_ = rv;
+    return GetResponseStatus();
+  }
 
   stream_->SetDelegate(this);
   if (request_info_->load_flags & LOAD_DISABLE_CONNECTION_MIGRATION) {
@@ -668,7 +666,7 @@ int QuicHttpStream::DoWaitForConfirmationComplete(int rv) {
 
 int QuicHttpStream::DoSendHeaders() {
   if (!stream_)
-    return ERR_UNEXPECTED;
+    return GetResponseStatus();
 
   // Log the actual request with the URL Request's net log.
   stream_net_log_.AddEvent(
@@ -692,7 +690,7 @@ int QuicHttpStream::DoSendHeadersComplete(int rv) {
 
   // If the stream is already closed, don't read the request body.
   if (!stream_)
-    return response_status_;
+    return GetResponseStatus();
 
   next_state_ = request_body_stream_ ? STATE_READ_REQUEST_BODY : STATE_OPEN;
 
@@ -709,7 +707,7 @@ int QuicHttpStream::DoReadRequestBody() {
 int QuicHttpStream::DoReadRequestBodyComplete(int rv) {
   // If the stream is already closed, don't continue.
   if (!stream_)
-    return response_status_;
+    return GetResponseStatus();
 
   // |rv| is the result of read from the request body from the last call to
   // DoSendBody().
@@ -731,7 +729,7 @@ int QuicHttpStream::DoReadRequestBodyComplete(int rv) {
 
 int QuicHttpStream::DoSendBody() {
   if (!stream_)
-    return ERR_UNEXPECTED;
+    return GetResponseStatus();
 
   CHECK(request_body_stream_);
   CHECK(request_body_buf_.get());
@@ -755,7 +753,7 @@ int QuicHttpStream::DoSendBodyComplete(int rv) {
 
   // If the stream is already closed, don't continue.
   if (!stream_)
-    return response_status_;
+    return GetResponseStatus();
 
   request_body_buf_->DidConsume(request_body_buf_->BytesRemaining());
 
@@ -805,6 +803,7 @@ int QuicHttpStream::ReadAvailableData(IOBuffer* buf, int buf_len) {
   if (stream_->IsDoneReading()) {
     stream_->SetDelegate(nullptr);
     stream_->OnFinRead();
+    SetResponseStatus(OK);
     ResetStream();
   }
   return rv;
@@ -831,4 +830,42 @@ void QuicHttpStream::ResetStream() {
     request_body_stream_->Reset();
 }
 
+int QuicHttpStream::GetResponseStatus() {
+  if (!has_response_status_) {
+    SetResponseStatus(ComputeResponseStatus());
+  }
+
+  return response_status_;
+}
+
+void QuicHttpStream::SetResponseStatus(int response_status) {
+  has_response_status_ = true;
+  response_status_ = response_status;
+}
+
+int QuicHttpStream::ComputeResponseStatus() const {
+  DCHECK(!has_response_status_);
+
+  // If the handshake has failed this will be handled by the
+  // QuicStreamFactory and HttpStreamFactory to mark QUIC as broken.
+  if (!was_handshake_confirmed_)
+    return ERR_QUIC_HANDSHAKE_FAILED;
+
+  // If the session was aborted by a higher layer, simply use that error code.
+  if (session_error_ != ERR_UNEXPECTED)
+    return session_error_;
+
+  // For bening errors, return ERR_CONNECTION_CLOSED to permit

[Buck, 2017/03/28 21:19:57]

s/bening/benign/

[Ryan Hamilton, 2017/03/28 21:54:03]

Done.

+  // HttpNetworkTransaction to retry the request (assuming that
+  // headers have not been received, and this is not the firs stream).

[Buck, 2017/03/28 21:19:57]

s/firs/first/

[Ryan Hamilton, 2017/03/28 21:54:03]

Done.

+  if ((quic_connection_error_ == QUIC_CONNECTION_IP_POOLED ||
+       quic_connection_error_ == QUIC_NETWORK_IDLE_TIMEOUT ||
+       quic_connection_error_ == QUIC_NO_ERROR) &&
+      quic_stream_error_ == QUIC_STREAM_NO_ERROR) {
+    return ERR_CONNECTION_CLOSED;
+  }
+
+  return ERR_QUIC_PROTOCOL_ERROR;
+}
+
 }  // namespace net