Fixes bugs in packet size computation in the creator and in the framer

net/quic/quic_packet_creator.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/quic_packet_creator.h"
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "net/quic/crypto/quic_random.h"
 #include "net/quic/quic_ack_notifier.h"
@@ skipping 119 matching lines @@
   return options_.max_packets_per_fec_group;
 }
 
 void QuicPacketCreator::set_max_packets_per_fec_group(
     size_t max_packets_per_fec_group) {
   // To turn off FEC protection, use StopFecProtectingPackets().
   DCHECK_NE(0u, max_packets_per_fec_group);
   options_.max_packets_per_fec_group = max_packets_per_fec_group;
 }
 
-InFecGroup QuicPacketCreator::MaybeStartFec() {
-  if (should_fec_protect_ && fec_group_.get() == NULL) {
-    DCHECK(queued_frames_.empty());
-    // Set the fec group number to the sequence number of the next packet.
-    fec_group_number_ = sequence_number() + 1;
-    fec_group_.reset(new QuicFecGroup());
+InFecGroup QuicPacketCreator::MaybeUpdateLengthsAndStartFec() {
+  if (fec_group_.get() != NULL) {
+    // Don't update any lengths when an FEC group is open, to ensure same
+    // packet header size in all packets within a group.
+    return IN_FEC_GROUP;
   }
-  return fec_group_.get() == NULL ? NOT_IN_FEC_GROUP : IN_FEC_GROUP;
+  if (!queued_frames_.empty()) {
+    // Don't change creator state if there are frames queued.
+    return fec_group_.get() == NULL ? NOT_IN_FEC_GROUP : IN_FEC_GROUP;
+  }
+  // TODO(jri): Add max_packet_length and send_connection_id_length here too.
+  sequence_number_length_ = options_.send_sequence_number_length;
+
+  if (!should_fec_protect_) {
+    return NOT_IN_FEC_GROUP;
+  }
+  // Start a new FEC group since protection is on. Set the fec group number to
+  // the sequence number of the next packet.
+  fec_group_number_ = sequence_number() + 1;
+  fec_group_.reset(new QuicFecGroup());
+  return IN_FEC_GROUP;
 }
 
 // Stops serializing version of the protocol in packets sent after this call.
 // A packet that is already open might send kQuicVersionSize bytes less than the
 // maximum packet size if we stop sending version before it is serialized.
 void QuicPacketCreator::StopSendingVersion() {
   DCHECK(send_version_in_packet_);
   send_version_in_packet_ = false;
   if (packet_size_ > 0) {
     DCHECK_LT(kQuicVersionSize, packet_size_);
@@ skipping 44 matching lines @@
 size_t QuicPacketCreator::CreateStreamFrame(QuicStreamId id,
                                             const IOVector& data,
                                             QuicStreamOffset offset,
                                             bool fin,
                                             QuicFrame* frame) {
   DCHECK_GT(options_.max_packet_length,
             StreamFramePacketOverhead(
                 framer_->version(), PACKET_8BYTE_CONNECTION_ID, kIncludeVersion,
                 PACKET_6BYTE_SEQUENCE_NUMBER, IN_FEC_GROUP));
 
-  InFecGroup is_in_fec_group = MaybeStartFec();
+  InFecGroup is_in_fec_group = MaybeUpdateLengthsAndStartFec();
 
   LOG_IF(DFATAL, !HasRoomForStreamFrame(id, offset))
       << "No room for Stream frame, BytesFree: " << BytesFree()
       << " MinStreamFrameSize: "
       << QuicFramer::GetMinStreamFrameSize(
           framer_->version(), id, offset, true, is_in_fec_group);
 
   if (data.Empty()) {
     LOG_IF(DFATAL, !fin)
         << "Creating a stream frame with no data or fin.";
@@ skipping 124 matching lines @@
       << "Attempt to serialize empty packet";
   DCHECK_GE(sequence_number_ + 1, fec_group_number_);
   QuicPacketHeader header;
   FillPacketHeader(should_fec_protect_ ? fec_group_number_ : 0, false, &header);
 
   MaybeAddPadding();
 
   size_t max_plaintext_size =
       framer_->GetMaxPlaintextSize(options_.max_packet_length);
   DCHECK_GE(max_plaintext_size, packet_size_);
-  // ACK and CONNECTION_CLOSE Frames will be truncated only if they're
-  // the first frame in the packet.  If truncation is to occur, then
-  // GetSerializedFrameLength will have returned all bytes free.
-  bool possibly_truncated =
-      packet_size_ != max_plaintext_size ||
-      queued_frames_.size() != 1 ||
-      (queued_frames_.back().type == ACK_FRAME ||
-       queued_frames_.back().type == CONNECTION_CLOSE_FRAME);
+  // ACK Frames will be truncated only if they're the only frame in the packet,
+  // and if packet_size_ was set to max_plaintext_size. If truncation occurred,
+  // then GetSerializedFrameLength will have returned all bytes free.
+  bool possibly_truncated = packet_size_ == max_plaintext_size &&
+                            queued_frames_.size() == 1 &&
+                            queued_frames_.back().type == ACK_FRAME;
   SerializedPacket serialized =
       framer_->BuildDataPacket(header, queued_frames_, packet_size_);
   LOG_IF(DFATAL, !serialized.packet)
       << "Failed to serialize " << queued_frames_.size() << " frames.";
   // Because of possible truncation, we can't be confident that our
   // packet size calculation worked correctly.
-  if (!possibly_truncated)
+  if (!possibly_truncated) {
     DCHECK_EQ(packet_size_, serialized.packet->length());
+  }
 
   packet_size_ = 0;
   queued_frames_.clear();
   serialized.retransmittable_frames = queued_retransmittable_frames_.release();
   return serialized;
 }
 
 SerializedPacket QuicPacketCreator::SerializeFec() {
   if (fec_group_.get() == NULL || fec_group_->NumReceivedPackets() <= 0) {
     LOG(DFATAL) << "SerializeFEC called but no group or zero packets in group.";
@@ skipping 60 matching lines @@
     case STOP_WAITING_FRAME:
       return false;
     default:
       return true;
   }
 }
 
 bool QuicPacketCreator::AddFrame(const QuicFrame& frame,
                                  bool save_retransmittable_frames) {
   DVLOG(1) << "Adding frame: " << frame;
-  InFecGroup is_in_fec_group = MaybeStartFec();
+  InFecGroup is_in_fec_group = MaybeUpdateLengthsAndStartFec();
+
   size_t frame_len = framer_->GetSerializedFrameLength(
       frame, BytesFree(), queued_frames_.empty(), true, is_in_fec_group,
-      options()->send_sequence_number_length);
+      sequence_number_length_);
   if (frame_len == 0) {
     return false;
   }
   DCHECK_LT(0u, packet_size_);
   packet_size_ += ExpansionOnNewFrame() + frame_len;
 
   if (save_retransmittable_frames && ShouldRetransmit(frame)) {
     if (queued_retransmittable_frames_.get() == NULL) {
       queued_retransmittable_frames_.reset(new RetransmittableFrames());
     }
@@ skipping 29 matching lines @@
   if (!is_handshake) {
     return;
   }
 
   QuicPaddingFrame padding;
   bool success = AddFrame(QuicFrame(&padding), false);
   DCHECK(success);
 }
 
 }  // namespace net