New issue
Advanced search Search tips

Issue 806949 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Network Quality Estimator may use the incorrect cached estimate

Project Member Reported by tbansal@chromium.org, Jan 29 2018

Issue description

Recently, logic was added to network quality estimator to skip disk caching if the estimated quality matches the typical quality of the network (https://chromium-review.googlesource.com/c/chromium/src/+/809781). Recently, another feature was added where the persisted quality was indexed by the signal strength (https://chromium-review.googlesource.com/c/785554/).

However, these two changes do not behave well when combined. Consider the scenario where a 4G network has network qualities of (i) ECT 2G when signal strength is 1 unit; and, (ii) ECT 4G when signal strength is 4 units. In this case, we would write the NQ to disk in case (i), but skip in (ii). 

Now, when the device is again on the same network, it will use ECT of 2G as the best estimate even if the signal strength is 4  units (since that's the only estimate available for that network). This is incorrect since the NQ is actually ECT 4G.

To fix this, we should persist NQ to disk even if the estimated quality matches the typical quality of the network.
 
Labels: Merge-Request-65
Requesting merge for CL in #1.
Project Member

Comment 3 by sheriffbot@chromium.org, Feb 2 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

Comment 4 by bugdroid1@chromium.org, Feb 2 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ccc12cb166ca114a7f0b35e291e6ca5c065e2840

commit ccc12cb166ca114a7f0b35e291e6ca5c065e2840
Author: Tarun Bansal <tbansal@chromium.org>
Date: Fri Feb 02 21:40:33 2018

Remove the logic for skipping the persistence of network
quality when it matches the typical quality of the network

Bug:  806949 
Change-Id: I02c8db315200e7ab28f72a0627015be15d8cfc4e
Reviewed-on: https://chromium-review.googlesource.com/890859
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#532636}(cherry picked from commit 6b1d4918f33619719fa2360ada662f1d6e97070c)
Reviewed-on: https://chromium-review.googlesource.com/900143
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#271}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/ccc12cb166ca114a7f0b35e291e6ca5c065e2840/net/nqe/network_qualities_prefs_manager.cc
[modify] https://crrev.com/ccc12cb166ca114a7f0b35e291e6ca5c065e2840/net/nqe/network_qualities_prefs_manager_unittest.cc
[modify] https://crrev.com/ccc12cb166ca114a7f0b35e291e6ca5c065e2840/net/nqe/network_quality_estimator_params.cc
[modify] https://crrev.com/ccc12cb166ca114a7f0b35e291e6ca5c065e2840/net/nqe/network_quality_estimator_params.h
[modify] https://crrev.com/ccc12cb166ca114a7f0b35e291e6ca5c065e2840/net/nqe/network_quality_estimator_params_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment