874996 - Geolocation providers have no way of notifying errors back to nsGeolocation

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Guilherme Gonçalves [:ggp]]

Geolocation providers might need to report errors back to nsGeolocation.

In particular, the CoreLocation provider being worked on in Bug 874587 causes OSX to ask the user for permission independently of our doorhanger; it is possible to detect in the provider whether this OSX-specific permission check fails, but there's no way to report that back to nsGeolocation so we can issue a PERMISSION_DENIED error event (or whatever other behavior is reasonable).

Comment 1

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874996 - Issue error events for geolocation provider errors

This patch adds an Error method to nsIGeolocationUpdate and makes the classes nsGeolocationRequest and Geolocation implement it (which they kind of already did by having an Update method).

We can now be notified of erros from the providers by having them call nsGeolocationService::Error (which forwards the error to Geolocation objects, which in turn forward it to their active requests) and dispatch error events when that happens.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 753469 [details] [diff] [review]
Bug 874996 - Issue error events for geolocation provider errors

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

jdm, can you look at this.  if not, please reassign it back to me.

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 753469 [details] [diff] [review]
Bug 874996 - Issue error events for geolocation provider errors

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

::: dom/src/geolocation/nsGeolocation.cpp
@@ +580,5 @@
> +nsGeolocationRequest::Error(uint16_t aErrorCode)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!mAllowed) {

Check for mCleared as well, or we'll end up notifying cancelled watchers.

@@ +875,5 @@
> +    Error(nsIDOMGeoPositionError::POSITION_UNAVAILABLE);
> +    return rv;
> +  }
> +
> +  rv = mProvider->Watch(this, aRequestPrivate);

Duplicate of the above; remove.

@@ +1177,5 @@
> +    Shutdown();
> +    return NS_OK;
> +  }
> +
> +  for (uint32_t i = mPendingCallbacks.Length(); i> 0; i--) {

Space before >

@@ +1184,5 @@
> +    }
> +  }
> +
> +  // notify everyone that is watching
> +  for (uint32_t i = 0; i< mWatchingCallbacks.Length(); i++) {

Space before <

::: xpcom/system/nsIGeolocationProvider.idl
@@ +12,4 @@
>  interface nsIDOMGeoPosition;
>  interface nsIGeolocationPrompt;
>  
> +

*snick*

@@ +37,5 @@
> +   * provider; for errors occurring inside the methods in
> +   * the nsIGeolocationProvider interface, just use the return
> +   * value.
> +   */
> +  void error(in unsigned short error);

I would prefer notifyError instead.

Comment 4

[Guilherme Gonçalves [:ggp]]

(In reply to Josh Matthews [:jdm] from comment #3)
> Check for mCleared as well, or we'll end up notifying cancelled watchers.

The previous NotifyError, which I pretty much just moved further down in the file, doesn't do that check (in fact, it doesn't even check for mAllowed). Unfortunately, adding the mCleared check breaks test_cancelCurrent.html, which expects to call NotifyError on a cleared request to issue a PERMISSION_DENIED event.

So while fixing this comment I realized that we don't really seem to need mAllowed and mCleared at all, provided we make sure a request is only added to a list of pending callbacks after being allowed and remove it when it would otherwise be cleared.

I'll attach a patch for that. There's still some room for refactoring, but let me know if you don't like where it's going and I'll be glad to revisit my first patch.

Comment 5

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874996 - Simplify handling of geolocation requests

This patch makes nsGeolocationRequests only be added to the lists of pending requests once they are allowed and expecting a new position, and be removed from those lists when they would otherwise just be cleared.

The lifetime of the requests should hopefully be easier to reason about now: they are either fulfilled by the cached position (in which case Geolocation never even learns about them), or added to one of the lists of pending requests, from which they are removed either when the first position arrives (for getCurrentPosition) or when clearWatch is called.

Comment 6

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874996 - Simplify handling of geolocation requests

Sorry, the changes to Geolocation::ServiceReady weren't really necessary. Refreshing the patch.

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 756803 [details] [diff] [review]
Bug 874996 - Simplify handling of geolocation requests

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

This is a sensible change, and I think it's much easier to reason about the geolocation code now. I have one concern - geolocation permission prompts don't stack, which means if multiple geolocation requests are made before the user approves the prompt, only the most recent will see any activity, while the previous one will silently die each time a new request is made. On the other hand, that's not really a user-visible behaviour change from the existing setup, where they just sit around and are never approved (see bug 683472). Therefore, I think this change to acceptable.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +446,5 @@
>        ( PRTime(PR_Now() / PR_USEC_PER_MSEC) - maximumAge <=
>          PRTime(cachedPositionTime) )) {
>      // okay, we can return a cached position
> +    // we will now be owned by the RequestSendLocationEvent
> +    Update(lastPosition);

Won't this mean that watchPosition requests services from the cache will never see any future updates?

@@ +1345,5 @@
> +  } else {
> +    mPendingCallbacks.AppendElement(aRequest);
> +  }
> +
> +  return;

Scrap this.

Comment 8

[Guilherme Gonçalves [:ggp]]

Attachment: Simplify handling of geolocation requests

Refreshing patch with jdm's comments and carrying over r+

Comment 9

[Guilherme Gonçalves [:ggp]]

Attachment: Issue error events for geolocation provider errors

Issue error events for geolocation provider errors

Comment 10

[Guilherme Gonçalves [:ggp]]

https://tbpl.mozilla.org/?tree=Try&rev=2d4032cc462e . This includes one more patch that just ensures I didn't break watchPosition again.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 760960 [details] [diff] [review]
Issue error events for geolocation provider errors

-  mProvider->Startup();
-  mProvider->Watch(this, aRequestPrivate);
+  nsresult rv;
+
+  if (NS_FAILED(rv = mProvider->Startup()) ||
+      NS_FAILED(rv = mProvider->Watch(this, aRequestPrivate))) {
+
+    NotifyError(nsIDOMGeoPositionError::POSITION_UNAVAILABLE);
+    return rv;
+  }

At least on the Mac, I think we need to distinguish here between an
ordinary POSITION_UNAVAILABLE error and one that's due to the user
having denied Firefox permission to use	Location Services (for which
we'd presumably return PERMISSION_DENIED).

On OS X	10.7 and up, OS	X has system settings to globally turn
Location Services on or off, and to allow individual apps to access it
or not.	 The UI	for the	global setting is reasonably well designed --
if Location Services is off, you're (usually) prompted to turn it on
when your app tries to use it.	But Apple's UI for the app-level
settings is *not* well designed.  For every app	on your	system,	you're
prompted exactly once to choose	whether	to allow or deny it access to
Location Services.  If you deny	it access, all subsequent attempts to
use Location Services fail silently.  So it's all too easy to forget
about a decision you probably made on	the spur of the	moment, possibly
months or years in the past.	

To make	up for Apple's bad design, we need to distinguish between
permission failures and other kinds of failures.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #11)

And no, upgrading or moving an app doesn't cause the OS to lose its authorization setting (the flag that indicates whether or not it's authorized to use Location Services).

Comment 13

[Josh Matthews [:jdm]]

What you say makes sense. We can probably specify that returning a particular error code from Startup (such as NS_ERROR_NOT_AVAILABLE) will indicate a PERMISSION_DENIED error.

Comment 14

[Guilherme Gonçalves [:ggp]]

I don't think we need that change right now, at least on Mac.

With the CoreLocation provider (bug 874587), with which Steven is concerned, we have an error callback (didFailWithError) that's called when permission is denied (either silently or explicitly by the user). My plan was to use NotifyError, introduced in this bug, to then cause a permission denied event to be raised.

Speaking from memory, the CoreLocation provider initializes correctly even when we don't have permissions. That is, Startup() and Watch() will always return NS_OK, and the user will never see a POSITION_UNAVAILABLE event because of permissions. Steven, please correct me if you observed otherwise while testing.

So it looks like we don't need this change in order to accommodate permission denied errors on Mac. However, if this could be an issue for another provider, I'm ok with including it.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

> Steven, please correct me if you observed otherwise while testing.

I didn't add logging to the patch code when testing.  So I was just guessing that Startup() and/or Watch() would fail when we don't have permission from the OS.

> My plan was to use NotifyError, introduced in this bug, to then cause a
> permission denied event to be raised.

I didn't fully understand this.

I'm fine with whatever is done here and at bug 874587, as long as the user is told that the errors that happen because an app has been denied permission to use Location Services are reported to the user as permission errors.

Comment 16

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874996 - Part 1 - Simplify handling of geolocation requests

The previous version of the patch had a few problems that kept requests alive for longer than they should.

For one, we can't just call Shutdown() on the destructor, since Shutdown() is supposed to cancel and release the timer, which holds a reference back to the request. Thus, without calling Shutdown(), the destructor is never called.

Another problem is that, for watchPosition requests, an event handler might call clearWatch(). This version of the patch reintroduces mCleared (now called mShutdown) so that watchPosition requests don't set timers to keep running after being cleared from event handlers.

jdm, could you please have another look at this? These aren't huge changes, but I wanted to make sure I got this right. Try seems fine with it: https://tbpl.mozilla.org/?tree=Try&rev=f2b991daf591

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 762369 [details] [diff] [review]
Bug 874996 - Part 1 - Simplify handling of geolocation requests

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

Do we have any tests that call clearWatch during the handler? If not, please add one.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +548,2 @@
>  bool
>  nsGeolocationRequest::Update(nsIDOMGeoPosition* aPosition)

This can return void now.

Comment 18

[Guilherme Gonçalves [:ggp]]

(In reply to Josh Matthews [:jdm] from comment #17)
> Comment on attachment 762369 [details] [diff] [review]
> Bug 874996 - Part 1 - Simplify handling of geolocation requests
> 
> Review of attachment 762369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have any tests that call clearWatch during the handler? If not, please
> add one.
> 

We do -- test_manyWatchSerial.html

Comment 19

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874996 - Part 1 - Simplify handling of geolocation requests

Refreshing patches and keeping r+ from jdm.

Comment 20

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874996 - Part 2 - Issue error events for geolocation provider errors

Comment 21

[John Schoenick [:johns]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ce5983b285
https://hg.mozilla.org/integration/mozilla-inbound/rev/59e7f2102e16

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f1ce5983b285
https://hg.mozilla.org/mozilla-central/rev/59e7f2102e16