Refactor HttpNetworkTransaction to remove side effects in some member functions.

net/http/http_network_transaction.cc

 // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_network_transaction.h"
 
 #include "base/scoped_ptr.h"
 #include "base/compiler_specific.h"
 #include "base/field_trial.h"
 #include "base/string_util.h"
@@ skipping 13 matching lines @@
 #include "net/http/http_chunked_decoder.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_request_info.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
 
 using base::Time;
 
 namespace net {
 
+namespace {
+
+void BuildRequestHeaders(const HttpRequestInfo* request_info,
+                         const std::string& authorization_headers,
+                         const UploadDataStream* upload_data_stream,
+                         bool using_proxy,
+                         std::string* request_headers) {
+  const std::string path = using_proxy ?
+      HttpUtil::SpecForRequest(request_info->url) :
+      HttpUtil::PathForRequest(request_info->url);
+  *request_headers =
+      StringPrintf("%s %s HTTP/1.1\r\nHost: %s",
+                   request_info->method.c_str(), path.c_str(),
+                   request_info->url.host().c_str());
+  if (request_info->url.IntPort() != -1)
+    *request_headers += ":" + request_info->url.port();
+  *request_headers += "\r\n";
+
+  // For compat with HTTP/1.0 servers and proxies:
+  if (using_proxy)
+    *request_headers += "Proxy-";
+  *request_headers += "Connection: keep-alive\r\n";
+
+  if (!request_info->user_agent.empty()) {
+    StringAppendF(request_headers, "User-Agent: %s\r\n",
+                  request_info->user_agent.c_str());
+  }
+
+  // Our consumer should have made sure that this is a safe referrer.  See for
+  // instance WebCore::FrameLoader::HideReferrer.
+  if (request_info->referrer.is_valid())
+    StringAppendF(request_headers, "Referer: %s\r\n",
+                  request_info->referrer.spec().c_str());
+
+  // Add a content length header?
+  if (upload_data_stream) {
+    StringAppendF(request_headers, "Content-Length: %llu\r\n",

[wtc, 2009/04/30 18:57:20]

I just learned that we're supposed to use the PRIu64 macro
instead of llu to print a uint64_t variable:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...

[willchan no longer on Chromium, 2009/04/30 19:32:40]

That's for uint64_t.  We're using uint64 here.  uint64!=uint64_t.

+                  upload_data_stream->size());
+  } else if (request_info->method == "POST" || request_info->method == "PUT" ||
+             request_info->method == "HEAD") {
+    // An empty POST/PUT request still needs a content length.  As for HEAD,
+    // IE and Safari also add a content length header.  Presumably it is to
+    // support sending a HEAD request to an URL that only expects to be sent a
+    // POST or some other method that normally would have a message body.
+    *request_headers += "Content-Length: 0\r\n";
+  }
+
+  // Honor load flags that impact proxy caches.
+  if (request_info->load_flags & LOAD_BYPASS_CACHE) {
+    *request_headers += "Pragma: no-cache\r\nCache-Control: no-cache\r\n";
+  } else if (request_info->load_flags & LOAD_VALIDATE_CACHE) {
+    *request_headers += "Cache-Control: max-age=0\r\n";
+  }
+
+  if (!authorization_headers.empty()) {
+    *request_headers += authorization_headers;
+  }
+
+  // TODO(darin): Need to prune out duplicate headers.
+
+  *request_headers += request_info->extra_headers;
+  *request_headers += "\r\n";
+}
+
+// The HTTP CONNECT method for establishing a tunnel connection is documented
+// in draft-luotonen-web-proxy-tunneling-01.txt and RFC 2817, Sections 5.2 and
+// 5.3.
+void BuildTunnelRequest(const HttpRequestInfo* request_info,
+                        const std::string& authorization_headers,
+                        std::string* request_headers) {
+  // RFC 2616 Section 9 says the Host request-header field MUST accompany all
+  // HTTP/1.1 requests.
+  *request_headers = StringPrintf("CONNECT %s:%d HTTP/1.1\r\n",
+      request_info->url.host().c_str(), request_info->url.EffectiveIntPort());
+  *request_headers += "Host: " + request_info->url.host();
+  if (request_info->url.has_port())
+    *request_headers += ":" + request_info->url.port();
+  *request_headers += "\r\n";
+
+  if (!request_info->user_agent.empty())
+    StringAppendF(request_headers, "User-Agent: %s\r\n",
+                  request_info->user_agent.c_str());
+
+  if (!authorization_headers.empty()) {
+    *request_headers += authorization_headers;
+  }
+
+  *request_headers += "\r\n";
+}
+
+}  // namespace
+
 //-----------------------------------------------------------------------------
 
 HttpNetworkTransaction::HttpNetworkTransaction(HttpNetworkSession* session,
                                                ClientSocketFactory* csf)
     : pending_auth_target_(HttpAuth::AUTH_NONE),
       ALLOW_THIS_IN_INITIALIZER_LIST(
           io_callback_(this, &HttpNetworkTransaction::OnIOComplete)),
       user_callback_(NULL),
       session_(session),
       request_(NULL),
@@ skipping 240 matching lines @@
 HttpNetworkTransaction::~HttpNetworkTransaction() {
   // If we still have an open socket, then make sure to close it so we don't
   // try to reuse it later on.
   if (connection_.is_initialized())
     connection_.set_socket(NULL);
 
   if (pac_request_)
     session_->proxy_service()->CancelPacRequest(pac_request_);
 }
 
-void HttpNetworkTransaction::BuildRequestHeaders() {
-  // For proxy use the full url. Otherwise just the absolute path.
-  // This strips out any reference/username/password.
-  std::string path = using_proxy_ ?
-      HttpUtil::SpecForRequest(request_->url) :
-      HttpUtil::PathForRequest(request_->url);
-
-  request_headers_ = request_->method + " " + path +
-      " HTTP/1.1\r\nHost: " + request_->url.host();
-  if (request_->url.IntPort() != -1)
-    request_headers_ += ":" + request_->url.port();
-  request_headers_ += "\r\n";
-
-  // For compat with HTTP/1.0 servers and proxies:
-  if (using_proxy_)
-    request_headers_ += "Proxy-";
-  request_headers_ += "Connection: keep-alive\r\n";
-
-  if (!request_->user_agent.empty())
-    request_headers_ += "User-Agent: " + request_->user_agent + "\r\n";
-
-  // Our consumer should have made sure that this is a safe referrer.  See for
-  // instance WebCore::FrameLoader::HideReferrer.
-  if (request_->referrer.is_valid())
-    request_headers_ += "Referer: " + request_->referrer.spec() + "\r\n";
-
-  // Add a content length header?
-  if (request_->upload_data) {
-    request_body_stream_.reset(new UploadDataStream(request_->upload_data));
-    request_headers_ +=
-        "Content-Length: " + Uint64ToString(request_body_stream_->size()) +
-        "\r\n";
-  } else if (request_->method == "POST" || request_->method == "PUT" ||
-             request_->method == "HEAD") {
-    // An empty POST/PUT request still needs a content length.  As for HEAD,
-    // IE and Safari also add a content length header.  Presumably it is to
-    // support sending a HEAD request to an URL that only expects to be sent a
-    // POST or some other method that normally would have a message body.
-    request_headers_ += "Content-Length: 0\r\n";
-  }
-
-  // Honor load flags that impact proxy caches.
-  if (request_->load_flags & LOAD_BYPASS_CACHE) {
-    request_headers_ += "Pragma: no-cache\r\nCache-Control: no-cache\r\n";
-  } else if (request_->load_flags & LOAD_VALIDATE_CACHE) {
-    request_headers_ += "Cache-Control: max-age=0\r\n";
-  }
-
-  ApplyAuth();
-
-  // TODO(darin): Need to prune out duplicate headers.
-
-  request_headers_ += request_->extra_headers;
-  request_headers_ += "\r\n";
-}
-
-// The HTTP CONNECT method for establishing a tunnel connection is documented
-// in draft-luotonen-web-proxy-tunneling-01.txt and RFC 2817, Sections 5.2 and
-// 5.3.
-void HttpNetworkTransaction::BuildTunnelRequest() {
-  // RFC 2616 Section 9 says the Host request-header field MUST accompany all
-  // HTTP/1.1 requests.
-  request_headers_ = StringPrintf("CONNECT %s:%d HTTP/1.1\r\n",
-      request_->url.host().c_str(), request_->url.EffectiveIntPort());
-  request_headers_ += "Host: " + request_->url.host();
-  if (request_->url.has_port())
-    request_headers_ += ":" + request_->url.port();
-  request_headers_ += "\r\n";
-
-  if (!request_->user_agent.empty())
-    request_headers_ += "User-Agent: " + request_->user_agent + "\r\n";
-
-  ApplyAuth();
-
-  request_headers_ += "\r\n";
-}
-
 void HttpNetworkTransaction::DoCallback(int rv) {
   DCHECK(rv != ERR_IO_PENDING);
   DCHECK(user_callback_);
 
   // Since Run may result in Read being called, clear user_callback_ up front.
   CompletionCallback* c = user_callback_;
   user_callback_ = NULL;
   c->Run(rv);
 }
 
@@ skipping 267 matching lines @@
   }
   return result;
 }
 
 int HttpNetworkTransaction::DoWriteHeaders() {
   next_state_ = STATE_WRITE_HEADERS_COMPLETE;
 
   // This is constructed lazily (instead of within our Start method), so that
   // we have proxy info available.
   if (request_headers_.empty()) {
+    // Figure out if we can/should add Proxy-Authentication & Authentication
+    // headers.
+    bool have_proxy_auth =
+        ShouldApplyProxyAuth() &&
+        (HaveAuth(HttpAuth::AUTH_PROXY) ||
+         SelectPreemptiveAuth(HttpAuth::AUTH_PROXY));
+    bool have_server_auth =
+        ShouldApplyServerAuth() &&
+        (HaveAuth(HttpAuth::AUTH_SERVER) ||
+         SelectPreemptiveAuth(HttpAuth::AUTH_SERVER));
+
+    std::string authorization_headers;
+
+    if (have_proxy_auth)
+      authorization_headers.append(
+          BuildAuthorizationHeader(HttpAuth::AUTH_PROXY));
+    if (have_server_auth)
+      authorization_headers.append(
+          BuildAuthorizationHeader(HttpAuth::AUTH_SERVER));
+
     if (establishing_tunnel_) {
-      BuildTunnelRequest();
+      BuildTunnelRequest(request_, authorization_headers, &request_headers_);
     } else {
-      BuildRequestHeaders();
+      if (request_->upload_data)
+        request_body_stream_.reset(new UploadDataStream(request_->upload_data));
+      BuildRequestHeaders(request_,
+                          authorization_headers,
+                          request_body_stream_.get(),
+                          using_proxy_,
+                          &request_headers_);
     }
   }
 
   // Record our best estimate of the 'request time' as the time when we send
   // out the first bytes of the request headers.
   if (request_headers_bytes_sent_ == 0) {
     response_.request_time = Time::Now();
   }
 
   const char* buf = request_headers_.data() + request_headers_bytes_sent_;
   int buf_len = static_cast<int>(request_headers_.size() -
                                  request_headers_bytes_sent_);
-  DCHECK(buf_len > 0);
+  DCHECK_GT(buf_len, 0);
 
   return connection_.socket()->Write(buf, buf_len, &io_callback_);
 }
 
 int HttpNetworkTransaction::DoWriteHeadersComplete(int result) {
   if (result < 0)
     return HandleIOError(result);
 
   request_headers_bytes_sent_ += result;
   if (request_headers_bytes_sent_ < request_headers_.size()) {
@@ skipping 73 matching lines @@
   // Assume everything else is a HTTP/0.9 response (including responses
   // of 'h', 'ht', 'htt').
   header_buf_body_offset_ = 0;
   return OK;
 }
 
 int HttpNetworkTransaction::DoReadHeadersComplete(int result) {
   if (result < 0)
     return HandleIOError(result);
 
-  if (result == 0 && ShouldResendRequest())
+  if (result == 0 && ShouldResendRequest()) {
+    ResetConnectionAndRequestForResend();
     return result;
+  }
 
   // Record our best estimate of the 'response time' as the time when we read
   // the first bytes of the response headers.
   if (header_buf_len_ == 0) {
     // After we call RestartWithAuth header_buf_len will be zero again, and
     // we need to be cautious about incorrectly logging the duration across the
     // authentication activitiy.
     bool first_response = response_.response_time == Time();
     response_.response_time = Time::Now();
     if (first_response)
@@ skipping 470 matching lines @@
 // other cases, such as a Connect error.
 int HttpNetworkTransaction::HandleIOError(int error) {
   switch (error) {
     // If we try to reuse a connection that the server is in the process of
     // closing, we may end up successfully writing out our request (or a
     // portion of our request) only to find a connection error when we try to
     // read from (or finish writing to) the socket.
     case ERR_CONNECTION_RESET:
     case ERR_CONNECTION_CLOSED:
     case ERR_CONNECTION_ABORTED:
-      if (ShouldResendRequest())
+      if (ShouldResendRequest()) {
+        ResetConnectionAndRequestForResend();
         error = OK;
+      }
       break;
   }
   return error;
 }
 
 void HttpNetworkTransaction::ResetStateForRestart() {
   pending_auth_target_ = HttpAuth::AUTH_NONE;
   header_buf_.reset();
   header_buf_capacity_ = 0;
   header_buf_len_ = 0;
   header_buf_body_offset_ = -1;
   header_buf_http_offset_ = -1;
   response_body_length_ = -1;
   response_body_read_ = 0;
   read_buf_ = NULL;
   read_buf_len_ = 0;
   request_headers_.clear();
   request_headers_bytes_sent_ = 0;
   chunked_decoder_.reset();
   // Reset all the members of response_.
   response_ = HttpResponseInfo();
 }
 
-bool HttpNetworkTransaction::ShouldResendRequest() {
+bool HttpNetworkTransaction::ShouldResendRequest() const {
   // 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.
   if (establishing_tunnel_ ||
       !reused_socket_ ||  // We didn't reuse a keep-alive connection.
       header_buf_len_) {  // We have received some response headers.
     return false;
   }
+  return true;
+}
+
+void HttpNetworkTransaction::ResetConnectionAndRequestForResend() {
   connection_.set_socket(NULL);
   connection_.Reset();
   // There are two reasons we need to clear request_headers_.  1) It contains
   // the real request headers, but we may need to resend the CONNECT request
   // first to recreate the SSL tunnel.  2) An empty request_headers_ causes
   // BuildRequestHeaders to be called, which rewinds request_body_stream_ to
   // the beginning of request_->upload_data.
   request_headers_.clear();
   request_headers_bytes_sent_ = 0;
   next_state_ = STATE_INIT_CONNECTION;  // Resend the request.
-  return true;
 }
 
 int HttpNetworkTransaction::ReconsiderProxyAfterError(int error) {
   DCHECK(!pac_request_);
 
   // A failure to resolve the hostname or any error related to establishing a
   // TCP connection could be grounds for trying a new proxy configuration.
   //
   // Why do this when a hostname cannot be resolved?  Some URLs only make sense
   // to proxy servers.  The hostname in those URLs might fail to resolve if we
@@ skipping 26 matching lines @@
     connection_.Reset();
     DCHECK(!request_headers_bytes_sent_);
     next_state_ = STATE_RESOLVE_PROXY_COMPLETE;
   } else {
     rv = error;
   }
 
   return rv;
 }
 
-void HttpNetworkTransaction::AddAuthorizationHeader(HttpAuth::Target target) {
-  // If we have no authentication information, check if we can select
-  // a cache entry preemptively (based on the path).
-  if (!HaveAuth(target) && !SelectPreemptiveAuth(target))
-    return;
+bool HttpNetworkTransaction::ShouldApplyProxyAuth() const {
+  return using_proxy_ || establishing_tunnel_;
+}
 
+bool HttpNetworkTransaction::ShouldApplyServerAuth() const {
+  return !establishing_tunnel_;
+}
+
+std::string HttpNetworkTransaction::BuildAuthorizationHeader(
+    HttpAuth::Target target) const {
   DCHECK(HaveAuth(target));
 
   // Add a Authorization/Proxy-Authorization header line.
   std::string credentials = auth_handler_[target]->GenerateCredentials(
       auth_identity_[target].username,
       auth_identity_[target].password,
       request_,
       &proxy_info_);
-  request_headers_ += HttpAuth::GetAuthorizationHeaderName(target) +
+
+  return HttpAuth::GetAuthorizationHeaderName(target) +
       ": "  + credentials + "\r\n";
 }
 
-void HttpNetworkTransaction::ApplyAuth() {
-  // We expect using_proxy_ and using_tunnel_ to be mutually exclusive.
-  DCHECK(!using_proxy_ || !using_tunnel_);
-
-  // Don't send proxy auth after tunnel has been established.
-  bool should_apply_proxy_auth = using_proxy_ || establishing_tunnel_;
-
-  // Don't send origin server auth while establishing tunnel.
-  bool should_apply_server_auth = !establishing_tunnel_;
-
-  if (should_apply_proxy_auth)
-    AddAuthorizationHeader(HttpAuth::AUTH_PROXY);
-  if (should_apply_server_auth)
-    AddAuthorizationHeader(HttpAuth::AUTH_SERVER);
-}
-
 GURL HttpNetworkTransaction::AuthOrigin(HttpAuth::Target target) const {
   return target == HttpAuth::AUTH_PROXY ?
       GURL("http://" + proxy_info_.proxy_server().host_and_port()) :
       request_->url.GetOrigin();
 }
 
 std::string HttpNetworkTransaction::AuthPath(HttpAuth::Target target)
     const {
   // Proxy authentication realms apply to all paths. So we will use
   // empty string in place of an absolute path.
@@ skipping 226 matching lines @@
   if (target == HttpAuth::AUTH_PROXY) {
     auth_info->host = ASCIIToWide(proxy_info_.proxy_server().host_and_port());
   } else {
     DCHECK(target == HttpAuth::AUTH_SERVER);
     auth_info->host = ASCIIToWide(request_->url.host());
   }
   response_.auth_challenge = auth_info;
 }
 
 }  // namespace net