New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 764954 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug


Show other hotlists

Hotlists containing this issue:
Geolocation


Sign in to add a comment

Throttle geolocation requests

Project Member Reported by scheib@chromium.org, Sep 13 2017

Issue description

Avoid malfunctioning web apps from causing too many geolocation requests to be made by the user agent.  Too many requests can consume power and or location service quotas.

https://jsbin.com/pigoq is an example page that uses maximumAge=0 to require new location requests and makes far too many requests.

Design questions:
- What quota thresholds should be used?


Googlers, see also:
https://buganizer.corp.google.com/issues/70883821
 
jsbin.pigoq.4.html
3.5 KB View Download
Components: Blink>Geolocation
Components: -Blink>Location
Owner: mattreynolds@chromium.org
Status: Started (was: Available)
Looking into this as part of investigating a geolocation quota issue on Chrome OS devices.

We want to know if:

WifiDataProvider::ScheduleNextScan can be called frequently enough to have multiple polling tasks scheduled at once, perhaps by calling watchPosition many times.

A failed/missing response to the network request (perhaps caused by quota issues) can put Chrome in a bad state.

Comment 4 by scheib@chromium.org, Dec 20 2017

Description: Show this description
Project Member

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

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

commit 29212f31a0213aa687111857a48267f494ca6d20
Author: Matt Reynolds <mattreynolds@google.com>
Date: Wed Jan 03 20:14:32 2018

Move WifiPollingPolicy to a global instance

The WifiPollingPolicy controls how frequently the network location
provider will initiate Wi-Fi scans. Previously, the policy was attached
to the WifiDataProvider, which is destroyed when there are no active
geolocation API calls. Destruction of the provider causes us to lose track
of our polling policy.

The policy allows the first scan to be performed immediately and schedules
subsequent scans at regular intervals. When the provider is destroyed and
recreated rapidly it considers each scan to be "first", which allows many
Wi-Fi scans to be performed in a short window.

This change moves the policy state to a global instance so it can be saved
when the provider is destroyed and recreated. The new policy allows the
first-ever scan to be performed immediately, and subsequent scans may also
be performed immediately if enough time has lapsed since the previous scan.
In all other situations, the configured polling interval is enforced.

BUG= 764954 

Change-Id: I00649dde2a707127e53e0330511172fef4a43471
Reviewed-on: https://chromium-review.googlesource.com/834988
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526782}
[modify] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/BUILD.gn
[modify] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/wifi_data_provider_common_unittest.cc
[add] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/29212f31a0213aa687111857a48267f494ca6d20/device/geolocation/wifi_polling_policy.h

Status: Fixed (was: Started)

Comment 7 by dougt@chromium.org, Jan 4 2018

Labels: -Pri-3 ReleaseBlock-Stable Merge-Request-64 OS-Chrome OS-Linux OS-Mac OS-Windows Pri-1
We should merge to m-64 if at all possible.
Cc: pbomm...@chromium.org abdulsyed@chromium.org manoranj...@chromium.org
Labels: M-64
We are seeing browser crashes on M65 Canary on Mac(Expect to see on Windows once we have the new canary out) due to this CL which is currently getting tracked in issue#798987.

At this point I don't think the CL would be safe to get merged to M64.
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 4 2018

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

commit 1694bd202077ab2bcee0784d0649294518528341
Author: Matt Reynolds <mattreynolds@chromium.org>
Date: Thu Jan 04 19:23:15 2018

Revert "Move WifiPollingPolicy to a global instance"

This reverts commit 29212f31a0213aa687111857a48267f494ca6d20.

Reason for revert: Crashing on Canary builds, see crbug.com/798987

Original change's description:
> Move WifiPollingPolicy to a global instance
> 
> The WifiPollingPolicy controls how frequently the network location
> provider will initiate Wi-Fi scans. Previously, the policy was attached
> to the WifiDataProvider, which is destroyed when there are no active
> geolocation API calls. Destruction of the provider causes us to lose track
> of our polling policy.
> 
> The policy allows the first scan to be performed immediately and schedules
> subsequent scans at regular intervals. When the provider is destroyed and
> recreated rapidly it considers each scan to be "first", which allows many
> Wi-Fi scans to be performed in a short window.
> 
> This change moves the policy state to a global instance so it can be saved
> when the provider is destroyed and recreated. The new policy allows the
> first-ever scan to be performed immediately, and subsequent scans may also
> be performed immediately if enough time has lapsed since the previous scan.
> In all other situations, the configured polling interval is enforced.
> 
> BUG= 764954 
> 
> Change-Id: I00649dde2a707127e53e0330511172fef4a43471
> Reviewed-on: https://chromium-review.googlesource.com/834988
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#526782}

TBR=reillyg@chromium.org,mattreynolds@chromium.org

Change-Id: I25d5596418df55e9de9e0f09e6ba7546b54551a4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  764954 
Reviewed-on: https://chromium-review.googlesource.com/851132
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527058}
[modify] https://crrev.com/1694bd202077ab2bcee0784d0649294518528341/device/geolocation/BUILD.gn
[modify] https://crrev.com/1694bd202077ab2bcee0784d0649294518528341/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/1694bd202077ab2bcee0784d0649294518528341/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/1694bd202077ab2bcee0784d0649294518528341/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/1694bd202077ab2bcee0784d0649294518528341/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/1694bd202077ab2bcee0784d0649294518528341/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/1694bd202077ab2bcee0784d0649294518528341/device/geolocation/wifi_data_provider_common_unittest.cc
[delete] https://crrev.com/b910d51d757bc376356d333812c0dc595f7a7beb/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/1694bd202077ab2bcee0784d0649294518528341/device/geolocation/wifi_polling_policy.h

Status: Assigned (was: Fixed)
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 4 2018

Labels: merge-merged-3311
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2

commit be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2
Author: Matt Reynolds <mattreynolds@chromium.org>
Date: Thu Jan 04 23:56:17 2018

Revert "Move WifiPollingPolicy to a global instance"

This reverts commit 29212f31a0213aa687111857a48267f494ca6d20.

Reason for revert: Crashing on Canary builds, see crbug.com/798987

Original change's description:
> Move WifiPollingPolicy to a global instance
> 
> The WifiPollingPolicy controls how frequently the network location
> provider will initiate Wi-Fi scans. Previously, the policy was attached
> to the WifiDataProvider, which is destroyed when there are no active
> geolocation API calls. Destruction of the provider causes us to lose track
> of our polling policy.
> 
> The policy allows the first scan to be performed immediately and schedules
> subsequent scans at regular intervals. When the provider is destroyed and
> recreated rapidly it considers each scan to be "first", which allows many
> Wi-Fi scans to be performed in a short window.
> 
> This change moves the policy state to a global instance so it can be saved
> when the provider is destroyed and recreated. The new policy allows the
> first-ever scan to be performed immediately, and subsequent scans may also
> be performed immediately if enough time has lapsed since the previous scan.
> In all other situations, the configured polling interval is enforced.
> 
> BUG= 764954 
> 
> Change-Id: I00649dde2a707127e53e0330511172fef4a43471
> Reviewed-on: https://chromium-review.googlesource.com/834988
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#526782}

TBR=reillyg@chromium.org,mattreynolds@chromium.org

Change-Id: I25d5596418df55e9de9e0f09e6ba7546b54551a4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  764954 
Reviewed-on: https://chromium-review.googlesource.com/851132
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#527058}(cherry picked from commit 1694bd202077ab2bcee0784d0649294518528341)
Reviewed-on: https://chromium-review.googlesource.com/851078
Cr-Commit-Position: refs/branch-heads/3311@{#3}
Cr-Branched-From: e475c8f306fab051ba9e7bcba48479691ee6b9b3-refs/heads/master@{#526879}
[modify] https://crrev.com/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2/device/geolocation/BUILD.gn
[modify] https://crrev.com/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2/device/geolocation/wifi_data_provider_common_unittest.cc
[delete] https://crrev.com/30ca4ab78b37dce312166e76570691498b2b6bc1/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/be7ec6a0e5e7086b1acf9ba58e5952fa532db1a2/device/geolocation/wifi_polling_policy.h

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 5 2018

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

commit bd9b9ea0315520281caad474df1fda846e84d6f5
Author: Matt Reynolds <mattreynolds@google.com>
Date: Fri Jan 05 04:38:10 2018

Move WifiPollingPolicy to a global instance

The WifiPollingPolicy controls how frequently the network location
provider will initiate Wi-Fi scans. Previously, the policy was attached
to the WifiDataProvider, which is destroyed when there are no active
geolocation API calls. Destruction of the provider causes us to lose track
of our polling policy.

The policy allows the first scan to be performed immediately and schedules
subsequent scans at regular intervals. When the provider is destroyed and
recreated rapidly it considers each scan to be "first", which allows many
Wi-Fi scans to be performed in a short window.

This change moves the policy state to a global instance so it can be saved
when the provider is destroyed and recreated. The new policy allows the
first-ever scan to be performed immediately, and subsequent scans may also
be performed immediately if enough time has lapsed since the previous scan.
In all other situations, the configured polling interval is enforced.

The global policy instance is leaked at shutdown.

BUG= 764954 

Change-Id: If16c73dfd7d6535346e86d0fab5df24f0c7b297d
Reviewed-on: https://chromium-review.googlesource.com/850444
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527219}
[modify] https://crrev.com/bd9b9ea0315520281caad474df1fda846e84d6f5/device/geolocation/BUILD.gn
[modify] https://crrev.com/bd9b9ea0315520281caad474df1fda846e84d6f5/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/bd9b9ea0315520281caad474df1fda846e84d6f5/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/bd9b9ea0315520281caad474df1fda846e84d6f5/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/bd9b9ea0315520281caad474df1fda846e84d6f5/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/bd9b9ea0315520281caad474df1fda846e84d6f5/device/geolocation/wifi_data_provider_common_unittest.cc
[add] https://crrev.com/bd9b9ea0315520281caad474df1fda846e84d6f5/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/bd9b9ea0315520281caad474df1fda846e84d6f5/device/geolocation/wifi_polling_policy.h

Project Member

Comment 13 by sheriffbot@chromium.org, Jan 6 2018

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
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 14 by ajha@chromium.org, Jan 8 2018

CL in C#12 landed again on canary and this is causing the top browser crash on Mac tracked in Issue 798987.

Still isnot valid candidate to get this merged to M-64.
Project Member

Comment 15 by bugdroid1@chromium.org, Jan 8 2018

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

commit 0a479fa69533ba001e8529f0e5d16bfba0adf710
Author: Reilly Grant <reillyg@chromium.org>
Date: Mon Jan 08 22:37:37 2018

Revert "Move WifiPollingPolicy to a global instance"

This reverts commit bd9b9ea0315520281caad474df1fda846e84d6f5.

Reason for revert: #1 crash on canary.

Bug: 798987

Original change's description:
> Move WifiPollingPolicy to a global instance
> 
> The WifiPollingPolicy controls how frequently the network location
> provider will initiate Wi-Fi scans. Previously, the policy was attached
> to the WifiDataProvider, which is destroyed when there are no active
> geolocation API calls. Destruction of the provider causes us to lose track
> of our polling policy.
> 
> The policy allows the first scan to be performed immediately and schedules
> subsequent scans at regular intervals. When the provider is destroyed and
> recreated rapidly it considers each scan to be "first", which allows many
> Wi-Fi scans to be performed in a short window.
> 
> This change moves the policy state to a global instance so it can be saved
> when the provider is destroyed and recreated. The new policy allows the
> first-ever scan to be performed immediately, and subsequent scans may also
> be performed immediately if enough time has lapsed since the previous scan.
> In all other situations, the configured polling interval is enforced.
> 
> The global policy instance is leaked at shutdown.
> 
> BUG= 764954 
> 
> Change-Id: If16c73dfd7d6535346e86d0fab5df24f0c7b297d
> Reviewed-on: https://chromium-review.googlesource.com/850444
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#527219}

TBR=reillyg@chromium.org,mattreynolds@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  764954 
Change-Id: I2d1abb922f63442120316d4d5beaf288622b4c0d
Reviewed-on: https://chromium-review.googlesource.com/854392
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527789}
[modify] https://crrev.com/0a479fa69533ba001e8529f0e5d16bfba0adf710/device/geolocation/BUILD.gn
[modify] https://crrev.com/0a479fa69533ba001e8529f0e5d16bfba0adf710/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/0a479fa69533ba001e8529f0e5d16bfba0adf710/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/0a479fa69533ba001e8529f0e5d16bfba0adf710/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/0a479fa69533ba001e8529f0e5d16bfba0adf710/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/0a479fa69533ba001e8529f0e5d16bfba0adf710/device/geolocation/wifi_data_provider_common_unittest.cc
[delete] https://crrev.com/716b6410cf9e716001b1c95dbd251ce66aa0861d/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/0a479fa69533ba001e8529f0e5d16bfba0adf710/device/geolocation/wifi_polling_policy.h

Project Member

Comment 16 by bugdroid1@chromium.org, Jan 9 2018

Labels: merge-merged-3315
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/269b7f6f457ccd6c4a3ebd51ba496c472ec489c1

commit 269b7f6f457ccd6c4a3ebd51ba496c472ec489c1
Author: Reilly Grant <reillyg@chromium.org>
Date: Tue Jan 09 00:39:44 2018

Revert "Move WifiPollingPolicy to a global instance"

This reverts commit bd9b9ea0315520281caad474df1fda846e84d6f5.

Reason for revert: #1 crash on canary.

Bug: 798987

Original change's description:
> Move WifiPollingPolicy to a global instance
> 
> The WifiPollingPolicy controls how frequently the network location
> provider will initiate Wi-Fi scans. Previously, the policy was attached
> to the WifiDataProvider, which is destroyed when there are no active
> geolocation API calls. Destruction of the provider causes us to lose track
> of our polling policy.
> 
> The policy allows the first scan to be performed immediately and schedules
> subsequent scans at regular intervals. When the provider is destroyed and
> recreated rapidly it considers each scan to be "first", which allows many
> Wi-Fi scans to be performed in a short window.
> 
> This change moves the policy state to a global instance so it can be saved
> when the provider is destroyed and recreated. The new policy allows the
> first-ever scan to be performed immediately, and subsequent scans may also
> be performed immediately if enough time has lapsed since the previous scan.
> In all other situations, the configured polling interval is enforced.
> 
> The global policy instance is leaked at shutdown.
> 
> BUG= 764954 
> 
> Change-Id: If16c73dfd7d6535346e86d0fab5df24f0c7b297d
> Reviewed-on: https://chromium-review.googlesource.com/850444
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#527219}

TBR=reillyg@chromium.org,mattreynolds@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  764954 
Change-Id: I2d1abb922f63442120316d4d5beaf288622b4c0d
Reviewed-on: https://chromium-review.googlesource.com/854392
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#527789}(cherry picked from commit 0a479fa69533ba001e8529f0e5d16bfba0adf710)
Reviewed-on: https://chromium-review.googlesource.com/855376
Reviewed-by: Abdul Syed <abdulsyed@google.com>
Commit-Queue: Abdul Syed <abdulsyed@google.com>
Cr-Commit-Position: refs/branch-heads/3315@{#3}
Cr-Branched-From: 4541e0f3e71680a4b1b6f33d1e0850a60d92677d-refs/heads/master@{#527552}
[modify] https://crrev.com/269b7f6f457ccd6c4a3ebd51ba496c472ec489c1/device/geolocation/BUILD.gn
[modify] https://crrev.com/269b7f6f457ccd6c4a3ebd51ba496c472ec489c1/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/269b7f6f457ccd6c4a3ebd51ba496c472ec489c1/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/269b7f6f457ccd6c4a3ebd51ba496c472ec489c1/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/269b7f6f457ccd6c4a3ebd51ba496c472ec489c1/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/269b7f6f457ccd6c4a3ebd51ba496c472ec489c1/device/geolocation/wifi_data_provider_common_unittest.cc
[delete] https://crrev.com/147e130a8d1a4814960352722efd6fe16fb936d0/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/269b7f6f457ccd6c4a3ebd51ba496c472ec489c1/device/geolocation/wifi_polling_policy.h

Labels: -Merge-Review-64 Merge-Rejected-64
Per #14, rejecting merge to M64. 
Project Member

Comment 18 by bugdroid1@chromium.org, Jan 10 2018

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

commit 4e0435834ec440a2e4938abe90946c2dfa73027c
Author: Matt Reynolds <mattreynolds@google.com>
Date: Wed Jan 10 23:11:11 2018

Reland "Move WifiPollingPolicy to a global instance"

This is a reland of bd9b9ea0315520281caad474df1fda846e84d6f5
Original change's description:
> Move WifiPollingPolicy to a global instance
> 
> The WifiPollingPolicy controls how frequently the network location
> provider will initiate Wi-Fi scans. Previously, the policy was attached
> to the WifiDataProvider, which is destroyed when there are no active
> geolocation API calls. Destruction of the provider causes us to lose track
> of our polling policy.
> 
> The policy allows the first scan to be performed immediately and schedules
> subsequent scans at regular intervals. When the provider is destroyed and
> recreated rapidly it considers each scan to be "first", which allows many
> Wi-Fi scans to be performed in a short window.
> 
> This change moves the policy state to a global instance so it can be saved
> when the provider is destroyed and recreated. The new policy allows the
> first-ever scan to be performed immediately, and subsequent scans may also
> be performed immediately if enough time has lapsed since the previous scan.
> In all other situations, the configured polling interval is enforced.
> 
> The global policy instance is leaked at shutdown.
> 
> BUG= 764954 
> 
> Change-Id: If16c73dfd7d6535346e86d0fab5df24f0c7b297d
> Reviewed-on: https://chromium-review.googlesource.com/850444
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#527219}

Bug:  764954 
Change-Id: I394a671cdbcdb58d6e80c201777465d1d304987b
Reviewed-on: https://chromium-review.googlesource.com/860778
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528466}
[modify] https://crrev.com/4e0435834ec440a2e4938abe90946c2dfa73027c/device/geolocation/BUILD.gn
[modify] https://crrev.com/4e0435834ec440a2e4938abe90946c2dfa73027c/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/4e0435834ec440a2e4938abe90946c2dfa73027c/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/4e0435834ec440a2e4938abe90946c2dfa73027c/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/4e0435834ec440a2e4938abe90946c2dfa73027c/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/4e0435834ec440a2e4938abe90946c2dfa73027c/device/geolocation/wifi_data_provider_common_unittest.cc
[add] https://crrev.com/4e0435834ec440a2e4938abe90946c2dfa73027c/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/4e0435834ec440a2e4938abe90946c2dfa73027c/device/geolocation/wifi_polling_policy.h

mattreynolds@, Just wanted to check, do we have any plans to merge this to M64 branch?
Status: Started (was: Assigned)
Labels: -Merge-Rejected-64 Merge-Request-64
abdulsyed@, the change has landed in Canary but we don't have data yet to verify that this fixes the issue that caused us to hit quota limits. How do you feel about taking this for M64?
Project Member

Comment 22 by sheriffbot@chromium.org, Jan 17 2018

Labels: -Merge-Request-64 Merge-Review-64
This bug requires manual review: We are only 5 days from stable.
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
Thanks Mattreynolds@ - we're less than 5 days away from Stable. My recommendation is to wait until M65 if this can wait. I realize this is marked as a Stable blocker; however, since there are no beta cycles left, we will need to ensure that this is well tested and extremely safe (no chances of new regressions). Can you please comment on how safe this fix is overall and whether we can wait until M65?
Thanks, Abdul.

I'm confident this will not cause crashes like we saw before but there is still a possibility that changing the wifi policy could adversely affect geolocation. I'd feel better waiting until M65 so we have more opportunity to catch regressions.
Cc: kbleicher@chromium.org
Thanks! Seems like this will impact ChromeOS more?
+Kbleicher@

Comment 26 by josa...@google.com, Jan 22 2018

Labels: -M-64 -Merge-Review-64 Merge-Rejected-64 M-65
ok, let's wait for M65 on this then
Matthew, please fee free to mark it as fixed if no pending CLs for M65
Status: Fixed (was: Started)
Project Member

Comment 28 by bugdroid1@chromium.org, Mar 27 2018

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

commit 37e76e702673b44f3b618c3af336105ed30d05e2
Author: Matt Reynolds <mattreynolds@chromium.org>
Date: Tue Mar 27 03:15:36 2018

Revert "Reland "Move WifiPollingPolicy to a global instance""

This reverts commit 4e0435834ec440a2e4938abe90946c2dfa73027c.

Reason for revert: Broke geolocation for NetworkLocationProvider

Original change's description:
> Reland "Move WifiPollingPolicy to a global instance"
> 
> This is a reland of bd9b9ea0315520281caad474df1fda846e84d6f5
> Original change's description:
> > Move WifiPollingPolicy to a global instance
> > 
> > The WifiPollingPolicy controls how frequently the network location
> > provider will initiate Wi-Fi scans. Previously, the policy was attached
> > to the WifiDataProvider, which is destroyed when there are no active
> > geolocation API calls. Destruction of the provider causes us to lose track
> > of our polling policy.
> > 
> > The policy allows the first scan to be performed immediately and schedules
> > subsequent scans at regular intervals. When the provider is destroyed and
> > recreated rapidly it considers each scan to be "first", which allows many
> > Wi-Fi scans to be performed in a short window.
> > 
> > This change moves the policy state to a global instance so it can be saved
> > when the provider is destroyed and recreated. The new policy allows the
> > first-ever scan to be performed immediately, and subsequent scans may also
> > be performed immediately if enough time has lapsed since the previous scan.
> > In all other situations, the configured polling interval is enforced.
> > 
> > The global policy instance is leaked at shutdown.
> > 
> > BUG= 764954 
> > 
> > Change-Id: If16c73dfd7d6535346e86d0fab5df24f0c7b297d
> > Reviewed-on: https://chromium-review.googlesource.com/850444
> > Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> > Reviewed-by: Reilly Grant <reillyg@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#527219}
> 
> Bug:  764954 
> Change-Id: I394a671cdbcdb58d6e80c201777465d1d304987b
> Reviewed-on: https://chromium-review.googlesource.com/860778
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#528466}

TBR=reillyg@chromium.org,mattreynolds@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  764954 
Change-Id: I402e6085d966e4ed0c385515b6edcedb0119bb69
Reviewed-on: https://chromium-review.googlesource.com/978422
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545945}
[modify] https://crrev.com/37e76e702673b44f3b618c3af336105ed30d05e2/device/geolocation/BUILD.gn
[modify] https://crrev.com/37e76e702673b44f3b618c3af336105ed30d05e2/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/37e76e702673b44f3b618c3af336105ed30d05e2/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/37e76e702673b44f3b618c3af336105ed30d05e2/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/37e76e702673b44f3b618c3af336105ed30d05e2/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/37e76e702673b44f3b618c3af336105ed30d05e2/device/geolocation/wifi_data_provider_common_unittest.cc
[delete] https://crrev.com/821f0e338ebbe420271d92c4b8fa4c78bc32aef8/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/37e76e702673b44f3b618c3af336105ed30d05e2/device/geolocation/wifi_polling_policy.h

Project Member

Comment 29 by bugdroid1@chromium.org, Mar 30 2018

Labels: merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26416be3e28bc16ebdd9576a69e44148f2acb59b

commit 26416be3e28bc16ebdd9576a69e44148f2acb59b
Author: Matt Reynolds <mattreynolds@chromium.org>
Date: Fri Mar 30 23:11:08 2018

Revert "Reland "Move WifiPollingPolicy to a global instance""

This reverts commit 4e0435834ec440a2e4938abe90946c2dfa73027c.

Reason for revert: Broke geolocation for NetworkLocationProvider

Original change's description:
> Reland "Move WifiPollingPolicy to a global instance"
> 
> This is a reland of bd9b9ea0315520281caad474df1fda846e84d6f5
> Original change's description:
> > Move WifiPollingPolicy to a global instance
> > 
> > The WifiPollingPolicy controls how frequently the network location
> > provider will initiate Wi-Fi scans. Previously, the policy was attached
> > to the WifiDataProvider, which is destroyed when there are no active
> > geolocation API calls. Destruction of the provider causes us to lose track
> > of our polling policy.
> > 
> > The policy allows the first scan to be performed immediately and schedules
> > subsequent scans at regular intervals. When the provider is destroyed and
> > recreated rapidly it considers each scan to be "first", which allows many
> > Wi-Fi scans to be performed in a short window.
> > 
> > This change moves the policy state to a global instance so it can be saved
> > when the provider is destroyed and recreated. The new policy allows the
> > first-ever scan to be performed immediately, and subsequent scans may also
> > be performed immediately if enough time has lapsed since the previous scan.
> > In all other situations, the configured polling interval is enforced.
> > 
> > The global policy instance is leaked at shutdown.
> > 
> > BUG= 764954 
> > 
> > Change-Id: If16c73dfd7d6535346e86d0fab5df24f0c7b297d
> > Reviewed-on: https://chromium-review.googlesource.com/850444
> > Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> > Reviewed-by: Reilly Grant <reillyg@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#527219}
> 
> Bug:  764954 
> Change-Id: I394a671cdbcdb58d6e80c201777465d1d304987b
> Reviewed-on: https://chromium-review.googlesource.com/860778
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#528466}

TBR=reillyg@chromium.org,mattreynolds@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  764954 
Change-Id: I402e6085d966e4ed0c385515b6edcedb0119bb69
Reviewed-on: https://chromium-review.googlesource.com/978422
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#545945}(cherry picked from commit 37e76e702673b44f3b618c3af336105ed30d05e2)
Reviewed-on: https://chromium-review.googlesource.com/988992
Cr-Commit-Position: refs/branch-heads/3359@{#515}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/26416be3e28bc16ebdd9576a69e44148f2acb59b/device/geolocation/BUILD.gn
[modify] https://crrev.com/26416be3e28bc16ebdd9576a69e44148f2acb59b/device/geolocation/wifi_data_provider_chromeos.cc
[modify] https://crrev.com/26416be3e28bc16ebdd9576a69e44148f2acb59b/device/geolocation/wifi_data_provider_chromeos.h
[modify] https://crrev.com/26416be3e28bc16ebdd9576a69e44148f2acb59b/device/geolocation/wifi_data_provider_common.cc
[modify] https://crrev.com/26416be3e28bc16ebdd9576a69e44148f2acb59b/device/geolocation/wifi_data_provider_common.h
[modify] https://crrev.com/26416be3e28bc16ebdd9576a69e44148f2acb59b/device/geolocation/wifi_data_provider_common_unittest.cc
[delete] https://crrev.com/a00e7f7ffb6cdc7bfc6d2779f19110e7681986a3/device/geolocation/wifi_polling_policy.cc
[modify] https://crrev.com/26416be3e28bc16ebdd9576a69e44148f2acb59b/device/geolocation/wifi_polling_policy.h

Status: Assigned (was: Fixed)
Given the revert in comment 28 is this actually fixed?
Yes, this fix was re-relanded as part of a later CL:

https://chromium-review.googlesource.com/c/chromium/src/+/1089190
Status: Fixed (was: Assigned)

Sign in to add a comment