Closed Bug 460063 Opened 13 years ago Closed 13 years ago

Geolocation prompt should not appear if there are no providers installed

Categories

(Core :: DOM: Geolocation, defect)

x86
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: bzbarsky, Assigned: dougt)

References

()

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 3 obsolete files)

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.
Flags: blocking1.9.1?
Assignee: nobody → doug.turner
Component: DOM → Geolocation
QA Contact: general → geolocation
Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #344092 - Flags: superreview?
Attachment #344092 - Flags: review?
Attachment #344092 - Flags: superreview?(bzbarsky)
Attachment #344092 - Flags: superreview?
Attachment #344092 - Flags: review?(Olli.Pettay)
Attachment #344092 - Flags: review?
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?
@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
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.
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.
> 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)?
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.
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?
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.
Setting the flag sounds good to me.
Attached patch patch v.2 (with the flag) (obsolete) — Splinter Review
Attachment #344092 - Attachment is obsolete: true
Attachment #344935 - Flags: superreview?
Attachment #344935 - Flags: review?
Attachment #344092 - Flags: superreview?(bzbarsky)
Attachment #344092 - Flags: review?(Olli.Pettay)
Attachment #344935 - Flags: superreview?(bzbarsky)
Attachment #344935 - Flags: superreview?
Attachment #344935 - Flags: review?(Olli.Pettay)
Attachment #344935 - Flags: review?
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 ?)
Attachment #344935 - Flags: review?(Olli.Pettay) → review-
Attached patch patch v.3 (obsolete) — Splinter Review
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).
Attachment #344935 - Attachment is obsolete: true
Attachment #345323 - Flags: review?
Attachment #344935 - Flags: superreview?(bzbarsky)
Attachment #345323 - Flags: review? → review?(Olli.Pettay)
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?
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 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)
Attachment #345323 - Flags: review?(Olli.Pettay) → review-
Attached patch patch v.4Splinter Review
this allows up to restart the provider after a disconnect timeout.  nice find smaug!
Attachment #345323 - Attachment is obsolete: true
Attachment #345538 - Flags: review?
Attachment #345538 - Flags: review? → review?(Olli.Pettay)
Attachment #345538 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 345538 [details] [diff] [review]
patch v.4

But I really would like to see some tests for the disconnect timeout.
Comment on attachment 345538 [details] [diff] [review]
patch v.4

me too.  let me build some.
Attachment #345538 - Flags: superreview?(bzbarsky)
test case in 462564.
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.
Attachment #345538 - Flags: superreview?(bzbarsky) → superreview+
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
pushed a1364547a182.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.