Closed Bug 1018383 Opened 6 years ago Closed 5 years ago

Add local cache for geolocation requests to avoid too many outbound network requests

Categories

(Core :: DOM: Geolocation, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 1.4+
Tracking Status
firefox31 --- verified
firefox32 --- verified
firefox33 --- verified
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: garvan, Assigned: garvan)

References

Details

Attachments

(4 files, 9 obsolete files)

This is the m-c version of: Bug 1009592, which is Tarako-only.

Please refer to the prev. bug for details. Essentially, we need a cache of the recent known good geolocation request for the purposes of preventing unnecessary http requests to location services.
Depends on: 1009592
I should note that although originally filed as B2G/Tarako specific, this is a request for all platforms to have this caching fix.
Assignee: nobody → gkeeley
Blocks: 1009592
No longer depends on: 1009592
The code for 1009592 is is not for m-c, for that patch we erred on the side of always providing location, regardless of how stale. This caching method does not meet the W3C geolocation spec requirements for PositionOptions timeout and maximumAge. Any patch must pass our full geolocation mochitest suite, which tests the PostionOptions.timeout and PositionOptions.maximumAge.
Attached patch bug-1018383.diff (obsolete) — Splinter Review
Patch applies cleanly to todays m-c pull. Waiting for try server results.

1) Added request-level caching in NetworkGeolocationProvider.js

This is a cleanup of the patch used in the Tarako bug. I made it clearer that this is request-level caching, NOT position caching. This is specifically for preventing unnecessary requests to the server. It is unrelated to the W3C PositionOptions and their effect on position caching. Hopefully this detail is clearer in this patch.

2) Updated mochitests that were relying on old logic to succeed

The failing mochitests were relying on the provider to have a specific behaviour -behaviours that are incidental to the test case. I updated these to reflect the behaviour of the current system.

3) Removed unnecessary and confusing additional cached location from Geolocation, cached location is on the service level only.

We had a strange extra cached location in this class, for no particular benefit other than to get some mochitests to succeed as written. 

4) Added isHighAccuracy to GeolocationService's cached location, and updated the nsGeolocationRequest::Allow() function that decides whether to use the service's cached location: this logic also needs to compare the current highAccuracy flag to the cached location's isHighAccuracy

As part of cleaning up this request caching/location caching I touched upon an obvious item we were missing in the service's cache.  Hmm, perhaps this should be split to another bug, it was done as part of cleaning up 3.
Attachment #8436568 - Flags: review?(josh)
Attachment #8436568 - Flags: review?(dougt)
Full: https://tbpl.mozilla.org/?tree=Try&rev=91914af9636d
On, 10.6.8, an unrelated mochitest failed, ran again, and all passed:
10.6.8: https://tbpl.mozilla.org/?tree=Try&rev=c98acbacdff8
The cache changes touch upon some of the requirements in bug 1013012.

I am going to annotate the cache changes in nsGeolocationRequest both for the reviewer and in case I split this change into that bug:

nsGeolocationRequest::Allow() checks the service for a cached location:

+  bool canUseCache = false;
+  CachedPositionType lastPosition = gs->GetCachedPosition();

Note to reader: CachedPositionType is both the position and its accuracy type. 

+  if (lastPosition.position) {
+    DOMTimeStamp cachedPositionTime_ms;
+    lastPosition.position->GetTimestamp(&cachedPositionTime_ms);
+    // check to see if we can use a cached value
+    // if the user has specified a maximumAge, return a cached value.
+    uint32_t maximumAge_ms = 0;
+    if (mOptions && mOptions->mMaximumAge > 0) {
+      maximumAge_ms = mOptions->mMaximumAge;
+      canUseCache = lastPosition.position &&
+        WantsHighAccuracy() <= lastPosition.isHighAccuracy &&

This is the important change, the current accuracy must be less than or equal the cached position accuracy. The cached location now fully respects the W3C PositionOptions (timeout and maximumAge were previously supported)

+        maximumAge_ms > 0 &&
+        DOMTimeStamp(PR_Now() / PR_USEC_PER_MSEC - maximumAge_ms) <= cachedPositionTime_ms;
+    }
Blocks: 1019061
No longer blocks: 1019061
Blocks: mls-dolphin
Hardware: x86 → ARM
This is a patch from Tarako to reduce unnecessary network usage for apps using geolocation.
blocking-b2g: --- → 1.4?
Garvan says:

This patch is a nice to have, but not critical because the Dolphin device has GPS, so you will only be hitting the server with requests until the GPS locations are coming in. Only in the case when the GPS is cold would it take a minute or so to stop using MLS. A warmed up GPS may be available in 5-20 seconds. This fix reduces the server requests to a tiny fraction of the requests that were being made (on my Tarako, it was make 1/100th of the requests without the patch, along with faster geolocation response time).
blocking-b2g: 1.4? → backlog
Blocks: 1013012
Blocks: 1001960
Comment on attachment 8436568 [details] [diff] [review]
bug-1018383.diff

Review of attachment 8436568 [details] [diff] [review]:
-----------------------------------------------------------------

I've run out of time today, but all that should be left is thinking about the test-related changes.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +432,5 @@
>    }
>  
> +  bool canUseCache = false;
> +  CachedPositionType lastPosition = gs->GetCachedPosition();
> +  if (lastPosition.position) {

I'm not a big fan of the consolidation here - I think the previous code in separate small blocks was easier to read.

@@ +437,5 @@
> +    DOMTimeStamp cachedPositionTime_ms;
> +    lastPosition.position->GetTimestamp(&cachedPositionTime_ms);
> +    // check to see if we can use a cached value
> +    // if the user has specified a maximumAge, return a cached value.
> +    uint32_t maximumAge_ms = 0;

No need for this declaration to exist outside of the inner scope, afaict.

@@ +440,5 @@
> +    // if the user has specified a maximumAge, return a cached value.
> +    uint32_t maximumAge_ms = 0;
> +    if (mOptions && mOptions->mMaximumAge > 0) {
> +      maximumAge_ms = mOptions->mMaximumAge;
> +      canUseCache = lastPosition.position &&

We're already inside a lastPosition.position if block.

@@ -505,5 @@
>      // Ignore SendLocationEvents issued before we were cleared.
>      return;
>    }
>  
> -  nsRefPtr<Position> wrapped, cachedWrapper = mLocator->GetCachedPosition();

This location-level caching is necessary because the spec implies that the position object visible to content is quite literally the object that was previously cached, not merely containing the same data.

::: dom/src/geolocation/nsGeolocation.h
@@ +46,5 @@
>  }
>  
> +
> +// Cached position and the accuracy type
> +struct CachedPositionType {

This sounds like a classification of kinds of cached positions. I think CachedPosition is a more descriptive name.

::: dom/system/NetworkGeolocationProvider.js
@@ +130,5 @@
> +// 1) do we have a cached request
> +// 2) is the cached reqest better than what newCell and newWifiList will obtain
> +// If the cached request exists, and we know it to have greater accuracy
> +// by the nature of its origin (wifi/cell/geoip), use its cached location.
> +// 

nit: trailing ws

@@ +138,5 @@
> +function isCachedRequestMoreAccurateThanServerRequest(newCell, newWifiList)
> +{
> +  let isNetworkRequestCacheEnabled = true;
> +  try {
> +     isNetworkRequestCacheEnabled = Services.prefs.getBoolPref("geo.wifi.requestCache.enabled", true);

No second argument.

@@ +174,5 @@
> +    return true;
> +  } else if (gCachedRequest.isCellAndWifi()) {
> +    if ((hasEqualCells && hasEqualWifis) ||
> +         (!newWifiList && hasEqualCells) ||
> +          (!newCell && hasEqualWifis))

Curious indentation here.

@@ +181,5 @@
> +    }
> +  }
> +
> +  return false;
> +};

Unnecessary semicolon.

@@ -223,3 @@
>      let url = Services.urlFormatter.formatURLPref("geo.wifi.uri");
>      let listener = this.listener;
> -    LOG("Sending request: " + url + "\n");

Can we leave this in?

::: dom/tests/mochitest/geolocation/test_cachedPosition.html
@@ +2,5 @@
>  <html>
>  <!--
>  https://bugzilla.mozilla.org/show_bug.cgi?id=850442
>  -->
> +<head

wat
Thanks for the review Josh, will update the patch.

> I'm not a big fan of the consolidation here - I think the previous code in
> separate small blocks was easier to read.

Agreed, and it wouldn't hurt for me to put some notes on certain lines as to how this fits in to the w3c spec. Explain why this logic for each of these conditions meets the requirements.

> This location-level caching is necessary because the spec implies that the
> position object visible to content is quite literally the object that was
> previously cached, not merely containing the same data.

I'll need some help understanding this case then. I can't see that in the doc. http://dev.w3.org/geo/api/spec-source.html, searched for cache, 21 hits, but I couldn't find that. Seems like an unnecessary implementation detail IMHO. The position options can already be used to requested a cache or an error, it would be odd for the spec to specify a 3rd condition, wouldn't be a redundant item? That is, already having a success callback on the cache request, is address of the returned object that important? 
Moreover, it isn't even useful in an obvious case like where you call getCurrentPosition() and want to distinguish between a cached object or a new object. 
In the watch position case, shouldn't the page only be getting updates on new positions? Not the way we have it implemented where we keep sending the same info to the page in a timer. 

(Somewhat as an aside, this patch can also be used as a basis to fix the watch position case, as we not only don't need to keep hitting the server every few seconds, we now also have enough info to not report back to the page unnecessarily.)

> This sounds like a classification of kinds of cached positions. I think
> CachedPosition is a more descriptive name.

Yeah, I was trying to make it clear to reader that this is more than just a cached position, but the word 'type' adds no clarity :). Can I call it CachedPositionAndAccuracy?
 
> 
> @@ +138,5 @@
> > +function isCachedRequestMoreAccurateThanServerRequest(newCell, newWifiList)
> > +{
> > +  let isNetworkRequestCacheEnabled = true;
> > +  try {
> > +     isNetworkRequestCacheEnabled = Services.prefs.getBoolPref("geo.wifi.requestCache.enabled", true);
> 
> No second argument.

Isn't that how explicit defaults are specified in the pref requests? 

> 
> @@ -223,3 @@
> >      let url = Services.urlFormatter.formatURLPref("geo.wifi.uri");
> >      let listener = this.listener;
> > -    LOG("Sending request: " + url + "\n");
> 
> Can we leave this in?

We had complaints that it too readily exposes the API key, and we haven't been using this line when looking at debug logs (the other debug info has been useful though).
(In reply to Garvan Keeley [:garvank] from comment #9)
> > This location-level caching is necessary because the spec implies that the
> > position object visible to content is quite literally the object that was
> > previously cached, not merely containing the same data.
> 
> I'll need some help understanding this case then. I can't see that in the
> doc. http://dev.w3.org/geo/api/spec-source.html, searched for cache, 21
> hits, but I couldn't find that. Seems like an unnecessary implementation
> detail IMHO. The position options can already be used to requested a cache
> or an error, it would be odd for the spec to specify a 3rd condition,
> wouldn't be a redundant item? That is, already having a success callback on
> the cache request, is address of the returned object that important? 
> Moreover, it isn't even useful in an obvious case like where you call
> getCurrentPosition() and want to distinguish between a cached object or a
> new object. 
> In the watch position case, shouldn't the page only be getting updates on
> new positions? Not the way we have it implemented where we keep sending the
> same info to the page in a timer. 

I'm going by this quote: "If a cached Position object, whose age is no greater than the value of the maximumAge variable, is available, invoke the successCallback with the cached Position object as a parameter"

> > This sounds like a classification of kinds of cached positions. I think
> > CachedPosition is a more descriptive name.
> 
> Yeah, I was trying to make it clear to reader that this is more than just a
> cached position, but the word 'type' adds no clarity :). Can I call it
> CachedPositionAndAccuracy?

Yep.

> > @@ +138,5 @@
> > > +function isCachedRequestMoreAccurateThanServerRequest(newCell, newWifiList)
> > > +{
> > > +  let isNetworkRequestCacheEnabled = true;
> > > +  try {
> > > +     isNetworkRequestCacheEnabled = Services.prefs.getBoolPref("geo.wifi.requestCache.enabled", true);
> > 
> > No second argument.
> 
> Isn't that how explicit defaults are specified in the pref requests? 

Not in JS - we're using http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/nsIPrefBranch.idl#65

> > @@ -223,3 @@
> > >      let url = Services.urlFormatter.formatURLPref("geo.wifi.uri");
> > >      let listener = this.listener;
> > > -    LOG("Sending request: " + url + "\n");
> > 
> > Can we leave this in?
> 
> We had complaints that it too readily exposes the API key, and we haven't
> been using this line when looking at debug logs (the other debug info has
> been useful though).

Sounds reasonable.
> I'm going by this quote: "If a cached Position object, whose age is no
> greater than the value of the maximumAge variable, is available, invoke the
> successCallback with the cached Position object as a parameter".

If they had written 'with a cached Position object' it would be clear to me, but they didn't, so I assume they meant that :). I will investigate this further and try find out what the use case might be for returning a Position object that isn't just value-equal, but is address-of equal.
This bug is a nice-to-have and no longer a Dolphin blocker.
No longer blocks: mls-dolphin
It looks like Dolphin might have a troublesome GPS. So we might see the cold-start GPS case with one to twelve minutes to first fix much more often than anticipated.

Without assisted GPS it will also fail to get any fix much more often, especially in urban canyons. In addition to all the places where GPS doesn't work, like indoors.

With that I'd ask to please reconsider this as an important fix for Dolphin.
I marked this for 1.4. Based on Hanno's comments, also that because we felt it was important enough for 1.3t. So, please lets consider this one last time for 1.4 inclusion.

To explain his comment, the nature of fixing this bug, was to use local caching much more aggressively, which produces better results in the cases he mentioned.

This bug was originally to clean up the 1.3t patch for moz-central, I will upload an updated patch that addresses Josh's comments and applies on todays m-c build. It will need to be rebased on top of 1.4.
blocking-b2g: backlog → 1.4?
Blocks: mls-dolphin
No longer blocks: 1009592
Depends on: 1009592
Summary: Avoid too many outbound network requests for Geo lookups (req. for mozilla-central) → Add local cache for geolocation requests to avoid too many outbound network requests
Attached patch bug1018383_netgeoprov.diff (obsolete) — Splinter Review
This bug is now just items 1 and 2 from the original patch. I'll apply the other changes to other bugs. 

1) Added request-level caching in NetworkGeolocationProvider.js
This is a cleanup of the patch used in the Tarako bug. I made it clearer that this is request-level caching, NOT position caching. This is specifically for preventing unnecessary requests to the server. It is unrelated to the W3C PositionOptions and their effect on position caching. Hopefully this detail is clearer in this patch.

2) Updated mochitests that were relying on old logic to succeed
The failing mochitests were relying on the provider to have a specific behaviour -behaviours that are incidental to the test case. I updated these to reflect the behaviour of the current system.
Attachment #8436568 - Attachment is obsolete: true
Attachment #8436568 - Flags: review?(josh)
Attachment #8436568 - Flags: review?(dougt)
Attachment #8446766 - Flags: review?(josh)
The bug for the failing mochitests from the Tarako version of this patch:
https://bugzilla.mozilla.org/show_bug.cgi?id=1018537
try:
Results will be displayed on TBPL as they come in:
https://tbpl.mozilla.org/?tree=Try&rev=f9e64ea7156d
Once completed, builds and logs will be available at:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/gkeeley@mozilla.com-f9e64ea7156d
What is the user impact to a Dolphin device user?

What's the downside of not accepting this fix into 1.4?
Flags: needinfo?(gkeeley)
- Less network requests could mean less cellular data usage, could mean less battery usage. It depends on how long they are running in a state where the device is trying to geolocate without a GPS fix.

- More accurate geolocation without GPS: the network cache used in this bug also takes into consideration the accuracy level of consecutive requests. The cache has a logic for:

Accuracy Of Request Order (with off-the-cuff approx accuracy levels)
 WIFI+CELL (20-100m) > WIFI > CELL (~30 km) > GeoIP (50 - 100 km)

If a consecutive request is known to have lower accuracy (for example, a GeoIP request (~100km accuracy) is following the Cell request (~25 km accuracy), then the GeoIP request will not be made, the cached location from the Cell is used.

- Faster geolocation without GPS after the first request, because of the network caching.
Flags: needinfo?(gkeeley)
Comment on attachment 8446766 [details] [diff] [review]
bug1018383_netgeoprov.diff

Review of attachment 8446766 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/NetworkGeolocationProvider.js
@@ +58,5 @@
> +  isWifiOnly: function() {
> +    return this.wifiList && !this.cellInfo;
> +  },
> +  // if 50% of the SSIDS match
> +  isWifiApproxEqual: function(wifiList) {

You may want to give the parameter a different name, like otherWifiList, to differentiate between wifiList and this.wifiList. You might want to rename this.wifiList to something like this.recentWifiList for the same reason. :)

@@ +76,5 @@
> +    }
> +
> +    let wifiHash = {};
> +    for (let i = 0; i < largerList.length; i++) {
> +      wifiHash[largerList[i].macAddress] = 1;

Instead of rehashing this.wifiList on every call to isWifiApproxEqual, why not hash the wifiList once in the CachedRequest constructor and save this.wifiHash?

Also, calling a dictionary a "hash" is a Perl-ism. :) I would just call it something like this.wifiMacAddresses or this.BSSIDs, omitting the implementation detail of the actual container from the variable name.

All these isWifiApproxEqual comments apply to isCellApproxEqual, too.

@@ +89,5 @@
> +    let kPercentMatch = 0.5;
> +    return common >= (largerList.length * kPercentMatch);
> +  },
> +  // if fields match
> +  isCellApproxEqual: function(cellInfo) {

Is isCellApproxEqual actually doing fuzzy comparison? AFAICT, for this function to return true, cellInfo and this.cellInfo must have the same length and every element in cellInfo must be in this.cellInfo. So isCellApproxEqual seems to be doing exact comparison.

@@ +132,5 @@
> +// 1) do we have a cached request
> +// 2) is the cached reqest better than what newCell and newWifiList will obtain
> +// If the cached request exists, and we know it to have greater accuracy
> +// by the nature of its origin (wifi/cell/geoip), use its cached location.
> +// 

Remove trailing whitespace.

@@ +140,5 @@
> +function isCachedRequestMoreAccurateThanServerRequest(newCell, newWifiList)
> +{
> +  let isNetworkRequestCacheEnabled = true;
> +  try {
> +     isNetworkRequestCacheEnabled = Services.prefs.getBoolPref("geo.wifi.requestCache.enabled");

You should initialize the geo.wifi.requestCache pref in modules/libpref/src/init/all.js. It's bad style to rely on prefs that are not initialized by default or documented in libpref.

Do you want this enabled on Firefox desktop, too? You may want to only enable the cache for #ifdef MOZ_WIDGET_GONK or #ifndef RELEASE_BUILD (i.e. only in the "non-release" Nightly and Aurora channels). RELEASE_BUILD, in this context, means "Firefox" branded builds in Beta and Release channels.
Attachment #8446766 - Flags: feedback+
Thanks for the review, will update the patch.

> Also, calling a dictionary a "hash" is a Perl-ism. :) 

I can't help it, I love Perl!

> Is isCellApproxEqual actually doing fuzzy comparison? AFAICT, for this
> function to return true, cellInfo and this.cellInfo must have the same
> length and every element in cellInfo must be in this.cellInfo. So
> isCellApproxEqual seems to be doing exact comparison.

Yes, but the cell info is to expand with additional items, such as strength, that won't be used for equality. However, I'll chance the function to isCellEqual, you are the second person to ask about that :).

> You should initialize the geo.wifi.requestCache pref in
> modules/libpref/src/init/all.js. It's bad style to rely on prefs that are
> not initialized by default or documented in libpref.

I copied the format of the other debugging-level prefs in that file:
geo.wifi.logging.enabled, geo.wifi.timeToWaitBeforeSending, I think there are 1 or 2 more.
It is there for mochitest only, to verify the cache is working when expected to do so. In light of all this, what is the correct approach?

> Do you want this enabled on Firefox desktop, too? 

Yes, this is for all platforms.
Lets request this patch for approval. It seems to be a nice to have and not compelling to block.
blocking-b2g: 1.4? → backlog
Attached patch bug1018383_netgeoprov.diff (obsolete) — Splinter Review
updated based on comments
- CachedRequest class has private cells and wifis members that are initialized in the constructor
- debugging pref is renamed to have the word 'debug' in it, hopefully that makes its purpose clearer
Attachment #8447415 - Flags: review?(josh)
Attachment #8446766 - Flags: review?(josh)
Attachment #8446766 - Attachment is obsolete: true
Attachment #8447415 - Attachment is obsolete: true
Comment on attachment 8447501 [details] [diff] [review]
bug1018383_netgeoprov.diff

still seeing an error in one of my tests, wasn't happening with an earlier patch, something is off in here
Attachment #8447501 - Attachment is obsolete: true
Attachment #8447501 - Flags: review?(josh)
Attached patch bug1018383_netgeoprov.diff (obsolete) — Splinter Review
updated based on comments
- CachedRequest class has private cells and wifis members that are initialized in the constructor
- Changed object dict to JS Set(), looks much cleaner
- debugging/mochitest pref is renamed and commented to make its purpose clearer
- added additional request cache debugging (off by default of course).

The 1.4 patches to the same file have just landed, I'll check this applies over top
Attachment #8447703 - Flags: review?(josh)
Attached patch bug1018383_b2g1.4.diff (obsolete) — Splinter Review
The prev. attachment 8447703 [details] [diff] [review] is for m-c.

This is for b2g 1.4.
Attachment #8447705 - Flags: review?(josh)
Attachment #8447703 - Attachment is obsolete: true
Attachment #8447703 - Flags: review?(josh)
Attachment #8447705 - Attachment is obsolete: true
Attachment #8447705 - Flags: review?(josh)
Attached patch bug1018383_netgeoprov.diff (obsolete) — Splinter Review
One more if-statement to the caching logic from previous. In the case where the cache is a low accuracy wifi, and a cell request is pending, don't use the cache.
Attachment #8448115 - Flags: review?(josh)
This is the 1.4 version of the patch
Attachment #8448116 - Flags: review?(josh)
Attached file test_bug1018383.js
This is my unit testing code I used to develop this. I would *like* to leave this in the file, but there doesn't appear to be a precedent for doing this.
To further emphasize the need for this, network geolocation does not stop when the GPS has a fix, it continues to ping every 5 seconds. I will need to investigate further to see if this is by design. 

(cpeterson, regarding our discussion: I thought I wasn't getting a GPS fix because of this, I had assumed the network location provider would be stopped when a fix arrived).
Updated try results:

https://tbpl.mozilla.org/?tree=Try&rev=2e3ff0cd8226
Once completed, builds and logs will be available at:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/gkeeley@mozilla.com-2e3ff0cd8226

Of note is the passing mochitests (or not, it is still running ATM)
QA Note: to see this bug, turn on Settings>Developer>Geolocation Debug in ADB.
Debug messages are prefixed by "*** WIFI", they will output every 5 seconds. Without this fix, with Here Maps open, you will see a message indicating network requests every 5 seconds: "*** WIFI GEO: sending <the request details>".
With the fix you would see that message rarely, perhaps once a minute if moving, and once every 5 minutes if standing still.
Comment on attachment 8448115 [details] [diff] [review]
bug1018383_netgeoprov.diff

Review of attachment 8448115 [details] [diff] [review]:
-----------------------------------------------------------------

These changes are basically fine, but I'd like to see you create a new test for request caching instead of repurposing test_cachedPositions, since they're fundamentally concerned with different things.

::: dom/system/NetworkGeolocationProvider.js
@@ +139,5 @@
> +{
> +  gDebugCacheReasoning = "";
> +  let isNetworkRequestCacheEnabled = true;
> +  try {
> +    // Mochitest needs this to pref simulate request failure

nit: to pref -> pref to

@@ +141,5 @@
> +  let isNetworkRequestCacheEnabled = true;
> +  try {
> +    // Mochitest needs this to pref simulate request failure
> +    isNetworkRequestCacheEnabled = Services.prefs.getBoolPref("geo.wifi.debug.requestCache.enabled");
> +    if (!isNetworkRequestCacheEnabled) gCachedRequest = null;

nit: newline for statement, please.

@@ +163,5 @@
> +  if (newCell && gCachedRequest.isWifiOnly()) {
> +    // In order to know if a cell-only request should trump a wifi-only request
> +    // need to know if wifi is low accuracy. >5km would be VERY low accuracy,
> +    // it is worth trying the cell
> +    var isHighAccuracyWifi = gCachedRequest.location.coords.accuracy < 5000; 

nit: trailing ws

@@ +274,5 @@
>      if (this.timer) {
>        this.timer.cancel();
>        this.timer = null;
>      }
> +    if (!this.timer) {

How can this ever not be the case, given the above?

@@ +342,5 @@
>      if (this.started == false) {
>        return;
>      }
>  
> +    // Without clearing this, we could endup using the cache almost indefinitely

nit: end up

@@ +450,5 @@
> +
> +    LOG("Use request cache:" + useCached + " reason:" + gDebugCacheReasoning);
> +
> +    if (useCached) {
> +      let newLoc = new WifiGeoPositionObject(gCachedRequest.location.coords.latitude,

Why build a new one instead of just using gCachedRequest.location?

::: dom/tests/mochitest/geolocation/test_errorcheck.html
@@ +31,5 @@
>  
>  function errorCallback(error) {
>      is(error.code, 
>          SpecialPowers.Ci.nsIDOMGeoPositionError.POSITION_UNAVAILABLE, "Geolocation error handler fired");
> +    set_network_request_cache_enabled(true, function() { SimpleTest.finish(); });

I get worried when I see this. I would expect to see a test explicitly disable the cache if it's going to be turning it back on.

::: dom/tests/mochitest/geolocation/test_timerRestartWatch.html
@@ +48,4 @@
>    // Now that we got a success callback, lets try to ensure
>    // that we get a timeout error.
> +  // Turn off the network cache, and stop the sjs from reponding to requests
> +  stop_geolocationProvider(function(){});

How does this turn off the cache?
Attachment #8448115 - Flags: review?(josh) → review+
Comment on attachment 8448116 [details] [diff] [review]
1.4-patch-of-bug1018383.-Adds-local-request-cache-to.patch

Clearing this; I'll r+ the appropriate one when the non-1.4 version is good.
Attachment #8448116 - Flags: review?(josh)
This bug appears to be B2G specific. As such, I'm clearing the desktop status and tracking flags.
It is more important on B2G, but applies to all platforms.

1) It works even better on a desktop/laptop to cut the unnecessary requests down. In the watchposition case, it cuts requests from hundreds to, typically, one. 

2) It also provides better geolocation because of the caching logic. The wifi scanning subsystem is not reliable, for every good wifi scan on my macbook, the next 2-5 wifi scans come back as empty***. The caching logic ensures we don't let worse information trump previously better information.

*** I am still trying to find out if this is expected behaviour or a bug.
> These changes are basically fine, but I'd like to see you create a new test
> for request caching instead of repurposing test_cachedPositions, since
> they're fundamentally concerned with different things.

That test shouldn't have been on this bug. When I split the code to bug 1013012 this test should have gone with it.

> @@ +274,5 @@
> >      if (this.timer) {
> >        this.timer.cancel();
> >        this.timer = null;
> >      }
> > +    if (!this.timer) {
> 
> How can this ever not be the case, given the above?

It can't, this change must be on one of the recent Dolphin patches I was testing, and snuck in to my code. And... I just found it: 
http://mxr.mozilla.org/mozilla-b2g30_v1_4/source/dom/system/NetworkGeolocationProvider.js#109
  
> @@ +450,5 @@
> > +
> > +    LOG("Use request cache:" + useCached + " reason:" + gDebugCacheReasoning);
> > +
> > +    if (useCached) {
> > +      let newLoc = new WifiGeoPositionObject(gCachedRequest.location.coords.latitude,
> 
> Why build a new one instead of just using gCachedRequest.location?

My inclination would be to not do that since I assumed I would be sharing ownership of the cached internals. A bigger reason is that 99% of mochitests fail with that change :). I would like to dig more to find out why exactly this happens. This passing of objects from the JS to C++ layer is something I would like to better understand in general, a lot of the syntax looks unsafe to me because of lack of const, or lack of copy-on-write objects. 

> ::: dom/tests/mochitest/geolocation/test_errorcheck.html
> ::: dom/tests/mochitest/geolocation/test_timerRestartWatch.html

Both of these files look like the cache turn off/on for the test is done incorrectly on my part. I'll fix this up. These would have failed if the order of tests changed.
> > @@ +450,5 @@
> > > +
> > > +    LOG("Use request cache:" + useCached + " reason:" + gDebugCacheReasoning);
> > > +
> > > +    if (useCached) {
> > > +      let newLoc = new WifiGeoPositionObject(gCachedRequest.location.coords.latitude,
> > 
> > Why build a new one instead of just using gCachedRequest.location?
> 
> My inclination would be to not do that since I assumed I would be sharing
> ownership of the cached internals. A bigger reason is that 99% of mochitests
> fail with that change :). 

Oh my brain, I was running tests in a different order to see if the cache was shut off, nothing to do with this change. 
I see now why I had this bug together with bug 1013012. Splitting them up has confused me, they are intertwined.

This must return a cached network request position with a changed timestamp (the network cache is not the JS W3C position cached object, that is elsewhere), and this *arguably* should not return the original object to defend against any assumption of address-of equality by developers. Both are achieved by returning a new object. 

Looking again at 1013012 and deciding what to do here with this split.
Attached patch bug1018383_netgeoprov.diff (obsolete) — Splinter Review
Addressed Josh's review items.
Additionally, split changes to mochitest for cached position to bug 1035044.
Attachment #8448115 - Attachment is obsolete: true
Attachment #8451611 - Flags: review?(josh)
Comment on attachment 8451611 [details] [diff] [review]
bug1018383_netgeoprov.diff

Review of attachment 8451611 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! You can remove the 2) from the commit message, since it lacks a 1).

::: dom/tests/mochitest/geolocation/test_cachedPosition.html
@@ +23,5 @@
>  
>  resume_geolocationProvider(function() {
> +    force_prompt(true, function () {
> +      set_network_request_cache_enabled(false, function() {
> +        test1();

This can just be set_network_request_cache_enabled(false, test1).

::: dom/tests/mochitest/geolocation/test_timerRestartWatch.html
@@ +48,3 @@
>    // Now that we got a success callback, lets try to ensure
>    // that we get a timeout error.
> +  // THe network cache is already off, now stop the sjs from reponding to requests

s/THe/The/
Attachment #8451611 - Flags: review?(josh) → review+
Addressed items from Josh's review, carrying over the r+
Attachment #8451611 - Attachment is obsolete: true
Attachment #8451631 - Flags: review+
Comment on attachment 8448116 [details] [diff] [review]
1.4-patch-of-bug1018383.-Adds-local-request-cache-to.patch

The patch for m-c on this bug applies cleanly to 1.4
Attachment #8448116 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e58c1f146d75
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8451631 [details] [diff] [review]
bug1018383_netgeoprov.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Originally reported on Tarako as bug 1009592
User impact if declined: 
Excessive network requests when using geolocation. If the bug was important enough to fix for Tarako, it is important to fix for Dolphin (now that we know Dolphin has a non-functional GPS chip, it is identical to Tarako for geolocation). 
Testing completed: 
Tested on 1.4 inagi device. Don't have a functional Dolphin build to test with.
Risk to taking this patch (and alternatives if risky):
No known risks.  
String or UUID changes made by this patch:
None.
Attachment #8451631 - Flags: approval-mozilla-b2g30?
Renom'ing this bug for 1.4 because it was fixed for 1.3T as per Garvan's comment 48.
blocking-b2g: backlog → 1.4?
Regarding comment 48: I was able to test this on a Dolphin device.

My test: 
1) Moving test: I have a route from my house that is heavily stumbled and has <10 metres accuracy. Turn on google maps + cell data, walk my route, blue dot on google maps follows me perfectly with no jumping. Without the patch, jumping can occur when less-accurate scans occur. Check ADB logs, verify that network requests are being made to MLS only when cell/wifi data changes.
2) Non-moving test: Leave device on google maps for 5 minutes without moving, check ADB logs to ensure only a few network requests are made (2 or 3) rather than 60 (12/min) as the code is hard-coded to do without the patch. (In my location the connected cell tower can change even when not moving, which is why I see 2-3 requests, not just a single request)
Change the blocking-b2g back to 1.3T+ and go through the approval list. ni? Lawrence for the approval.
blocking-b2g: 1.4? → 1.3T+
Flags: needinfo?(lmandel)
Comment 51: could someone add a note for the newbie -me :)- as to what affect this has on the landing process for this bug.
No need for 1.3t+ here since this is fixed on tarako by bug 1009592. The b2g30 nomination is what we need for 1.4, but we also want to backport on 2.0 (ie aurora) I guess?
blocking-b2g: 1.3T+ → 1.4+
Fabrice is correct, bug 1009592 is the fix for 1.3t. That patch was an incomplete solution. This bug is the improved version of that patch.

For 2.0, if any device is planned that uses our geolocation stack (vs. the vendor replacing it with their proprietary geolocation stack), then yes we should have this fix.
Comment on attachment 8451631 [details] [diff] [review]
bug1018383_netgeoprov.diff

I'm clearing the approval nom. This bug is 1.4+ so you can land when ready.

This bug is also clear to land on 2.0 as Garvan tells me that this will only impact partners who use the Moz GPS stack.
Attachment #8451631 - Flags: approval-mozilla-b2g30?
Flags: needinfo?(lmandel)
Comment on attachment 8451631 [details] [diff] [review]
bug1018383_netgeoprov.diff

This fix needs to be everywhere or we will not be able to use GLS.
Attachment #8451631 - Flags: approval-mozilla-beta+
Attachment #8451631 - Flags: approval-mozilla-aurora+
Beta is missing a bunch of Geolocation uplifts that went to Aurora and b2g30. Consequently, this doesn't apply at all. Either it's going to need rebasing for uplift or a lot of other patches are going to need to be uplifted to beta as well.
Flags: needinfo?(gkeeley)
Rebased standalone patch for mozilla-beta.
Assignee: gkeeley → josh
Assignee: josh → gkeeley
Flags: needinfo?(gkeeley)
Thanks everyone.
Aurora just updated with the patch. Here is the log of leaving a page open with a watch position for a few minutes. Instead of ~100 network requests only one is made. 

That said, I am not convinced this patch is applicable to the GLS request problem.
On further thought, unless I am misunderstanding the GLS request problem, I definitely see it as a separate issue. On the desktop, very few web sites use watchPosition. 
For example, only the mobile version of Google Maps uses watchPosition (using a user-agent switcher will show this). The desktop version does not.

In order for this patch to also fix the GLS issue, there would have to be a major website that has switched to using watchPosition at around the time the request count went through the roof.
(In reply to Garvan Keeley [:garvank] from comment #63)
> On further thought, unless I am misunderstanding the GLS request problem, I
> definitely see it as a separate issue. On the desktop, very few web sites
> use watchPosition. 
> For example, only the mobile version of Google Maps uses watchPosition
> (using a user-agent switcher will show this). The desktop version does not.
> 
> In order for this patch to also fix the GLS issue, there would have to be a
> major website that has switched to using watchPosition at around the time
> the request count went through the roof.

We're looking to verify this fix for Firefox Desktop, but it's not clear to us whether this is possible, and if yes how can we do it. Could you help clarify this for us Garvan? If we can verify it, could you provide us with some testing details specific to Desktop?
Flags: needinfo?(gkeeley)
In about:config, set geo.wifi.logging.enabled to true.

Then run firefox from the command line, I do:
./firefox-bin | grep WIFI

All the debugging lines are prefixed with "WIFI". Compare a build with and without the patch on a page with a javascript watchPosition request such as:

https://thedotproduct.org/experiments/geo

Without the patch, you should see lots of logging indicating network requests are being made, and with the patch you should see very few network requests, and lots of messages saying the local network cache is being used.
Flags: needinfo?(gkeeley)
I`ve tested on Ubuntu 13.04 64bit and Mac OS X 10.9.4 using a Aurora build before the patch I get:
*** WIFI GEO: gls returned status: 200 --> {"location":{"lat":47.666667000000004,"lng":23.583333},"accuracy":18000}
*** WIFI GEO: Sending request: https://www.googleapis.com/geolocation/v1/geolocate?key=AIzaSyD-s-mXL4mBzF7KMRkhTCIbG2RKnRGXzJc
*** WIFI GEO: sending {}

Using Firefox 31.0RC I get no output in the console.

Using Latest Aurora:
*** WIFI GEO: Use request cache:true reason:New req. is GeoIP.
*** WIFI GEO: Use request cache:true reason:New req. is GeoIP.
*** WIFI GEO: Use request cache:true reason:New req. is GeoIP.

Using Latest Nightly:
*** WIFI GEO: Use request cache:true reason:New req. is GeoIP.
*** WIFI GEO: Use request cache:true reason:New req. is GeoIP.
*** WIFI GEO: Use request cache:true reason:New req. is GeoIP.

Is it ok that I don`t get anything in console using Firefox 31.0RC? I`ve tested on 31beta9 as well but with the same result.
Flags: needinfo?(gkeeley)
The debug flag in the prefs was broken in 31, so that is ok. All the other results look good.

(1) "WIFI GEO: Using the request cache:true" means we didn't hit the network. 

(2) "WIFI GEO: sending" means we made a network request.

Your results are exactly what we want to see, lots of (1) little of (2).
Flags: needinfo?(gkeeley)
(In reply to Garvan Keeley [:garvank] from comment #67)
> The debug flag in the prefs was broken in 31, so that is ok. All the other
> results look good.
> 
> (1) "WIFI GEO: Using the request cache:true" means we didn't hit the
> network. 
> 
> (2) "WIFI GEO: sending" means we made a network request.
> 
> Your results are exactly what we want to see, lots of (1) little of (2).

Great, thanks. Marking this verified as fixed then.
Blocks: 1028818
No longer depends on: 1028818
Duplicate of this bug: 1030216
You need to log in before you can comment on or make changes to this bug.