New issue
Advanced search Search tips

Issue 679123 link

Starred by 2 users
Status: Fixed
Owner:
tbansal@chromium.org
Closed: Feb 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 343579



Sign in to add a comment

QuicHttpStream::GetTotalReceivedBytes() includes transport layer retransmissions

Project Member Reported by tbansal@chromium.org, Jan 7 2017 
QuicHttpStream::GetTotalReceivedBytes() includes duplicate data received. Chrome may receive duplicate data if the sender retransmits the same data (e.g., if the sender believes that the data sent previously was lost).

This is in contrast with other implementations of GetTotalReceivedBytes() which do not include byte counts from the transport layer.

Since data saver approximates the bytes used by calling QuicHttpStream::GetTotalReceivedBytes(), it sometimes results in data saver computing the bytes used to be more than the original byte size since the former include retransmissions at the transport layer.

QUIC's implementation of GetTotalReceivedBytes()  should be fixed so as to exclude transport layer's retransmissions.

Comment 1 by bengr@chromium.org, Jan 9 2017 

I'm not sure that's what we want to do. From a network layering perspective, what you say makes sense. However, from the perspective of reporting all bytes except kernel overheads, it doesn't. 

Is there a specific feature that suffers from the current manner of reporting? If not, I think this is WAI.

Comment 2 by tbansal@chromium.org, Jan 9 2017 

Yes, if data saver is using QUIC, then we would report lower savings since the request.GetTotalReceivedBytes() would include retransmissions while original content length would not.

DRP uses request.GetTotalReceivedBytes() to get the count of bytes that were used on the wire. (See https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc?rcl=1483964700&l=377).  request.GetTotalReceivedBytes() ultimately calls QuicHttpStream::GetTotalReceivedBytes() which also includes duplicate bytes in the returned value.

For QUIC, this includes retransmissions while the original content length still does not include retransmissions, which makes for an unfair comparison.

Comment 3 by mmenke@chromium.org, Jan 9 2017

Components: Internals>Network>QUIC
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 7 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c097fbdf154063ede201dd70561fec8974eeb227

commit c097fbdf154063ede201dd70561fec8974eeb227
Author: tbansal <tbansal@chromium.org>
Date: Tue Feb 07 19:23:50 2017

QUIC: Do not count the duplicate received bytes in GetTotalReceivedBytes

This CL changes QuicHttpStream::GetTotalReceivedBytes to not count
the duplicate received bytes. This ensures that GetTotalReceivedBytes
for QUIC maintains parity with other implementations of
GetTotalReceivedBytes that do not include duplicate bytes.

BUG= 679123 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

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

[modify] https://crrev.com/c097fbdf154063ede201dd70561fec8974eeb227/net/quic/chromium/quic_chromium_client_stream.h
[modify] https://crrev.com/c097fbdf154063ede201dd70561fec8974eeb227/net/quic/chromium/quic_http_stream.cc

Comment 5 by bengr@chromium.org, Feb 13 2017

Owner: tbansal@chromium.org
Status: Assigned (was: Untriaged)

Comment 6 by tbansal@chromium.org, Feb 13 2017

Status: Fixed (was: Assigned)
Fixed in CL in comment #4.

Comment 7 by tbansal@chromium.org, Feb 13 2017

Labels: M-58

Comment 8 by tbansal@chromium.org, Feb 22 2017

Blocking: 343579
►

Sign in to add a comment