Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and streams

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index e889bbfb7526b73500d272bc56d1ea43258d53bf..2b43fc19069d1f8bb5cc6e61e24b0107ab136467 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -121,7 +121,6 @@ HttpNetworkTransaction::~HttpNetworkTransaction() {
       stream->Drain(session_);
     }
   }
-
   if (request_ && request_->upload_data_stream)
     request_->upload_data_stream->Reset();  // Invalidate pending callbacks.
 }
@@ -131,6 +130,7 @@ int HttpNetworkTransaction::Start(const HttpRequestInfo* request_info,
                                   const BoundNetLog& net_log) {
   net_log_ = net_log;
   request_ = request_info;
+  url_ = request_->url;
 
   // Now that we have an HttpRequestInfo object, update server_ssl_config_.
   session_->GetSSLConfig(*request_, &server_ssl_config_, &proxy_ssl_config_);
@@ -294,8 +294,6 @@ int HttpNetworkTransaction::Read(IOBuffer* buf, int buf_len,
   DCHECK(buf);
   DCHECK_LT(0, buf_len);
 
-  State next_state = STATE_NONE;
-
   scoped_refptr<HttpResponseHeaders> headers(GetResponseHeaders());
   if (headers_valid_ && headers.get() && stream_request_.get()) {
     // We're trying to read the body of the response but we're still trying
@@ -311,17 +309,26 @@ int HttpNetworkTransaction::Read(IOBuffer* buf, int buf_len,
     DCHECK_EQ(headers->response_code(), HTTP_PROXY_AUTHENTICATION_REQUIRED);
     LOG(WARNING) << "Blocked proxy response with status "
                  << headers->response_code() << " to CONNECT request for "
-                 << GetHostAndPort(request_->url) << ".";
+                 << GetHostAndPort(url_) << ".";
     return ERR_TUNNEL_CONNECTION_FAILED;
   }
 
   // Are we using SPDY or HTTP?
-  next_state = STATE_READ_BODY;
+  next_state_ = STATE_READ_BODY;
+
+  // We have reached the end of Start state machine, reset the requestinfo to
+  // null.
+  // RequestInfo is a member of the HttpTransaction's consumer and is useful
+  // only till final response headers are received. Also, its possible that the
+  // creating consumer is destroyed but the HttpNetworkTransaction outlives it
+  // as it may be shared across multiple consumers. In such cases
+  // the HttpRequestInfo will be invalid if not marked null here.
+  if (request_)
+    ResetRequestInfo();
 
   read_buf_ = buf;
   read_buf_len_ = buf_len;
 
-  next_state_ = next_state;
   int rv = DoLoop(OK);
   if (rv == ERR_IO_PENDING)
     callback_ = callback;
@@ -378,13 +385,6 @@ LoadState HttpNetworkTransaction::GetLoadState() const {
   }
 }
 
-UploadProgress HttpNetworkTransaction::GetUploadProgress() const {
-  if (!stream_.get())
-    return UploadProgress();
-
-  return stream_->GetUploadProgress();
-}
-
 void HttpNetworkTransaction::SetQuicServerInfo(
     QuicServerInfo* quic_server_info) {}
 
@@ -1627,6 +1627,19 @@ GURL HttpNetworkTransaction::AuthURL(HttpAuth::Target target) const {
   }
 }
 
+void HttpNetworkTransaction::ResetRequestInfo() {
+  // Reset will ensure that HttpRequestInfo is only used up until final response
+  // headers are received.
+  // Resetting is allowed so that the transaction can outlive its creating
+  // consumer in cases where it is shared for writing to the cache. Do

[Randy Smith (Not in Mondays), 2016/09/09 17:28:24]

nit, suggestion: I'd phrase this slightly differently, maybe something like "so
that the transaction can be disassociated from its creating consumer in cases
where it is shared for writing ...".  That covers the
sharing-without-creating-consumer-destroyed case as well.

[shivanisha, 2016/09/13 19:58:34]

done.

+  // not allow resetting before the Read state machine is invoked. It is also
+  // safe to reset it to null at this point since upload_data_stream is also
+  // not used in the Read state machine.
+  DCHECK_EQ(STATE_READ_BODY, next_state_);

[Randy Smith (Not in Mondays), 2016/09/09 17:28:24]

nit, suggestion: It feels weird to have a functionally one line function that's
called from a single place and then has a DCHECK that basically means "make sure
this is called from the one place from which it's called".  My personal
preference would be to have the request_ = nullptr assignment inline in
HttpNetworkTransacation::Read() with appropriate comments.  (I'd even suggest
getting rid of the non-null test on request_; I think a unilateral assignment to
nullptr is probably cheaper and no more confusing than doing the test for
non-null before the assignment).

[shivanisha, 2016/09/13 19:58:34]

Sounds good. done.

+
+  request_ = nullptr;
+}
+
 bool HttpNetworkTransaction::ForWebSocketHandshake() const {
   return websocket_handshake_stream_base_create_helper_ &&
          request_->url.SchemeIsWSOrWSS();