Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(489)

Issue 157803007: Land Recent QUIC Changes. (Closed)

Created:
6 years, 10 months ago by ramant (doing other things)
Modified:
6 years, 10 months ago
Reviewers:
Ryan Hamilton
CC:
chromium-reviews, cbentzel+watch_chromium.org
Visibility:
Public.

Description

Land Recent QUIC Changes. Add a LOG(DFATAL) if the sequence number length is too small to fit the least_unacked delta in a QUIC ack frame. Merge internal change: 61228351 https://codereview.chromium.org/156233004/ Change the types of the arguments to two methods in QuicReceivedPacketManager to be more specific. No behavior changes. Merge internal change: 61215502 https://codereview.chromium.org/157803006/ Add convenience version() method to QuicServerSessionTest. Merge internal change: 61210235 https://codereview.chromium.org/158173002/ Add convenience version() method to QuicSessionTest. Merge internal change: 61206589 https://codereview.chromium.org/158063003/ Fix bug in QuicSentPacketManager::ClearPreviousRetransmissions where pending packets were abandoned instead of being kept unacked so they could be detected to be lost. Merge internal change: 61197933 https://codereview.chromium.org/158073007/ Add QUIC_VERSION_15 to add a revived_packets set to replace the deprecated accumulated_number_of_lost_packets field. FEC requires a way to communicate that the peer doesn't need a packet retransmitted, without indicating it's been received to the send algorithm. Merge internal change: 61159374 https://codereview.chromium.org/157403006/ Rename previous_transmissions to all_transmission in the QUIC unacked packet map. Change all_transmission to include all transmissions of a packet, even if there is only one, instead of becoming null. Merge internal change: 61144526 https://codereview.chromium.org/158153002/ Use an alternative fix for OneShotVisitor of CryptoFramer that doesn't require a done() method. Detect an old-style Public Reset packet by checking for a sequence number length of 6 in public flags. This requires updating the expected error details messages in PublicResetPacket unit tests. Merge internal change: 61152017 https://codereview.chromium.org/153993015/ Refactor QuicSentPacketManager::MarkPacketHandled to simplify implementation in preparation for breaking out the unacked packet map. Also fixes a minor bug in which a packet which has been marked for retransmission would still be retransmitted if a previous transmission was acked. Merge internal change: 61139074 https://codereview.chromium.org/137923007/ Doing early validation of quic config, to change a potential crash-during-serving to a crash-on-startup. Changing a serving crash to a start-up crash. Not flag protected Merge internal change: 61136961 https://codereview.chromium.org/157383006/ Remove parity tracking in FEC groups as part of the migration to QUIC_VERSION_15, which does not need it. Merge internal change: 61136827 https://codereview.chromium.org/157913003/ Comment out flaky test expectation while the root cause is investigated. Merge internal change: 61104556 https://codereview.chromium.org/135933005/ QUIC - clean up changes - fixed during merge with internal source code. Merge internal change: 61045294 https://codereview.chromium.org/157403005/ Fix problems with absolute value function. The standard library provides seven absolute value functions.  From C, there is abs, labs, llabs, fabs, fabsf, and fabsl for the types int, long, long long, double, float and long double.  Due to numeric conversions, these functions can accept arguments of any numeric types, silently converting between types.  C++ libraries has std::abs, which provides overloads for all these types, making it a better choice.  The following problems have been observed, along with their fixes: 1) The wrong sized absolute value was used.  Using abs for a long long would cause a cast to int first, resulting in unexpected values outside of the range (INT_MIN, INT_MAX).  Switch to using std::abs 2) Using the wrong type of absolute value function.  Passing a floating point number to an integer absolute value will cause rounding of values, especially important for values close to zero.  Switch to using std::abs 3) Taking the absolute value of an unsigned value.  This usually has no effect, but may have some effects from casting.  Fixed by removing the absolute value function. 4) Attempting to find the absolute difference between two unsigned values.  These were transformed from abs(a - b) to (a > b ? a - b : b - a).  This ternary expression is usually stored in another variable. 5) Accidentally capturing the conditional such as abs(a - b < threshold).  Moving the parentheses over to fix. 6) Some static_casts were used to, such as from uint32 -> int64, to get a signed value in the absolute value. 7) A few types were changed from unsigned to signed. Merge internal change: 61013787 https://codereview.chromium.org/136123012/ Pass the whole QuicRstStreamFrame to OnStreamReset. Preparation for dealing with byte_offset updates that arrive in the RST frame. (minor) QUIC OnStreamReset now takes QuicRstStreamFrame as argument. Merge internal change: 60995908 https://codereview.chromium.org/142373006/ R=rch@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249994

Patch Set 1 #

Patch Set 2 : Fix windows compiler error #

Patch Set 3 : win_x64 compiler error fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1244 lines, -412 lines) Patch
M net/quic/congestion_control/cubic.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/quic/congestion_control/cubic_test.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/quic/congestion_control/inter_arrival_overuse_detector.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/quic/congestion_control/tcp_cubic_sender.cc View 1 chunk +2 lines, -1 line 0 comments Download
M net/quic/crypto/crypto_framer.cc View 2 chunks +7 lines, -14 lines 0 comments Download
M net/quic/crypto/strike_register.h View 1 chunk +3 lines, -0 lines 0 comments Download
M net/quic/crypto/strike_register.cc View 2 chunks +8 lines, -4 lines 0 comments Download
M net/quic/quic_connection.cc View 5 chunks +26 lines, -8 lines 0 comments Download
M net/quic/quic_connection_test.cc View 5 chunks +21 lines, -6 lines 0 comments Download
M net/quic/quic_fec_group.h View 4 chunks +1 line, -7 lines 0 comments Download
M net/quic/quic_fec_group.cc View 1 7 chunks +6 lines, -12 lines 0 comments Download
M net/quic/quic_fec_group_test.cc View 1 8 chunks +6 lines, -9 lines 0 comments Download
M net/quic/quic_framer.h View 1 chunk +2 lines, -0 lines 0 comments Download
M net/quic/quic_framer.cc View 13 chunks +105 lines, -31 lines 0 comments Download
M net/quic/quic_framer_test.cc View 18 chunks +746 lines, -63 lines 0 comments Download
M net/quic/quic_packet_creator.h View 1 chunk +0 lines, -1 line 0 comments Download
M net/quic/quic_packet_creator.cc View 5 chunks +6 lines, -15 lines 0 comments Download
M net/quic/quic_protocol.h View 3 chunks +8 lines, -2 lines 0 comments Download
M net/quic/quic_protocol.cc View 3 chunks +9 lines, -0 lines 0 comments Download
M net/quic/quic_received_packet_manager.h View 2 chunks +8 lines, -10 lines 0 comments Download
M net/quic/quic_received_packet_manager.cc View 4 chunks +24 lines, -24 lines 0 comments Download
M net/quic/quic_received_packet_manager_test.cc View 3 chunks +4 lines, -4 lines 0 comments Download
M net/quic/quic_sent_packet_manager.h View 1 2 3 chunks +18 lines, -21 lines 0 comments Download
M net/quic/quic_sent_packet_manager.cc View 13 chunks +130 lines, -145 lines 0 comments Download
M net/quic/quic_sent_packet_manager_test.cc View 5 chunks +70 lines, -5 lines 0 comments Download
M net/quic/quic_session.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/quic/quic_session_test.cc View 8 chunks +12 lines, -10 lines 0 comments Download
M net/quic/reliable_quic_stream.h View 1 chunk +1 line, -1 line 0 comments Download
M net/quic/reliable_quic_stream.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/quic/test_tools/quic_sent_packet_manager_peer.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/tools/quic/end_to_end_test.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/tools/quic/quic_server_session_test.cc View 8 chunks +10 lines, -8 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
ramant (doing other things)
6 years, 10 months ago (2014-02-08 02:47:16 UTC) #1
Ryan Hamilton
lgtm
6 years, 10 months ago (2014-02-08 03:42:53 UTC) #2
ramant (doing other things)
The CQ bit was checked by rtenneti@chromium.org
6 years, 10 months ago (2014-02-09 00:17:24 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/157803007/410001
6 years, 10 months ago (2014-02-09 00:17:31 UTC) #4
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-09 01:52:03 UTC) #5
commit-bot: I haz the power
Retried try job too often on win_x64_rel for step(s) base_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_rel&number=74506
6 years, 10 months ago (2014-02-09 01:52:04 UTC) #6
ramant (doing other things)
The CQ bit was checked by rtenneti@chromium.org
6 years, 10 months ago (2014-02-09 03:43:54 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/157803007/740001
6 years, 10 months ago (2014-02-09 03:44:03 UTC) #8
commit-bot: I haz the power
6 years, 10 months ago (2014-02-09 10:45:48 UTC) #9
Message was sent while issue was closed.
Change committed as 249994

Powered by Google App Engine
This is Rietveld 408576698