1336194 - Support geolocation permission on Android

Status: NEW

(GeckoView :: Extensions, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

Tests are disabled.  It also appears that android uses "geolocation" for the permission type rather than "geo" used on desktop.

Comment 1

[Aastha]

Where exactly is it using 'geolocation' for the permission type in android? Can you tell me more about it because I tried looking for it and couldn't find it.

Comment 2

[Shane Caraveo (:mixedpuppy)]

http://searchfox.org/mozilla-central/search?q=%22geolocation%22&case=false&regexp=false&path=

You'll see where permission type checking is using "geolocation" under mobile/android.  A similar search for "geo" will show that is used on desktop.  

I haven't fully examined the use on android, but there is no use of the string "geo" on android that I can see.

Comment 3

[Aastha]

The permission type is 'geolocation' in mobile/android and 'geo' in desktop.
There is no 'geo' in android.

So the issue is to change geolocation to geo in android??

Comment 4

[Shane Caraveo (:mixedpuppy)]

That would be my guess.  I don't know this part of android, would be better to ni? someone who does.  Lets at least get Luca on it, he may know what to look for.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

I took a look at this, and in my opinion it would be easier to use a different site permission name for the Desktop and Android platform here:

- http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/toolkit/components/extensions/Extension.jsm#1407-1408,1413-1414

using something like:

    const geoSitePermissionName = (
      AppConstants.platform === "android" ? "geolocation" : "geo"
    );
 
and then use the `geoSitePermissionName` value (instead of using the "geo" string) in the testPermission/Services.perms.addFromPrincipal/Services.perms.removeFromPrincipal calls.

I gave it a quick try and this seems to be enough to make the following test case to pass successfully:

http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/toolkit/components/extensions/test/mochitest/test_ext_geolocation.html#19-38

Unfortunately, the other two tests doesn't currently pass: 

In particular the test at line 40 (http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/toolkit/components/extensions/test/mochitest/test_ext_geolocation.html#40) 
raises the following error (visible from adb logcat):

[JavaScript Error: "Error: window is not a GeckoView-connected window and does not have a message manager" {file: "resource://gre/modules/Messaging.jsm" line: 236}]

while the test case expects the geolocation request to be denied (because the extension doesn't have the geolocation permission and the request is coming from the background page).

The related window seems to have url "chrome://extensions/content/dummy.xul", and so it should be the XUL browser that contains the background page (created here: http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/toolkit/components/extensions/ExtensionParent.jsm#907)

The other test case at line 56 is related to the "extension tab page" scenario, and it seems that it is currently timing out, and the reason for the timeout seems to be because the "geo.prompt.testing" and "geo.prompt.testing.allow" preferences are not currently handled on Fennec as they are on Firefox for Desktop (I looked for other tests on Fennec that uses this testing preferences but I didn't found any yet.

It is also worth to mention that if I do not set the above testing preferences, I see the Permission Prompt opened for the extension tab for the test case at line 56.

I'm needinfo :sebastian to get a feedback about these two issues (in particular if "geo.prompt.testing.allow" preference is currently fully implemented and used on Fennec, and also if he has any further details related to the "window is not a GeckoView connected window" error)

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Mh, that's not really my area of expertise. I'll redirect to snorp and jim - hopefully they can help here.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Luca Greco [:rpl] from comment #5)
> I'm needinfo :sebastian to get a feedback about these two issues (in
> particular if "geo.prompt.testing.allow" preference is currently fully
> implemented and used on Fennec,

The prefs are checked inside nsGeolocation.cpp, which is shared between desktop and mobile, so I would think we do handle these prefs on mobile.

> and also if he has any further details
> related to the "window is not a GeckoView connected window" error)

Do you have a stack from where this error is thrown? This error happens because we don't know how to show a prompt for this particular XUL window (since it's not associated with a window on the Java side). I think the proper fix is to catch this error somewhere along the stack and perform some default action.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment #7

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Clearing old needinfo (too late to get a stack trace from the original issue being investigated on Fennec)

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Agi,
it may be worth to double-check if this issue is still relevant for the extensions running on GeckoView,
this is currently marked as P3 and it shouldn't be something used by the initial set of extensions that are going to be installable on Fenix, and so there is no need to rush (but I wanted to be sure to put it on your radar).

The test Shane is referring too in commen 0 is the toolkit-level mochitest named toolkit/components/extensions/test/mochitest/test_ext_geolocation.html, which is currently still skipped on android builds.

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

I think the original problem for this is gone, but now we have another problem, the GeckoView code is trying to route the message through a GeckoSession, but we don't have one for the extension. https://searchfox.org/mozilla-central/source/mobile/android/components/geckoview/GeckoViewPermission.jsm#250-252 We need to add a permission delegate on the WebExtension object to handle this.

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

Moving to GeckoView:Extensions since this needs some GV API work.

Comment 13

[Rob Wu [:robwu]]

Do we really need to route the call to the Java part? The JS side has all information available to make a decision.

Comment 14

[[ex-Mozilla] Agi Sferro | :agi]

How? Don't we need to prompt the user for the geolocation permission? Also the Android app needs to get the system-level geolocation permission too if it hasn't gotten it yet.

Comment 15

[Rob Wu [:robwu]]

When the extension has declared the "geolocation" permission, then it can use the geolocation API without prompting the user.

Comment 16

[[ex-Mozilla] Agi Sferro | :agi]

Gotcha. We still need to request the android geolocation permission though.

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

As I mentioned in Bug 1845911 comment 0, this issue is similar to the issue with the expected mapping between the "unlimitedStorage" extension permission (granted to the extensions through their manifest.json file) and the "persistent-storage" webAPI permission.

Bug 1845911 has a patch attached with a small set of proposed changes to make GeckoView taking that expected mapping into account when the "persistent-storage" WebAPI permission is being requested from an extension page but (unlike "persistent-storage") "geolocation" does also require the embedder app to request the android geolocation permission (as Agi mentioned in comment 16) and so in addition to the proposed changes attached to Bug 1845911, doing the same for geolocation will also require a few more changes (e.g. requesting the android geolocation permission at install time for extensions requesting it as a non-optional permission in their manifest, or at runtime along with handling a call to the browser.permission.request WebExtensions API method).

The test coverage for the expected behavior is covered by test_ext_geolocation.html (which is currently disabled on Android builds, and re-enabling it is part of what this bug is tracking).