1121265 - CoreLocation provider only geolocates Wi-Fi APs, but GeoIP also needed

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[:garvan]

We should query MLS for GeoIP in the case that CoreLocation can't geolocate due to no Wi-Fi APs (either the device has no wifi, the wifi is off, or there are no known APs).

Hopefully CoreLocation returns an error that is specific to this case so it is easy to know when to call out to MLS for GeoIP.

Comment 1

[:garvan]

locationManager:didfailWithError: is called regularly during normal operation. This error callback can't be used to know when to call MLS. The error that is reported in both the successful case and the case where no APs are available is kCLErrorLocationUnknown. Indeed you read that right, in the case where the location is not immediately available (which seems to happen on every first use instance on my macbook), CL calls the error callback with kCLErrorLocationUnknown, then the success callback. 
The JS geo API therefore is also called in the same fashion, first the error, and then the success callback.

https://developer.apple.com/library/ios/documentation/CoreLocation/Reference/CLLocationManagerDelegate_Protocol/index.html#//apple_ref/occ/intfm/CLLocationManagerDelegate/locationManager:didFailWithError:

Comment 2

[:garvan]

Attachment: Fallback to NetworkGeolocationProvider

On CoreLocation geolocation error, fallback (a.k.a. handoff responsibility) to MLS network provider, which will report GeoIP, or error. 

Considerations:
- is there specific error codes that can be used to know exactly when to handoff to MLS?
No, the success case uses an error code of 0, which is also used in the error case when Wi-Fi is disabled. 

- are there any other conditions that can be used to determine whether NetworkGeolocationProvider is of value before instantiating it? 
We *could* check if there is Wi-Fi, and if Wi-Fi is available, and if not, bypass the CoreLocation provider. I didn't see any added value in this. 

- is the fact that NetworkGeolocationProvider attempting to perform a Wi-Fi scan a concern?
I think it adds unnecessary complexity (for little benefit) to add a GeoIP-only state.

- is it possible for both CoreLocation provider to query Apple's location service and NetworkGeolocationProvider to query MLS, thereby having an additional network call?
Yes, it is possible. Once CL gets a location, it will shutdown NetworkGeolocationProvider, but the latter may have already executed a request. In practice, this will rarely ever make a network request because of NetworkGeolocationProvider local caching, which in the GeoIP-only case, is guaranteed to use the cache.

Comment 3

[:garvan]

Two more notes:

1) Still testing this ATM

2) The obj-c formatting in this file is inconsistent. I'll add another patch to make the formatting in the entire file consistent. I need to figure out what our code standard is for obj-c. Also for C++ and obj-c I don't know if our coding standard is to place the pointer/address-of operator against the type or the name. Usually I just follow what is in the surrounding code. This file has no clear dominant style for me to follow.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> I'll add another patch to make the formatting in the entire file consistent.

Sounds like you're going to put all the style changes in a separate patch, which is good.  Please make sure the commit message makes clear that it only contains style changes.

Please don't put style-only changes in "substantive" patches -- that makes it a lot harder to track the history of code changes.  Do, though, rectify the style of code that's going to be changed anyway.

Comment 5

[:garvan]

Steven good point about muddling up this bug with that. I'll do another bug for re-formatting the un-touched code.
 
New code follows formatting conventions found in accessible/mac/*.mm and widget/cocoa.*mm:
- the * operator is nestled against the type
- functions are declared with a single space after '-'/'+' member decl., no other spaces
- multi-line functions are colon aligned
- longer line length than 80

Comment 6

[:garvan]

Attachment: Fallback to NetworkGeolocationProvider for GeoIP

Did some minor cleanup pre-review, see Comment 2 for details on this patch.

Comment 7

[:garvan]

For testing:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1121497 must be applied.
- Ensure you have both ethernet and Wi-Fi
- I tested disabling Wi-Fi before loading a page, also during a watch position.
- After disabling, reenabling Wi-Fi and reloading page, and during a watch position. 
- Did not test unknown Wi-Fi AP geolocation, because that is difficult to setup that scenario, but there is no reason this wouldn't call the CL error handler (which then triggers the MLS fallback).

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 8549750 [details] [diff] [review]
Fallback to NetworkGeolocationProvider for GeoIP

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

And yeah, we should think hard if we want to try to scan for wifi, or just do a XHR to MLS in this case.  I worry that the user's going to have to wait a long time for a course location.

::: dom/system/mac/CoreLocationLocationProvider.h
@@ +32,5 @@
>    CoreLocationLocationProvider();
>    void NotifyError(uint16_t aErrorCode);
>    void Update(nsIDOMGeoPosition* aSomewhere);
> +  void CreateGeoIPFallbackProvider();
> +  void CancelGeoIPFallbackProvider();

This is more than a GeoIP fallback.  Maybe you should call it C*MLSFallBackProvider?

Another thought is to just return an error to something higher in the stack (nsGeolocation.cpp), and have it manage the fallback provider.  You'll probably want this when the Windows provider is written and may/will need something similar.

::: dom/system/mac/CoreLocationLocationProvider.mm
@@ +52,5 @@
>  }
>  
> +- (void)shutdownHandoffTimer
> +{
> +  if (mHandoffTimer) {

nit: I typically like to return early to avoid the intention.

if (mHandoffTimer == nil) {
  return;
}
...
...

@@ +84,5 @@
>    if ([aError code] == kCLErrorDenied) {
>      err = nsIDOMGeoPositionError::PERMISSION_DENIED;
>    }
>  
> +  if (!mHandoffTimer) {

I think this should only fire when err == POSITION_UNAVAILABLE.  We should return the error immediately if it denied, right?

@@ +97,5 @@
> +    mHandoffTimer = [NSTimer scheduledTimerWithTimeInterval:2.0
> +                                              target:self
> +                                            selector:@selector(handoffToGeoIPProvider)
> +                                            userInfo:nil
> +                                             repeats:NO];

nit: This whitespace is wrong.

@@ +106,5 @@
>  {
> +  [self shutdownHandoffTimer];
> +  mProvider->CancelGeoIPFallbackProvider();
> +
> +  NS_ASSERTION(aLocations.count > 0, "Unexpected faiure: didUpdateLocations got empty array.");

why this assertion?  can there be a case where there are no locations, and we just want to return early?

@@ +143,5 @@
> +
> +NS_IMETHODIMP
> +MLSUpdate::Update(nsIDOMGeoPosition *position)
> +{
> +  nsCOMPtr<nsIDOMGeoPositionCoords> coords;

guard for null.  iirc, position can be null

@@ +285,5 @@
> +  mGeoIPFallbackProvider = do_CreateInstance("@mozilla.org/geolocation/mls-provider;1");
> +  if (mGeoIPFallbackProvider) {
> +    nsresult rv = mGeoIPFallbackProvider->Startup();
> +    if (NS_SUCCEEDED(rv)) {
> +      nsRefPtr<MLSUpdate> update = new MLSUpdate(*this);

How does the lifecycle of MLSUpdate work?  Are we sure that we don't leak or use-after-free?

Would it make sense to just make this class concrete and part of CoreLocationLocationProvider

Comment 9

[:garvan]

> And yeah, we should think hard if we want to try to scan for wifi, or just
> do a XHR to MLS in this case.  I worry that the user's going to have to wait
> a long time for a course location.

A 2-3 sec on first response, and while the MLS provider is alive, exactly 2 second response on consecutive coarse location requests.

To explain, the code flow is:
- Try CL request
-- on error (such as no wi-fi, or wi-fi APs not recognized) start MLS provider
---- 2 secs later, MLS provider makes request (which is for the purpose of GeoIP).
------ A) if first request since MLS provider has been instantiated: response in the time it takes for MLS server to respond (lets say 1 sec).
------ B) or, immediate response from cache, if this is a consecutive request and the MLS provider has not been shutdown yet.

> ::: dom/system/mac/CoreLocationLocationProvider.h
> @@ +32,5 @@
> >    CoreLocationLocationProvider();
> >    void NotifyError(uint16_t aErrorCode);
> >    void Update(nsIDOMGeoPosition* aSomewhere);
> > +  void CreateGeoIPFallbackProvider();
> > +  void CancelGeoIPFallbackProvider();
> 
> This is more than a GeoIP fallback.  Maybe you should call it
> C*MLSFallBackProvider?

I'll rename it as I trust reviewer's feel for what names make sense. I wanted the next person reading the code to know *exactly* why we were using the MLS provider (I think I documented that in enough places in the code), I am concerned about someone not understanding that Core Location doesn't provide GeoIP (which is seriously weird). 
It is *possible* for the MLS provider to return a non GeoIP request.

> Another thought is to just return an error to something higher in the stack
> (nsGeolocation.cpp), and have it manage the fallback provider.  You'll
> probably want this when the Windows provider is written and may/will need
> something similar.

We don't need to do this for the Windows provider, it provides GeoIP from my understanding. 

> ::: dom/system/mac/CoreLocationLocationProvider.mm
> @@ +52,5 @@
> >  }
> >  
> > +- (void)shutdownHandoffTimer
> > +{
> > +  if (mHandoffTimer) {
> 
> nit: I typically like to return early to avoid the intention.

Agreed, me too.
 
> 
> @@ +84,5 @@
> >    if ([aError code] == kCLErrorDenied) {
> >      err = nsIDOMGeoPositionError::PERMISSION_DENIED;
> >    }
> >  
> > +  if (!mHandoffTimer) {
> 
> I think this should only fire when err == POSITION_UNAVAILABLE.  We should
> return the error immediately if it denied, right?

Whoopsy. yes. 

> @@ +106,5 @@
> >  {
> > +  [self shutdownHandoffTimer];
> > +  mProvider->CancelGeoIPFallbackProvider();
> > +
> > +  NS_ASSERTION(aLocations.count > 0, "Unexpected faiure: didUpdateLocations got empty array.");
> 
> why this assertion?  can there be a case where there are no locations, and
> we just want to return early?

By contract/apple docs, this is supposed to contain a position object, I want the reader or the code to see this confirmed in some way. 

I prefer returning to an assertion actually. I couldn't find an example of:
if (precondition_unexpectedly_false) {
  WHAT_LOG_MACRO_DO_WE_USE("If you see this, please file a bug so it can be tracked");
  return;
}
 
> @@ +143,5 @@
> > +
> > +NS_IMETHODIMP
> > +MLSUpdate::Update(nsIDOMGeoPosition *position)
> > +{
> > +  nsCOMPtr<nsIDOMGeoPositionCoords> coords;
> 
> guard for null.  iirc, position can be null

Will do.
 
> @@ +285,5 @@
> > +  mGeoIPFallbackProvider = do_CreateInstance("@mozilla.org/geolocation/mls-provider;1");
> > +  if (mGeoIPFallbackProvider) {
> > +    nsresult rv = mGeoIPFallbackProvider->Startup();
> > +    if (NS_SUCCEEDED(rv)) {
> > +      nsRefPtr<MLSUpdate> update = new MLSUpdate(*this);
> 
> How does the lifecycle of MLSUpdate work?  Are we sure that we don't leak or
> use-after-free?

Leak no. [Aside: assigning this to nsRefPtr is unnecessary, I just followed the Gonk provider code style which had this. I would have just done mGeoIPFallbackProvider->Watch(new MLSUpdate(*this)) ]
The JS XPCOM implementation of Watch() wraps this to an nsCOMPtr and it is appropriately refcounted. It is held as a member of WifiGeoPositionProvider object. 
Use-after-free no, the MLSUpdate callback object never gets callbacks after the WifiGeoPositionProvider is shutdown. 
[Use-after-free would have extra likelihood of fatality because the MLSUpdate holds a reference to a CoreLocationLocationProvider instance which is likely gone.]

> Would it make sense to just make this class concrete and part of
> CoreLocationLocationProvider

MLSUpdate? I understand the term 'concrete' to mean 'non-abstract', I think you mean it differently. 
Anyhoo, declaring it as 
class CoreLocationLocationProvider{
class MLSUpdate }}
is fine by me. I treat classes local to the compilation unit as the same meaning as that, but in this case there is yet another class declared in the cpp, which makes things less clear.

Comment 10

[:garvan]

Attachment: CL_1121265.diff

Updated for your perusal, as per my comments to your comments :).

Comment 11

[:garvan]

Attachment: CoreLocation fallback to MLS GeoIP

Carry over r+ from dougt, updated patch for different clang version on build machines.

Try run green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77561076bc2b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f507c077b24

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back out this changes in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e28ef6984108 since one of this changes caused a bustage in  OS X static analysis builds like:

https://treeherder.mozilla.org/logviewer.html#?job_id=5955163&repo=mozilla-inbound

Comment 14

[:garvan]

Attachment: CoreLocation fallback to MLS GeoIP

Blocking implicit constructor conversions is a good thing. (wish i knew how to set that up locally)
Updated patch with 'explicit' keyword on constructor.
Do you know if there is some way I can run this static analysis on try? Running static analysis locally doesn't repro (I am just using the default clang static analyzer).

Comment 15

[:garvan]

Hi, do you know if there is some way I can run this static analysis on try?

Comment 16

[:garvan]

secret try option macosx64-st-an :)

bug 1126385

Will run it now

Comment 17

[:garvan]

Yay! Try green for static analysis:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a337bbb2113d

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a90a7ba532a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a90a7ba532a