Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and streams

net/spdy/spdy_http_stream.cc

 // Copyright (c) 2012 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/spdy/spdy_http_stream.h"
 
 #include <algorithm>
 #include <list>
 #include <utility>
 
@@ skipping 81 matching lines @@
 
   if (rv == OK) {
     stream_ = stream_request_.ReleaseStream();
     InitializeStreamHelper();
   }
 
   return rv;
 }
 
 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(),
                         request_info_->upload_data_stream->size());
 }
 
 int SpdyHttpStream::ReadResponseHeaders(const CompletionCallback& callback) {
   CHECK(!callback.is_null());
   if (stream_closed_)
     return closed_stream_status_;
 
   CHECK(stream_.get());
 
   // Check if we already have the response headers. If so, return synchronously.
   if (response_headers_status_ == RESPONSE_HEADERS_ARE_COMPLETE) {
     CHECK(!stream_->IsIdle());
     return OK;
   }
 
   // Still waiting for the response, return IO_PENDING.
   CHECK(response_callback_.is_null());
   response_callback_ = callback;
   return ERR_IO_PENDING;
 }
 
 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());
 
   CHECK(buf);
   CHECK(buf_len);
   CHECK(!callback.is_null());
 
   // If we have data buffered, complete the IO immediately.
   if (!response_body_queue_.IsEmpty()) {
     return response_body_queue_.Dequeue(buf->data(), buf_len);
@@ skipping 227 matching lines @@
 void SpdyHttpStream::OnDataSent() {
   request_body_buf_size_ = 0;
   ReadAndSendRequestBodyData();
 }
 
 // TODO(xunjieli): Maybe do something with the trailers. crbug.com/422958.
 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()) {
     stream_closed_ = true;
     closed_stream_status_ = status;
     closed_stream_id_ = stream_->stream_id();
     closed_stream_has_load_timing_info_ =
         stream_->GetLoadTimingInfo(&closed_stream_load_timing_info_);
     closed_stream_received_bytes_ = stream_->raw_received_bytes();
     closed_stream_sent_bytes_ = stream_->raw_sent_bytes();
@@ skipping 224 matching lines @@
 void SpdyHttpStream::PopulateNetErrorDetails(NetErrorDetails* details) {
   details->connection_info = HttpResponseInfo::CONNECTION_INFO_HTTP2;
   return;
 }
 
 void SpdyHttpStream::SetPriority(RequestPriority priority) {
   // TODO(akalin): Plumb this through to |stream_request_| and
   // |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