New issue
Advanced search Search tips

Issue 798568 link

Starred by 1 user
Status: Fixed
Owner:
tbansal@chromium.org
Closed: Jan 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Clear network Quality pref if the observed quality matches the expected value

Project Member Reported by tbansal@chromium.org, Jan 2 2018 
Currently, if the observed network quality matches the expected value for the current connection type, then the network quality value is not persisted to the disk. This was an optimization to reduce the size of the prefs.

However, it is possible that the current network has a persisted ECT which is different from the expected value. In that case, when the prefs are read back at the next startup, the prefs manager will read the old ECT value because of this optimization. 

To avoid this, we should clear the network quality pref for the current network if the observed quality matches the expected value.
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 3 2018 

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

commit b7a3516cac7d736b8dcabd95e4fdd6112fada45d
Author: Tarun Bansal <tbansal@chromium.org>
Date: Wed Jan 03 04:47:50 2018

Clear the network quality pref if the observed quality matches the expected value

Also increase the network quality store size from 10 to 20 to match
the maximum prefs size.

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Bug:  798568 
Change-Id: I5c0957adbd201e3ef52102032ec5c0f207132367
Reviewed-on: https://chromium-review.googlesource.com/846252
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526620}
[modify] https://crrev.com/b7a3516cac7d736b8dcabd95e4fdd6112fada45d/net/nqe/network_qualities_prefs_manager.cc
[modify] https://crrev.com/b7a3516cac7d736b8dcabd95e4fdd6112fada45d/net/nqe/network_qualities_prefs_manager_unittest.cc
[modify] https://crrev.com/b7a3516cac7d736b8dcabd95e4fdd6112fada45d/net/nqe/network_quality_store.cc
[modify] https://crrev.com/b7a3516cac7d736b8dcabd95e4fdd6112fada45d/net/nqe/network_quality_store.h
[modify] https://crrev.com/b7a3516cac7d736b8dcabd95e4fdd6112fada45d/net/nqe/network_quality_store_unittest.cc

Comment 2 by tbansal@chromium.org, Jan 3 2018

Labels: -Pri-3 Pri-1

Comment 3 by tbansal@chromium.org, Jan 8 2018

Labels: Merge-Request-64
Requesting merge for CL in #1. It's a pretty safe CL, and well covered by tests. Thanks.
Project Member

Comment 4 by sheriffbot@chromium.org, Jan 8 2018

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: M64 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by abdulsyed@google.com, Jan 8 2018 

Thanks tbansal@ for the fix. Can you please confirm why this is urgent for M64 vs waiting until M65?

Comment 6 by tbansal@chromium.org, Jan 8 2018 

This bug causes network quality estimator to compute the network quality incorrectly at the time of Chrome startup. This leads to incorrect results in NetInfo.Connection.EffectiveType and NetInfo.Connection.RTT JavaScript APIs which is in use by developers.

Comment 7 by abdulsyed@google.com, Jan 9 2018 

 Can you please confirm if the fix has been manually verified in canary/dev?

Comment 8 Deleted

Comment 9 by tbansal@chromium.org, Jan 9 2018 

Yes, I have verified that the fix has been working for Canary.

Comment 10 by abdulsyed@chromium.org, Jan 9 2018

Labels: -Merge-Review-64 Merge-Approved-64
Approving merge. Branch:3282

Comment 11 by tbansal@chromium.org, Jan 10 2018

Labels: -M-64 -Merge-Approved-64 M-65
Status: Fixed (was: Started)
I decided not to merge this back since there are few merge conflicts.
►

Sign in to add a comment