Fix bug where pushed SPDY streams were never actually closed.

The fix uncovers a few things.  Previously, when we had an unclaimed
push stream, we'd leave it as an "active" stream, even if the EOF had
already been received on that stream.  I changed it so that the push
stream is removed from the active stream as soon as it is inactive,
but it remains on the unclaimed_pushed_streams list.  This seems more
correct.

The hardest part of the test was getting the test to verify the fix.
To verify the fix, I modified the Push tests so that we leave the
session open (e.g. terminate the list of reads with an ERR_IO_PENDING
rather than an EOF so that it sits open), which gives us the opportunity
to check if there are active streams left on the session when we 
completed the test.  If so, then we'll pop an error.

BUG=52898
TEST=Updated existing tests.


Review URL: http://codereview.chromium.org/3108038

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@57023 0039d316-1c4b-4281-b951-d872f2087c98

net/spdy/spdy_http_stream.cc

@@ -391,7 +391,8 @@ void SpdyHttpStream::OnDataReceived(const char* data, int length) {
   // Note that data may be received for a SpdyStream prior to the user calling
   // ReadResponseBody(), therefore user_buffer_ may be NULL.  This may often
   // happen for server initiated streams.
-  if (length > 0 && !stream_->closed()) {
+  DCHECK(!stream_->closed() || stream_->pushed());
+  if (length > 0) {
     // Save the received data.
     IOBufferWithSize* io_buffer = new IOBufferWithSize(length);
     memcpy(io_buffer->data(), data, length);

net/spdy/spdy_session.cc

@@ -198,6 +198,10 @@ SpdySession::~SpdySession() {
     connection_->socket()->Disconnect();
   }
 
+  // Streams should all be gone now.
+  DCHECK_EQ(0u, num_active_streams());
+  DCHECK_EQ(0u, num_unclaimed_pushed_streams());
+
   RecordHistograms();
 
   net_log_.EndEvent(NetLog::TYPE_SPDY_SESSION, NULL);
@@ -781,6 +785,7 @@ void SpdySession::CloseAllStreams(net::Error status) {
   if (!unclaimed_pushed_streams_.empty()) {
     streams_abandoned_count_ += unclaimed_pushed_streams_.size();
     abandoned_push_streams.Add(unclaimed_pushed_streams_.size());
+    unclaimed_pushed_streams_.clear();
   }
 
   for (int i = 0;i < NUM_PRIORITIES;++i) {
@@ -854,14 +859,19 @@ void SpdySession::ActivateStream(SpdyStream* stream) {
 
 void SpdySession::DeleteStream(spdy::SpdyStreamId id, int status) {
   DLOG(INFO) << "Removing SpdyStream " << id << " from active stream list.";
-  // Remove the stream from unclaimed_pushed_streams_ and active_streams_.
-  PushedStreamMap::iterator it;
-  for (it = unclaimed_pushed_streams_.begin();
-       it != unclaimed_pushed_streams_.end(); ++it) {
-    scoped_refptr<SpdyStream> curr = it->second;
-    if (id == curr->stream_id()) {
-      unclaimed_pushed_streams_.erase(it);
-      break;
+
+  // For push streams, if they are being deleted normally, we leave
+  // the stream in the unclaimed_pushed_streams_ list.  However, if
+  // the stream is errored out, clean it up entirely.
+  if (status != OK) {
+    PushedStreamMap::iterator it;
+    for (it = unclaimed_pushed_streams_.begin();
+         it != unclaimed_pushed_streams_.end(); ++it) {
+      scoped_refptr<SpdyStream> curr = it->second;
+      if (id == curr->stream_id()) {
+        unclaimed_pushed_streams_.erase(it);
+        break;
+      }
     }
   }
 

net/spdy/spdy_session.h

@@ -162,6 +162,13 @@ class SpdySession : public base::RefCounted<SpdySession>,
 
   void set_in_session_pool(bool val) { in_session_pool_ = val; }
 
+  // Access to the number of active and pending streams.  These are primarily
+  // available for testing and diagnostics.
+  size_t num_active_streams() const { return active_streams_.size(); }
+  size_t num_unclaimed_pushed_streams() const {
+      return unclaimed_pushed_streams_.size();
+  }
+
  private:
   friend class base::RefCounted<SpdySession>;
   FRIEND_TEST_ALL_PREFIXES(SpdySessionTest, GetActivePushStream);

net/spdy/spdy_stream.cc

@@ -29,11 +29,12 @@ SpdyStream::SpdyStream(
       response_status_(OK),
       cancelled_(false),
       send_bytes_(0),
-      recv_bytes_(0),
-      histograms_recorded_(false) {}
+      recv_bytes_(0) {
+}
 
 SpdyStream::~SpdyStream() {
   DLOG(INFO) << "Deleting SpdyStream for stream " << stream_id_;
+  UpdateHistograms();
 }
 
 void SpdyStream::SetDelegate(Delegate* delegate) {
@@ -183,9 +184,12 @@ void SpdyStream::OnDataReceived(const char* data, int length) {
       IOBufferWithSize* buf = new IOBufferWithSize(length);
       memcpy(buf->data(), data, length);
       pending_buffers_.push_back(buf);
-    }
-    else
+    } else {
       pending_buffers_.push_back(NULL);
+      metrics_.StopStream();
+      session_->CloseStream(stream_id_, net::OK);
+      // Note: |this| may be deleted after calling CloseStream.
+    }
     return;
   }
 
@@ -202,9 +206,8 @@ void SpdyStream::OnDataReceived(const char* data, int length) {
   // A zero-length read means that the stream is being closed.
   if (!length) {
     metrics_.StopStream();
-    scoped_refptr<SpdyStream> self(this);
     session_->CloseStream(stream_id_, net::OK);
-    UpdateHistograms();
+    // Note: |this| may be deleted after calling CloseStream.
     return;
   }
 
@@ -419,11 +422,6 @@ int SpdyStream::DoOpen(int result) {
 }
 
 void SpdyStream::UpdateHistograms() {
-  if (histograms_recorded_)
-    return;
-
-  histograms_recorded_ = true;
-
   // We need all timers to be filled in, otherwise metrics can be bogus.
   if (send_time_.is_null() || recv_first_byte_time_.is_null() ||
       recv_last_byte_time_.is_null())