Do not invoke QuicHttpStream::DoCallback when in DoLoop

Latest crash dumps suggest that callback is invoked
re-entrantly during QuicHttpStream::DoLoop(). This can be
problematic because the caller can delete the
QuicHttpStream when the callback is run. This CL changes
QuicHttpStream::OnClose() to not invoke callback when
already in DoLoop().

This CL also adds a unit test which will crash without the
change or the CHECKs.

BUG=629043

Review-Url: https://codereview.chromium.org/2163883004
Cr-Commit-Position: refs/heads/master@{#407152}

net/quic/quic_http_stream.cc

@@ -510,6 +510,10 @@ void QuicHttpStream::OnClose() {
 
   quic_connection_error_ = stream_->connection_error();
   ResetStream();
+  if (in_loop_) {
+    // If already in DoLoop(), |callback_| will be handled when DoLoop() exits.
+    return;
+  }
   if (!callback_.is_null()) {
     DoCallback(response_status_);
   }

net/quic/quic_http_stream_test.cc

@@ -144,6 +144,26 @@ class ReadErrorUploadDataStream : public UploadDataStream {
   DISALLOW_COPY_AND_ASSIGN(ReadErrorUploadDataStream);
 };
 
+// A Callback that deletes the QuicHttpStream.
+class DeleteStreamCallback : public TestCompletionCallbackBase {
+ public:
+  DeleteStreamCallback(std::unique_ptr<QuicHttpStream> stream)
+      : stream_(std::move(stream)),
+        callback_(base::Bind(&DeleteStreamCallback::DeleteStream,
+                             base::Unretained(this))) {}
+
+  const CompletionCallback& callback() const { return callback_; }
+
+ private:
+  void DeleteStream(int result) {
+    stream_.reset();
+    SetResult(result);
+  }
+
+  std::unique_ptr<QuicHttpStream> stream_;
+  CompletionCallback callback_;
+};
+
 }  // namespace
 
 class QuicHttpStreamPeer {
@@ -1312,6 +1332,41 @@ TEST_P(QuicHttpStreamTest, CheckPriorityWithNoDelegate) {
   EXPECT_EQ(0, stream_->GetTotalReceivedBytes());
 }
 
+TEST_P(QuicHttpStreamTest, SessionClosedDuringDoLoop) {
+  SetRequest("POST", "/", DEFAULT_PRIORITY);
+  size_t spdy_request_headers_frame_length;
+  AddWrite(ConstructRequestHeadersPacket(1, !kFin, DEFAULT_PRIORITY,
+                                         &spdy_request_headers_frame_length));
+  AddWrite(
+      ConstructClientDataPacket(2, kIncludeVersion, !kFin, 0, kUploadData));
+  // Second data write will result in a synchronous failure which will close
+  // the session.
+  AddWrite(SYNCHRONOUS, ERR_FAILED);
+  Initialize();
+
+  ChunkedUploadDataStream upload_data_stream(0);
+
+  request_.method = "POST";
+  request_.url = GURL("http://www.example.org/");
+  request_.upload_data_stream = &upload_data_stream;
+  ASSERT_EQ(OK, request_.upload_data_stream->Init(
+                    TestCompletionCallback().callback()));
+
+  size_t chunk_size = strlen(kUploadData);
+  upload_data_stream.AppendData(kUploadData, chunk_size, false);
+  ASSERT_EQ(OK,
+            stream_->InitializeStream(&request_, DEFAULT_PRIORITY,
+                                      net_log_.bound(), callback_.callback()));
+  QuicHttpStream* stream = stream_.get();
+  DeleteStreamCallback delete_stream_callback(std::move(stream_));
+  // SendRequest() completes asynchronously after the final chunk is added.
+  ASSERT_EQ(ERR_IO_PENDING,
+            stream->SendRequest(headers_, &response_, callback_.callback()));
+  upload_data_stream.AppendData(kUploadData, chunk_size, true);
+  int rv = callback_.WaitForResult();
+  EXPECT_EQ(ERR_QUIC_PROTOCOL_ERROR, rv);
+}
+
 TEST_P(QuicHttpStreamTest, SessionClosedBeforeSendHeadersComplete) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   AddWrite(SYNCHRONOUS, ERR_FAILED);