Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and streams

net/spdy/spdy_http_stream.cc

Index: net/spdy/spdy_http_stream.cc
diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index 31b033aea06f0c30fbf0baaee878fd7732b11c13..4877408c2697d734d3bd89c06b8d0c6b22282214 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -99,7 +99,10 @@ int SpdyHttpStream::InitializeStream(const HttpRequestInfo* request_info,
 }
 
 UploadProgress SpdyHttpStream::GetUploadProgress() const {
-  if (!request_info_ || !HasUploadData())
+  if (!request_info_)
+    return upload_progress_;

[mmenke, 2016/08/31 21:57:59]

Hrm...I wonder about all this UploadProgress plumbing.  The URLRequest itself is
what actually owns the UploadDataStream.  Maybe we should just rip out all of
the UploadProgress data sharing, and have it query the stream directly?

I suppose with this API, we could report the amount StreamSocket::Send has said
we've send, instead of including the amount that's currently in the write
buffer, but it seems a minor distinction, and we're not doing that now, anyways.

I suppose that would introduce some potential problems:  Before we create the
stream, and while we're retrying, we have to rewind the stream.  While we're
doing that, we probably report an empty UploadProgress, rather than calling into
the UploadDataStream, as we have no idea what it will return.

We could get around it by just report it at the HttpNetworkTransaction layer. 
That's the layer responsible for rewinding it.  If there's no HttpStream, we
just report an UploadProgress().  The responsibility for who does what is a
little funky here, regardless of where we do this, but as-is, seems like we have
the exact same code in all three HttpStream implementations of this method. 
Anyhow, may be worth spending a little time thinking about what's most sane
here.

[shivanisha, 2016/09/01 20:30:59]

Why do we even need to go to the URLRequestJob, HttpTransaction and HttpStream
layers for getting upload progress. Since UploadDataStream has all the
information about its current progress, isn't returning that at the URLRequest
layer sufficient? That sounds like the correct approach to me since URLRequest
actually owns UploadDataStream. Am I missing something here?

[shivanisha, 2016/09/01 20:32:12]

Ah ok, I see your concerns about rewinding the stream. 

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

Removed all the plumbing for getting upload progress. Now URLRequest can invoke
GetUploadProgress directly on upload stream. The stream will return empty
UploadProgress if it is not yet successfully initialized. Also added a test for
the uninitialized condition.

+
+  if (!HasUploadData())
     return UploadProgress();
 
   return UploadProgress(request_info_->upload_data_stream->position(),
@@ -127,6 +130,12 @@ int SpdyHttpStream::ReadResponseHeaders(const CompletionCallback& callback) {
 
 int SpdyHttpStream::ReadResponseBody(
     IOBuffer* buf, int buf_len, const CompletionCallback& callback) {
+  // Invalidate HttpRequestInfo pointer. This is to allow the stream to be
+  // shared across multiple HttpCache::Transactions which might require this
+  // stream to outlive the request_'s owner URLRequestHttpJob.
+  if (request_info_)
+    ResetRequestInfo();
+
   if (stream_.get())
     CHECK(!stream_->IsIdle());
 
@@ -374,7 +383,7 @@ void SpdyHttpStream::OnTrailers(const SpdyHeaderBlock& trailers) {}
 
 void SpdyHttpStream::OnClose(int status) {
   // Cancel any pending reads from the upload data stream.
-  if (request_info_->upload_data_stream)
+  if (request_info_ && request_info_->upload_data_stream)
     request_info_->upload_data_stream->Reset();
 
   if (stream_.get()) {
@@ -619,4 +628,15 @@ void SpdyHttpStream::SetPriority(RequestPriority priority) {
   // |stream_|.
 }
 
+void SpdyHttpStream::ResetRequestInfo() {
+  // Only allowed when Reading of response body starts. It is safe to reset it
+  // at this point since request_->upload_data_stream is also not needed
+  // anymore.
+
+  // Save upload progress if any.
+  upload_progress_ = GetUploadProgress();
+
+  request_info_ = nullptr;
+}
+
 }  // namespace net