I think there might be another level to the problem of unnecessary googleapis.com requests.  device::NetworkLocationProvider looks designed to cache the googleapis results and only invalidate the cache when the set of WiFi APs visible to the device changes.  However, device::GeolocationProviderImpl discards its device::NetworkLocationProvider (along with any cached location) when it discovers no clients are interested in location updates anymore.  And I'm seeing GeolocationServiceImpl being created for each Geolocation.getCurrentPosition(), subscribing to GeolocationProviderImpl, and then being destroyed once the result is reported to blink.  Thus, the one and only GeolocationProviderImpl subscriber goes away, and so does the cached location.

I'll file a separate bug for this.

Or not :)  I'm not sure anymore whether the "other level" is a problem in device:: or blink::.

From what I've seen, the destruction of device::GeolocationServiceImpl happens as a result of the mojo connection getting closed, which happens due to blink::Geolocation resetting its GeolocationServicePtr in updateGeolocationServiceConnection() after reporting the position in makeSuccessCallbacks().

On the one hand, it makes sense for blink::Geolocation to drop its GeolocationServicePtr when it doesn't see an immediate need to update the position.  But then, as we can see, this strategy easily leads to device::GeolocationProviderImpl forgetting any cached position between two consecutive calls to Geolocation.getCurrentPosition().  Is it intentional that there is effectively no position caching unless some page calls Geolocation.watchPosition(), or some browser component establishes a permanent subscription to device::GeolocationProviderImpl directly?

The description in #6 about how mojo works seems accurate. It's also true 
that this might cause the position provider to be destroyed and not honour 
any potential reuses coming shortly thereafter, but: Does the spec define 
that position caching should be system-wide, or instantiation-wide?

If it's the first, a potential solution would be to have a singleton 
Geolocation provider service, that will be alive after its first use, 
and could keep the provider alive for as long as needed.  Which reminds
me that //device/geolocation should eventually be //services/geolocation.

Please note that there seem to be two levels where the location caching goes wrong currently.

1. Blink.  From my reading of https://www.w3.org/TR/geolocation-API/, there is no requirement that the caching is instantiation-wide.  I would also be surprised if there was one, because that would limit the caching efficiency severely with no benefit for the applications (the device location doesn't depend on which application is asking).  Blink currently only caches the location per-instantiation.

2. //device/geolocation.  There already is a singleton, lazily instantiated Geolocation provider, device::GeolocationProvider, which keeps a device::NetworkLocationProvider (along with a cached location) alive for as long as needed.  The problem is, "needed" is defined as "there are one or more GeolocationProvider subscribers", and once blink::Geolocation finishes servicing one Geolocation.getCurrentPosition() call, the number of subscribers drops to 0.


I suspect the caching in Blink could be unnecessary if //device/geolocation does it right.  device::Geoposition already contains a timestamp that could be used to evaluate the cached location against PositionOptions.maximumAge.  Now, how do we combat the problem of the disappearing device::NetworkLocationProvider?  Here are two ideas:

  - device::NetworkLocationProvider could be kept alive for some time T since the last GeolocationProvider subscriber went away, with some maximumAge-based heuristics to determine a good value of T.

  - IIUC, the main cost of keeping device::NetworkLocationProvider alive is that it must poll the system for the list of visible WiFi APs.  If this is indeed something we want to avoid (even with the polling intervals on the order of several minutes) then I propose the following.  When there are no GeolocationProvider subscribers anymore, stop polling for APs, but remember the last list of APs.  When the location is requested again, refresh the list of APs and compare it to the stored list.  Only call googleapis.com/geolocation/ if the list of APs has changed.

It is reasonable for a user agent to place a limit on how long it will continue to store a cached location. In this case I think option (2) in comment 8 is the right way to go. If the number of subscribers reaches 0 then we can discard the cache along with shutting down the provider.

Hmm, but that's what is happening now (or at least was the last time I checked -- I haven't been following the implementation of Location recently).  When no Geolocation.getCurrentPosition() call is currently being serviced, the number of GeolocationProvider subscribers is 0, and device::NetworkLocatinProvider (which holds the cached location) is lost.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

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

mattreynolds@ has done work recently to fix caching for network location requests so that it is not discarded when there are no more subscribers.

Seems that wasn't enough. Currently, LastPositionCache is a persistent delegate that outlives the transient NetworkLocationProviders but NetworkLocationProvider::PositionCache, which actually stores the mappings of WifiData to Geoposition, is still reconstructed on each page reload.

I've added some debug logs and see that on subsequent reloads PositionCache is queried for the same WifiData over and over, but it's empty, since it's just been constructed. This leads to requests being sent to the geolocation API almost on each reload (sometimes, if I'm quick enough with the F5, I manage to trigger a LastPositionCache hit).

Note that I can only reproduce it with an actual wifi adapter attached to my computer, if I'm on a wired network, LastPositionCache works well enough and requests are only sent once every 10 minutes, as the time constant suggests.

Should we reopen this bug or should I file a new one?

I've prepared a patch: extracting PositionCache to an external class, with LastPositionCache's functionality merged in and making it persistent (ie. held by LocationArbitrator, which currently owns and outlives the transient NetworkLocationProviders). Seems to reduce the number of API requests considerably without tripping any of the existing tests. New tests are added.

Note, this is not about extending NetworkLocationProvider life via subscriptions.

I'll upload it for review.