Http Pipelinining: Make unit tests more friendly to greedily reading from sockets.

net/http/http_pipelined_connection_impl_unittest.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/http/http_pipelined_connection_impl.h"
 
 #include <string>
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 556 matching lines @@
 TEST_F(HttpPipelinedConnectionImplTest, ConnectionSuddenlyClosedAfterResponse) {
   MockWrite writes[] = {
     MockWrite(SYNCHRONOUS, 0, "GET /ok.html HTTP/1.1\r\n\r\n"),
     MockWrite(SYNCHRONOUS, 1, "GET /read_evicted.html HTTP/1.1\r\n\r\n"),
     MockWrite(SYNCHRONOUS, 2, "GET /read_rejected.html HTTP/1.1\r\n\r\n"),
     MockWrite(ASYNC, ERR_SOCKET_NOT_CONNECTED, 5),
   };
   MockRead reads[] = {
     MockRead(SYNCHRONOUS, 3, "HTTP/1.1 200 OK\r\n\r\n"),
     MockRead(SYNCHRONOUS, 4, "ok.html"),
+    MockRead(ASYNC, OK, 6),  // Connection closed message.  Not read before the
+                             // ERR_SOCKET_NOT_CONNECTED.

[mmenke, 2013/01/07 21:57:15]

Greedily reading data would run right off the end of the list.

   };
   Initialize(reads, arraysize(reads), writes, arraysize(writes));
 
   scoped_ptr<HttpStream> closed_stream(NewTestStream("ok.html"));
   scoped_ptr<HttpStream> read_evicted_stream(
       NewTestStream("read_evicted.html"));
   scoped_ptr<HttpStream> read_rejected_stream(
       NewTestStream("read_rejected.html"));
   scoped_ptr<HttpStream> send_closed_stream(
       NewTestStream("send_closed.html"));
@@ skipping 346 matching lines @@
   // 1. DoReadHeadersLoop, which will post:
   // 2. InvokeUserCallback
   SuddenCloseObserver observer(evicted_stream.get(), 2);
   MessageLoop::current()->AddTaskObserver(&observer);
   MessageLoop::current()->RunUntilIdle();
   EXPECT_FALSE(evicted_callback.have_result());
 }
 
 class StreamDeleter {
  public:
-  StreamDeleter(HttpStream* stream) :
-      stream_(stream),
-      ALLOW_THIS_IN_INITIALIZER_LIST(callback_(
-          base::Bind(&StreamDeleter::OnIOComplete, base::Unretained(this)))) {
+  StreamDeleter(HttpStream* stream)
+      : stream_(stream),
+        ALLOW_THIS_IN_INITIALIZER_LIST(callback_(
+            base::Bind(&StreamDeleter::OnIOComplete, base::Unretained(this)))) {
+  }
+
+  ~StreamDeleter() {
+    EXPECT_FALSE(stream_);
   }
 
   const CompletionCallback& callback() { return callback_; }
 
  private:
   void OnIOComplete(int result) {
-    delete stream_;
+    // stream_->Close(true);
+    stream_.reset();
   }
 
-  HttpStream* stream_;
+  scoped_ptr<HttpStream> stream_;
   CompletionCallback callback_;
 };
 
 TEST_F(HttpPipelinedConnectionImplTest, CloseCalledDuringSendCallback) {
   MockWrite writes[] = {
     MockWrite(ASYNC, 0, "GET /ok.html HTTP/1.1\r\n\r\n"),
   };
   Initialize(NULL, 0, writes, arraysize(writes));
 
   HttpStream* stream(NewTestStream("ok.html"));
@@ skipping 49 matching lines @@
                                            callback_.callback()));
   EXPECT_EQ(OK, evicted_stream->SendRequest(headers, &response,
                                             callback_.callback()));
 
   StreamDeleter failed_deleter(failed_stream);
   EXPECT_EQ(ERR_IO_PENDING,
             failed_stream->ReadResponseHeaders(failed_deleter.callback()));
   StreamDeleter evicted_deleter(evicted_stream);
   EXPECT_EQ(ERR_IO_PENDING,
             evicted_stream->ReadResponseHeaders(evicted_deleter.callback()));
   data_->RunFor(1);

[mmenke, 2013/01/07 21:57:15]

This test was really weird - we have a stream fail, but rather than closing it
as non-reusable, we were just deleting it.  At this point we also have an async
read pending when we switch from one parser to the other.  HttpStreamParser also
doesn't expect us to try and reuse the socket after reading only part of a
response, and as a result, the second parser is passed the leftover headers from
the first HttpStreamParser.

My refactoring doesn't like this code.  It's sufficiently weird that I don't
think it's worth figuring out just which of these conditions results in
different behavior in my refactored code.  The StreamDeleter::Close(true) call
prevents the connection from being reused, and result in eviction of the second
stream, which what the test is supposed to do, anyways.

[James Simonsen, 2013/01/07 22:02:24]

IIRC, this was the reproduction of a crash in the old code. If you've refactored
it away, then I don't think we need to worry about it.

 }
 
 TEST_F(HttpPipelinedConnectionImplTest, CloseOtherDuringReadCallback) {
   MockWrite writes[] = {
     MockWrite(SYNCHRONOUS, 0, "GET /deleter.html HTTP/1.1\r\n\r\n"),
     MockWrite(SYNCHRONOUS, 1, "GET /deleted.html HTTP/1.1\r\n\r\n"),
   };
   MockRead reads[] = {
     MockRead(SYNCHRONOUS, 2, "HTTP/1.1 200 OK\r\n"),
     MockRead(ASYNC, 3, "Content-Length: 7\r\n\r\n"),
@@ skipping 499 matching lines @@
   MessageLoop::current()->RunUntilIdle();
 
   stream->Close(false);
   EXPECT_CALL(delegate_, OnPipelineHasCapacity(pipeline_.get())).Times(1);
   stream.reset(NULL);
 }
 
 }  // anonymous namespace
 
 }  // namespace net