QUIC - instrumentation for testing null QuicSpdyStream.

net/quic/quic_http_stream.cc

Index: net/quic/quic_http_stream.cc
diff --git a/net/quic/quic_http_stream.cc b/net/quic/quic_http_stream.cc
index db671f22e925e3313ae37f648a0e7e762eac4f1b..88b307865b945c70761487980e714f107a8537d8 100644
--- a/net/quic/quic_http_stream.cc
+++ b/net/quic/quic_http_stream.cc
@@ -5,6 +5,9 @@
 #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"
@@ -556,7 +559,19 @@ int QuicHttpStream::ProcessResponseHeaders(const SpdyHeaderBlock& headers) {
 }
 
 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();
@@ -566,6 +581,16 @@ int QuicHttpStream::ReadAvailableData(IOBuffer* buf, int buf_len) {
 }
 
 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();