diff options
24 files changed, 372 insertions, 201 deletions
diff --git a/net/quic/crypto/null_decrypter_test.cc b/net/quic/crypto/null_decrypter_test.cc index 71f1970..ad73cea 100644 --- a/net/quic/crypto/null_decrypter_test.cc +++ b/net/quic/crypto/null_decrypter_test.cc @@ -8,7 +8,6 @@ using base::StringPiece; namespace net { - namespace test { TEST(NullDecrypterTest, Decrypt) { @@ -67,5 +66,4 @@ TEST(NullDecrypterTest, ShortInput) { } } // namespace test - } // namespace net diff --git a/net/quic/crypto/null_encrypter_test.cc b/net/quic/crypto/null_encrypter_test.cc index f027af4..9f2cec2 100644 --- a/net/quic/crypto/null_encrypter_test.cc +++ b/net/quic/crypto/null_encrypter_test.cc @@ -8,7 +8,6 @@ using base::StringPiece; namespace net { - namespace test { TEST(NullEncrypterTest, Encrypt) { @@ -46,5 +45,4 @@ TEST(NullEncrypterTest, GetCiphertextSize) { } } // namespace test - } // namespace net diff --git a/net/quic/crypto/quic_random_test.cc b/net/quic/crypto/quic_random_test.cc index 655ae98..425089a 100644 --- a/net/quic/crypto/quic_random_test.cc +++ b/net/quic/crypto/quic_random_test.cc @@ -7,7 +7,6 @@ #include "testing/gtest/include/gtest/gtest.h" namespace net { - namespace test { TEST(QuicRandomTest, RandBytes) { @@ -38,5 +37,4 @@ TEST(QuicRandomTest, Reseed) { } } // namespace test - } // namespace net diff --git a/net/quic/quic_clock_test.cc b/net/quic/quic_clock_test.cc index 839f011c..00323ba 100644 --- a/net/quic/quic_clock_test.cc +++ b/net/quic/quic_clock_test.cc @@ -7,7 +7,6 @@ #include "testing/gtest/include/gtest/gtest.h" namespace net { - namespace test { TEST(QuicClockTest, Now) { @@ -22,5 +21,4 @@ TEST(QuicClockTest, Now) { } } // namespace test - } // namespace net diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc index e77369a..3bda17d 100644 --- a/net/quic/quic_connection.cc +++ b/net/quic/quic_connection.cc @@ -24,9 +24,15 @@ using std::set; namespace net { +// TODO(pwestin): kDefaultTimeoutUs is in int64. +int32 kNegotiatedTimeoutUs = kDefaultTimeoutUs; + +// The largest gap in packets we'll accept without closing the connection. +// This will likely have to be tuned. +const QuicPacketSequenceNumber kMaxPacketGap = 5000; + // The maximum number of nacks which can be transmitted in a single ack packet -// without exceeding kMaxPacketSize. Due to nacks, this also sets a limit on -// the sequence number gap for two consecutive packets. +// without exceeding kMaxPacketSize. const QuicPacketSequenceNumber kMaxUnackedPackets = 192u; // The amount of time we wait before resending a packet. @@ -53,7 +59,7 @@ const int kMaxPacketsToSerializeAtOnce = 6; bool Near(QuicPacketSequenceNumber a, QuicPacketSequenceNumber b) { QuicPacketSequenceNumber delta = (a > b) ? a - b : b - a; - return delta <= kMaxUnackedPackets; + return delta <= kMaxPacketGap; } QuicConnection::QuicConnection(QuicGuid guid, @@ -125,9 +131,9 @@ void QuicConnection::OnRevivedPacket() { bool QuicConnection::OnPacketHeader(const QuicPacketHeader& header) { if (!Near(header.packet_sequence_number, last_header_.packet_sequence_number)) { - DLOG(INFO) << "Packet out of bounds. Discarding"; - // TODO(ianswett): Deal with this by truncating the ack packet instead of - // discarding the packet entirely. + DLOG(INFO) << "Packet " << header.packet_sequence_number + << " out of bounds. Discarding"; + // TODO(alyssar) close the connection entirely. return false; } @@ -203,7 +209,7 @@ bool QuicConnection::ValidateAckFrame(const QuicAckFrame& incoming_ack) { // We can't have too many unacked packets, or our ack frames go over // kMaxPacketSize. - DCHECK_LT(incoming_ack.received_info.missing_packets.size(), + DCHECK_LE(incoming_ack.received_info.missing_packets.size(), kMaxUnackedPackets); if (incoming_ack.sent_info.least_unacked < least_packet_awaiting_ack_) { @@ -223,17 +229,6 @@ bool QuicConnection::ValidateAckFrame(const QuicAckFrame& incoming_ack) { return false; } - for (SequenceSet::const_iterator iter = - incoming_ack.received_info.missing_packets.begin(); - iter != incoming_ack.received_info.missing_packets.end(); ++iter) { - if (*iter >= incoming_ack.received_info.largest_received) { - DLOG(INFO) << "Cannot nack a packet:" << *iter - << " greater than or equal to largest received:" - << incoming_ack.received_info.largest_received; - return false; - } - } - return true; } @@ -514,7 +509,7 @@ void QuicConnection::MaybeResendPacket( QuicPacketSequenceNumber new_sequence_number = packet_creator_.SetNewSequenceNumber(packet); DVLOG(1) << "Resending unacked packet " << sequence_number << " as " - << new_sequence_number; + << new_sequence_number; // Clear the FEC group. framer_.WriteFecGroup(0u, packet); unacked_packets_.insert(make_pair(new_sequence_number, @@ -539,10 +534,6 @@ bool QuicConnection::SendPacket(QuicPacketSequenceNumber sequence_number, bool should_resend, bool force, bool is_retransmit) { - DCHECK(packet->length() < kMaxPacketSize) - << "Packet " << sequence_number << " will not be read; too large: " - << packet->length() << " " << outgoing_ack_; - // If this packet is being forced, don't bother checking to see if we should // write, just write. if (!force) { @@ -578,6 +569,10 @@ bool QuicConnection::SendPacket(QuicPacketSequenceNumber sequence_number, DLOG(INFO) << "Sending packet : " << (should_resend ? "data bearing " : " ack only ") << "packet " << sequence_number; + DCHECK(encrypted->length() <= kMaxPacketSize) + << "Packet " << sequence_number << " will not be read; too large: " + << packet->length() << " " << encrypted->length() << " " << outgoing_ack_; + int rv = helper_->WritePacketToWire(*encrypted, &error); if (rv == -1) { if (error == ERR_IO_PENDING) { @@ -668,7 +663,6 @@ void QuicConnection::SendConnectionClose(QuicErrorCode error) { QuicConnectionCloseFrame frame; frame.error_code = error; frame.ack_frame = outgoing_ack_; - LOG(INFO) << "outgoing_ack_: " << outgoing_ack_; PacketPair packetpair = packet_creator_.CloseConnection(&frame); // There's no point in resending this: we're closing the connection. diff --git a/net/quic/quic_connection.h b/net/quic/quic_connection.h index f98b4e2..761e887 100644 --- a/net/quic/quic_connection.h +++ b/net/quic/quic_connection.h @@ -119,6 +119,7 @@ class NET_EXPORT_PRIVATE QuicConnection : public QuicFramerVisitorInterface { virtual ~QuicConnection(); // Send the data payload to the peer. + // TODO(wtc): document the return value. size_t SendStreamData(QuicStreamId id, base::StringPiece data, QuicStreamOffset offset, @@ -167,6 +168,7 @@ class NET_EXPORT_PRIVATE QuicConnection : public QuicFramerVisitorInterface { const IPEndPoint& self_address() const { return self_address_; } const IPEndPoint& peer_address() const { return peer_address_; } QuicGuid guid() const { return guid_; } + const QuicClock* clock() const { return clock_; } // Updates the internal state concerning which packets have been acked, and // sends an ack if new data frames have been received. @@ -177,8 +179,6 @@ class NET_EXPORT_PRIVATE QuicConnection : public QuicFramerVisitorInterface { // scheduled. void MaybeResendPacket(QuicPacketSequenceNumber sequence_number); - QuicGuid guid() { return guid_; } - QuicPacketCreator::Options* options() { return packet_creator_.options(); } bool connected() { return connected_; } @@ -201,6 +201,7 @@ class NET_EXPORT_PRIVATE QuicConnection : public QuicFramerVisitorInterface { // get an ack. If force is true, then the packet will be sent immediately and // the send scheduler will not be consulted. If is_retransmit is true, this // packet is being retransmitted with a new sequence number. + // TODO(wtc): none of the callers check the return value. virtual bool SendPacket(QuicPacketSequenceNumber number, QuicPacket* packet, bool should_resend, @@ -228,7 +229,6 @@ class NET_EXPORT_PRIVATE QuicConnection : public QuicFramerVisitorInterface { // This finds the new least unacked packet and updates the outgoing ack frame. void UpdateLeastUnacked(QuicPacketSequenceNumber acked_sequence_number); -protected: QuicConnectionHelperInterface* helper() { return helper_; } private: @@ -289,7 +289,7 @@ protected: QuicFramer framer_; const QuicClock* clock_; - QuicGuid guid_; + const QuicGuid guid_; IPEndPoint self_address_; IPEndPoint peer_address_; diff --git a/net/quic/quic_connection_helper_test.cc b/net/quic/quic_connection_helper_test.cc index 95068e0..7528869 100644 --- a/net/quic/quic_connection_helper_test.cc +++ b/net/quic/quic_connection_helper_test.cc @@ -184,11 +184,9 @@ class QuicConnectionHelperTest : public ::testing::Test { const QuicFrame& frame) { QuicFrames frames; frames.push_back(frame); - QuicPacket* packet; - framer_.ConstructFrameDataPacket(header_, frames, &packet); - QuicEncryptedPacket* encrypted = framer_.EncryptPacket(*packet); - delete packet; - return encrypted; + scoped_ptr<QuicPacket> packet( + framer_.ConstructFrameDataPacket(header_, frames)); + return framer_.EncryptPacket(*packet); } QuicGuid guid_; diff --git a/net/quic/quic_connection_test.cc b/net/quic/quic_connection_test.cc index c6700e7..6e14df7 100644 --- a/net/quic/quic_connection_test.cc +++ b/net/quic/quic_connection_test.cc @@ -288,18 +288,17 @@ class QuicConnectionTest : public ::testing::Test { } } fec_data.redundancy = data_packet->FecProtectedData(); - QuicPacket* fec_packet; - framer_.ConstructFecPacket(header_, fec_data, &fec_packet); + scoped_ptr<QuicPacket> fec_packet( + framer_.ConstructFecPacket(header_, fec_data)); scoped_ptr<QuicEncryptedPacket> encrypted( framer_.EncryptPacket(*fec_packet)); connection_.ProcessUdpPacket(IPEndPoint(), IPEndPoint(), *encrypted); - delete fec_packet; } void SendStreamDataToPeer(QuicStreamId id, StringPiece data, - QuicStreamOffset offset, bool fin, - QuicPacketSequenceNumber* last_packet) { + QuicStreamOffset offset, bool fin, + QuicPacketSequenceNumber* last_packet) { EXPECT_CALL(*scheduler_, SentPacket(_, _, _)); connection_.SendStreamData(id, data, offset, fin, last_packet); } @@ -332,8 +331,8 @@ class QuicConnectionTest : public ::testing::Test { QuicFrames frames; QuicFrame frame(&frame1_); frames.push_back(frame); - QuicPacket* packet = NULL; - EXPECT_TRUE(framer_.ConstructFrameDataPacket(header_, frames, &packet)); + QuicPacket* packet = framer_.ConstructFrameDataPacket(header_, frames); + EXPECT_TRUE(packet != NULL); return packet; } @@ -492,7 +491,8 @@ TEST_F(QuicConnectionTest, LeastUnackedGreaterThanPacketSequenceNumber) { ProcessAckPacket(&frame, false); } -TEST_F(QuicConnectionTest, NackSequenceNumberGreaterThanLargestReceived) { +TEST_F(QuicConnectionTest, + DISABLED_NackSequenceNumberGreaterThanLargestReceived) { SendStreamDataToPeer(1, "foo", 0, false, NULL); SendStreamDataToPeer(1, "bar", 3, false, NULL); SendStreamDataToPeer(1, "eep", 6, false, NULL); diff --git a/net/quic/quic_data_writer.cc b/net/quic/quic_data_writer.cc index 42b5dc4..0261d94 100644 --- a/net/quic/quic_data_writer.cc +++ b/net/quic/quic_data_writer.cc @@ -6,6 +6,7 @@ #include <algorithm> #include <limits> +#include <string> #include "base/basictypes.h" #include "base/logging.h" @@ -81,7 +82,7 @@ char* QuicDataWriter::BeginWrite(size_t length) { return buffer_ + length_; } -bool QuicDataWriter::WriteBytes(const void* data, uint32 data_len) { +bool QuicDataWriter::WriteBytes(const void* data, size_t data_len) { char* dest = BeginWrite(data_len); if (!dest) { return false; @@ -123,4 +124,20 @@ void QuicDataWriter::WriteUint128ToBuffer(uint128 value, char* buffer) { WriteUint64ToBuffer(high, buffer + sizeof(low)); } +bool QuicDataWriter::WriteUInt8ToOffset(uint8 value, size_t offset) { + int latched_length = length_; + length_ = offset; + bool success = WriteUInt8(value); + length_ = latched_length; + return success; +} + +bool QuicDataWriter::WriteUInt48ToOffset(uint64 value, size_t offset) { + int latched_length = length_; + length_ = offset; + bool success = WriteUInt48(value); + length_ = latched_length; + return success; +} + } // namespace net diff --git a/net/quic/quic_data_writer.h b/net/quic/quic_data_writer.h index e03bb83..c5ecd34 100644 --- a/net/quic/quic_data_writer.h +++ b/net/quic/quic_data_writer.h @@ -45,7 +45,12 @@ class NET_EXPORT_PRIVATE QuicDataWriter { bool WriteUInt64(uint64 value); bool WriteUInt128(uint128 value); bool WriteStringPiece16(base::StringPiece val); - bool WriteBytes(const void* data, uint32 data_len); + bool WriteBytes(const void* data, size_t data_len); + + // Methods for editing the payload at a specific offset. + // Return true if there is enough space at that offset, false otherwise. + bool WriteUInt8ToOffset(uint8 value, size_t offset); + bool WriteUInt48ToOffset(uint64 value, size_t offset); static void WriteUint8ToBuffer(uint8 value, char* buffer); static void WriteUint16ToBuffer(uint16 value, char* buffer); diff --git a/net/quic/quic_framer.cc b/net/quic/quic_framer.cc index 07693fa..36c0153 100644 --- a/net/quic/quic_framer.cc +++ b/net/quic/quic_framer.cc @@ -17,6 +17,8 @@ using std::numeric_limits; namespace net { +bool kQuicAllowOversizedPacketsForTest = false; + QuicFramer::QuicFramer(QuicDecrypter* decrypter, QuicEncrypter* encrypter) : visitor_(NULL), fec_builder_(NULL), @@ -27,10 +29,18 @@ QuicFramer::QuicFramer(QuicDecrypter* decrypter, QuicEncrypter* encrypter) QuicFramer::~QuicFramer() {} -bool QuicFramer::ConstructFrameDataPacket( +bool CanTruncate(const QuicFrames& frames) { + if (frames.size() == 1 && ( + frames[0].type == ACK_FRAME || + frames[0].type == CONNECTION_CLOSE_FRAME)) { + return true; + } + return false; +} + +QuicPacket* QuicFramer::ConstructFrameDataPacket( const QuicPacketHeader& header, - const QuicFrames& frames, - QuicPacket** packet) { + const QuicFrames& frames) { // Compute the length of the packet. We use "magic numbers" here because // sizeof(member_) is not necessarily the same as sizeof(member_wire_format). size_t len = kPacketHeaderSize; @@ -40,76 +50,92 @@ bool QuicFramer::ConstructFrameDataPacket( len += ComputeFramePayloadLength(frames[i]); } + bool truncating = false; + size_t max_plaintext_size = GetMaxPlaintextSize(kMaxPacketSize); + if (len > max_plaintext_size) { + if (CanTruncate(frames)) { + // Truncate the ack frame so the packet will not exceed kMaxPacketSize. + // Note that we may not use every byte of the writer in this case. + len = max_plaintext_size; + truncating = true; + DLOG(INFO) << "Truncating large ack"; + } else { + return NULL; + } + } + QuicDataWriter writer(len); if (!WritePacketHeader(header, &writer)) { - return false; + return NULL; } // frame count if (frames.size() > 256u) { - return false; + return NULL; } if (!writer.WriteUInt8(frames.size())) { - return false; + return NULL; } for (size_t i = 0; i < frames.size(); ++i) { const QuicFrame& frame = frames[i]; if (!writer.WriteUInt8(frame.type)) { - return false; + return NULL; } switch (frame.type) { case STREAM_FRAME: if (!AppendStreamFramePayload(*frame.stream_frame, &writer)) { - return false; + return NULL; } break; case PDU_FRAME: - return RaiseError(QUIC_INVALID_FRAME_DATA); + RaiseError(QUIC_INVALID_FRAME_DATA); + return NULL; case ACK_FRAME: if (!AppendAckFramePayload(*frame.ack_frame, &writer)) { - return false; + return NULL; } break; case CONGESTION_FEEDBACK_FRAME: if (!AppendQuicCongestionFeedbackFramePayload( *frame.congestion_feedback_frame, &writer)) { - return false; + return NULL; } break; case RST_STREAM_FRAME: if (!AppendRstStreamFramePayload(*frame.rst_stream_frame, &writer)) { - return false; + return NULL; } break; case CONNECTION_CLOSE_FRAME: if (!AppendConnectionCloseFramePayload( *frame.connection_close_frame, &writer)) { - return false; + return NULL; } break; default: - return RaiseError(QUIC_INVALID_FRAME_DATA); + RaiseError(QUIC_INVALID_FRAME_DATA); + return NULL; } } - DCHECK_EQ(len, writer.length()); - *packet = new QuicPacket(writer.take(), len, true, PACKET_FLAGS_NONE); + DCHECK(truncating || len == writer.length()); + QuicPacket* packet = new QuicPacket(writer.take(), len, true, + PACKET_FLAGS_NONE); if (fec_builder_) { fec_builder_->OnBuiltFecProtectedPayload(header, - (*packet)->FecProtectedData()); + packet->FecProtectedData()); } - return true; + return packet; } -bool QuicFramer::ConstructFecPacket(const QuicPacketHeader& header, - const QuicFecData& fec, - QuicPacket** packet) { +QuicPacket* QuicFramer::ConstructFecPacket(const QuicPacketHeader& header, + const QuicFecData& fec) { // Compute the length of the packet. We use "magic numbers" here because // sizeof(member_) is not necessairly the same as sizeof(member_wire_format). size_t len = kPacketHeaderSize; @@ -119,20 +145,18 @@ bool QuicFramer::ConstructFecPacket(const QuicPacketHeader& header, QuicDataWriter writer(len); if (!WritePacketHeader(header, &writer)) { - return false; + return NULL; } if (!writer.WriteUInt48(fec.min_protected_packet_sequence_number)) { - return false; + return NULL; } if (!writer.WriteBytes(fec.redundancy.data(), fec.redundancy.length())) { - return false; + return NULL; } - *packet = new QuicPacket(writer.take(), len, true, PACKET_FLAGS_FEC); - - return true; + return new QuicPacket(writer.take(), len, true, PACKET_FLAGS_FEC); } bool QuicFramer::ProcessPacket(const IPEndPoint& self_address, @@ -372,10 +396,10 @@ bool QuicFramer::ProcessPDUFrame() { } bool QuicFramer::ProcessAckFrame(QuicAckFrame* frame) { - if (!ProcessReceivedInfo(&frame->received_info)) { + if (!ProcessSentInfo(&frame->sent_info)) { return false; } - if (!ProcessSentInfo(&frame->sent_info)) { + if (!ProcessReceivedInfo(&frame->received_info)) { return false; } visitor_->OnAckFrame(*frame); @@ -641,7 +665,7 @@ size_t QuicFramer::ComputeFramePayloadLength(const QuicFrame& frame) { len += 1; // num missing packets len += 6 * ack.received_info.missing_packets.size(); len += 6; // least packet sequence number awaiting an ack - break; + break; } case CONGESTION_FEEDBACK_FRAME: { const QuicCongestionFeedbackFrame& congestion_feedback = @@ -719,31 +743,75 @@ bool QuicFramer::AppendStreamFramePayload( return true; } +// TODO(alyssar): revisit the complexity here to rch's satisfaction +QuicPacketSequenceNumber QuicFramer::CalculateLargestReceived( + const SequenceSet& missing_packets, + SequenceSet::const_iterator largest_written) { + SequenceSet::const_iterator it = largest_written; + QuicPacketSequenceNumber previous_missing = *it; + ++it; + + // Try to find a gap in the missing packets: any gap indicates a non-missing + // packet which we can then return. + for (; it != missing_packets.end(); ++it) { + if (previous_missing + 1 != *it) { + return *it - 1; + } + previous_missing = *it; + } + + // If we've hit the end of the list, and we're not missing any packets, try + // finding a gap between the largest written and the beginning of the set. + it = largest_written++; + previous_missing = *it; + do { + --it; + if (previous_missing - 1 != *it) { + return previous_missing - 1; + } + previous_missing = *it; + } while (it != missing_packets.begin()); + + // The missing packets are entirely contiguous. Return the value of the first + // missing packet - 1, as that must have been seen. + return *missing_packets.begin() - 1; +} + // TODO(ianswett): Use varints or another more compact approach for all deltas. -// TODO(ianswett): Deal with acks which are too large to fit into a packet. bool QuicFramer::AppendAckFramePayload( const QuicAckFrame& frame, QuicDataWriter* writer) { + if (!writer->WriteUInt48(frame.sent_info.least_unacked)) { + return false; + } + + size_t largest_received_offset = writer->length(); if (!writer->WriteUInt48(frame.received_info.largest_received)) { return false; } - DCHECK_GE(numeric_limits<uint8>::max(), - frame.received_info.missing_packets.size()); + // We don't check for overflowing uint8 here, because we only can fit 192 acks + // per packet, so if we overflow we will be truncated. uint8 num_missing_packets = frame.received_info.missing_packets.size(); + size_t num_missing_packets_offset = writer->length(); if (!writer->WriteBytes(&num_missing_packets, 1)) { return false; } SequenceSet::const_iterator it = frame.received_info.missing_packets.begin(); + int num_missing_packets_written = 0; for (; it != frame.received_info.missing_packets.end(); ++it) { if (!writer->WriteUInt48(*it)) { - return false; + // We are truncating. Overwrite largest_received. + QuicPacketSequenceNumber largest_received = + CalculateLargestReceived(frame.received_info.missing_packets, --it); + writer->WriteUInt48ToOffset(largest_received, largest_received_offset); + writer->WriteUInt8ToOffset(num_missing_packets_written, + num_missing_packets_offset); + return true; } - } - - if (!writer->WriteUInt48(frame.sent_info.least_unacked)) { - return false; + ++num_missing_packets_written; + DCHECK_GE(numeric_limits<uint8>::max(), num_missing_packets_written); } return true; diff --git a/net/quic/quic_framer.h b/net/quic/quic_framer.h index 2b58165..d2cc378 100644 --- a/net/quic/quic_framer.h +++ b/net/quic/quic_framer.h @@ -95,6 +95,13 @@ class NET_EXPORT_PRIVATE QuicFramer { virtual ~QuicFramer(); + // Calculates the largest received packet to advertise in the case an Ack + // Frame was truncated. last_written in this case is the iterator for the + // last missing packet which fit in the outgoing ack. + static QuicPacketSequenceNumber CalculateLargestReceived( + const SequenceSet& missing_packets, + SequenceSet::const_iterator last_written); + // Set callbacks to be called from the framer. A visitor must be set, or // else the framer will likely crash. It is acceptable for the visitor // to do nothing. If this is called multiple times, only the last visitor @@ -130,19 +137,15 @@ class NET_EXPORT_PRIVATE QuicFramer { bool ProcessRevivedPacket(const QuicPacketHeader& header, base::StringPiece payload); - // Creates a new QuicPacket populated with the fields in |header| and - // |frames|. Assigns |*packet| to the address of the new object. - // Returns true upon success. - bool ConstructFrameDataPacket(const QuicPacketHeader& header, - const QuicFrames& frames, - QuicPacket** packet); - - // Creates a new QuicPacket populated with the fields in |header| and - // |fec|. Assigns |*packet| to the address of the new object. - // Returns true upon success. - bool ConstructFecPacket(const QuicPacketHeader& header, - const QuicFecData& fec, - QuicPacket** packet); + // Returns a new QuicPacket, owned by the caller, populated with the fields + // in |header| and |frames|, or NULL if the packet could not be created. + QuicPacket* ConstructFrameDataPacket(const QuicPacketHeader& header, + const QuicFrames& frames); + + // Returns a new QuicPacket, owned by the caller, populated with the fields + // in |header| and |fec|, or NULL if the packet could not be created. + QuicPacket* ConstructFecPacket(const QuicPacketHeader& header, + const QuicFecData& fec); void WriteSequenceNumber(QuicPacketSequenceNumber sequence_number, QuicPacket* packet); diff --git a/net/quic/quic_framer_test.cc b/net/quic/quic_framer_test.cc index edb6775..25c3f39 100644 --- a/net/quic/quic_framer_test.cc +++ b/net/quic/quic_framer_test.cc @@ -24,7 +24,6 @@ using std::string; using std::vector; namespace net { - namespace test { class TestEncrypter : public QuicEncrypter { @@ -215,7 +214,8 @@ class QuicFramerTest : public ::testing::Test { return reinterpret_cast<char*>(data); } - void CheckProcessingFails(unsigned char* packet, size_t len, + void CheckProcessingFails(unsigned char* packet, + size_t len, string expected_error, QuicErrorCode error_code) { QuicEncryptedPacket encrypted(AsChars(packet), len, false); @@ -225,6 +225,15 @@ class QuicFramerTest : public ::testing::Test { EXPECT_EQ(error_code, framer_.error()) << "len: " << len; } + void ValidateTruncatedAck(const QuicAckFrame* ack, int keys) { + for (int i = 1; i < keys; ++i) { + EXPECT_TRUE(ContainsKey(ack->received_info.missing_packets, i)) << i; + } + // With no gaps in the missing packets, we can't admit to having received + // anything. + EXPECT_EQ(0u, ack->received_info.largest_received); + } + test::TestEncrypter* encrypter_; test::TestDecrypter* decrypter_; QuicFramer framer_; @@ -547,6 +556,9 @@ TEST_F(QuicFramerTest, AckFrame) { 0x01, // frame type (ack frame) 0x02, + // least packet sequence number awaiting an ack + 0xA0, 0x9A, 0x78, 0x56, + 0x34, 0x12, // largest received packet sequence number 0xBF, 0x9A, 0x78, 0x56, 0x34, 0x12, @@ -555,9 +567,6 @@ TEST_F(QuicFramerTest, AckFrame) { // missing packet 0xBE, 0x9A, 0x78, 0x56, 0x34, 0x12, - // least packet sequence number awaiting an ack - 0xA0, 0x9A, 0x78, 0x56, - 0x34, 0x12, }; QuicEncryptedPacket encrypted(AsChars(packet), arraysize(packet), false); @@ -578,22 +587,21 @@ TEST_F(QuicFramerTest, AckFrame) { EXPECT_EQ(GG_UINT64_C(0x0123456789AA0), frame.sent_info.least_unacked); // Now test framing boundaries - for (size_t i = 0; i < 16; ++i) { + for (size_t i = 0; i < 21; ++i) { string expected_error; if (i < 1) { expected_error = "Unable to read frame count."; } else if (i < 2) { expected_error = "Unable to read frame type."; } else if (i < 8) { + expected_error = "Unable to read least unacked."; + } else if (i < 14) { expected_error = "Unable to read largest received."; - } else if (i < 9) { - expected_error = "Unable to read num missing packets."; } else if (i < 15) { + expected_error = "Unable to read num missing packets."; + } else if (i < 21) { expected_error = "Unable to read sequence number in missing packets."; - } else if (i < 16) { - expected_error = "Unable to read least unacked."; - } - CheckProcessingFails(packet, i + kPacketHeaderSize, expected_error, + } CheckProcessingFails(packet, i + kPacketHeaderSize, expected_error, QUIC_INVALID_FRAME_DATA); } } @@ -945,6 +953,9 @@ TEST_F(QuicFramerTest, ConnectionCloseFrame) { 'n', // Ack frame. + // least packet sequence number awaiting an ack + 0xA0, 0x9A, 0x78, 0x56, + 0x34, 0x12, // largest received packet sequence number 0xBF, 0x9A, 0x78, 0x56, 0x34, 0x12, @@ -953,9 +964,6 @@ TEST_F(QuicFramerTest, ConnectionCloseFrame) { // missing packet 0xBE, 0x9A, 0x78, 0x56, 0x34, 0x12, - // least packet sequence number awaiting an ack - 0xA0, 0x9A, 0x78, 0x56, - 0x34, 0x12, // congestion feedback type (inter arrival) 0x02, // accumulated_number_of_lost_packets @@ -1086,14 +1094,12 @@ TEST_F(QuicFramerTest, ConstructStreamFramePacket) { 'r', 'l', 'd', '!', }; - QuicPacket* data; - ASSERT_TRUE(framer_.ConstructFrameDataPacket(header, frames, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(data != NULL); test::CompareCharArraysWithHexError("constructed packet", data->data(), data->length(), AsChars(packet), arraysize(packet)); - - delete data; } TEST_F(QuicFramerTest, ConstructAckFramePacket) { @@ -1127,6 +1133,9 @@ TEST_F(QuicFramerTest, ConstructAckFramePacket) { 0x01, // frame type (ack frame) 0x02, + // least packet sequence number awaiting an ack + 0xA0, 0x9A, 0x78, 0x56, + 0x34, 0x12, // largest received packet sequence number 0xBF, 0x9A, 0x78, 0x56, 0x34, 0x12, @@ -1135,19 +1144,14 @@ TEST_F(QuicFramerTest, ConstructAckFramePacket) { // missing packet 0xBE, 0x9A, 0x78, 0x56, 0x34, 0x12, - // least packet sequence number awaiting an ack - 0xA0, 0x9A, 0x78, 0x56, - 0x34, 0x12, }; - QuicPacket* data; - ASSERT_TRUE(framer_.ConstructFrameDataPacket(header, frames, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(data != NULL); test::CompareCharArraysWithHexError("constructed packet", data->data(), data->length(), AsChars(packet), arraysize(packet)); - - delete data; } TEST_F(QuicFramerTest, ConstructCongestionFeedbackFramePacketTCP) { @@ -1189,14 +1193,12 @@ TEST_F(QuicFramerTest, ConstructCongestionFeedbackFramePacketTCP) { 0x03, 0x04, }; - QuicPacket* data; - ASSERT_TRUE(framer_.ConstructFrameDataPacket(header, frames, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(data != NULL); test::CompareCharArraysWithHexError("constructed packet", data->data(), data->length(), AsChars(packet), arraysize(packet)); - - delete data; } TEST_F(QuicFramerTest, ConstructCongestionFeedbackFramePacketInterArrival) { @@ -1264,14 +1266,12 @@ TEST_F(QuicFramerTest, ConstructCongestionFeedbackFramePacketInterArrival) { 0x02, 0x00, 0x00, 0x00, }; - QuicPacket* data; - ASSERT_TRUE(framer_.ConstructFrameDataPacket(header, frames, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(data != NULL); test::CompareCharArraysWithHexError("constructed packet", data->data(), data->length(), AsChars(packet), arraysize(packet)); - - delete data; } TEST_F(QuicFramerTest, ConstructCongestionFeedbackFramePacketFixRate) { @@ -1311,14 +1311,12 @@ TEST_F(QuicFramerTest, ConstructCongestionFeedbackFramePacketFixRate) { 0x01, 0x02, 0x03, 0x04, }; - QuicPacket* data; - ASSERT_TRUE(framer_.ConstructFrameDataPacket(header, frames, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(data != NULL); test::CompareCharArraysWithHexError("constructed packet", data->data(), data->length(), AsChars(packet), arraysize(packet)); - - delete data; } TEST_F(QuicFramerTest, ConstructCongestionFeedbackFramePacketInvalidFeedback) { @@ -1335,8 +1333,8 @@ TEST_F(QuicFramerTest, ConstructCongestionFeedbackFramePacketInvalidFeedback) { QuicFrames frames; frames.push_back(QuicFrame(&congestion_feedback_frame)); - QuicPacket* data; - EXPECT_FALSE(framer_.ConstructFrameDataPacket(header, frames, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(data == NULL); } TEST_F(QuicFramerTest, ConstructRstFramePacket) { @@ -1387,14 +1385,12 @@ TEST_F(QuicFramerTest, ConstructRstFramePacket) { QuicFrames frames; frames.push_back(QuicFrame(&rst_frame)); - QuicPacket* data; - ASSERT_TRUE(framer_.ConstructFrameDataPacket(header, frames, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(data != NULL); test::CompareCharArraysWithHexError("constructed packet", data->data(), data->length(), AsChars(packet), arraysize(packet)); - - delete data; } TEST_F(QuicFramerTest, ConstructCloseFramePacket) { @@ -1443,6 +1439,9 @@ TEST_F(QuicFramerTest, ConstructCloseFramePacket) { 'n', // Ack frame. + // least packet sequence number awaiting an ack + 0xA0, 0x9A, 0x78, 0x56, + 0x34, 0x12, // largest received packet sequence number 0xBF, 0x9A, 0x78, 0x56, 0x34, 0x12, @@ -1451,20 +1450,14 @@ TEST_F(QuicFramerTest, ConstructCloseFramePacket) { // missing packet 0xBE, 0x9A, 0x78, 0x56, 0x34, 0x12, - - // least packet sequence number awaiting an ack - 0xA0, 0x9A, 0x78, 0x56, - 0x34, 0x12, }; - QuicPacket* data; - ASSERT_TRUE(framer_.ConstructFrameDataPacket(header, frames, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(data != NULL); test::CompareCharArraysWithHexError("constructed packet", data->data(), data->length(), AsChars(packet), arraysize(packet)); - - delete data; } TEST_F(QuicFramerTest, ConstructFecPacket) { @@ -1501,14 +1494,12 @@ TEST_F(QuicFramerTest, ConstructFecPacket) { 'm', 'n', 'o', 'p', }; - QuicPacket* data; - ASSERT_TRUE(framer_.ConstructFecPacket(header, fec_data, &data)); + scoped_ptr<QuicPacket> data(framer_.ConstructFecPacket(header, fec_data)); + ASSERT_TRUE(data != NULL); test::CompareCharArraysWithHexError("constructed packet", data->data(), data->length(), AsChars(packet), arraysize(packet)); - - delete data; } TEST_F(QuicFramerTest, EncryptPacket) { @@ -1540,6 +1531,106 @@ TEST_F(QuicFramerTest, EncryptPacket) { EXPECT_TRUE(CheckEncryption(StringPiece(AsChars(packet), arraysize(packet)))); } -} // namespace test +TEST_F(QuicFramerTest, CalculateLargestReceived) { + SequenceSet missing; + missing.insert(1); + missing.insert(5); + missing.insert(7); + missing.insert(8); + missing.insert(9); + + // These two we just walk to the next gap, and return the largest seen. + EXPECT_EQ(4u, QuicFramer::CalculateLargestReceived(missing, missing.find(1))); + EXPECT_EQ(6u, QuicFramer::CalculateLargestReceived(missing, missing.find(5))); + + // 7+ are fun because there is no subsequent gap: we have to walk before + // them to find 6. + EXPECT_EQ(6u, QuicFramer::CalculateLargestReceived(missing, missing.find(7))); + EXPECT_EQ(6u, QuicFramer::CalculateLargestReceived(missing, missing.find(8))); + EXPECT_EQ(6u, QuicFramer::CalculateLargestReceived(missing, missing.find(9))); + + // Fill in the gap of 6 to make sure we handle a gap of more than 1 by + // returning 4 rather than 3. + missing.insert(6); + EXPECT_EQ(4u, QuicFramer::CalculateLargestReceived(missing, missing.find(9))); + + // Fill in the rest of the gaps: we should walk to 1 and return 0. + missing.insert(4); + missing.insert(3); + missing.insert(2); + EXPECT_EQ(0u, QuicFramer::CalculateLargestReceived(missing, missing.find(7))); + + // If we add a gap after 7-9, we will return that. + missing.insert(11); + EXPECT_EQ(10u, QuicFramer::CalculateLargestReceived(missing, + missing.find(7))); + EXPECT_EQ(10u, QuicFramer::CalculateLargestReceived(missing, + missing.find(8))); + EXPECT_EQ(10u, QuicFramer::CalculateLargestReceived(missing, + missing.find(9))); +} + +TEST_F(QuicFramerTest, Truncation) { + QuicPacketHeader header; + header.guid = GG_UINT64_C(0xFEDCBA9876543210); + header.packet_sequence_number = GG_UINT64_C(0x123456789ABC); + header.flags = PACKET_FLAGS_NONE; + header.fec_group = 0; + + QuicConnectionCloseFrame close_frame; + QuicAckFrame* ack_frame = &close_frame.ack_frame; + close_frame.error_code = static_cast<QuicErrorCode>(0x05060708); + close_frame.error_details = "because I can"; + ack_frame->received_info.largest_received = 201; + for (uint64 i = 1; i < ack_frame->received_info.largest_received; ++i) { + ack_frame->received_info.missing_packets.insert(i); + } + + // Create a packet with just the ack + QuicFrame frame; + frame.type = ACK_FRAME; + frame.ack_frame = ack_frame; + QuicFrames frames; + frames.push_back(frame); + + scoped_ptr<QuicPacket> raw_ack_packet( + framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(raw_ack_packet != NULL); + + scoped_ptr<QuicEncryptedPacket> ack_packet( + framer_.EncryptPacket(*raw_ack_packet)); + + // Create a packet with just connection close. + frames.clear(); + frame.type = CONNECTION_CLOSE_FRAME; + frame.connection_close_frame = &close_frame; + frames.push_back(frame); + + scoped_ptr<QuicPacket> raw_close_packet( + framer_.ConstructFrameDataPacket(header, frames)); + ASSERT_TRUE(raw_close_packet != NULL); + + scoped_ptr<QuicEncryptedPacket> close_packet( + framer_.EncryptPacket(*raw_close_packet)); + + // Now make sure we can turn our ack packet back into an ack frame + ASSERT_TRUE(framer_.ProcessPacket(self_address_, peer_address_, *ack_packet)); + EXPECT_EQ(QUIC_NO_ERROR, framer_.error()); + ASSERT_TRUE(visitor_.header_.get()); + ASSERT_EQ(peer_address_, visitor_.peer_address_); + ASSERT_EQ(self_address_, visitor_.self_address_); + EXPECT_EQ(1u, visitor_.ack_frames_.size()); + + // And do the same for the close frame. + ASSERT_TRUE(framer_.ProcessPacket(self_address_, peer_address_, + *close_packet)); + EXPECT_EQ(QUIC_NO_ERROR, framer_.error()); + EXPECT_EQ(0x05060708, visitor_.connection_close_frame_.error_code); + EXPECT_EQ("because I can", visitor_.connection_close_frame_.error_details); + ValidateTruncatedAck(visitor_.ack_frames_[0], 190); + ValidateTruncatedAck(&visitor_.connection_close_frame_.ack_frame, 180); +} + +} // namespace test } // namespace net diff --git a/net/quic/quic_http_stream.cc b/net/quic/quic_http_stream.cc index 3ae4c00..4ea35bf 100644 --- a/net/quic/quic_http_stream.cc +++ b/net/quic/quic_http_stream.cc @@ -439,6 +439,8 @@ int QuicHttpStream::ParseResponseHeaders() { } void QuicHttpStream::BufferResponseBody(const char* data, int length) { + if (length == 0) + return; IOBufferWithSize* io_buffer = new IOBufferWithSize(length); memcpy(io_buffer->data(), data, length); response_body_.push_back(make_scoped_refptr(io_buffer)); diff --git a/net/quic/quic_http_stream_test.cc b/net/quic/quic_http_stream_test.cc index 21d631a..517342e 100644 --- a/net/quic/quic_http_stream_test.cc +++ b/net/quic/quic_http_stream_test.cc @@ -225,11 +225,9 @@ class QuicHttpStreamTest : public ::testing::Test { const QuicFrame& frame) { QuicFrames frames; frames.push_back(frame); - QuicPacket* packet; - framer_.ConstructFrameDataPacket(header_, frames, &packet); - QuicEncryptedPacket* encrypted = framer_.EncryptPacket(*packet); - delete packet; - return encrypted; + scoped_ptr<QuicPacket> packet( + framer_.ConstructFrameDataPacket(header_, frames)); + return framer_.EncryptPacket(*packet); } const QuicGuid guid_; diff --git a/net/quic/quic_packet_creator.cc b/net/quic/quic_packet_creator.cc index cbca68a..95642bd 100644 --- a/net/quic/quic_packet_creator.cc +++ b/net/quic/quic_packet_creator.cc @@ -41,7 +41,7 @@ size_t QuicPacketCreator::DataToStream(QuicStreamId id, vector<PacketPair>* packets) { DCHECK_GT(options_.max_packet_length, QuicUtils::StreamFramePacketOverhead(1)); - + DCHECK_LT(0u, options_.max_num_packets); QuicPacketHeader header; QuicPacket* packet = NULL; @@ -49,7 +49,7 @@ size_t QuicPacketCreator::DataToStream(QuicStreamId id, QuicFecGroupNumber current_fec_group = 0; QuicFecData fec_data; - int num_data_packets = options_.max_num_packets; + size_t num_data_packets = options_.max_num_packets; if (options_.use_fec) { DCHECK_LT(1u, options_.max_num_packets); @@ -86,7 +86,8 @@ size_t QuicPacketCreator::DataToStream(QuicStreamId id, unconsumed_bytes -= frame_len; // Produce the data packet (which might fin the stream). - framer_->ConstructFrameDataPacket(header, frames, &packet); + packet = framer_->ConstructFrameDataPacket(header, frames); + DCHECK(packet); DCHECK_GE(options_.max_packet_length, packet->length()); packets->push_back(make_pair(header.packet_sequence_number, packet)); frames.clear(); @@ -102,7 +103,8 @@ size_t QuicPacketCreator::DataToStream(QuicStreamId id, FillPacketHeader(current_fec_group, PACKET_FLAGS_NONE, &header); QuicStreamFrame frame(id, true, offset, ""); frames.push_back(QuicFrame(&frame)); - framer_->ConstructFrameDataPacket(header, frames, &packet); + packet = framer_->ConstructFrameDataPacket(header, frames); + DCHECK(packet); packets->push_back(make_pair(header.packet_sequence_number, packet)); frames.clear(); } @@ -111,8 +113,8 @@ size_t QuicPacketCreator::DataToStream(QuicStreamId id, if (current_fec_group != 0) { FillPacketHeader(current_fec_group, PACKET_FLAGS_FEC, &header); fec_data.redundancy = fec_group_->parity(); - QuicPacket* fec_packet; - framer_->ConstructFecPacket(header, fec_data, &fec_packet); + QuicPacket* fec_packet = framer_->ConstructFecPacket(header, fec_data); + DCHECK(fec_packet); packets->push_back(make_pair(header.packet_sequence_number, fec_packet)); ++fec_group_number_; } @@ -147,10 +149,10 @@ QuicPacketCreator::PacketPair QuicPacketCreator::ResetStream( QuicRstStreamFrame close_frame(id, offset, error); - QuicPacket* packet; QuicFrames frames; frames.push_back(QuicFrame(&close_frame)); - framer_->ConstructFrameDataPacket(header, frames, &packet); + QuicPacket* packet = framer_->ConstructFrameDataPacket(header, frames); + DCHECK(packet); return make_pair(header.packet_sequence_number, packet); } @@ -160,10 +162,10 @@ QuicPacketCreator::PacketPair QuicPacketCreator::CloseConnection( QuicPacketHeader header; FillPacketHeader(0, PACKET_FLAGS_NONE, &header); - QuicPacket* packet; QuicFrames frames; frames.push_back(QuicFrame(close_frame)); - framer_->ConstructFrameDataPacket(header, frames, &packet); + QuicPacket* packet = framer_->ConstructFrameDataPacket(header, frames); + DCHECK(packet); return make_pair(header.packet_sequence_number, packet); } @@ -173,10 +175,10 @@ QuicPacketCreator::PacketPair QuicPacketCreator::AckPacket( QuicPacketHeader header; FillPacketHeader(0, PACKET_FLAGS_NONE, &header); - QuicPacket* packet; QuicFrames frames; frames.push_back(QuicFrame(ack_frame)); - framer_->ConstructFrameDataPacket(header, frames, &packet); + QuicPacket* packet = framer_->ConstructFrameDataPacket(header, frames); + DCHECK(packet); return make_pair(header.packet_sequence_number, packet); } @@ -186,10 +188,10 @@ QuicPacketCreator::PacketPair QuicPacketCreator::CongestionFeedbackPacket( QuicPacketHeader header; FillPacketHeader(0, PACKET_FLAGS_NONE, &header); - QuicPacket* packet; QuicFrames frames; frames.push_back(QuicFrame(feedback_frame)); - framer_->ConstructFrameDataPacket(header, frames, &packet); + QuicPacket* packet = framer_->ConstructFrameDataPacket(header, frames); + DCHECK(packet); return make_pair(header.packet_sequence_number, packet); } diff --git a/net/quic/quic_packet_creator.h b/net/quic/quic_packet_creator.h index 8e57ef8..ff43021 100644 --- a/net/quic/quic_packet_creator.h +++ b/net/quic/quic_packet_creator.h @@ -28,7 +28,7 @@ class NET_EXPORT_PRIVATE QuicPacketCreator : public QuicFecBuilderInterface { void Clear() { memset(this, 0, sizeof(Options)); max_packet_length = kMaxPacketSize; - max_num_packets = std::numeric_limits<int>::max(); + max_num_packets = std::numeric_limits<size_t>::max(); } // TODO(alyssar, rch) max frames/packet @@ -70,7 +70,8 @@ class NET_EXPORT_PRIVATE QuicPacketCreator : public QuicFecBuilderInterface { PacketPair AckPacket(QuicAckFrame* ack_frame); - PacketPair CongestionFeedbackPacket(QuicCongestionFeedbackFrame* ack_frame); + PacketPair CongestionFeedbackPacket( + QuicCongestionFeedbackFrame* feedback_frame); // Increments the current sequence number in QuicPacketCreator and sets it // into the packet and returns the new sequence number. diff --git a/net/quic/quic_protocol.h b/net/quic/quic_protocol.h index b947e19..471b797 100644 --- a/net/quic/quic_protocol.h +++ b/net/quic/quic_protocol.h @@ -241,6 +241,10 @@ struct NET_EXPORT_PRIVATE CongestionFeedbackMessageInterArrival { CongestionFeedbackMessageInterArrival(); ~CongestionFeedbackMessageInterArrival(); uint16 accumulated_number_of_lost_packets; + // TODO(rch): These times should be QuicTime instances. We can write + // them to the wire in the format specified below, but we should avoid + // storing them in memory that way. As such, we should move this comment + // to QuicFramer. int16 offset_time; uint16 delta_time; // delta time is described below. // The set of received packets since the last feedback was sent, along with @@ -307,13 +311,16 @@ struct NET_EXPORT_PRIVATE QuicConnectionCloseFrame { struct NET_EXPORT_PRIVATE QuicFrame { QuicFrame() {} explicit QuicFrame(QuicStreamFrame* stream_frame) - : type(STREAM_FRAME), stream_frame(stream_frame) { + : type(STREAM_FRAME), + stream_frame(stream_frame) { } explicit QuicFrame(QuicAckFrame* frame) - : type(ACK_FRAME), ack_frame(frame) { + : type(ACK_FRAME), + ack_frame(frame) { } explicit QuicFrame(QuicCongestionFeedbackFrame* frame) - : type(CONGESTION_FEEDBACK_FRAME), congestion_feedback_frame(frame) { + : type(CONGESTION_FEEDBACK_FRAME), + congestion_feedback_frame(frame) { } explicit QuicFrame(QuicRstStreamFrame* frame) : type(RST_STREAM_FRAME), diff --git a/net/quic/quic_session.cc b/net/quic/quic_session.cc index 0c04cb7..8e2774a 100644 --- a/net/quic/quic_session.cc +++ b/net/quic/quic_session.cc @@ -150,6 +150,10 @@ ReliableQuicStream* QuicSession::GetStream(const QuicStreamId stream_id) { return it->second; } + if (IsClosedStream(stream_id)) { + return NULL; + } + if (stream_id % 2 == next_stream_id_ % 2) { // We've received a frame for a locally-created stream that is not // currently active. This is an error. diff --git a/net/quic/quic_session.h b/net/quic/quic_session.h index f1375cdb9..6473467 100644 --- a/net/quic/quic_session.h +++ b/net/quic/quic_session.h @@ -124,7 +124,7 @@ class NET_EXPORT_PRIVATE QuicSession : public QuicConnectionVisitorInterface { // of a stream id larger than the next expected stream id. base::hash_set<QuicStreamId> implicitly_created_streams_; - // A list of packets which need to write more data. + // A list of streams which need to write more data. std::list<QuicStreamId> write_blocked_streams_; QuicStreamId largest_peer_created_stream_id_; diff --git a/net/quic/quic_stream_factory_test.cc b/net/quic/quic_stream_factory_test.cc index 53f093d..33f5bbd 100644 --- a/net/quic/quic_stream_factory_test.cc +++ b/net/quic/quic_stream_factory_test.cc @@ -18,7 +18,6 @@ #include "testing/gtest/include/gtest/gtest.h" namespace net { - namespace test { class QuicStreamFactoryTest : public ::testing::Test { @@ -99,11 +98,9 @@ class QuicStreamFactoryTest : public ::testing::Test { QuicEncrypter::Create(kNULL)); QuicFrames frames; frames.push_back(frame); - QuicPacket* packet; - framer.ConstructFrameDataPacket(header, frames, &packet); - QuicEncryptedPacket* encrypted = framer.EncryptPacket(*packet); - delete packet; - return scoped_ptr<QuicEncryptedPacket>(encrypted); + scoped_ptr<QuicPacket> packet( + framer.ConstructFrameDataPacket(header, frames)); + return scoped_ptr<QuicEncryptedPacket>(framer.EncryptPacket(*packet)); } MockHostResolver host_resolver_; @@ -228,5 +225,4 @@ TEST_F(QuicStreamFactoryTest, CancelCreate) { } } // namespace test - } // namespace net diff --git a/net/quic/reliable_quic_stream.cc b/net/quic/reliable_quic_stream.cc index 855fbf2..6b284e6 100644 --- a/net/quic/reliable_quic_stream.cc +++ b/net/quic/reliable_quic_stream.cc @@ -118,8 +118,7 @@ int ReliableQuicStream::WriteOrBuffer(StringPiece data, bool fin) { void ReliableQuicStream::OnCanWrite() { bool fin = false; - bool all_bytes_written = true; - while (all_bytes_written && !queued_data_.empty()) { + while (!queued_data_.empty()) { const string& data = queued_data_.front(); if (queued_data_.size() == 1 && fin_buffered_) { fin = true; @@ -128,9 +127,9 @@ void ReliableQuicStream::OnCanWrite() { if (bytes_written == static_cast<int>(data.size())) { queued_data_.pop_front(); } else { - all_bytes_written = false; queued_data_.front() = string(data.data() + bytes_written, data.length() - bytes_written); + break; } } } diff --git a/net/quic/test_tools/quic_test_utils.cc b/net/quic/test_tools/quic_test_utils.cc index 133f6a6..0971e06 100644 --- a/net/quic/test_tools/quic_test_utils.cc +++ b/net/quic/test_tools/quic_test_utils.cc @@ -11,7 +11,6 @@ using std::min; using std::string; namespace net { - namespace test { MockFramerVisitor::MockFramerVisitor() { @@ -205,11 +204,8 @@ QuicPacket* ConstructHandshakePacket(QuicGuid guid, CryptoTag tag) { QuicFrame frame(&stream_frame); QuicFrames frames; frames.push_back(frame); - QuicPacket* packet; - quic_framer.ConstructFrameDataPacket(header, frames, &packet); - return packet; + return quic_framer.ConstructFrameDataPacket(header, frames); } } // namespace test - } // namespace net diff --git a/net/quic/test_tools/test_task_runner.cc b/net/quic/test_tools/test_task_runner.cc index a2521fa..695e2c4 100644 --- a/net/quic/test_tools/test_task_runner.cc +++ b/net/quic/test_tools/test_task_runner.cc @@ -15,7 +15,6 @@ #include "testing/gtest/include/gtest/gtest.h" namespace net { - namespace test { PostedTask::PostedTask(const tracked_objects::Location& location, @@ -84,5 +83,4 @@ void TestTaskRunner::RunNextTask() { } } // namespace test - } // namespace net |