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_client_session_base.cc

diff --git a/quic/core/http/quic_spdy_client_session_base.cc b/quic/core/http/quic_spdy_client_session_base.cc
index 2db5b84..93f6880 100644
--- a/quic/core/http/quic_spdy_client_session_base.cc
+++ b/quic/core/http/quic_spdy_client_session_base.cc
@@ -64,7 +64,7 @@
     QuicStreamId promised_stream_id,
     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);
@@ -95,12 +95,6 @@
     // It's quite possible to receive headers after a stream has been reset.
     return;
   }
-  if (eliminate_static_stream_map() && stream->is_static()) {
-    connection()->CloseConnection(
-        QUIC_INVALID_HEADERS_STREAM_DATA, "stream is static",
-        ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
-    return;
-  }
   stream->OnPromiseHeaderList(promised_stream_id, frame_len, header_list);
 }
 

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() -

quic/core/http/quic_spdy_session_test.cc

diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index 8217cd7..0e2fcf8 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -926,7 +926,7 @@
     EXPECT_CALL(*crypto_stream, OnCanWrite());
   }
   TestHeadersStream* headers_stream;
-  if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) &&
+  if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_3) &&
       !QuicVersionUsesCryptoFrames(connection_->transport_version())) {
     QuicSpdySessionPeer::SetHeadersStream(&session_, nullptr);
     headers_stream = new TestHeadersStream(&session_);
@@ -1669,7 +1669,7 @@
 
 TEST_P(QuicSpdySessionTestClient, WritePriority) {
   TestHeadersStream* headers_stream;
-  if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) &&
+  if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_3) &&
       !QuicVersionUsesCryptoFrames(connection_->transport_version())) {
     QuicSpdySessionPeer::SetHeadersStream(&session_, nullptr);
     headers_stream = new TestHeadersStream(&session_);

quic/core/quic_session.cc

diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc
index abc6331..6d526ab 100644
--- a/quic/core/quic_session.cc
+++ b/quic/core/quic_session.cc
@@ -89,7 +89,7 @@
       closed_streams_clean_up_alarm_(nullptr),
       supported_versions_(supported_versions),
       eliminate_static_stream_map_(
-          GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) ||
+          GetQuicReloadableFlag(quic_eliminate_static_stream_map_3) ||
           QuicVersionUsesCryptoFrames(connection->transport_version())) {
   closed_streams_clean_up_alarm_ =
       QuicWrapUnique<QuicAlarm>(connection_->alarm_factory()->CreateAlarm(
@@ -113,7 +113,7 @@
         QuicUtils::GetCryptoStreamId(connection_->transport_version()),
         GetMutableCryptoStream());
   } else {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 10, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 10, 17);
     QuicStreamId id =
         QuicUtils::GetCryptoStreamId(connection_->transport_version());
     largest_static_stream_id_ = std::max(id, largest_static_stream_id_);
@@ -216,7 +216,7 @@
   }
   if (eliminate_static_stream_map_ && frame.fin &&
       handler.stream->is_static()) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 1, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 1, 17);
     connection()->CloseConnection(
         QUIC_INVALID_STREAM_ID, "Attempt to close a static stream",
         ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
@@ -304,7 +304,7 @@
   }
 
   if (eliminate_static_stream_map_ && stream->is_static()) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 2, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 2, 17);
     QUIC_DVLOG(1) << ENDPOINT
                   << "Received STOP_SENDING for a static stream, id: "
                   << stream_id << " Closing connection";
@@ -380,7 +380,7 @@
     return;  // Errors are handled by GetOrCreateStream.
   }
   if (eliminate_static_stream_map_ && handler.stream->is_static()) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 3, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 3, 17);
     connection()->CloseConnection(
         QUIC_INVALID_STREAM_ID, "Attempt to reset a static stream",
         ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
@@ -439,7 +439,7 @@
       }
     }
   } else {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 4, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 4, 17);
     // Copy all non static streams in a new map for the ease of deleting.
     QuicSmallMap<QuicStreamId, QuicStream*, 10> non_static_streams;
     for (const auto& it : dynamic_stream_map_) {
@@ -770,7 +770,7 @@
   DynamicStreamMap::iterator it = dynamic_stream_map_.find(id);
   if (it != dynamic_stream_map_.end()) {
     if (eliminate_static_stream_map_ && it->second->is_static()) {
-      QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 5, 17);
+      QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 5, 17);
       QUIC_DVLOG(1) << ENDPOINT
                     << "Try to send rst for a static stream, id: " << id
                     << " Closing connection";
@@ -845,7 +845,7 @@
   }
   QuicStream* stream = it->second.get();
   if (eliminate_static_stream_map_ && stream->is_static()) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 6, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 6, 17);
     QUIC_DVLOG(1) << ENDPOINT
                   << "Try to close a static stream, id: " << stream_id
                   << " Closing connection";
@@ -1093,7 +1093,7 @@
   }
   if (eliminate_static_stream_map_ &&
       !QuicVersionUsesCryptoFrames(connection_->transport_version())) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 11, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 11, 17);
     GetMutableCryptoStream()->flow_controller()->UpdateReceiveWindowSize(
         stream_window);
   }
@@ -1144,7 +1144,7 @@
   }
   if (eliminate_static_stream_map_ &&
       !QuicVersionUsesCryptoFrames(connection_->transport_version())) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 12, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 12, 17);
     GetMutableCryptoStream()->UpdateSendWindowOffset(new_window);
   }
 }
@@ -1276,7 +1276,7 @@
   if (eliminate_static_stream_map_ &&
       QuicUtils::IsCryptoStreamId(connection_->transport_version(),
                                   stream_id)) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 13, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 13, 17);
     return StreamHandler(GetMutableCryptoStream());
   }
   StaticStreamMap::iterator it = static_stream_map_.find(stream_id);
@@ -1423,6 +1423,18 @@
   return false;
 }
 
+bool QuicSession::IsStaticStream(QuicStreamId id) const {
+  if (eliminate_static_stream_map()) {
+    auto it = dynamic_stream_map_.find(id);
+    if (it == dynamic_stream_map_.end()) {
+      return false;
+    }
+    return it->second->is_static();
+  }
+
+  return QuicContainsKey(static_streams(), id);
+}
+
 size_t QuicSession::GetNumOpenIncomingStreams() const {
   return num_dynamic_incoming_streams_ - num_draining_incoming_streams_ +
          num_locally_closed_incoming_streams_highest_offset_;
@@ -1511,7 +1523,7 @@
   if (eliminate_static_stream_map_ &&
       !QuicVersionUsesCryptoFrames(connection_->transport_version()) &&
       GetMutableCryptoStream()->flow_controller()->IsBlocked()) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 14, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 14, 17);
     return true;
   }
   return false;
@@ -1572,7 +1584,7 @@
 
   if (eliminate_static_stream_map_ &&
       QuicUtils::IsCryptoStreamId(connection_->transport_version(), id)) {
-    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 15, 17);
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 15, 17);
     return const_cast<QuicCryptoStream*>(GetCryptoStream());
   }
 

quic/core/quic_session.h

diff --git a/quic/core/quic_session.h b/quic/core/quic_session.h
index 7da8ec3..d8f40c9 100644
--- a/quic/core/quic_session.h
+++ b/quic/core/quic_session.h
@@ -525,6 +525,9 @@
   // Returns true if the stream is still active.
   bool IsOpenStream(QuicStreamId id);
 
+  // Returns true if the stream is a static stream.
+  bool IsStaticStream(QuicStreamId id) const;
+
   // Close connection when receive a frame for a locally-created nonexistant
   // stream.
   // Prerequisite: IsClosedStream(stream_id) == false

quic/core/quic_session_test.cc

diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc
index 5af49fa..5b86bdd 100644
--- a/quic/core/quic_session_test.cc
+++ b/quic/core/quic_session_test.cc
@@ -1268,7 +1268,7 @@
       QuicUtils::GetHeadersStreamId(connection_->transport_version());
   std::unique_ptr<TestStream> fake_headers_stream = QuicMakeUnique<TestStream>(
       headers_stream_id, &session_, /*is_static*/ true, BIDIRECTIONAL);
-  if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+  if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_3)) {
     QuicSessionPeer::RegisterStaticStreamNew(&session_,
                                              std::move(fake_headers_stream));
   } else {
@@ -1289,7 +1289,7 @@
       QuicUtils::GetHeadersStreamId(connection_->transport_version());
   std::unique_ptr<TestStream> fake_headers_stream = QuicMakeUnique<TestStream>(
       headers_stream_id, &session_, /*is_static*/ true, BIDIRECTIONAL);
-  if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+  if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_3)) {
     QuicSessionPeer::RegisterStaticStreamNew(&session_,
                                              std::move(fake_headers_stream));
   } else {
@@ -2428,7 +2428,7 @@
   QuicStreamId stream_id = 0;
   std::unique_ptr<TestStream> fake_static_stream = QuicMakeUnique<TestStream>(
       stream_id, &session_, /*is_static*/ true, BIDIRECTIONAL);
-  if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+  if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_3)) {
     QuicSessionPeer::RegisterStaticStreamNew(&session_,
                                              std::move(fake_static_stream));
   } else {