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 5b59a957dffd6e8a75380e960a5d4fca8a006ea6..c08950721d97d5b1f9a88fca9d22852d9c87fda9 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -120,7 +120,6 @@ HttpNetworkTransaction::~HttpNetworkTransaction() {
       stream->Drain(session_);
     }
   }
-
   if (request_ && request_->upload_data_stream)
     request_->upload_data_stream->Reset();  // Invalidate pending callbacks.
 }
@@ -130,6 +129,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_);
@@ -293,8 +293,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
@@ -310,17 +308,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 calling URLRequestHttpJob and is useful only
+  // till final response headers are received. Also, its possible that the
+  // URLRequestHttpJob is destroyed but the HttpNetworkTransaction outlives it
+  // as it may be shared across multiple HttpCache::Transactions. In such cases
+  // the HttpRequestInfo will be invalid if not marked null here.

[Randy Smith (Not in Mondays), 2016/09/01 20:18:31]

Request: Could you rewrite this paragraph in terms of a generic consumer of
HttpNetworkTransaction, as opposed to specifying that URLRequestHttpJob is the
owner?  Describing it this way creates a dependency (if only conceptual) between
this class and the classes that use it; better to just describe this classes
behavior.

[shivanisha, 2016/09/08 20:43:54]

done.

+  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;
@@ -1626,6 +1633,19 @@ GURL HttpNetworkTransaction::AuthURL(HttpAuth::Target target) const {
   }
 }
 
+void HttpNetworkTransaction::ResetRequestInfo() {
+  // Reset will ensure that HttpRequestInfo is only used uptill final response

[Randy Smith (Not in Mondays), 2016/09/01 20:18:31]

nit: up until?

[shivanisha, 2016/09/08 20:43:54]

done.

+  // headers are received.
+  // Resetting is allowed so that the transaction can outlive its owning
+  // URLRequestHttpJob in cases where it is shared for writing to the cache. Do
+  // 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(next_state_ == STATE_READ_BODY);
+
+  request_ = nullptr;
+}
+
 bool HttpNetworkTransaction::ForWebSocketHandshake() const {
   return websocket_handshake_stream_base_create_helper_ &&
          request_->url.SchemeIsWSOrWSS();