Land Recent QUIC Changes

Set the delayed ack timer to 0ms intstead of 100ms for crypto handshake
packets.

This both provides a more accurate RTT estimate early on in a connection
and gives the server confidence the handshake has completed and it's
safe to increase the CWND further.

Merge internal change: 64134758
https://codereview.chromium.org/221373005/


Making framer capable of handling stream frame headers in packets
correctly when using FEC. Stream frame length is always written, even
for the last stream frame with FEC, to avoid boundary condition and
padding issues in the framer.

These changes are working towards getting FEC up and running correctly.

Merge internal change: 64080142
https://codereview.chromium.org/212223011/


Change to QUIC's TLP timer to set it based on the last pending packet,
even when that packet has no retransmittable frames.

Merge internal change: 64059923
https://codereview.chromium.org/220163008/


QUIC - cleanup changes fixed while sync'ing with internal source tree.

https://codereview.chromium.org/212523003/

R=rch@chromium.org

Review URL: https://codereview.chromium.org/220723013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@261606 0039d316-1c4b-4281-b951-d872f2087c98

22 files changed, 258 insertions, 95 deletions

diff --git a/net/quic/congestion_control/inter_arrival_bitrate_ramp_up.cc b/net/quic/congestion_control/inter_arrival_bitrate_ramp_up.cc
index 801a526..03a3fa1 100644
--- a/net/quic/congestion_control/inter_arrival_bitrate_ramp_up.cc
+++ b/net/quic/congestion_control/inter_arrival_bitrate_ramp_up.cc
@@ -9,6 +9,8 @@
 #include "net/quic/congestion_control/cube_root.h"
 #include "net/quic/quic_protocol.h"
 
+using std::max;
+
 namespace {
 // The following constants are in 2^10 fractions of a second instead of ms to
 // allow a 10 shift right to divide.
@@ -37,8 +39,8 @@ void InterArrivalBitrateRampUp::Reset(QuicBandwidth new_rate,
                                       QuicBandwidth channel_estimate) {
   epoch_ = clock_->ApproximateNow();
   last_update_time_ = epoch_;
-  available_channel_estimate_ = std::max(new_rate, available_channel_estimate);
-  channel_estimate_ = std::max(channel_estimate, available_channel_estimate_);
+  available_channel_estimate_ = max(new_rate, available_channel_estimate);
+  channel_estimate_ = max(channel_estimate, available_channel_estimate_);
 
   halfway_point_ = available_channel_estimate_.Add(
       (channel_estimate_.Subtract(available_channel_estimate_)).Scale(0.5f));
diff --git a/net/quic/congestion_control/inter_arrival_overuse_detector.cc b/net/quic/congestion_control/inter_arrival_overuse_detector.cc
index f3f0250..11e788b 100644
--- a/net/quic/congestion_control/inter_arrival_overuse_detector.cc
+++ b/net/quic/congestion_control/inter_arrival_overuse_detector.cc
@@ -9,6 +9,8 @@
 
 #include <algorithm>
 
+using std::max;
+
 // Initial noise variance, equal to a standard deviation of 1 millisecond.
 static const float kInitialVarianceNoise = 1000000.0;
 
@@ -120,7 +122,7 @@ BandwidthUsage InterArrivalOveruseDetector::GetState(
   int64 sigma_delta = sqrt(static_cast<double>(delta_variance_));
   DetectSlope(sigma_delta);
   DetectDrift(sigma_delta);
-  return std::max(slope_estimate_, delta_estimate_);
+  return max(slope_estimate_, delta_estimate_);
 }
 
 void InterArrivalOveruseDetector::UpdateFilter(QuicTime::Delta received_delta,
diff --git a/net/quic/crypto/quic_crypto_client_config.cc b/net/quic/crypto/quic_crypto_client_config.cc
index a6b0e39..a614e6b 100644
--- a/net/quic/crypto/quic_crypto_client_config.cc
+++ b/net/quic/crypto/quic_crypto_client_config.cc
@@ -17,7 +17,6 @@
 #include "net/quic/crypto/p256_key_exchange.h"
 #include "net/quic/crypto/proof_verifier.h"
 #include "net/quic/crypto/quic_encrypter.h"
-#include "net/quic/quic_server_id.h"
 #include "net/quic/quic_utils.h"
 
 using base::StringPiece;
diff --git a/net/quic/crypto/quic_crypto_server_config.cc b/net/quic/crypto/quic_crypto_server_config.cc
index 24623a7..a6c099c 100644
--- a/net/quic/crypto/quic_crypto_server_config.cc
+++ b/net/quic/crypto/quic_crypto_server_config.cc
@@ -40,6 +40,7 @@
 using base::StringPiece;
 using crypto::SecureHash;
 using std::map;
+using std::sort;
 using std::string;
 using std::vector;
 
@@ -767,7 +768,7 @@ void QuicCryptoServerConfig::SelectNewPrimaryConfig(
     return;
   }
 
-  std::sort(configs.begin(), configs.end(), ConfigPrimaryTimeLessThan);
+  sort(configs.begin(), configs.end(), ConfigPrimaryTimeLessThan);
 
   Config* best_candidate = configs[0];
 
diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc
index 4f409a0..f0218a7 100644
--- a/net/quic/quic_connection.cc
+++ b/net/quic/quic_connection.cc
@@ -813,8 +813,13 @@ void QuicConnection::MaybeQueueAck() {
     if (ack_alarm_->IsSet()) {
       ack_queued_ = true;
     } else {
-      ack_alarm_->Set(clock_->ApproximateNow().Add(
-          sent_packet_manager_.DelayedAckTime()));
+      // Send an ack much more quickly for crypto handshake packets.
+      QuicTime::Delta delayed_ack_time = sent_packet_manager_.DelayedAckTime();
+      if (last_stream_frames_.size() == 1 &&
+          last_stream_frames_[0].stream_id == kCryptoStreamId) {
+        delayed_ack_time = QuicTime::Delta::Zero();
+      }
+      ack_alarm_->Set(clock_->ApproximateNow().Add(delayed_ack_time));
       DVLOG(1) << "Ack timer set; next packet or timer will trigger ACK.";
     }
   }
@@ -959,10 +964,9 @@ QuicConsumedData QuicConnection::SendStreamData(
     notifier = new QuicAckNotifier(delegate);
   }
 
-  // Opportunistically bundle an ack with every outgoing packet, unless it's the
-  // crypto stream.
-  ScopedPacketBundler ack_bundler(
-      this, id != kCryptoStreamId ? BUNDLE_PENDING_ACK : NO_ACK);
+  // Opportunistically bundle an ack with every outgoing packet.
+  // TODO(ianswett): Consider not bundling an ack when there is no encryption.
+  ScopedPacketBundler ack_bundler(this, BUNDLE_PENDING_ACK);
   QuicConsumedData consumed_data =
       packet_generator_.ConsumeData(id, data, offset, fin, notifier);
 
diff --git a/net/quic/quic_connection_test.cc b/net/quic/quic_connection_test.cc
index 6b2bc1f..bf1f464 100644
--- a/net/quic/quic_connection_test.cc
+++ b/net/quic/quic_connection_test.cc
@@ -2823,10 +2823,34 @@ TEST_P(QuicConnectionTest, LoopThroughSendingPackets) {
                                                  !kFin, NULL).bytes_consumed);
 }
 
-TEST_P(QuicConnectionTest, SendDelayedAckOnTimer) {
+TEST_P(QuicConnectionTest, SendDelayedAck) {
   QuicTime ack_time = clock_.ApproximateNow().Add(DefaultDelayedAckTime());
   EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
   EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
+  // Process a packet from the non-crypto stream.
+  frame1_.stream_id = 3;
+  ProcessPacket(1);
+  // Check if delayed ack timer is running for the expected interval.
+  EXPECT_TRUE(connection_.GetAckAlarm()->IsSet());
+  EXPECT_EQ(ack_time, connection_.GetAckAlarm()->deadline());
+  // Simulate delayed ack alarm firing.
+  connection_.GetAckAlarm()->Fire();
+  // Check that ack is sent and that delayed ack alarm is reset.
+  if (version() > QUIC_VERSION_15) {
+    EXPECT_EQ(2u, writer_->frame_count());
+    EXPECT_TRUE(writer_->stop_waiting());
+  } else {
+    EXPECT_EQ(1u, writer_->frame_count());
+  }
+  EXPECT_TRUE(writer_->ack());
+  EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
+}
+
+TEST_P(QuicConnectionTest, SendEarlyDelayedAckForCrypto) {
+  QuicTime ack_time = clock_.ApproximateNow();
+  EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+  EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
+  // Process a packet from the crypto stream, which is frame1_'s default.
   ProcessPacket(1);
   // Check if delayed ack timer is running for the expected interval.
   EXPECT_TRUE(connection_.GetAckAlarm()->IsSet());
@@ -2901,14 +2925,14 @@ TEST_P(QuicConnectionTest, SendDelayedAckOnOutgoingPacket) {
   EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
 }
 
-TEST_P(QuicConnectionTest, DontSendDelayedAckOnOutgoingCryptoPacket) {
+TEST_P(QuicConnectionTest, SendDelayedAckOnOutgoingCryptoPacket) {
   EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
   ProcessPacket(1);
   connection_.SendStreamDataWithString(kCryptoStreamId, "foo", 0, !kFin, NULL);
-  // Check that ack is not bundled with outgoing data.
-  EXPECT_EQ(1u, writer_->frame_count());
-  EXPECT_FALSE(writer_->ack());
-  EXPECT_TRUE(connection_.GetAckAlarm()->IsSet());
+  // Check that ack is bundled with outgoing crypto data.
+  EXPECT_EQ(version() <= QUIC_VERSION_15 ? 2u : 3u, writer_->frame_count());
+  EXPECT_TRUE(writer_->ack());
+  EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
 }
 
 TEST_P(QuicConnectionTest, BundleAckWithDataOnIncomingAck) {
diff --git a/net/quic/quic_framer.cc b/net/quic/quic_framer.cc
index 674000b..3077c57 100644
--- a/net/quic/quic_framer.cc
+++ b/net/quic/quic_framer.cc
@@ -182,10 +182,13 @@ QuicFramer::~QuicFramer() {}
 size_t QuicFramer::GetMinStreamFrameSize(QuicVersion version,
                                          QuicStreamId stream_id,
                                          QuicStreamOffset offset,
-                                         bool last_frame_in_packet) {
+                                         bool last_frame_in_packet,
+                                         InFecGroup is_in_fec_group) {
+  bool no_stream_frame_length = last_frame_in_packet &&
+                                is_in_fec_group == NOT_IN_FEC_GROUP;
   return kQuicFrameTypeSize + GetStreamIdSize(stream_id) +
       GetStreamOffsetSize(offset) +
-      (last_frame_in_packet ? 0 : kQuicStreamPayloadLengthSize);
+      (no_stream_frame_length ? 0 : kQuicStreamPayloadLengthSize);
 }
 
 // static
@@ -289,13 +292,15 @@ size_t QuicFramer::GetSerializedFrameLength(
     size_t free_bytes,
     bool first_frame,
     bool last_frame,
+    InFecGroup is_in_fec_group,
     QuicSequenceNumberLength sequence_number_length) {
   if (frame.type == PADDING_FRAME) {
     // PADDING implies end of packet.
     return free_bytes;
   }
   size_t frame_len =
-      ComputeFrameLength(frame, last_frame, sequence_number_length);
+      ComputeFrameLength(frame, last_frame, is_in_fec_group,
+                         sequence_number_length);
   if (frame_len > free_bytes) {
     // Only truncate the first frame in a packet, so if subsequent ones go
     // over, stop including more frames.
@@ -335,6 +340,7 @@ SerializedPacket QuicFramer::BuildUnsizedDataPacket(
     bool last_frame = i == frames.size() - 1;
     const size_t frame_size = GetSerializedFrameLength(
         frames[i], max_plaintext_size - packet_size, first_frame, last_frame,
+        header.is_in_fec_group,
         header.public_header.sequence_number_length);
     DCHECK(frame_size);
     packet_size += frame_size;
@@ -357,8 +363,11 @@ SerializedPacket QuicFramer::BuildDataPacket(
   for (size_t i = 0; i < frames.size(); ++i) {
     const QuicFrame& frame = frames[i];
 
-    const bool last_frame_in_packet = i == (frames.size() - 1);
-    if (!AppendTypeByte(frame, last_frame_in_packet, &writer)) {
+    // Determine if we should write stream frame length in header.
+    const bool no_stream_frame_length =
+        (header.is_in_fec_group == NOT_IN_FEC_GROUP) &&
+        (i == frames.size() - 1);
+    if (!AppendTypeByte(frame, no_stream_frame_length, &writer)) {
       LOG(DFATAL) << "AppendTypeByte failed";
       return kNoPacket;
     }
@@ -369,7 +378,7 @@ SerializedPacket QuicFramer::BuildDataPacket(
         break;
       case STREAM_FRAME:
         if (!AppendStreamFrame(
-            *frame.stream_frame, last_frame_in_packet, &writer)) {
+            *frame.stream_frame, no_stream_frame_length, &writer)) {
           LOG(DFATAL) << "AppendStreamFrame failed";
           return kNoPacket;
         }
@@ -1863,13 +1872,15 @@ size_t QuicFramer::GetAckFrameSize(
 size_t QuicFramer::ComputeFrameLength(
     const QuicFrame& frame,
     bool last_frame_in_packet,
+    InFecGroup is_in_fec_group,
     QuicSequenceNumberLength sequence_number_length) {
   switch (frame.type) {
     case STREAM_FRAME:
       return GetMinStreamFrameSize(quic_version_,
                                    frame.stream_frame->stream_id,
                                    frame.stream_frame->offset,
-                                   last_frame_in_packet) +
+                                   last_frame_in_packet,
+                                   is_in_fec_group) +
           frame.stream_frame->data.TotalBufferSize();
     case ACK_FRAME: {
       return GetAckFrameSize(*frame.ack_frame, sequence_number_length);
@@ -1941,7 +1952,7 @@ size_t QuicFramer::ComputeFrameLength(
 }
 
 bool QuicFramer::AppendTypeByte(const QuicFrame& frame,
-                                bool last_frame_in_packet,
+                                bool no_stream_frame_length,
                                 QuicDataWriter* writer) {
   uint8 type_byte = 0;
   switch (frame.type) {
@@ -1954,7 +1965,7 @@ bool QuicFramer::AppendTypeByte(const QuicFrame& frame,
 
       // Data Length bit.
       type_byte <<= kQuicStreamDataLengthShift;
-      type_byte |= last_frame_in_packet ? 0 : kQuicStreamDataLengthMask;
+      type_byte |= no_stream_frame_length ? 0: kQuicStreamDataLengthMask;
 
       // Offset 3 bits.
       type_byte <<= kQuicStreamOffsetShift;
@@ -2019,21 +2030,25 @@ bool QuicFramer::AppendPacketSequenceNumber(
 
 bool QuicFramer::AppendStreamFrame(
     const QuicStreamFrame& frame,
-    bool last_frame_in_packet,
+    bool no_stream_frame_length,
     QuicDataWriter* writer) {
   if (!writer->WriteBytes(&frame.stream_id, GetStreamIdSize(frame.stream_id))) {
+    LOG(DFATAL) << "Writing stream id size failed.";
     return false;
   }
   if (!writer->WriteBytes(&frame.offset, GetStreamOffsetSize(frame.offset))) {
+    LOG(DFATAL) << "Writing offset size failed.";
     return false;
   }
-  if (!last_frame_in_packet) {
+  if (!no_stream_frame_length) {
     if (!writer->WriteUInt16(frame.data.TotalBufferSize())) {
+      LOG(DFATAL) << "Writing stream frame length failed";
       return false;
     }
   }
 
   if (!writer->WriteIOVector(frame.data)) {
+    LOG(DFATAL) << "Writing frame data failed.";
     return false;
   }
   return true;
diff --git a/net/quic/quic_framer.h b/net/quic/quic_framer.h
index d757954..e1a5602 100644
--- a/net/quic/quic_framer.h
+++ b/net/quic/quic_framer.h
@@ -245,7 +245,8 @@ class NET_EXPORT_PRIVATE QuicFramer {
   static size_t GetMinStreamFrameSize(QuicVersion version,
                                       QuicStreamId stream_id,
                                       QuicStreamOffset offset,
-                                      bool last_frame_in_packet);
+                                      bool last_frame_in_packet,
+                                      InFecGroup is_in_fec_group);
   // Size in bytes of all ack frame fields without the missing packets.
   static size_t GetMinAckFrameSize(
       QuicVersion version,
@@ -278,8 +279,9 @@ class NET_EXPORT_PRIVATE QuicFramer {
   size_t GetSerializedFrameLength(
       const QuicFrame& frame,
       size_t free_bytes,
-      bool first_frame,
-      bool last_frame,
+      bool first_frame_in_packet,
+      bool last_frame_in_packet,
+      InFecGroup is_in_fec_group,
       QuicSequenceNumberLength sequence_number_length);
 
   // Returns the associated data from the encrypted packet |encrypted| as a
@@ -432,6 +434,7 @@ class NET_EXPORT_PRIVATE QuicFramer {
   // Computes the wire size in bytes of the payload of |frame|.
   size_t ComputeFrameLength(const QuicFrame& frame,
                             bool last_frame_in_packet,
+                            InFecGroup is_in_fec_group,
                             QuicSequenceNumberLength sequence_number_length);
 
   static bool AppendPacketSequenceNumber(
diff --git a/net/quic/quic_framer_test.cc b/net/quic/quic_framer_test.cc
index 96fc39b..56adfc9 100644
--- a/net/quic/quic_framer_test.cc
+++ b/net/quic/quic_framer_test.cc
@@ -3899,6 +3899,62 @@ TEST_P(QuicFramerTest, BuildStreamFramePacket) {
                                       AsChars(packet), arraysize(packet));
 }
 
+TEST_P(QuicFramerTest, BuildStreamFramePacketInFecGroup) {
+  QuicPacketHeader header;
+  header.public_header.connection_id = GG_UINT64_C(0xFEDCBA9876543210);
+  header.public_header.reset_flag = false;
+  header.public_header.version_flag = false;
+  header.fec_flag = false;
+  header.entropy_flag = true;
+  header.packet_sequence_number = GG_UINT64_C(0x77123456789ABC);
+  header.is_in_fec_group = IN_FEC_GROUP;
+  header.fec_group = GG_UINT64_C(0x77123456789ABC);
+
+  QuicStreamFrame stream_frame;
+  stream_frame.stream_id = 0x01020304;
+  stream_frame.fin = true;
+  stream_frame.offset = GG_UINT64_C(0xBA98FEDC32107654);
+  stream_frame.data = MakeIOVector("hello world!");
+
+  QuicFrames frames;
+  frames.push_back(QuicFrame(&stream_frame));
+  unsigned char packet[] = {
+    // public flags (8 byte connection_id)
+    0x3C,
+    // connection_id
+    0x10, 0x32, 0x54, 0x76,
+    0x98, 0xBA, 0xDC, 0xFE,
+    // packet sequence number
+    0xBC, 0x9A, 0x78, 0x56,
+    0x34, 0x12,
+    // private flags (entropy, is_in_fec_group)
+    0x03,
+    // FEC group
+    0x00,
+    // frame type (stream frame with fin and data length field)
+    0xFF,
+    // stream id
+    0x04, 0x03, 0x02, 0x01,
+    // offset
+    0x54, 0x76, 0x10, 0x32,
+    0xDC, 0xFE, 0x98, 0xBA,
+    // data length (since packet is in an FEC group)
+    0x0C, 0x00,
+    // data
+    'h',  'e',  'l',  'l',
+    'o',  ' ',  'w',  'o',
+    'r',  'l',  'd',  '!',
+  };
+
+  scoped_ptr<QuicPacket> data(
+      framer_.BuildUnsizedDataPacket(header, frames).packet);
+  ASSERT_TRUE(data != NULL);
+
+  test::CompareCharArraysWithHexError("constructed packet",
+                                      data->data(), data->length(),
+                                      AsChars(packet), arraysize(packet));
+}
+
 TEST_P(QuicFramerTest, BuildStreamFramePacketWithVersionFlag) {
   QuicPacketHeader header;
   header.public_header.connection_id = GG_UINT64_C(0xFEDCBA9876543210);
diff --git a/net/quic/quic_http_stream_test.cc b/net/quic/quic_http_stream_test.cc
index 46bfa57..586e1e3 100644
--- a/net/quic/quic_http_stream_test.cc
+++ b/net/quic/quic_http_stream_test.cc
@@ -216,6 +216,7 @@ class QuicHttpStreamTest : public ::testing::TestWithParam<QuicVersion> {
     stream_.reset(use_closing_stream_ ?
                   new AutoClosingStream(session_->GetWeakPtr()) :
                   new QuicHttpStream(session_->GetWeakPtr()));
+    clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(20));
   }
 
   void SetRequest(const std::string& method,
diff --git a/net/quic/quic_network_transaction_unittest.cc b/net/quic/quic_network_transaction_unittest.cc
index 75411f5..c904549 100644
--- a/net/quic/quic_network_transaction_unittest.cc
+++ b/net/quic/quic_network_transaction_unittest.cc
@@ -119,6 +119,7 @@ class QuicNetworkTransactionTest
     request_.method = "GET";
     request_.url = GURL("http://www.google.com/");
     request_.load_flags = 0;
+    clock_->AdvanceTime(QuicTime::Delta::FromMilliseconds(20));
   }
 
   virtual void SetUp() {
diff --git a/net/quic/quic_packet_creator.cc b/net/quic/quic_packet_creator.cc
index aa52c7d..072c536 100644
--- a/net/quic/quic_packet_creator.cc
+++ b/net/quic/quic_packet_creator.cc
@@ -88,7 +88,7 @@ bool QuicPacketCreator::ShouldSendFec(bool force_close) const {
        fec_group_->NumReceivedPackets() >= options_.max_packets_per_fec_group);
 }
 
-void QuicPacketCreator::MaybeStartFEC() {
+InFecGroup QuicPacketCreator::MaybeStartFEC() {
   // Don't send FEC until QUIC_VERSION_15.
   if (framer_->version() != QUIC_VERSION_13 &&
       options_.max_packets_per_fec_group > 0 && fec_group_.get() == NULL) {
@@ -97,6 +97,7 @@ void QuicPacketCreator::MaybeStartFEC() {
     fec_group_number_ = sequence_number() + 1;
     fec_group_.reset(new QuicFecGroup());
   }
+  return fec_group_.get() == NULL ? NOT_IN_FEC_GROUP : IN_FEC_GROUP;
 }
 
 // Stops serializing version of the protocol in packets sent after this call.
@@ -130,8 +131,14 @@ void QuicPacketCreator::UpdateSequenceNumberLength(
 
 bool QuicPacketCreator::HasRoomForStreamFrame(QuicStreamId id,
                                               QuicStreamOffset offset) const {
+  // TODO(jri): This is a simple safe decision for now, but make
+  // is_in_fec_group a parameter. Same as with all public methods in
+  // QuicPacketCreator.
+  InFecGroup is_in_fec_group = options_.max_packets_per_fec_group == 0 ?
+                               NOT_IN_FEC_GROUP : IN_FEC_GROUP;
   return BytesFree() >
-      QuicFramer::GetMinStreamFrameSize(framer_->version(), id, offset, true);
+      QuicFramer::GetMinStreamFrameSize(framer_->version(), id, offset, true,
+                                        is_in_fec_group);
 }
 
 // static
@@ -144,7 +151,7 @@ size_t QuicPacketCreator::StreamFramePacketOverhead(
   return GetPacketHeaderSize(connection_id_length, include_version,
                              sequence_number_length, is_in_fec_group) +
       // Assumes this is a stream with a single lone packet.
-      QuicFramer::GetMinStreamFrameSize(version, 1u, 0u, true);
+      QuicFramer::GetMinStreamFrameSize(version, 1u, 0u, true, is_in_fec_group);
 }
 
 size_t QuicPacketCreator::CreateStreamFrame(QuicStreamId id,
@@ -156,11 +163,13 @@ size_t QuicPacketCreator::CreateStreamFrame(QuicStreamId id,
             StreamFramePacketOverhead(
                 framer_->version(), PACKET_8BYTE_CONNECTION_ID, kIncludeVersion,
                 PACKET_6BYTE_SEQUENCE_NUMBER, IN_FEC_GROUP));
+  InFecGroup is_in_fec_group = MaybeStartFEC();
+
   LOG_IF(DFATAL, !HasRoomForStreamFrame(id, offset))
       << "No room for Stream frame, BytesFree: " << BytesFree()
       << " MinStreamFrameSize: "
       << QuicFramer::GetMinStreamFrameSize(
-          framer_->version(), id, offset, true);
+          framer_->version(), id, offset, true, is_in_fec_group);
 
   if (data.Empty()) {
     LOG_IF(DFATAL, !fin)
@@ -171,10 +180,10 @@ size_t QuicPacketCreator::CreateStreamFrame(QuicStreamId id,
   }
 
   const size_t data_size = data.TotalBufferSize();
-  size_t min_last_frame_size = QuicFramer::GetMinStreamFrameSize(
-      framer_->version(), id, offset, /*last_frame_in_packet=*/ true);
-  size_t bytes_consumed =
-      min<size_t>(BytesFree() - min_last_frame_size, data_size);
+  size_t min_frame_size = QuicFramer::GetMinStreamFrameSize(
+      framer_->version(), id, offset, /*last_frame_in_packet=*/ true,
+      is_in_fec_group);
+  size_t bytes_consumed = min<size_t>(BytesFree() - min_frame_size, data_size);
 
   bool set_fin = fin && bytes_consumed == data_size;  // Last frame.
   IOVector frame_data;
@@ -249,23 +258,24 @@ bool QuicPacketCreator::HasPendingFrames() {
   return !queued_frames_.empty();
 }
 
+size_t QuicPacketCreator::ExpansionOnNewFrame() const {
+  // If packet is FEC protected, there's no expansion.
+  if (fec_group_.get() != NULL) {
+      return 0;
+  }
+  // If the last frame in the packet is a stream frame, then it will expand to
+  // include the stream_length field when a new frame is added.
+  bool has_trailing_stream_frame =
+      !queued_frames_.empty() && queued_frames_.back().type == STREAM_FRAME;
+  return has_trailing_stream_frame ? kQuicStreamPayloadLengthSize : 0;
+}
+
 size_t QuicPacketCreator::BytesFree() const {
   const size_t max_plaintext_size =
       framer_->GetMaxPlaintextSize(options_.max_packet_length);
   DCHECK_GE(max_plaintext_size, PacketSize());
-
-  // If the last frame in the packet is a stream frame, then it can be
-  // two bytes smaller than if it were not the last.  So this means that
-  // there are two fewer bytes available to the next frame in this case.
-  bool has_trailing_stream_frame =
-      !queued_frames_.empty() && queued_frames_.back().type == STREAM_FRAME;
-  size_t expanded_packet_size = PacketSize() +
-      (has_trailing_stream_frame ? kQuicStreamPayloadLengthSize : 0);
-
-  if (expanded_packet_size  >= max_plaintext_size) {
-    return 0;
-  }
-  return max_plaintext_size - expanded_packet_size;
+  return max_plaintext_size - min(max_plaintext_size, PacketSize()
+                                  + ExpansionOnNewFrame());
 }
 
 size_t QuicPacketCreator::PacketSize() const {
@@ -392,20 +402,16 @@ bool QuicPacketCreator::ShouldRetransmit(const QuicFrame& frame) {
 bool QuicPacketCreator::AddFrame(const QuicFrame& frame,
                                  bool save_retransmittable_frames) {
   DVLOG(1) << "Adding frame: " << frame;
+  InFecGroup is_in_fec_group = MaybeStartFEC();
   size_t frame_len = framer_->GetSerializedFrameLength(
-      frame, BytesFree(), queued_frames_.empty(), true,
+      frame, BytesFree(), queued_frames_.empty(), true, is_in_fec_group,
       options()->send_sequence_number_length);
   if (frame_len == 0) {
     return false;
   }
   DCHECK_LT(0u, packet_size_);
-  MaybeStartFEC();
-  packet_size_ += frame_len;
-  // If the last frame in the packet was a stream frame, then once we add the
-  // new frame it's serialization will be two bytes larger.
-  if (!queued_frames_.empty() && queued_frames_.back().type == STREAM_FRAME) {
-    packet_size_ += kQuicStreamPayloadLengthSize;
-  }
+  packet_size_ += ExpansionOnNewFrame() + frame_len;
+
   if (save_retransmittable_frames && ShouldRetransmit(frame)) {
     if (queued_retransmittable_frames_.get() == NULL) {
       queued_retransmittable_frames_.reset(new RetransmittableFrames());
diff --git a/net/quic/quic_packet_creator.h b/net/quic/quic_packet_creator.h
index 5b01319..3cbae77 100644
--- a/net/quic/quic_packet_creator.h
+++ b/net/quic/quic_packet_creator.h
@@ -121,6 +121,13 @@ class NET_EXPORT_PRIVATE QuicPacketCreator : public QuicFecBuilderInterface {
   // value than max_packet_size - PacketSize(), in this case.
   size_t BytesFree() const;
 
+  // Returns the number of bytes that the packet will expand by if a new frame
+  // is added to the packet. If the last frame was a stream frame, it will
+  // expand slightly when a new frame is added, and this method returns the
+  // amount of expected expansion. If the packet is in an FEC group, no
+  // expansion happens and this method always returns zero.
+  size_t ExpansionOnNewFrame() const;
+
   // Returns the number of bytes in the current packet, including the header,
   // if serialized with the current frames.  Adding a frame to the packet
   // may change the serialized length of existing frames, as per the comment
@@ -182,7 +189,7 @@ class NET_EXPORT_PRIVATE QuicPacketCreator : public QuicFecBuilderInterface {
 
   // Starts a new FEC group with the next serialized packet, if FEC is enabled
   // and there is not already an FEC group open.
-  void MaybeStartFEC();
+  InFecGroup MaybeStartFEC();
 
   void FillPacketHeader(QuicFecGroupNumber fec_group,
                         bool fec_flag,
diff --git a/net/quic/quic_packet_creator_test.cc b/net/quic/quic_packet_creator_test.cc
index 3730e14..4b69515 100644
--- a/net/quic/quic_packet_creator_test.cc
+++ b/net/quic/quic_packet_creator_test.cc
@@ -64,12 +64,12 @@ class QuicPacketCreatorTest : public ::testing::TestWithParam<bool> {
   }
 
   // Returns the number of bytes consumed by the header of packet, including
-  // the version, that is not in an FEC group.
-  size_t GetPacketHeaderOverhead() {
+  // the version.
+  size_t GetPacketHeaderOverhead(InFecGroup is_in_fec_group) {
     return GetPacketHeaderSize(creator_.options()->send_connection_id_length,
                                kIncludeVersion,
                                creator_.options()->send_sequence_number_length,
-                               NOT_IN_FEC_GROUP);
+                               is_in_fec_group);
   }
 
   // Returns the number of bytes of overhead that will be added to a packet
@@ -82,9 +82,9 @@ class QuicPacketCreatorTest : public ::testing::TestWithParam<bool> {
 
   // Returns the number of bytes consumed by the non-data fields of a stream
   // frame, assuming it is the last frame in the packet
-  size_t GetStreamFrameOverhead() {
+  size_t GetStreamFrameOverhead(InFecGroup is_in_fec_group) {
     return QuicFramer::GetMinStreamFrameSize(
-        client_framer_.version(), kStreamId, kOffset, true);
+        client_framer_.version(), kStreamId, kOffset, true, is_in_fec_group);
   }
 
   static const QuicStreamId kStreamId = 1u;
@@ -336,10 +336,12 @@ TEST_F(QuicPacketCreatorTest, CreateStreamFrameFinOnly) {
 }
 
 TEST_F(QuicPacketCreatorTest, CreateAllFreeBytesForStreamFrames) {
-  const size_t overhead = GetPacketHeaderOverhead() + GetEncryptionOverhead();
+  const size_t overhead = GetPacketHeaderOverhead(NOT_IN_FEC_GROUP)
+                          + GetEncryptionOverhead();
   for (size_t i = overhead; i < overhead + 100; ++i) {
     creator_.options()->max_packet_length = i;
-    const bool should_have_room = i > overhead + GetStreamFrameOverhead();
+    const bool should_have_room = i > overhead + GetStreamFrameOverhead(
+        NOT_IN_FEC_GROUP);
     ASSERT_EQ(should_have_room,
               creator_.HasRoomForStreamFrame(kStreamId, kOffset));
     if (should_have_room) {
@@ -358,8 +360,8 @@ TEST_F(QuicPacketCreatorTest, CreateAllFreeBytesForStreamFrames) {
 
 TEST_F(QuicPacketCreatorTest, StreamFrameConsumption) {
   // Compute the total overhead for a single frame in packet.
-  const size_t overhead = GetPacketHeaderOverhead() + GetEncryptionOverhead()
-      + GetStreamFrameOverhead();
+  const size_t overhead = GetPacketHeaderOverhead(NOT_IN_FEC_GROUP)
+      + GetEncryptionOverhead() + GetStreamFrameOverhead(NOT_IN_FEC_GROUP);
   size_t capacity = kDefaultMaxPacketSize - overhead;
   // Now, test various sizes around this size.
   for (int delta = -5; delta <= 5; ++delta) {
@@ -373,6 +375,7 @@ TEST_F(QuicPacketCreatorTest, StreamFrameConsumption) {
     ASSERT_TRUE(creator_.AddSavedFrame(frame));
     // BytesFree() returns bytes available for the next frame, which will
     // be two bytes smaller since the stream frame would need to be grown.
+    EXPECT_EQ(2u, creator_.ExpansionOnNewFrame());
     size_t expected_bytes_free = bytes_free < 3 ? 0 : bytes_free - 2;
     EXPECT_EQ(expected_bytes_free, creator_.BytesFree()) << "delta: " << delta;
     SerializedPacket serialized_packet = creator_.SerializePacket();
@@ -382,10 +385,40 @@ TEST_F(QuicPacketCreatorTest, StreamFrameConsumption) {
   }
 }
 
+TEST_F(QuicPacketCreatorTest, StreamFrameConsumptionInFecProtectedPacket) {
+  // Turn on FEC protection.
+  creator_.options()->max_packets_per_fec_group = 6;
+  // Compute the total overhead for a single frame in packet.
+  const size_t overhead = GetPacketHeaderOverhead(IN_FEC_GROUP)
+      + GetEncryptionOverhead() + GetStreamFrameOverhead(IN_FEC_GROUP);
+  size_t capacity = kDefaultMaxPacketSize - overhead;
+  // Now, test various sizes around this size.
+  for (int delta = -5; delta <= 5; ++delta) {
+    string data(capacity + delta, 'A');
+    size_t bytes_free = delta > 0 ? 0 : 0 - delta;
+    QuicFrame frame;
+    size_t bytes_consumed = creator_.CreateStreamFrame(
+        kStreamId, MakeIOVector(data), kOffset, false, &frame);
+    EXPECT_EQ(capacity - bytes_free, bytes_consumed);
+
+    ASSERT_TRUE(creator_.AddSavedFrame(frame));
+    // BytesFree() returns bytes available for the next frame. Since stream
+    // frame does not grow for FEC protected packets, this should be the same
+    // as bytes_free (bound by 0).
+    EXPECT_EQ(0u, creator_.ExpansionOnNewFrame());
+    size_t expected_bytes_free = bytes_free > 0 ? bytes_free : 0;
+    EXPECT_EQ(expected_bytes_free, creator_.BytesFree()) << "delta: " << delta;
+    SerializedPacket serialized_packet = creator_.SerializePacket();
+    ASSERT_TRUE(serialized_packet.packet);
+    delete serialized_packet.packet;
+    delete serialized_packet.retransmittable_frames;
+  }
+}
+
 TEST_F(QuicPacketCreatorTest, CryptoStreamFramePacketPadding) {
   // Compute the total overhead for a single frame in packet.
-  const size_t overhead = GetPacketHeaderOverhead() + GetEncryptionOverhead()
-      + GetStreamFrameOverhead();
+  const size_t overhead = GetPacketHeaderOverhead(NOT_IN_FEC_GROUP)
+      + GetEncryptionOverhead() + GetStreamFrameOverhead(NOT_IN_FEC_GROUP);
   ASSERT_GT(kMaxPacketSize, overhead);
   size_t capacity = kDefaultMaxPacketSize - overhead;
   // Now, test various sizes around this size.
@@ -417,8 +450,8 @@ TEST_F(QuicPacketCreatorTest, CryptoStreamFramePacketPadding) {
 
 TEST_F(QuicPacketCreatorTest, NonCryptoStreamFramePacketNonPadding) {
   // Compute the total overhead for a single frame in packet.
-  const size_t overhead = GetPacketHeaderOverhead() + GetEncryptionOverhead()
-      + GetStreamFrameOverhead();
+  const size_t overhead = GetPacketHeaderOverhead(NOT_IN_FEC_GROUP)
+      + GetEncryptionOverhead() + GetStreamFrameOverhead(NOT_IN_FEC_GROUP);
   ASSERT_GT(kDefaultMaxPacketSize, overhead);
   size_t capacity = kDefaultMaxPacketSize - overhead;
   // Now, test various sizes around this size.
diff --git a/net/quic/quic_packet_generator_test.cc b/net/quic/quic_packet_generator_test.cc
index 6593a3e..ef123a9 100644
--- a/net/quic/quic_packet_generator_test.cc
+++ b/net/quic/quic_packet_generator_test.cc
@@ -511,8 +511,10 @@ TEST_F(QuicPacketGeneratorTest, ConsumeData_FramesPreviouslyQueued) {
                           NOT_IN_FEC_GROUP) +
       // Add an extra 3 bytes for the payload and 1 byte so BytesFree is larger
       // than the GetMinStreamFrameSize.
-      QuicFramer::GetMinStreamFrameSize(framer_.version(), 1, 0, false) + 3 +
-      QuicFramer::GetMinStreamFrameSize(framer_.version(), 1, 0, true) + 1;
+      QuicFramer::GetMinStreamFrameSize(framer_.version(), 1, 0, false,
+                                        NOT_IN_FEC_GROUP) + 3 +
+      QuicFramer::GetMinStreamFrameSize(framer_.version(), 1, 0, true,
+                                        NOT_IN_FEC_GROUP) + 1;
   delegate_.SetCanWriteAnything();
   {
      InSequence dummy;
diff --git a/net/quic/quic_protocol.cc b/net/quic/quic_protocol.cc
index 05e373c..6caa99c 100644
--- a/net/quic/quic_protocol.cc
+++ b/net/quic/quic_protocol.cc
@@ -366,6 +366,8 @@ ostream& operator<<(ostream& os, const ReceivedPacketInfo& received_info) {
   os << "entropy_hash: " << static_cast<int>(received_info.entropy_hash)
      << " is_truncated: " << received_info.is_truncated
      << " largest_observed: " << received_info.largest_observed
+     << " delta_time_largest_observed: "
+     << received_info.delta_time_largest_observed.ToMicroseconds()
      << " missing_packets: [ ";
   for (SequenceNumberSet::const_iterator it =
            received_info.missing_packets.begin();
diff --git a/net/quic/quic_sent_packet_manager.cc b/net/quic/quic_sent_packet_manager.cc
index eccffbf..5aaa45c 100644
--- a/net/quic/quic_sent_packet_manager.cc
+++ b/net/quic/quic_sent_packet_manager.cc
@@ -649,9 +649,6 @@ const QuicTime QuicSentPacketManager::GetRetransmissionTime() const {
       // TODO(ianswett): When CWND is available, it would be preferable to
       // set the timer based on the earliest retransmittable packet.
       // Base the updated timer on the send time of the last packet.
-      // TODO(ianswett): I believe this is a subtle mis-implementation of tail
-      // loss probe, since GetLastPacketSentTime actually returns the sent time
-      // of the last pending packet which still has retransmittable frames.
       const QuicTime sent_time = unacked_packets_.GetLastPacketSentTime();
       const QuicTime tlp_time = sent_time.Add(GetTailLossProbeDelay());
       // Ensure the tlp timer never gets set to a time in the past.
diff --git a/net/quic/quic_unacked_packet_map.cc b/net/quic/quic_unacked_packet_map.cc
index 46cf2b8..a39d63e 100644
--- a/net/quic/quic_unacked_packet_map.cc
+++ b/net/quic/quic_unacked_packet_map.cc
@@ -255,16 +255,17 @@ const QuicUnackedPacketMap::TransmissionInfo&
 
 QuicTime QuicUnackedPacketMap::GetLastPacketSentTime() const {
   UnackedPacketMap::const_reverse_iterator it = unacked_packets_.rbegin();
-  while (it != unacked_packets_.rend() &&
-         (!it->second.pending ||
-          it->second.retransmittable_frames == NULL)) {
+  while (it != unacked_packets_.rend()) {
+    if (it->second.pending) {
+      LOG_IF(DFATAL, it->second.sent_time == QuicTime::Zero())
+          << "Sent time can never be zero for a pending packet.";
+      return it->second.sent_time;
+    }
     ++it;
   }
-  if (it == unacked_packets_.rend()) {
-    LOG(DFATAL) << "Unable to find sent time.";
-    return QuicTime::Zero();
-  }
-  return it->second.sent_time;
+  LOG(DFATAL) << "Unable to find sent time.  "
+              << "This method is only intended when there are pending packets.";
+  return QuicTime::Zero();
 }
 
 QuicTime QuicUnackedPacketMap::GetFirstPendingPacketSentTime() const {
diff --git a/net/quic/quic_unacked_packet_map.h b/net/quic/quic_unacked_packet_map.h
index a687bb6..552c0d6 100644
--- a/net/quic/quic_unacked_packet_map.h
+++ b/net/quic/quic_unacked_packet_map.h
@@ -16,7 +16,7 @@ namespace net {
 class NET_EXPORT_PRIVATE QuicUnackedPacketMap {
  public:
   struct NET_EXPORT_PRIVATE TransmissionInfo {
-    // Used by STL when assinging into a map.
+    // Used by STL when assigning into a map.
     TransmissionInfo();
 
     // Constructs a Transmission with a new all_tranmissions set
diff --git a/net/quic/test_tools/quic_test_packet_maker.cc b/net/quic/test_tools/quic_test_packet_maker.cc
index f93f5fb..ad14d86 100644
--- a/net/quic/test_tools/quic_test_packet_maker.cc
+++ b/net/quic/test_tools/quic_test_packet_maker.cc
@@ -62,6 +62,7 @@ scoped_ptr<QuicEncryptedPacket> QuicTestPacketMaker::MakeAckAndRstPacket(
   header.fec_group = 0;
 
   QuicAckFrame ack(largest_received, QuicTime::Zero(), least_unacked);
+  ack.received_info.delta_time_largest_observed = QuicTime::Delta::Zero();
   QuicFrames frames;
   frames.push_back(QuicFrame(&ack));
   QuicCongestionFeedbackFrame feedback;
@@ -122,6 +123,7 @@ scoped_ptr<QuicEncryptedPacket> QuicTestPacketMaker::MakeAckPacket(
   header.fec_group = 0;
 
   QuicAckFrame ack(largest_received, QuicTime::Zero(), least_unacked);
+  ack.received_info.delta_time_largest_observed = QuicTime::Delta::Zero();
 
   QuicCongestionFeedbackFrame feedback;
   feedback.type = kTCP;
diff --git a/net/tools/quic/test_tools/quic_test_client.cc b/net/tools/quic/test_tools/quic_test_client.cc
index acc7ad7..9a41ea0 100644
--- a/net/tools/quic/test_tools/quic_test_client.cc
+++ b/net/tools/quic/test_tools/quic_test_client.cc
@@ -202,6 +202,12 @@ QuicTestClient::QuicTestClient(
 QuicTestClient::QuicTestClient() {
 }
 
+QuicTestClient::~QuicTestClient() {
+  if (stream_) {
+    stream_->set_visitor(NULL);
+  }
+}
+
 void QuicTestClient::Initialize(bool secure) {
   priority_ = 3;
   connect_attempted_ = false;
@@ -213,12 +219,6 @@ void QuicTestClient::Initialize(bool secure) {
   ExpectCertificates(secure_);
 }
 
-QuicTestClient::~QuicTestClient() {
-  if (stream_) {
-    stream_->set_visitor(NULL);
-  }
-}
-
 void QuicTestClient::ExpectCertificates(bool on) {
   if (on) {
     proof_verifier_ = new RecordingProofVerifier;
@@ -514,23 +514,29 @@ ssize_t QuicTestClient::SendAndWaitForResponse(const void *buffer,
   LOG(DFATAL) << "Not implemented";
   return 0;
 }
+
 void QuicTestClient::Bind(IPEndPoint* local_address) {
   DLOG(WARNING) << "Bind will be done during connect";
 }
+
 string QuicTestClient::SerializeMessage(const HTTPMessage& message) {
   LOG(DFATAL) << "Not implemented";
   return "";
 }
+
 IPAddressNumber QuicTestClient::bind_to_address() const {
   return client_->bind_to_address();
 }
+
 void QuicTestClient::set_bind_to_address(IPAddressNumber address) {
   client_->set_bind_to_address(address);
 }
+
 const IPEndPoint& QuicTestClient::address() const {
   LOG(DFATAL) << "Not implemented";
   return client_->server_address();
 }
+
 size_t QuicTestClient::requests_sent() const {
   LOG(DFATAL) << "Not implemented";
   return 0;
diff --git a/net/tools/quic/test_tools/quic_test_client.h b/net/tools/quic/test_tools/quic_test_client.h
index d9cc672..616c463 100644
--- a/net/tools/quic/test_tools/quic_test_client.h
+++ b/net/tools/quic/test_tools/quic_test_client.h
@@ -20,7 +20,6 @@
 namespace net {
 
 class ProofVerifier;
-class QuicServerId;
 
 namespace tools {