Set QUIC connection's last_send_for_timeout_ to the send time of the

first sent packet after receiving a packet, even if the sent packet is
not a retransmission. Protected by default enabled
--quic_better_last_send_for_timeout

Merge internal change: 131623416

BUG=642017

Review-Url: https://codereview.chromium.org/2288843002
Cr-Commit-Position: refs/heads/master@{#415157}

net/quic/core/quic_connection.cc

@@ -1665,6 +1665,18 @@ bool QuicConnection::WritePacket(SerializedPacket* packet) {
   }
   if (packet->transmission_type == NOT_RETRANSMISSION) {
     time_of_last_sent_new_packet_ = packet_send_time;
+    if (!FLAGS_quic_better_last_send_for_timeout) {
+      if (IsRetransmittable(*packet) == HAS_RETRANSMITTABLE_DATA &&
+          last_send_for_timeout_ <= time_of_last_received_packet_) {
+        last_send_for_timeout_ = packet_send_time;
+      }
+    }
+  }
+  if (FLAGS_quic_better_last_send_for_timeout) {
+    // Only adjust the last sent time (for the purpose of tracking the idle
+    // timeout) if this is the first retransmittable packet sent after a
+    // packet is received. If it were updated on every sent packet, then
+    // sending into a black hole might never timeout.
     if (IsRetransmittable(*packet) == HAS_RETRANSMITTABLE_DATA &&
         last_send_for_timeout_ <= time_of_last_received_packet_) {
       last_send_for_timeout_ = packet_send_time;
@@ -2207,6 +2219,10 @@ void QuicConnection::CheckForTimeout() {
 void QuicConnection::SetTimeoutAlarm() {
   QuicTime time_of_last_packet =
       max(time_of_last_received_packet_, time_of_last_sent_new_packet_);
+  if (FLAGS_quic_better_last_send_for_timeout) {
+    time_of_last_packet =
+        max(time_of_last_received_packet_, last_send_for_timeout_);
+  }
 
   QuicTime deadline = time_of_last_packet + idle_network_timeout_;
   if (!handshake_timeout_.IsInfinite()) {

net/quic/core/quic_connection_test.cc

@@ -851,7 +851,7 @@ class QuicConnectionTest : public ::testing::TestWithParam<TestParams> {
         level, path_id, number, *packet, buffer, kMaxPacketSize);
     connection_.ProcessUdpPacket(
         kSelfAddress, kPeerAddress,
-        QuicReceivedPacket(buffer, encrypted_length, QuicTime::Zero(), false));
+        QuicReceivedPacket(buffer, encrypted_length, clock_.Now(), false));
     if (connection_.GetSendAlarm()->IsSet()) {
       connection_.GetSendAlarm()->Fire();
     }
@@ -3247,8 +3247,13 @@ TEST_P(QuicConnectionTest, TimeoutAfterSend) {
   connection_.GetTimeoutAlarm()->Fire();
   EXPECT_TRUE(connection_.GetTimeoutAlarm()->IsSet());
   EXPECT_TRUE(connection_.connected());
-  EXPECT_EQ(default_timeout + five_ms + five_ms,
-            connection_.GetTimeoutAlarm()->deadline());
+  if (FLAGS_quic_better_last_send_for_timeout) {
+    EXPECT_EQ(default_timeout + five_ms,
+              connection_.GetTimeoutAlarm()->deadline());
+  } else {
+    EXPECT_EQ(default_timeout + five_ms + five_ms,
+              connection_.GetTimeoutAlarm()->deadline());
+  }
 
   // This time, we should time out.
   EXPECT_CALL(visitor_, OnConnectionClosed(QUIC_NETWORK_IDLE_TIMEOUT, _,
@@ -3261,6 +3266,80 @@ TEST_P(QuicConnectionTest, TimeoutAfterSend) {
   EXPECT_FALSE(connection_.connected());
 }
 
+TEST_P(QuicConnectionTest, TimeoutAfterRetransmission) {
+  FLAGS_quic_better_last_send_for_timeout = true;
+  EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+  EXPECT_TRUE(connection_.connected());
+  EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _));
+  QuicConfig config;
+  connection_.SetFromConfig(config);
+  EXPECT_FALSE(QuicConnectionPeer::IsSilentCloseEnabled(&connection_));
+
+  const QuicTime start_time = clock_.Now();
+  const QuicTime::Delta initial_idle_timeout =
+      QuicTime::Delta::FromSeconds(kInitialIdleTimeoutSecs - 1);
+  QuicTime default_timeout = clock_.Now() + initial_idle_timeout;
+
+  connection_.SetMaxTailLossProbes(kDefaultPathId, 0);
+  const QuicTime default_retransmission_time =
+      start_time + DefaultRetransmissionTime();
+
+  ASSERT_LT(default_retransmission_time, default_timeout);
+
+  // When we send a packet, the timeout will change to 5 ms +
+  // kInitialIdleTimeoutSecs (but it will not reschedule the alarm).
+  const QuicTime::Delta five_ms = QuicTime::Delta::FromMilliseconds(5);
+  const QuicTime send_time = start_time + five_ms;
+  clock_.AdvanceTime(five_ms);
+  ASSERT_EQ(send_time, clock_.Now());
+  SendStreamDataToPeer(kClientDataStreamId1, "foo", 0, kFin, nullptr);
+  EXPECT_EQ(default_timeout, connection_.GetTimeoutAlarm()->deadline());
+
+  // Move forward 5 ms and receive a packet, which will move the timeout
+  // forward 5 ms more (but will not reschedule the alarm).
+  const QuicTime receive_time = send_time + five_ms;
+  clock_.AdvanceTime(receive_time - clock_.Now());
+  ASSERT_EQ(receive_time, clock_.Now());
+  ProcessPacket(kDefaultPathId, 1);
+
+  // Now move forward to the retransmission time and retransmit the
+  // packet, which should move the timeout forward again (but will not
+  // reschedule the alarm).
+  EXPECT_EQ(default_retransmission_time + five_ms,
+            connection_.GetRetransmissionAlarm()->deadline());
+  // Simulate the retransmission alarm firing.
+  const QuicTime rto_time = send_time + DefaultRetransmissionTime();
+  const QuicTime final_timeout = rto_time + initial_idle_timeout;
+  clock_.AdvanceTime(rto_time - clock_.Now());
+  ASSERT_EQ(rto_time, clock_.Now());
+  EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, 2u, _, _));
+  connection_.GetRetransmissionAlarm()->Fire();
+
+  // Advance to the original timeout and fire the alarm. The connection should
+  // timeout, and the alarm should be registered based on the time of the
+  // retransmission.
+  clock_.AdvanceTime(default_timeout - clock_.Now());
+  ASSERT_EQ(default_timeout.ToDebuggingValue(),
+            clock_.Now().ToDebuggingValue());
+  EXPECT_EQ(default_timeout, clock_.Now());
+  connection_.GetTimeoutAlarm()->Fire();
+  EXPECT_TRUE(connection_.GetTimeoutAlarm()->IsSet());
+  EXPECT_TRUE(connection_.connected());
+  ASSERT_EQ(final_timeout.ToDebuggingValue(),
+            connection_.GetTimeoutAlarm()->deadline().ToDebuggingValue());
+
+  // This time, we should time out.
+  EXPECT_CALL(visitor_, OnConnectionClosed(QUIC_NETWORK_IDLE_TIMEOUT, _,
+                                           ConnectionCloseSource::FROM_SELF));
+  EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _));
+  clock_.AdvanceTime(final_timeout - clock_.Now());
+  EXPECT_EQ(connection_.GetTimeoutAlarm()->deadline(), clock_.Now());
+  EXPECT_EQ(final_timeout, clock_.Now());
+  connection_.GetTimeoutAlarm()->Fire();
+  EXPECT_FALSE(connection_.GetTimeoutAlarm()->IsSet());
+  EXPECT_FALSE(connection_.connected());
+}
+
 TEST_P(QuicConnectionTest, NewTimeoutAfterSendSilentClose) {
   // Same test as above, but complete a handshake which enables silent close,
   // causing no connection close packet to be sent.
@@ -3311,8 +3390,13 @@ TEST_P(QuicConnectionTest, NewTimeoutAfterSendSilentClose) {
   connection_.GetTimeoutAlarm()->Fire();
   EXPECT_TRUE(connection_.GetTimeoutAlarm()->IsSet());
   EXPECT_TRUE(connection_.connected());
-  EXPECT_EQ(default_timeout + five_ms + five_ms,
-            connection_.GetTimeoutAlarm()->deadline());
+  if (FLAGS_quic_better_last_send_for_timeout) {
+    EXPECT_EQ(default_timeout + five_ms,
+              connection_.GetTimeoutAlarm()->deadline());
+  } else {
+    EXPECT_EQ(default_timeout + five_ms + five_ms,
+              connection_.GetTimeoutAlarm()->deadline());
+  }
 
   // This time, we should time out.
   EXPECT_CALL(visitor_, OnConnectionClosed(QUIC_NETWORK_IDLE_TIMEOUT, _,

net/quic/core/quic_flags.cc

@@ -160,3 +160,8 @@ bool FLAGS_quic_do_not_send_ack_on_emsgsize = true;
 
 // If true, postpone multipath flag validation to ProcessValidatedPacket.
 bool FLAGS_quic_postpone_multipath_flag_validation = true;
+
+// If true, set a QUIC connection's last_sent_for_timeout_ to the send time of
+// the first packet sent after receiving a packet, even if the sent packet is
+// a retransmission
+bool FLAGS_quic_better_last_send_for_timeout = true;

net/quic/core/quic_flags.h

@@ -50,5 +50,6 @@ NET_EXPORT_PRIVATE extern bool FLAGS_quic_enforce_mtu_limit;
 NET_EXPORT_PRIVATE extern bool FLAGS_graceful_emsgsize_on_mtu_probe;
 NET_EXPORT_PRIVATE extern bool FLAGS_quic_do_not_send_ack_on_emsgsize;
 NET_EXPORT_PRIVATE extern bool FLAGS_quic_postpone_multipath_flag_validation;
+NET_EXPORT_PRIVATE extern bool FLAGS_quic_better_last_send_for_timeout;
 
 #endif  // NET_QUIC_QUIC_FLAGS_H_