493615 - Allow Addons the use of the DOM Geolocation API

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

Right now, addon developers have to use a different API than those developers building websites.  It would be great if these two APIs were the same.

Webdevelopers use:
https://developer.mozilla.org/En/Using_geolocation

Addons use:

function foo(position) {}

var geoprovider = Cc["@mozilla.org/geolocation/provider;1"].getService(Ci.nsIGeolocationProvider);
geoprovider.startup();
geoprovider.watch(foo);

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

As long as chrome code doesn't need to access the geolocation object via the navigator object.

Comment 2

[Doug Turner (:dougt)]

right, there should be a separate way to get the geolocation object.

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.1 (WIP)

Comment 4

[Doug Turner (:dougt)]

Attachment: example component

Sample Component

Comment 5

[Fabrice Desré]

This patch works well for me (tested on Fennec).

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 398165 [details]
example component

createInstance should be getService.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 398164 [details] [diff] [review]
patch v.1 (WIP)

> PRBool
> nsGeolocation::OwnerStillExists()
> {
>   nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mOwner);
> 
>-  if (!window)
>-    return PR_FALSE;
>-
>   if (window)
>   {
>     PRBool closed = PR_FALSE;
>     window->GetClosed(&closed);
>     if (closed)
>       return PR_FALSE;
>+
>+    nsPIDOMWindow* outer = window->GetOuterWindow();
>+    if (!outer || outer->GetCurrentInnerWindow() != window)
>+      return PR_FALSE;
>   }
> 
>-  nsPIDOMWindow* outer = window->GetOuterWindow();
>-  if (!outer || outer->GetCurrentInnerWindow() != window)
>-    return PR_FALSE;
>-
>   return PR_TRUE;

So if the content window has disappeared, you return true. Does look quite right.
Should you add
NS_ENSURE_TRUE(!mOwner || window, PR_FALSE);

Comment 8

[Doug Turner (:dougt)]

well, if there is no window, then this object is being used by an addon/chrome.

Comment 9

[Will Wagner]

 (In reply to comment #0)
> Right now, addon developers have to use a different API than those developers
> building websites.  It would be great if these two APIs were the same.
> 
> Webdevelopers use:
> https://developer.mozilla.org/En/Using_geolocation
> 
> Addons use:
> 
> function foo(position) {}
> 
> var geoprovider =
> Cc["@mozilla.org/geolocation/provider;1"].getService(Ci.nsIGeolocationProvider);
> geoprovider.startup();
> geoprovider.watch(foo);

FWIW, this was a glimmer of hope for me because I've been banging my head trying to get geolocation working with my addon.  Unfortunately, I don't think the original workaround of calling the nsIGeoLocationProvider directly actually works for addons that are using the wifi service.  If you look at the NetworkGeoLocation provider the watch method doesn't add callbacks to any list; it simply starts up the service if it's not running:

194     watch: function(c) {
195         LOG("watch called");
196         if (!this.wifi_service) {
197             this.wifi_service = Cc["@mozilla.org/wifi/monitor;1"].getService(Components.interfaces.nsIWifiMonitor);
198             this.wifi_service.startWatching(this);
199         }
200     },

When an position changes, it calls directly into the geolocation service:

309             var update = Cc["@mozilla.org/geolocation/service;1"].getService(Ci.nsIGeolocationUpdate);
310             update.update(newLocation);

I'd be happy to write this up as a separate bug if you'd like (assuming it is a bug).

Cheers,
Will Wagner

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.1

w/ tests.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 405342 [details] [diff] [review]
patch v.1

>@@ -813,95 +815,124 @@ nsGeolocation::GetCurrentPosition(nsIDOM
>                                   nsIDOMGeoPositionErrorCallback *errorCallback,
>                                   nsIDOMGeoPositionOptions *options)
...
>+  if (!nsContentUtils::IsCallerChrome())
>+    return NS_ERROR_FAILURE;
Why this check? It makes it more difficult to use this API in C++.


>+NS_IMETHODIMP
>+nsGeolocation::WatchPosition(nsIDOMGeoPositionCallback *callback,
>+                             nsIDOMGeoPositionErrorCallback *errorCallback,
>+                             nsIDOMGeoPositionOptions *options,
>+                             PRInt32 *_retval NS_OUTPARAM)
...
>+  if (!nsContentUtils::IsCallerChrome())
>+    return NS_ERROR_FAILURE;
Same here.

>+<!--
>+  Test for Persistent Storage in chrome
>+  -->
Really?

Did you check whether window.navigator.geolocation works? Or if it doesn't work, why not? (Because it really should be made working. That is IMO more important than this bug, at least if chrome windows have .navigator object.)

Comment 12

[Doug Turner (:dougt)]

>>+  if (!nsContentUtils::IsCallerChrome())
>>+    return NS_ERROR_FAILURE;
>Why this check? It makes it more difficult to use this API in C++.


non-chrome callers should fail here -- we are double checking that only chrome callers will have a null mOwner.

I will fix up the comment in the test cases.

Comment 13

[Doug Turner (:dougt)]

window.navigator.geolocation in chrome windows isn't supported and is probably a separate feature request bug.  I want js components to be able to access geolocation data.

Comment 14

[Doug Turner (:dougt)]

Attachment: patch v.2

same as last patch, but fixes comments in test cases

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> >>+  if (!nsContentUtils::IsCallerChrome())
> >>+    return NS_ERROR_FAILURE;
> >Why this check? It makes it more difficult to use this API in C++.
> 
> 
> non-chrome callers should fail here -- we are double checking that only chrome
> callers will have a null mOwner.

What about C++ callers? Do they have to push chrome JS context to JS stack?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Or push null context.

If we're adding some API for components, it should be useable both from JS and
C++ components.

Comment 17

[Doug Turner (:dougt)]

i can drop the calls to:

+  if (!nsContentUtils::IsCallerChrome())
+    return NS_ERROR_FAILURE;


It it not required.  Olli, any other issues before I update the patch?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 405946 [details] [diff] [review]
patch v.2


> nsGeolocation::nsGeolocation(nsIDOMWindow* aContentDom) 
> : mUpdateInProgress(PR_FALSE)
> {
>   // Remember the window
>-  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aContentDom);
>-  if (window)
>-    mOwner = do_GetWeakReference(window->GetCurrentInnerWindow());
>+  if (aContentDom) {
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aContentDom);
>+    if (window)
>+      mOwner = do_GetWeakReference(window->GetCurrentInnerWindow());
This should somehow fail if window, or mOwner is null or if GetCurrentInnerWindow() returns null.

Maybe Init method?

Comment 19

[Doug Turner (:dougt)]

> This should somehow fail if window, or mOwner is null or if
GetCurrentInnerWindow() returns null.


How could that fail?  aContentDom is a strong reference passed from the calling function which isa DOM window, and don't all content windows have innerwindows?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

I want to be pretty neurotic in this case to make sure things are right.

And no, not all the outer windows have inner windows all the time, like
during deletion. Those are the cases where we need to be extremely careful.

Comment 21

[Doug Turner (:dougt)]

Attachment: patch v.3

I added a init() method.  from the nsGlobalWindow, we will NEVER call init without a content window.  from xpcom (e.g. addons), we will pass a null content window.

Comment 22

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/f4c04bd52811

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a192=beltzner; tests, and he even updated the reviewers' guide

Comment 24

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/3264564c019b  (fixes return value from init(). r=dholbert via irc)

Comment 25

[WildcatRay]

Does this respect the user setting the pref geo.enabled to false? Has there been any testing to insure there are no workarounds for this pref being set to false?

Comment 26

[Steffen Wilberg]

Do we need to reflect this in devmo somewhere?

Comment 27

[Doug Turner (:dougt)]

@wildcatray -

geo.enabled is a global setting and anyone accessing geolocation w/ this pref set to false will not be able to use geolocation.  http://mxr.mozilla.org/mozilla-central/search?string=sGeoEnabled

I do not understand your second question about workarounds.

@steffen

yes, that is a great idea.

Comment 28

[WildcatRay]

(In reply to comment #27)
> @wildcatray -
> 
> geo.enabled is a global setting and anyone accessing geolocation w/ this pref
> set to false will not be able to use geolocation. 
> http://mxr.mozilla.org/mozilla-central/search?string=sGeoEnabled
> 
> I do not understand your second question about workarounds.
>

Has any effort been made to see if someone can craft an add-on that provides a way to "get around" the fact that the pref is set to false?

Comment 29

[Doug Turner (:dougt)]

addons can do anything -- they have full permissions.

Addons hosted on addons.mozilla.org go through a review processes.  Reviewers are suppose to look at privacy considerations.  For a outline of what they review, please see:

https://wiki.mozilla.org/Update:Editors/ReviewingGuide

And in particular:

https://wiki.mozilla.org/Update:Editors/ReviewingGuide#Reviewing_Geolocation_stuff

Comment 30

[Eric Shepherd [:sheppy]]

This is documented now, currently tagged as becoming available in Gecko 1.9.3. Wrote it up before realizing this wasn't landed for 1.9.2. Oh well.

See:

https://developer.mozilla.org/En/Using_geolocation#The_geolocation_object
https://developer.mozilla.org/en/XPCOM_Interface_Reference/NsIDOMGeoGeolocation

Comment 31

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e61bdd8f9379
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b95a13622372