In QUIC, consider session has pending crypto data if crypto stream has...

In QUIC, consider session has pending crypto data if crypto stream has buffered data waiting to be sent. Protected by FLAGS_quic_reloadable_flag_quic_fix_has_pending_crypto_data.

Also rename HasPendingCryptoData to HasUnackedCryptoData.

Merge internal change: 213505364

R=rch@chromium.org

Change-Id: I7225bf701a96dd4f07b1b0388d632d286fcfd4b2
Reviewed-on: https://chromium-review.googlesource.com/1232036
Commit-Queue: Fan Yang <fayang@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592406}

net/quic/quic_flags_list.h

@@ -234,3 +234,10 @@ QUIC_FLAG(bool,
 
 // If true, enable version 45.
 QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_enable_version_45, false)
+
+// If true, QuicSession::HasPendingCryptoData checks whether the crypto stream's
+// send buffer is empty. This flag fixes a bug where the retransmission alarm
+// mode is wrong for the first CHLO packet.
+QUIC_FLAG(bool,
+          FLAGS_quic_reloadable_flag_quic_fix_has_pending_crypto_data,
+          false)

net/third_party/quic/core/quic_crypto_stream_test.cc

@@ -245,6 +245,25 @@ TEST_F(QuicCryptoStreamTest, RetransmitStreamData) {
   EXPECT_TRUE(stream_->RetransmitStreamData(0, 0, false));
 }
 
+// Regression test for b/115926584.
+TEST_F(QuicCryptoStreamTest, HasUnackedCryptoData) {
+  QuicString data(1350, 'a');
+  EXPECT_CALL(session_, WritevData(_, kCryptoStreamId, 1350, 0, _))
+      .WillOnce(testing::Return(QuicConsumedData(0, false)));
+  stream_->WriteOrBufferData(data, false, nullptr);
+  EXPECT_FALSE(stream_->IsWaitingForAcks());
+  // Although there is no outstanding data, verify session has pending crypto
+  // data.
+  EXPECT_EQ(GetQuicReloadableFlag(quic_fix_has_pending_crypto_data),
+            session_.HasUnackedCryptoData());
+
+  EXPECT_CALL(session_, WritevData(_, kCryptoStreamId, 1350, 0, _))
+      .WillOnce(Invoke(MockQuicSession::ConsumeData));
+  stream_->OnCanWrite();
+  EXPECT_TRUE(stream_->IsWaitingForAcks());
+  EXPECT_TRUE(session_.HasUnackedCryptoData());
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic

net/third_party/quic/core/quic_sent_packet_manager_test.cc

@@ -100,7 +100,7 @@ class QuicSentPacketManagerTest : public QuicTestWithParam<bool> {
     EXPECT_CALL(*network_change_visitor_, OnPathMtuIncreased(1000))
         .Times(AnyNumber());
     EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(true));
-    EXPECT_CALL(notifier_, HasPendingCryptoData())
+    EXPECT_CALL(notifier_, HasUnackedCryptoData())
         .WillRepeatedly(Return(false));
     EXPECT_CALL(notifier_, OnStreamFrameRetransmitted(_)).Times(AnyNumber());
     EXPECT_CALL(notifier_, OnFrameAcked(_, _)).WillRepeatedly(Return(true));
@@ -277,7 +277,7 @@ class QuicSentPacketManagerTest : public QuicTestWithParam<bool> {
     manager_.OnPacketSent(&packet, 0, clock_.Now(), NOT_RETRANSMISSION,
                           HAS_RETRANSMITTABLE_DATA);
     if (manager_.session_decides_what_to_write()) {
-      EXPECT_CALL(notifier_, HasPendingCryptoData())
+      EXPECT_CALL(notifier_, HasUnackedCryptoData())
           .WillRepeatedly(Return(true));
     }
   }
@@ -1037,7 +1037,7 @@ TEST_P(QuicSentPacketManagerTest, CryptoHandshakeTimeout) {
   QuicPacketNumber acked[] = {3, 4, 5, 8, 9};
   ExpectAcksAndLosses(true, acked, QUIC_ARRAYSIZE(acked), nullptr, 0);
   if (manager_.session_decides_what_to_write()) {
-    EXPECT_CALL(notifier_, HasPendingCryptoData())
+    EXPECT_CALL(notifier_, HasUnackedCryptoData())
         .WillRepeatedly(Return(false));
   }
   manager_.OnAckFrameStart(9, QuicTime::Delta::Infinite(), clock_.Now());
@@ -1113,7 +1113,7 @@ TEST_P(QuicSentPacketManagerTest, CryptoHandshakeTimeoutVersionNegotiation) {
   manager_.OnAckRange(8, 10);
   EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now()));
   if (manager_.session_decides_what_to_write()) {
-    EXPECT_CALL(notifier_, HasPendingCryptoData())
+    EXPECT_CALL(notifier_, HasUnackedCryptoData())
         .WillRepeatedly(Return(false));
   }
   EXPECT_EQ(10u, manager_.GetLeastUnacked());
@@ -1149,7 +1149,7 @@ TEST_P(QuicSentPacketManagerTest, CryptoHandshakeSpuriousRetransmission) {
   QuicPacketNumber acked[] = {2};
   ExpectAcksAndLosses(true, acked, QUIC_ARRAYSIZE(acked), nullptr, 0);
   if (manager_.session_decides_what_to_write()) {
-    EXPECT_CALL(notifier_, HasPendingCryptoData())
+    EXPECT_CALL(notifier_, HasUnackedCryptoData())
         .WillRepeatedly(Return(false));
     EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false));
   }
@@ -1257,7 +1257,7 @@ TEST_P(QuicSentPacketManagerTest,
   // connection goes forward secure.
   manager_.NeuterUnencryptedPackets();
   if (manager_.session_decides_what_to_write()) {
-    EXPECT_CALL(notifier_, HasPendingCryptoData())
+    EXPECT_CALL(notifier_, HasUnackedCryptoData())
         .WillRepeatedly(Return(false));
     EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false));
   }

net/third_party/quic/core/quic_session.cc

@@ -1165,8 +1165,17 @@ bool QuicSession::IsFrameOutstanding(const QuicFrame& frame) const {
                                           frame.stream_frame.fin);
 }
 
-bool QuicSession::HasPendingCryptoData() const {
-  return GetStream(kCryptoStreamId)->IsWaitingForAcks();
+bool QuicSession::HasUnackedCryptoData() const {
+  const QuicStream* crypto_stream = GetCryptoStream();
+  if (crypto_stream->IsWaitingForAcks()) {
+    return true;
+  }
+  if (GetQuicReloadableFlag(quic_fix_has_pending_crypto_data) &&
+      crypto_stream->HasBufferedData()) {
+    QUIC_FLAG_COUNT(quic_reloadable_flag_quic_fix_has_pending_crypto_data);
+    return true;
+  }
+  return false;
 }
 
 WriteStreamDataResult QuicSession::WriteStreamData(QuicStreamId id,

net/third_party/quic/core/quic_session.h

@@ -135,7 +135,7 @@ class QUIC_EXPORT_PRIVATE QuicSession : public QuicConnectionVisitorInterface,
   void RetransmitFrames(const QuicFrames& frames,
                         TransmissionType type) override;
   bool IsFrameOutstanding(const QuicFrame& frame) const override;
-  bool HasPendingCryptoData() const override;
+  bool HasUnackedCryptoData() const override;
 
   // Called on every incoming packet. Passes |packet| through to |connection_|.
   virtual void ProcessUdpPacket(const QuicSocketAddress& self_address,

net/third_party/quic/core/quic_unacked_packet_map.cc

@@ -355,7 +355,7 @@ bool QuicUnackedPacketMap::HasPendingCryptoPackets() const {
   if (!session_decides_what_to_write_) {
     return pending_crypto_packet_count_ > 0;
   }
-  return session_notifier_->HasPendingCryptoData();
+  return session_notifier_->HasUnackedCryptoData();
 }
 
 bool QuicUnackedPacketMap::HasUnackedRetransmittableFrames() const {

net/third_party/quic/core/quic_unacked_packet_map.h

@@ -145,7 +145,7 @@ class QUIC_EXPORT_PRIVATE QuicUnackedPacketMap {
 
   // Returns true if there are any pending crypto packets.
   // TODO(fayang): Remove this method and call session_notifier_'s
-  // HasPendingCryptoData() when session_decides_what_to_write_ is default true.
+  // HasUnackedCryptoData() when session_decides_what_to_write_ is default true.
   bool HasPendingCryptoPackets() const;
 
   // Removes any retransmittable frames from this transmission or an associated

net/third_party/quic/core/session_notifier_interface.h

@@ -35,7 +35,7 @@ class QUIC_EXPORT_PRIVATE SessionNotifierInterface {
   virtual bool IsFrameOutstanding(const QuicFrame& frame) const = 0;
 
   // Returns true if crypto stream is waiting for acks.
-  virtual bool HasPendingCryptoData() const = 0;
+  virtual bool HasUnackedCryptoData() const = 0;
 };
 
 }  // namespace quic

net/third_party/quic/test_tools/quic_test_utils.h

@@ -1014,7 +1014,7 @@ class MockSessionNotifier : public SessionNotifierInterface {
   MOCK_METHOD2(RetransmitFrames,
                void(const QuicFrames&, TransmissionType type));
   MOCK_CONST_METHOD1(IsFrameOutstanding, bool(const QuicFrame&));
-  MOCK_CONST_METHOD0(HasPendingCryptoData, bool());
+  MOCK_CONST_METHOD0(HasUnackedCryptoData, bool());
 };
 
 // Creates a client session for testing.

net/third_party/quic/test_tools/simple_session_notifier.cc

@@ -316,11 +316,14 @@ bool SimpleSessionNotifier::IsFrameOutstanding(const QuicFrame& frame) const {
          (frame.stream_frame.fin && state.fin_outstanding);
 }
 
-bool SimpleSessionNotifier::HasPendingCryptoData() const {
+bool SimpleSessionNotifier::HasUnackedCryptoData() const {
   if (!QuicContainsKey(stream_map_, kCryptoStreamId)) {
     return false;
   }
   const auto& state = stream_map_.find(kCryptoStreamId)->second;
+  if (state.bytes_total > state.bytes_sent) {
+    return true;
+  }
   QuicIntervalSet<QuicStreamOffset> bytes_to_ack(0, state.bytes_total);
   bytes_to_ack.Difference(state.bytes_acked);
   return !bytes_to_ack.Empty();

net/third_party/quic/test_tools/simple_session_notifier.h

@@ -63,7 +63,7 @@ class SimpleSessionNotifier : public SessionNotifierInterface {
   void RetransmitFrames(const QuicFrames& frames,
                         TransmissionType type) override;
   bool IsFrameOutstanding(const QuicFrame& frame) const override;
-  bool HasPendingCryptoData() const override;
+  bool HasUnackedCryptoData() const override;
 
  private:
   struct StreamState {

net/third_party/quic/test_tools/simulator/quic_endpoint.cc

@@ -273,7 +273,7 @@ bool QuicEndpoint::IsFrameOutstanding(const QuicFrame& frame) const {
   return notifier_->IsFrameOutstanding(frame);
 }
 
-bool QuicEndpoint::HasPendingCryptoData() const {
+bool QuicEndpoint::HasUnackedCryptoData() const {
   return false;
 }
 

net/third_party/quic/test_tools/simulator/quic_endpoint.h

@@ -117,7 +117,7 @@ class QuicEndpoint : public Endpoint,
   void RetransmitFrames(const QuicFrames& frames,
                         TransmissionType type) override;
   bool IsFrameOutstanding(const QuicFrame& frame) const override;
-  bool HasPendingCryptoData() const override;
+  bool HasUnackedCryptoData() const override;
   // End SessionNotifierInterface implementation.
 
  private: