460063 - Geolocation prompt should not appear if there are no providers installed

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current trunk build

STEPS TO REPRODUCE:
1) Run the javascript: URI in the URL field

EXPECTED RESULTS:  User is not prompted to allow or deny, since there is no location information to report anyway.  Just return the right error to the page.

ACTUAL RESULTS: User is prompted as to whether a location should be provided to the web page. This makes it seem like the webpage _could_ conceivably get the location, when in fact it can't.  This is a serious problem in terms of user perception of the privacy status of our product.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

I expose a Init() method off of the nsGeolocationRequest object that is called before the user sees any dialog.  This method is responsible for doing any precheck to see if a dialog should be displayed.  Currently, it simply looks to see if we have a geolocation provider available.  If it doesn't, we send an error callback and bail.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Is synchronously notifying the callback OK per spec?

Also, we call StartDevice() on every geolocation get attempt, right?  Is it really OK to call Startup() on an already-started provider?  And tell it to watch |this| again?

Comment 3

[Doug Turner (:dougt)]

@bz Nothing is the spec prevents a synchronous callback.  but now that you point that out, i wonder if it is good form.  Thoughts bz?


It is fine to call start device multiple times:
  http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/geolocation/nsIGeolocationProvider.idl#108

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  Is calling watch() with the same callback multiple times good too?  Clearly allowed, but will it have the behavior we want?

No opinion on the sync-vs-async issue, really; just worth checking with the WG.

Comment 5

[Doug Turner (:dougt)]

it is allowed and the behavior isn't that terrible.  I would there may be embeders also consuming from the geolocation provider's interface as well.  More documentation on how to write a geolocation provider is needed.  Part of this will be fleshed out in bug 459073.

I will talk with the WG wrt the functionality here.  I do not think we need to block for it.  we can do it sync for now, then go the other way if we decide that an asynchronous response is desired.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> and the behavior isn't that terrible

So we won't get tons of callbacks on every single location change (one per past location request)?

Comment 7

[Doug Turner (:dougt)]

well, the sync callback we are talking about only happens when someone initially calls watchPosition or getCurrentPosition.  This only happens one in most cases.

So, if you call watchPosition with a positionErrorCallback, and there isn't a geolocation provider installed, you will get a call on positionErrorCallback before watchPosition returns.

Other errors (for example, the GPS not acquiring, or the user presses cancel) will happen well after watchPosition returns.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm not talking about the sync callback.  I'm talking about the

  mProvider->Watch(this);

call which is going from being called once to being called once per getCurrentPosition/watchPosition.  Say a page calls getCurrentPosition 1000 times and then the position changes.  Are we supposed to get 1000 calls to nsGeolocationService::Update?  What's preventing that from happening?

Comment 9

[Doug Turner (:dougt)]

Boris, this should work fine, but as you point out, it is not ideal.  The |this| passed to the mProvider will never change between the multiple calls - it will always be "the geolocation service".  To reduce this unnecessary startup+watch calls, the service could set a flag that indicates that this has already been done.  Would that work for you?  I could do this here, or in the overhaul of this code in bug 454490.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Setting the flag sounds good to me.

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.2 (with the flag)

Comment 12

[Olli Pettay [:smaug]]

Comment on attachment 344935 [details] [diff] [review]
patch v.2 (with the flag)

This patch isn't generated against trunk, right? r-
 
>@@ -434,16 +450,25 @@ nsGeolocationService::nsGeolocationServi
>     // point in meters, and the direction in degrees
>     mDisplacementMagnitude = rand() % DISPLACEMENT_MAX_DISTANCE_IN_METERS;
>     mDisplacementDirection = rand() % 360;
> 
>     nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
>     pref->SetIntPref("geo.displacement.magnitude", mDisplacementMagnitude);
>     pref->SetIntPref("geo.displacement.direction", mDisplacementDirection);
>   }
>+
>+  mProvider = do_GetService(NS_GEOLOCATION_PROVIDER_CONTRACTID);
>+
>+  // if NS_MAEMO_LOCATION, see if we should try the MAEMO location provider
>+#ifdef NS_MAEMO_LOCATION
>+  if (!mProvider)
>+    mProvider = new MaemoLocationProvider();
>+#endif
>+
Are you moving these to ctor? Why? IMO better to keep any object
creationg in Init()/Startup() or some such so that it is easier to handle OOM.


> nsGeolocationService::StartDevice()
I think this method would be easier to read if you
return early if provider has been started.

   
>     // if we have one, start it up.
>     nsresult rv = mProvider->Startup();
>     if (NS_FAILED(rv)) 
>       return NS_ERROR_NOT_AVAILABLE;
>- 
>+  
No need for this whitespace change.

>     // lets monitor it for any changes.
>     mProvider->Watch(this);
>-    
>+
>+    // remember that we are started up
>+    mProviderStarted = PR_TRUE;
>+
>     // we do not want to keep the geolocation devices online
>     // indefinitely.  Close them down after a reasonable period of
>     // inactivivity
>     SetDisconnectTimer();
>+
nor this. (Or is this patch -w ?)

Comment 13

[Doug Turner (:dougt)]

Attachment: patch v.3

Cleaned up patch.  I will follow up with another bug/patch that moves this code out of the constructor and puts it into an ::Init method (see bug 462192).

Comment 14

[Olli Pettay [:smaug]]

Comment on attachment 345323 [details] [diff] [review]
patch v.3

>+
>+  mProvider = do_GetService(NS_GEOLOCATION_PROVIDER_CONTRACTID);
>+  
>+  // if NS_MAEMO_LOCATION, see if we should try the MAEMO location provider
>+#ifdef NS_MAEMO_LOCATION
>+  if (!mProvider)
>+    mProvider = new MaemoLocationProvider();
>+#endif
>+
Why you need to move these from StartDevice to ctor?
Shouldn't it be ok to create mProvider when StartDevice
is first time called?

Comment 15

[Doug Turner (:dougt)]

StartDevice occurs after the user give permission.  This is wanted because we do not want to turn on any geolocation device (think GPS) without the user's expressed permission.  I needed to move looking up the mProvider into the service's ctor because I want to immediately post from getCurrentPosition and watchPosition if no provider is available.

Comment 16

[Olli Pettay [:smaug]]

Comment on attachment 345323 [details] [diff] [review]
patch v.3

Per IRC, I'm expecting a new patch. (Would be really great to have tests to check that everything works after disconnect timer)

Comment 17

[Doug Turner (:dougt)]

Attachment: patch v.4

this allows up to restart the provider after a disconnect timeout.  nice find smaug!

Comment 18

[Olli Pettay [:smaug]]

Comment on attachment 345538 [details] [diff] [review]
patch v.4

But I really would like to see some tests for the disconnect timeout.

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 345538 [details] [diff] [review]
patch v.4

me too.  let me build some.

Comment 20

[Doug Turner (:dougt)]

test case in 462564.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 345538 [details] [diff] [review]
patch v.4

Fix the whitespace weirdness that seems to be present in some places, and looks good.

Comment 22

[Doug Turner (:dougt)]

Attachment: test cases for this change.

Comment 23

[Doug Turner (:dougt)]

pushed a1364547a182.