1129444 - OS X CoreLocation provider is falling back to MLS too quickly

Status: RESOLVED WORKSFORME

(Core :: DOM: Geolocation, defect)

Description

[:garvan]

The way the code works is to try and get a location from CL using wifi (if possible), and fallback to MLS typically for GeoIP.

The telemetry results on nightly indicate that the code is falling back to MLS in nearly every case.

Comment 1

[:garvan]

Attachment: MLS fallback based on wifi state

Checks the wifi state to fallback to MLS,
if no wifi APs are found, fallback immediately,
otherwise wait 5 seconds to give CL a head start.

Reviewer: I am going to land bug 1129696 (retain/release missing) first, and check the results of that fix on nightly. I'd like to see the telemetry on known 'good' code before landing this patch on nightly, and then compare the results.

Comment 2

[:garvan]

Comment on attachment 8559806 [details] [diff] [review]
MLS fallback based on wifi state

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

::: dom/system/mac/CoreLocationLocationProvider.mm
@@ +115,5 @@
>    if (!mHandoffTimer) {
> +    NSTimeInterval startupDelay = (mProvider->DidWifiScannerReportNoVisibleAPs()) ?
> +                                   0 : MLS_FALLBACK_DELAY_STARTUP_SEC;
> +    
> +    printf("MLS startup delay %d\n" , (int)startupDelay);

Oops, will remove this.

@@ +175,5 @@
>    if (!coords) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  printf("CoreLocationLocationProvider got MLS location\n");

Can I leave this in? This info is really critical for bug reports.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Garvan Keeley [:garvank] from comment #1)
> Checks the wifi state to fallback to MLS,
> if no wifi APs are found, fallback immediately,
> otherwise wait 5 seconds to give CL a head start.

Instead of waiting 5 seconds before sending the MLS request, can you send CoreLocation and MLS requests in parallel? You can hold on to the MLS response until CoreLocation returns an error or the 5 second timeout expires, at which time you can return the MLS location immediately.

Comment 4

[:garvan]

Either of these will work:
1) make a request to MLS immediately, and wait X seconds to respond, 
or
2) wait X seconds to request MLS location. This will make 1 network call instead of 2 in most cases. 

There must be an arbitrary wait period to allow CL to respond. The CL error callback happens both when it doesn't have a cached location, and when I have turned off wifi on my mac. Both have the same error code. 

The wait time is could be shortened, I certainly never see my mac taking 5 seconds to get CL network response, maybe 1-3 seconds. I should have telemetry on this so we can adjust this logic.

I should link to the ADC on CL error in case anyone sees something my eyes didn't:
https://developer.apple.com/library/ios/documentation/CoreLocation/Reference/CLLocationManagerDelegate_Protocol/index.html#//apple_ref/occ/intfm/CLLocationManagerDelegate/locationManager:didFailWithError:

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Garvan Keeley [:garvank] from comment #4)
> Either of these will work:
> 1) make a request to MLS immediately, and wait X seconds to respond, 
> or
> 2) wait X seconds to request MLS location. This will make 1 network call
> instead of 2 in most cases. 

I was originally unconcerned about having two network calls, but I now realize that one of those network calls will hit a Mozilla server, which costs us money even if the client uses the CoreLocation result. :)

Your current solution sounds like the right one (and you've already written the code).

Comment 6

[Hanno Schlichting]

(In reply to Chris Peterson [:cpeterson] from comment #5)
> I was originally unconcerned about having two network calls, but I now
> realize that one of those network calls will hit a Mozilla server, which
> costs us money even if the client uses the CoreLocation result. :)

I talked with Garvan about this. The number of geo requests from OS X users is so small, it has no measurable financial impact for Mozilla.

But Garvan had some more ideas about this bug.

Comment 7

[:garvan]

Except now Hanno has me leaning your way :). The extra request he feels is not significant (relative all the other traffic), and it is more deterministic your way.

To explain the determinism:
- lets say we want a response from either one in 5 sec
- Method A) query MLS at 0s, get a response at 2-3s, wait until 5s exactly to return it
- Method B) query MLS at 3s, get a response at 5-6s, and return it immediately

Comment 8

[:garvan]

Comment on attachment 8559806 [details] [diff] [review]
MLS fallback based on wifi state

Not quite ready for review, but feedback welcome

Comment 9

[:garvan]

Comment on attachment 8559806 [details] [diff] [review]
MLS fallback based on wifi state

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

Clearing review request for now. I am not currently seeing that GeoIP is being returned significantly more frequently with the original code, which I prefer for its simplicity.
Will add telemetry ASAP and conclude from that what approach we would like to take.

Comment 10

[:garvan]

Based on the telemetry on nightly: http://mzl.la/1A2B8KP
99% of geolocation results are Core Location, not MLS.

Also, the only determinant I see to decide MLS vs CL is the wifi state, but since all macs come with wifi, and this would rarely be disabled, this isn't a very useful determinant.

Closing bug.