Closed Bug 1386240 Opened 7 years ago Closed 7 years ago

Geolocation error callback should be called when Android location is off

Categories

(Firefox for Android Graveyard :: General, defect)

54 Branch
ARM
Android
defect
Not set
normal

Tracking

(fennec+, firefox55 wontfix, firefox56 verified, firefox57 verified)

RESOLVED FIXED
Firefox 57
Tracking Status
fennec + ---
firefox55 --- wontfix
firefox56 --- verified
firefox57 --- verified

People

(Reporter: thomthomthom, Assigned: esawin)

Details

Attachments

(4 files, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170801100311

Steps to reproduce:

With Android location turned off:

- Go to https://developer.mozilla.org/en-US/docs/Web/API/Geolocation/Using_geolocation#Geolocation_Live_Example .
- Click in "Show my location" button.
- Click "Share" to share your current location.


Actual results:

- Browser is stuck, without calling the success or error callback.


Expected results:

- Call the error callback, just like Chrome for Android does.
Firefox for Android behavior
Chrome for Android behavior
I was able to reproduce this with Nexus 5 (Android 6.0.1) following the steps from description. I will mark this bug as NEW.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → ARM
Eugen, can you take a look?
Assignee: nobody → esawin
tracking-fennec: ? → +
Flags: needinfo?(esawin)
We need to check whether the geolocation providers are enabled when enabling location events.
Flags: needinfo?(esawin)
Attachment #8893758 - Flags: review?(nchen)
Removed the exception handling, we don't want to log anything in the handler.
Attachment #8893758 - Attachment is obsolete: true
Attachment #8893758 - Flags: review?(nchen)
Attachment #8893761 - Flags: review?(nchen)
Comment on attachment 8893761 [details] [diff] [review]
0001-Bug-1386240-1.1-Check-success-status-when-enabling-o.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
@@ +318,4 @@
>      @WrapForJNI(calledFrom = "gecko")
>      // Permissions are explicitly checked when requesting content permission.
>      @SuppressLint("MissingPermission")
> +    /* package */ static synchronized boolean enableLocation(final boolean enable) {

Use `private` since we're not calling this from a Runnable anymore.

@@ +330,4 @@
>          }
>  
> +        if (!lm.isProviderEnabled(LocationManager.GPS_PROVIDER) &&
> +            !lm.isProviderEnabled(LocationManager.NETWORK_PROVIDER)) {

I don't think we need to check for specific providers. We should be okay as long as we do have a provider.

@@ +486,2 @@
>              // No logging here: user-identifying information.
> +            if (!ThreadUtils.isOnUiThread()) {

Don't need to post to UI thread here, but you do have to change the `calledFrom` parameter for the `WrapForJNI` annotation for `GeckoAppShell.onLocationChanged`.
Attachment #8893761 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> I don't think we need to check for specific providers. We should be okay as
> long as we do have a provider.

We need to check whether the GPS or network provider are enabled since disabling Android location will not disable the passive provider, which will not give us a last known location or guarantee about location updates.
 
> Don't need to post to UI thread here, but you do have to change the
> `calledFrom` parameter for the `WrapForJNI` annotation for
> `GeckoAppShell.onLocationChanged`.

I've changed it to calledFrom="any".
Attachment #8893761 - Attachment is obsolete: true
Attachment #8894550 - Flags: review+
(In reply to Eugen Sawin [:esawin] from comment #8)
> Created attachment 8894550 [details] [diff] [review]
> 0001-Bug-1386240-1.2-Check-success-status-when-enabling-o.patch
> 
> (In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> > I don't think we need to check for specific providers. We should be okay as
> > long as we do have a provider.
> 
> We need to check whether the GPS or network provider are enabled since
> disabling Android location will not disable the passive provider, which will
> not give us a last known location or guarantee about location updates.

Okay.

> > Don't need to post to UI thread here, but you do have to change the
> > `calledFrom` parameter for the `WrapForJNI` annotation for
> > `GeckoAppShell.onLocationChanged`.
> 
> I've changed it to calledFrom="any".

Cool. Now you can remove the `if (!ThreadUtils.isOnUiThread()) {` block.
(In reply to Jim Chen [:jchen] [:darchons] from comment #9)
> Cool. Now you can remove the `if (!ThreadUtils.isOnUiThread()) {` block.

Sorry, that's meant to be in this patch, failed rebase.
Attachment #8894550 - Attachment is obsolete: true
Attachment #8894582 - Flags: review+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/426c6d65c25e
[1.3] Check success status when enabling or disabling geolocation events. r=jchen
https://hg.mozilla.org/mozilla-central/rev/426c6d65c25e
https://hg.mozilla.org/mozilla-central/rev/05aee76d966b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Should we consider this for uplift to Beta or can it ride the 57 train?
Flags: needinfo?(esawin)
Fixed JNI wrappers (merged the two commits).
Attachment #8894582 - Attachment is obsolete: true
Flags: needinfo?(esawin)
Attachment #8896241 - Flags: review+
Comment on attachment 8896241 [details] [diff] [review]
0001-Bug-1386240-1.4-Check-success-status-when-enabling-o.patch

Approval Request Comment
[Feature/Bug causing the regression]: Geolocation.
[User impact if declined]: When trying to access geolocation while it is disabled in Android, we fail to report the failure to the content.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No. 
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It only affects geolocation processing and it's a simple extension of the current mechanics.
[String changes made/needed]: None.
Attachment #8896241 - Flags: approval-mozilla-beta?
Hi Ioana, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(ioana.chiorean)
Comment on attachment 8896241 [details] [diff] [review]
0001-Bug-1386240-1.4-Check-success-status-when-enabling-o.patch

Fix a geolocation related issue. Beta56+. Should be in 56.0b3.
Attachment #8896241 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached image ScreenShot
Following the steps from the description, we are now returning the message that we were unable to fetch the location 

Pixel Android 8.0
Flags: needinfo?(ioana.chiorean)
As per last comment - it was on Nightly 
I will also check in the next beta this Wed.
Why do we still ask for location permission even if Android location is off? It will fail even with user permission. I think it should fail without asking for permission, just like Chrome does.
Verified as fixed on the latest Beta build(56.0b3).
This issue was tested on a Samsung Galaxy S6 edge and S8 (both with Android 7.0) and on a Nexus 6P(Android 8)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.