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"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/io_buffer.h"
@@ skipping 538 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.
+  stream_->CrashIfInvalid();
   int rv = stream_->Read(buf, buf_len);
+  // TODO(rtenneti): Temporary until crbug.com/585591 is solved.
+  stream_->CrashIfInvalid();

[eroman, 2016/02/11 02:16:33]

Driveby comments:

According to the crash data for that bug, this is going to blow up without
telling us anything new since it will be a null-ptr de-ref (the crash data shows
that the crashes in IsDoneReading are small offsets from 0 [1])

Based on the working theory in the bug, that stream_->Read() is executing code
that ultimately NULL's stream_, I think a better instrumentation would be to see
if QuicHttpStream::ResetStream() has been called. Since that will definitively
confirm that code in Read() is causing a re-entrancy into this QuickHttpStream.

How about before calling stream_->CrashIfInvalid() you assert that ResetStream()
wasn't called? (Are there any other ways to cause stream_ to be NULL-ed?)

Maybe you have a member variable 'int reset_stream_count_ = 0"  which
ResetStream() increments. If that gives us data, you can go a step further and
add a base::Debug::StackTrace which captures the callstack that ResetStream()
was called from.

[1]
https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.ptype%3D%...

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

Acknowledged.

Thanks Eric. Added more instrumentation. +1 to ResetStream() being called during
Read(). Hopefully, new instrumentation will show the unusual conditions under
which it could happen.

   if (stream_->IsDoneReading()) {
     stream_->SetDelegate(nullptr);
     stream_->OnFinRead();
     ResetStream();
   }
   return rv;
 }
 
 void QuicHttpStream::ResetStream() {
   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