gfe-relnote: No longer send a RESET_STREAM in response to a STOP_SENDING if the stream is write closed.  Protected by disabled gfe2_reloadable_flag_quic_enable_version_99.

PiperOrigin-RevId: 293529515
Change-Id: I0a837b79a776233d7a21fb8f87dbb7e173241a26

quic/core/http/end_to_end_test.cc

diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc
index 7621288..97623a7 100644
--- a/quic/core/http/end_to_end_test.cc
+++ b/quic/core/http/end_to_end_test.cc
@@ -4013,10 +4013,8 @@
   }
 }
 
-// Test that STOP_SENDING makes it to the other side. Set up a client & server,
-// create a stream (do not close it), and then send a STOP_SENDING from one
-// side. The other side should get a call to QuicStream::OnStopSending.
-// (aside, test cribbed from RequestAndStreamRstInOnePacket)
+// Test that STOP_SENDING makes it to the peer.  Create a stream and send a
+// STOP_SENDING. The receiver should get a call to QuicStream::OnStopSending.
 TEST_P(EndToEndTest, SimpleStopSendingTest) {
   const uint16_t kStopSendingTestCode = 123;
   ASSERT_TRUE(Initialize());
@@ -4028,9 +4026,6 @@
   QuicConnection* client_connection = client_session->connection();
   ASSERT_NE(nullptr, client_connection);
 
-  // STOP_SENDING will cause the server to not to send the trailer
-  // (and the FIN) after the response body. Instead, it sends a STOP_SENDING
-  // frame for the stream.
   std::string response_body(1305, 'a');
   SpdyHeaderBlock response_headers;
   response_headers[":status"] = quiche::QuicheTextUtils::Uint64ToString(200);
@@ -4057,14 +4052,13 @@
   // Wait for the connection to become idle.
   client_->WaitForDelayedAcks();
 
-  // The real expectation is the test does not crash or timeout.
   EXPECT_THAT(client_->connection_error(), IsQuicNoError());
-  // And that the stop-sending code is received.
   QuicSimpleClientStream* client_stream =
       static_cast<QuicSimpleClientStream*>(client_->latest_created_stream());
   ASSERT_NE(nullptr, client_stream);
-  // Make sure we have the correct stream
+  // Ensure the stream has been write closed upon receiving STOP_SENDING.
   EXPECT_EQ(stream_id, client_stream->id());
+  EXPECT_TRUE(client_stream->write_side_closed());
   EXPECT_EQ(kStopSendingTestCode,
             static_cast<uint16_t>(client_stream->stream_error()));
 }

quic/core/quic_session.cc

diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc
index b8de367..b63f729 100644
--- a/quic/core/quic_session.cc
+++ b/quic/core/quic_session.cc
@@ -221,9 +221,13 @@
 void QuicSession::OnStopSendingFrame(const QuicStopSendingFrame& frame) {
   // STOP_SENDING is in IETF QUIC only.
   DCHECK(VersionHasIetfQuicFrames(transport_version()));
+  DCHECK(QuicVersionUsesCryptoFrames(transport_version()));
 
   QuicStreamId stream_id = frame.stream_id;
   // If Stream ID is invalid then close the connection.
+  // TODO(ianswett): This check is redundant to checks for IsClosedStream,
+  // but removing it requires removing multiple DCHECKs.
+  // TODO(ianswett): Multiple QUIC_DVLOGs could be QUIC_PEER_BUGs.
   if (stream_id == QuicUtils::GetInvalidStreamId(transport_version())) {
     QUIC_DVLOG(1) << ENDPOINT
                   << "Received STOP_SENDING with invalid stream_id: "
@@ -234,6 +238,19 @@
     return;
   }
 
+  // If stream_id is READ_UNIDIRECTIONAL, close the connection.
+  if (QuicUtils::GetStreamType(stream_id, perspective(),
+                               IsIncomingStream(stream_id)) ==
+      READ_UNIDIRECTIONAL) {
+    QUIC_DVLOG(1) << ENDPOINT
+                  << "Received STOP_SENDING for a read-only stream_id: "
+                  << stream_id << ".";
+    connection()->CloseConnection(
+        QUIC_INVALID_STREAM_ID, "Received STOP_SENDING for a read-only stream",
+        ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+    return;
+  }
+
   if (visitor_) {
     visitor_->OnStopSendingReceived(frame);
   }
@@ -246,7 +263,9 @@
         << stream_id << " Ignoring.";
     return;
   }
-  // If stream is non-existent, close the connection
+  // If stream is non-existent, close the connection.
+  // TODO(b/148842616): IETF QUIC allows a STOP_SENDING to arrive before a
+  // STREAM frame for peer-intiated bidirectional steams
   StreamMap::iterator it = stream_map_.find(stream_id);
   if (it == stream_map_.end()) {
     QUIC_DVLOG(1) << ENDPOINT
@@ -260,10 +279,22 @@
   }
 
   QuicStream* stream = it->second.get();
+  // If the stream is null, it's an implementation error.
   if (stream == nullptr) {
     QUIC_BUG << ENDPOINT
              << "Received STOP_SENDING for NULL QuicStream, stream_id: "
              << stream_id << ". Ignoring.";
+    connection()->CloseConnection(
+        QUIC_INTERNAL_ERROR, "Received STOP_SENDING for a null stream",
+        ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+    return;
+  }
+
+  // Do not reset the sream if all data has been sent and acknowledged.
+  if (stream->write_side_closed() && !stream->IsWaitingForAcks()) {
+    QUIC_DVLOG(1) << ENDPOINT
+                  << "Ignoring STOP_SENDING for a write closed stream, id: "
+                  << stream_id;
     return;
   }
 

quic/core/quic_session_test.cc

diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc
index 5bfa76b..153c787 100644
--- a/quic/core/quic_session_test.cc
+++ b/quic/core/quic_session_test.cc
@@ -1623,10 +1623,8 @@
                                QUIC_STREAM_CANCELLED, kByteOffset);
   session_.OnRstStream(rst_frame);
   if (VersionHasIetfQuicFrames(transport_version())) {
-    // The test is predicated on the stream being fully closed. For IETF QUIC,
-    // the RST_STREAM only does one side (the read side from the perspective of
-    // the node receiving the RST_STREAM). This is needed to fully close the
-    // stream and therefore fulfill all of the expects.
+    // The test requires the stream to be fully closed in both directions. For
+    // IETF QUIC, the RST_STREAM only closes one side.
     QuicStopSendingFrame frame(kInvalidControlFrameId, stream->id(),
                                QUIC_STREAM_CANCELLED);
     EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
@@ -2138,10 +2136,8 @@
   stream2->OnStreamReset(rst_frame);
 
   if (VersionHasIetfQuicFrames(transport_version())) {
-    // The test is predicated on the stream being fully closed. For IETF QUIC,
-    // the RST_STREAM only does one side (the read side from the perspective of
-    // the node receiving the RST_STREAM). This is needed to fully close the
-    // stream and therefore fulfill all of the expects.
+    // The test requires the stream to be fully closed in both directions. For
+    // IETF QUIC, the RST_STREAM only closes one side.
     QuicStopSendingFrame frame(kInvalidControlFrameId, stream2->id(),
                                QUIC_STREAM_CANCELLED);
     EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
@@ -2641,15 +2637,12 @@
   session_.OnStreamFrame(unidirectional_stream_frame);
 }
 
-// Check that the OnStopSendingFrame upcall handles bad input properly
-// First test checks that invalid stream ids are handled.
-TEST_P(QuicSessionTestServer, OnStopSendingInputInvalidStreamId) {
+// Checks that invalid stream ids are handled.
+TEST_P(QuicSessionTestServer, OnStopSendingInvalidStreamId) {
   if (!VersionHasIetfQuicFrames(transport_version())) {
-    // Applicable only to IETF QUIC
     return;
   }
   // Check that "invalid" stream ids are rejected.
-  // Note that the notion of an invalid stream id is Google-specific.
   QuicStopSendingFrame frame(1, -1, 123);
   EXPECT_CALL(
       *connection_,
@@ -2658,11 +2651,22 @@
   session_.OnStopSendingFrame(frame);
 }
 
-// Second test, streams in the static stream map are not subject to
-// STOP_SENDING; it's ignored.
-TEST_P(QuicSessionTestServer, OnStopSendingInputStaticStreams) {
+TEST_P(QuicSessionTestServer, OnStopSendingReadUnidirectional) {
   if (!VersionHasIetfQuicFrames(transport_version())) {
-    // Applicable only to IETF QUIC
+    return;
+  }
+  // It's illegal to send STOP_SENDING with a stream ID that is read-only.
+  QuicStopSendingFrame frame(1, GetNthClientInitiatedUnidirectionalId(1), 123);
+  EXPECT_CALL(
+      *connection_,
+      CloseConnection(QUIC_INVALID_STREAM_ID,
+                      "Received STOP_SENDING for a read-only stream", _));
+  session_.OnStopSendingFrame(frame);
+}
+
+// Static streams ignore STOP_SENDING.
+TEST_P(QuicSessionTestServer, OnStopSendingStaticStreams) {
+  if (!VersionHasIetfQuicFrames(transport_version())) {
     return;
   }
   QuicStreamId stream_id = 0;
@@ -2670,7 +2674,6 @@
       stream_id, &session_, /*is_static*/ true, BIDIRECTIONAL);
   QuicSessionPeer::ActivateStream(&session_, std::move(fake_static_stream));
   // Check that a stream id in the static stream map is ignored.
-  // Note that the notion of a static stream is Google-specific.
   QuicStopSendingFrame frame(1, stream_id, 123);
   EXPECT_CALL(*connection_,
               CloseConnection(QUIC_INVALID_STREAM_ID,
@@ -2678,31 +2681,41 @@
   session_.OnStopSendingFrame(frame);
 }
 
-// Third test, if stream id specifies a closed stream:
-// return true and do not close the connection.
-TEST_P(QuicSessionTestServer, OnStopSendingInputClosedStream) {
+// If stream is write closed, do not send a RESET_STREAM frame.
+TEST_P(QuicSessionTestServer, OnStopSendingForWriteClosedStream) {
   if (!VersionHasIetfQuicFrames(transport_version())) {
-    // Applicable only to IETF QUIC
     return;
   }
 
   TestStream* stream = session_.CreateOutgoingBidirectionalStream();
   QuicStreamId stream_id = stream->id();
-  // Expect these as side effect of the close operations.
-  EXPECT_CALL(*connection_, SendControlFrame(_));
-  EXPECT_CALL(*connection_, OnStreamReset(_, _));
   stream->CloseWriteSide();
-  stream->CloseReadSide();
+  EXPECT_TRUE(stream->write_side_closed());
   QuicStopSendingFrame frame(1, stream_id, 123);
   EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
   session_.OnStopSendingFrame(frame);
 }
 
-// Fourth test, if stream id specifies a nonexistent stream, return false and
-// close the connection
+// If stream is closed, return true and do not close the connection.
+TEST_P(QuicSessionTestServer, OnStopSendingClosedStream) {
+  if (!VersionHasIetfQuicFrames(transport_version())) {
+    return;
+  }
+
+  TestStream* stream = session_.CreateOutgoingBidirectionalStream();
+  QuicStreamId stream_id = stream->id();
+  // Expect these as side effect of closing the stream.
+  EXPECT_CALL(*connection_, SendControlFrame(_));
+  EXPECT_CALL(*connection_, OnStreamReset(_, _));
+  session_.CloseStream(stream_id);
+  QuicStopSendingFrame frame(1, stream_id, 123);
+  EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
+  session_.OnStopSendingFrame(frame);
+}
+
+// If stream id is a nonexistent stream, return false and close the connection.
 TEST_P(QuicSessionTestServer, OnStopSendingInputNonExistentStream) {
   if (!VersionHasIetfQuicFrames(transport_version())) {
-    // Applicable only to IETF QUIC
     return;
   }