[SPDY] Refactor SpdyStream's handling of response headers

[SPDY] Refactor SpdyStream's handling of response headers

Rename OnResponseHeadersReceived() to
OnResponseHeadersUpdated() and remove some of
its extraneous parameters. Document its semantics and that
of the other delegate functions.

Fix bug in PushedStreamReplayData() where the stream being
closed/deleted wasn't handled correctly. Also fix memory leaks
of the pending frames.

Use continue_buffering_data_ only for push streams.

Rename request_/response_ to request_headers_/response_headers_.
Keep track of whether all required response headers are complete,
and send data to the delegate only after that becomes true.

Rename OnResponseHeadersReceived() and OnHeaders() to
On{Initial,Additional}ResponseHeadersReceived(), respectively.

Add MergeWithResponseHeaders() utility function and call it
from On{Initial,Additional}ResponseHeadersReceived(). Always convert
ERR_INCOMPLETE_SPDY_HEADERS to OK for push streams.

Rename ContainsUpperAscii() (ambiguous) to ContainsUppercaseAscii().

Move some tests from SpdyNetworkTransaction to SpdyStream.

Some other miscellaneous renaming and cleanup.

BUG=251442
R=rch@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208169

[akalin, 2013-06-18 22:04:40]

+rch for review

[akalin, 2013-06-18 22:05:03]

On 2013/06/18 22:04:40, akalin wrote:
> +rch for review

Recommended order of review:

spdy_stream.*
spdy_session.*
everything else

[Ryan Hamilton, 2013-06-19 18:58:36]

Nice cleanup.  The semantics are much clearer now.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_unittest.cc (left):

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_unittest.cc:6192: << expected_push_result;
nit: are you sure you don't want to keep this extra logging?  I don't feel at
all strongly.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (left):

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ol...
net/spdy/spdy_stream.cc:385: response_time_ = base::Time::Now();
I see that you pushed this logic up from the stream into the session, which
required widening the interface slightly and doing a tad more bookkeeping there.
 How come?

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:126: Delegate* old_delegate = delegate_;
Is it "valid" to have more than one delegate in the lifetime of a spdy stream? 
I sure would not have expected that.  I'd be tempted to CHECK(!delegate_);

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:127: CHECK(delegate);
nit: can you make this the first line

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:185: return;
do we need to make this check every time we invoke the delegate?  (are there
other places in the the file which might need this as well?)

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:425: metrics_.StartStream();
I notice that this moved to here.  This means that it will not be started in the
event of any of the early returns above.  Is that change significant?

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:979: DCHECK(rv == OK || rv ==
ERR_INCOMPLETE_SPDY_HEADERS);
boy, this sure screams "this method should return a boolean"

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:94: //   ERR_INCOMPLETE_SPDY_HEADERS.
nit: if there are only 2 return values, should it return an enum instead?

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:99: //   that results in the closing of the stream.
nit: can you rephrase this to something like:
- If OK is returned, the delegate has processed the headers successfully. 
However, ...

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:156: bool ReceivedInitialResponseHeaders() const;
nit: HasReceivedInitialResponseHeaders

[akalin, 2013-06-21 21:13:25]

PTAL!

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_unittest.cc (left):

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_unittest.cc:6192: << expected_push_result;
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> nit: are you sure you don't want to keep this extra logging?  I don't feel at
> all strongly.

The same data is already logged by EXPECT_EQ, since it prints out both values

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (left):

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ol...
net/spdy/spdy_stream.cc:385: response_time_ = base::Time::Now();
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> I see that you pushed this logic up from the stream into the session, which
> required widening the interface slightly and doing a tad more bookkeeping
there.
>  How come?

There was a TODO somewhere to record the response time as close as possible to
actually receiving the data. I figure it doesn't hurt to implement that.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:126: Delegate* old_delegate = delegate_;
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> Is it "valid" to have more than one delegate in the lifetime of a spdy stream?

> I sure would not have expected that.  I'd be tempted to CHECK(!delegate_);

Yeap, in the redirect case in SpdyProxyClientSocket. Added bug and TODOs to fix
that.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:127: CHECK(delegate);
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> nit: can you make this the first line

Done.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:185: return;
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> do we need to make this check every time we invoke the delegate?  (are there
> other places in the the file which might need this as well?)

Audited the other delegate methods and added comments, and also removed some
overly-conservative checks. (The in_do_loop_ check helps a lot).

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:425: metrics_.StartStream();
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> I notice that this moved to here.  This means that it will not be started in
the
> event of any of the early returns above.  Is that change significant?

I think so. That just means that erroneous streams won't be counted in bandwidth
metrics. I think that's okay, though, since they're closed almost immediately
after being opened.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:979: DCHECK(rv == OK || rv ==
ERR_INCOMPLETE_SPDY_HEADERS);
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> boy, this sure screams "this method should return a boolean"

Done.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:94: //   ERR_INCOMPLETE_SPDY_HEADERS.
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> nit: if there are only 2 return values, should it return an enum instead?

Done! Also use the enum in various other places.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:99: //   that results in the closing of the stream.
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> nit: can you rephrase this to something like:
> - If OK is returned, the delegate has processed the headers successfully. 
> However, ...
> 

Done.

https://codereview.chromium.org/17382012/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:156: bool ReceivedInitialResponseHeaders() const;
On 2013/06/19 18:58:36, Ryan Hamilton wrote:
> nit: HasReceivedInitialResponseHeaders

Done.

[Ryan Hamilton, 2013-06-23 15:06:41]

LGTM!

[commit-bot: I haz the power, 2013-06-23 17:30:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/17382012/37002

[commit-bot: I haz the power, 2013-06-24 06:15:40]

Failed to apply patch for net/spdy/spdy_http_stream.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file net/spdy/spdy_http_stream.cc
  Hunk #1 succeeded at 33 (offset -1 lines).
  Hunk #2 FAILED at 44.
  Hunk #3 succeeded at 106 (offset -6 lines).
  Hunk #4 succeeded at 292 (offset -6 lines).
  Hunk #5 succeeded at 340 (offset -6 lines).
  Hunk #6 succeeded at 364 (offset -6 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
net/spdy/spdy_http_stream.cc.rej

Patch:       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
e10aaa6c3c2d2b70e8ece53554502f0a21f3b17d..a786371fa59ba18d9d6ef0fd3d87fbe3ec7c4d48
100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -34,7 +34,7 @@ SpdyHttpStream::SpdyHttpStream(SpdySession* spdy_session,
       closed_stream_id_(0),
       request_info_(NULL),
       response_info_(NULL),
-      response_headers_received_(false),
+      response_headers_status_(RESPONSE_HEADERS_ARE_INCOMPLETE),
       user_buffer_len_(0),
       request_body_buf_size_(0),
       buffered_read_callback_pending_(false),
@@ -44,8 +44,12 @@ SpdyHttpStream::SpdyHttpStream(SpdySession* spdy_session,
 void SpdyHttpStream::InitializeWithExistingStream(
     const base::WeakPtr<SpdyStream>& spdy_stream) {
   stream_ = spdy_stream;
+  // Even though the stream is bidirectional, we still expect it to
+  // behave like a request/response stream (with the request part
+  // already done). Also, see http://crbug.com/252433 .
+  DCHECK_EQ(spdy_stream->type(), SPDY_BIDIRECTIONAL_STREAM);
   stream_->SetDelegate(this);
-  response_headers_received_ = true;
+  response_headers_status_ = RESPONSE_HEADERS_ARE_COMPLETE;
 }
 
 SpdyHttpStream::~SpdyHttpStream() {
@@ -112,7 +116,7 @@ int SpdyHttpStream::ReadResponseHeaders(const
CompletionCallback& callback) {
   CHECK(stream_.get());
 
   // Check if we already have the response headers. If so, return
synchronously.
-  if(stream_->response_received()) {
+  if (response_headers_status_ == RESPONSE_HEADERS_ARE_COMPLETE) {
     CHECK(stream_->is_idle());
     return OK;
   }
@@ -298,32 +302,29 @@ void SpdyHttpStream::OnRequestHeadersSent() {
     ReadAndSendRequestBodyData();
 }
 
-int SpdyHttpStream::OnResponseHeadersReceived(const SpdyHeaderBlock& response,
-                                              base::Time response_time,
-                                              int status) {
+SpdyResponseHeadersStatus SpdyHttpStream::OnResponseHeadersUpdated(
+    const SpdyHeaderBlock& response_headers) {
+  if (stream_->type() == SPDY_REQUEST_RESPONSE_STREAM) {
+    CHECK_EQ(response_headers_status_, RESPONSE_HEADERS_ARE_INCOMPLETE);
+  } else if (response_headers_status_ == RESPONSE_HEADERS_ARE_COMPLETE) {
+    LOG(WARNING) << "SpdyHttpStream headers received after response started";
+    return RESPONSE_HEADERS_ARE_COMPLETE;
+  }
+
   if (!response_info_) {
     DCHECK_EQ(stream_->type(), SPDY_PUSH_STREAM);
     push_response_info_.reset(new HttpResponseInfo);
     response_info_ = push_response_info_.get();
   }
 
-  // If the response is already received, these headers are too late.
-  if (response_headers_received_) {
-    LOG(WARNING) << "SpdyHttpStream headers received after response started.";
-    return OK;
-  }
-
-  // TODO(mbelshe): This is the time of all headers received, not just time
-  // to first byte.
-  response_info_->response_time = base::Time::Now();
-
-  if (!SpdyHeadersToHttpResponse(response, stream_->GetProtocolVersion(),
-                                 response_info_)) {
-    // We might not have complete headers yet.
-    return ERR_INCOMPLETE_SPDY_HEADERS;
+  if (!SpdyHeadersToHttpResponse(
+          response_headers, stream_->GetProtocolVersion(), response_info_)) {
+    // We do not have complete headers yet.
+    return RESPONSE_HEADERS_ARE_INCOMPLETE;
   }
 
-  response_headers_received_ = true;
+  response_info_->response_time = stream_->response_time();
+  response_headers_status_ = RESPONSE_HEADERS_ARE_COMPLETE;
   // Don't store the SSLInfo in the response here, HttpNetworkTransaction
   // will take care of that part.
   SSLInfo ssl_info;
@@ -349,22 +350,15 @@ int SpdyHttpStream::OnResponseHeadersReceived(const
SpdyHeaderBlock& response,
   }
   response_info_->vary_data
       .Init(*request_info_, *response_info_->headers.get());
-  // TODO(ahendrickson): This is recorded after the entire SYN_STREAM control
-  // frame has been received and processed.  Move to framer?
-  response_info_->response_time = response_time;
 
   if (!callback_.is_null())
-    DoCallback(status);
+    DoCallback(OK);
 
-  return status;
+  return RESPONSE_HEADERS_ARE_COMPLETE;
 }
 
-int SpdyHttpStream::OnDataReceived(scoped_ptr<SpdyBuffer> buffer) {
-  // SpdyStream won't call us with data if the header block didn't contain a
-  // valid set of headers.  So we don't expect to not have headers received
-  // here.
-  if (!response_headers_received_)
-    return ERR_INCOMPLETE_SPDY_HEADERS;
+void SpdyHttpStream::OnDataReceived(scoped_ptr<SpdyBuffer> buffer) {
+  CHECK_EQ(response_headers_status_, RESPONSE_HEADERS_ARE_COMPLETE);
 
   // Note that data may be received for a SpdyStream prior to the user calling
   // ReadResponseBody(), therefore user_buffer_ may be NULL.  This may often
@@ -380,7 +374,6 @@ int SpdyHttpStream::OnDataReceived(scoped_ptr<SpdyBuffer>
buffer) {
       ScheduleBufferedReadCallback();
     }
   }
-  return OK;
 }
 
 void SpdyHttpStream::OnDataSent() {

[commit-bot: I haz the power, 2013-06-24 07:30:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/17382012/51002

[akalin, 2013-06-24 07:42:23]

Committed patchset #7 manually as r208169 (presubmit successful).