QUIC - instrumentation for testing null QuicSpdyStream.

net/quic/quic_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/quic/quic_http_stream.h"
 
 #include "base/callback_helpers.h"
+// TODO(rtenneti): Temporary while investigating crbug.com/585591.
+#include "base/debug/debugger.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
 #include "net/quic/quic_chromium_client_session.h"
 #include "net/quic/quic_chromium_client_stream.h"
 #include "net/quic/quic_http_utils.h"
 #include "net/quic/quic_utils.h"
@@ skipping 531 matching lines @@
   response_info_->was_npn_negotiated = true;
   response_info_->npn_negotiated_protocol = "quic/1+spdy/3";
   response_info_->response_time = base::Time::Now();
   response_info_->request_time = request_time_;
   response_headers_received_ = true;
 
   return OK;
 }
 
 int QuicHttpStream::ReadAvailableData(IOBuffer* buf, int buf_len) {
+  // TODO(rtenneti): Temporary until crbug.com/585591 is solved.
+  CHECK(!read_is_called_);

[ramant (doing other things), 2016/02/11 19:45:46]

Assuming Read() is not a recursive call. If it could be called recursively, then
will change this to a counter.

[eroman, 2016/02/12 01:42:46]

I don't know enough about this code to say if this is right.
Assuming there is no re-entrancy here, then looks good.

[ramant (doing other things), 2016/02/12 02:47:09]

Acknowledged.

If there is a recursion and if that calls ResetStream() at line# 578, then that
could be another reason why |stream_| could become nullptr.

+  read_is_called_ = true;
+  stream_->set_read_is_called(read_is_called_);
+
   int rv = stream_->Read(buf, buf_len);

[eroman, 2016/02/12 01:42:46]

I suggest calling stream_->CrashIfInvalid() before calling Read() too, to assert
that it wasn't bad upon entry.

[ramant (doing other things), 2016/02/12 02:47:09]

Done.

+  // TODO(rtenneti): Temporary until crbug.com/585591 is solved.
+  // CrashIfInvalid() may not be necessary. See if |stream_| became a nullptr
+  // due to memory corruptions.
+  read_is_called_ = false;

[eroman, 2016/02/12 01:42:46]

Might want to add CHECK(read_is_called_) here too, just for extra paranoia.

[ramant (doing other things), 2016/02/12 02:47:09]

Done.

+  stream_->set_read_is_called(read_is_called_);
+  stream_->CrashIfInvalid();

[ramant (doing other things), 2016/02/11 19:45:46]

Added this CrashIfInvalid() in case some one stomped on the memory (we had seen
only one or very few crashes on windows, but there were more crashes in linux
platforms). This should never happen.

+
   if (stream_->IsDoneReading()) {
     stream_->SetDelegate(nullptr);
     stream_->OnFinRead();
     ResetStream();
   }
   return rv;
 }
 
 void QuicHttpStream::ResetStream() {
+  // TODO(rtenneti): Temporary until crbug.com/585591 is solved.
+  if (read_is_called_) {
+// |stream_| is going away when Read is called. Capture the callstack.
+#if defined(NDEBUG)

[eroman, 2016/02/12 01:42:46]

I think you can just CHECK(false) here -- if this is reached, then will just
crash later in ReadAvailableData() anyway right?

[ramant (doing other things), 2016/02/12 02:47:09]

Done.

+    base::debug::DumpWithoutCrashing();
+#else
+    // Should never happen??
+    CHECK(false);
+#endif
+  }
   if (!stream_)
     return;
   closed_stream_received_bytes_ = stream_->stream_bytes_read();
   closed_stream_sent_bytes_ = stream_->stream_bytes_written();
   stream_ = nullptr;
 
   // If |request_body_stream_| is non-NULL, Reset it, to abort any in progress
   // read.
   if (request_body_stream_)
     request_body_stream_->Reset();
 }
 
 }  // namespace net