commit da26cdbf35740c4263a931c477eb0dbc9ff9d90a
author rch <rch@google.com> Fri May 17 11:57:37 2019 -0700
committer Copybara-Service <copybara-worker@google.com> Fri May 17 11:58:47 2019 -0700
tree fb428f50b836d6e0175e7ad5e5c472f0c8cb5e88
parent c050d7a289a2aeb231b85f499027ea71be52baff

gfe-relnote: Fix a bug in QuicSpdyClientStreamBase. Protected by --gfe2_reloadable_flag_quic_eliminate_static_stream_map_3

When eliminate_static_stream_map is true, GetSpdyDataStream() will be called with a stream id that may not be a spdy stream. This results in an invalid cast which blows up in some Chrome tests. I've added a DCHECK in GetSpdyDataStream() to make sure we never do this again, and have added and IsStaticStream() method which can be used for checks in this case. Renames gfe2_reloadable_flag_quic_eliminate_static_stream_map_2 to gfe2_reloadable_flag_quic_eliminate_static_stream_map_3.

PiperOrigin-RevId: 248762017
Change-Id: Iab361cc46408cd57e38848f172a7f826f346b28d

quic/core/http/quic_spdy_session.cc

diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc
index 5eec97d..bdaa433 100644
--- a/quic/core/http/quic_spdy_session.cc
+++ b/quic/core/http/quic_spdy_session.cc
@@ -363,7 +363,7 @@
         QuicUtils::GetHeadersStreamId(connection()->transport_version()),
         headers_stream_.get());
   } else {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 7, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 7, 17);
     unowned_headers_stream_ = headers_stream_.get();
     RegisterStaticStreamNew(std::move(headers_stream_));
   }
@@ -416,7 +416,7 @@
                                          bool fin,
                                          size_t frame_len,
                                          const QuicHeaderList& header_list) {
-  if (QuicContainsKey(static_streams(), stream_id)) {
+  if (IsStaticStream(stream_id)) {
     connection()->CloseConnection(
         QUIC_INVALID_HEADERS_STREAM_DATA, "stream is static",
         ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
@@ -447,13 +447,6 @@
     // It's quite possible to receive headers after a stream has been reset.
     return;
   }
-  if (eliminate_static_stream_map() && stream->is_static()) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 8, 17);
-    connection()->CloseConnection(
-        QUIC_INVALID_HEADERS_STREAM_DATA, "stream is static",
-        ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
-    return;
-  }
   stream->OnStreamHeaderList(fin, frame_len, header_list);
 }
 
@@ -542,7 +535,9 @@
 
 QuicSpdyStream* QuicSpdySession::GetSpdyDataStream(
     const QuicStreamId stream_id) {
-  return static_cast<QuicSpdyStream*>(GetOrCreateDynamicStream(stream_id));
+  QuicStream* stream = GetOrCreateDynamicStream(stream_id);
+  DCHECK(!stream || !stream->is_static());
+  return static_cast<QuicSpdyStream*>(stream);
 }
 
 void QuicSpdySession::OnCryptoHandshakeEvent(CryptoHandshakeEvent event) {
@@ -716,7 +711,7 @@
   // In the case where session is destructed by calling
   // dynamic_streams().clear(), we will have incorrect accounting here.
   // TODO(renjietang): Modify destructors and make this a DCHECK.
-  QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 9, 17);
+  QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 9, 17);
   if (static_cast<size_t>(dynamic_streams().size()) >
       num_incoming_static_streams() + num_outgoing_static_streams()) {
     return dynamic_streams().size() - num_incoming_static_streams() -