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)
Tracking
(fennec+, firefox55 wontfix, firefox56 verified, firefox57 verified)
RESOLVED
FIXED
Firefox 57
People
(Reporter: thomthomthom, Assigned: esawin)
Details
Attachments
(4 files, 4 obsolete files)
2.11 MB,
image/gif
|
Details | |
682.08 KB,
image/gif
|
Details | |
7.50 KB,
patch
|
esawin
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
55.60 KB,
image/jpeg
|
Details |
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.
Comment 3•7 years ago
|
||
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: --- → ?
status-firefox55:
--- → affected
status-firefox56:
--- → affected
Ever confirmed: true
Updated•7 years ago
|
OS: Unspecified → Android
Hardware: Unspecified → ARM
Eugen, can you take a look?
Assignee: nobody → esawin
tracking-fennec: ? → +
Flags: needinfo?(esawin)
Assignee | ||
Comment 5•7 years ago
|
||
We need to check whether the geolocation providers are enabled when enabling location events.
Flags: needinfo?(esawin)
Attachment #8893758 -
Flags: review?(nchen)
Assignee | ||
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
(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+
Comment 9•7 years ago
|
||
(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.
Assignee | ||
Comment 10•7 years ago
|
||
(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.
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8894550 -
Attachment is obsolete: true
Attachment #8894582 -
Flags: review+
Comment 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/05aee76d966b [2.0] Fix JNI wrappers. r=me
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/426c6d65c25e https://hg.mozilla.org/mozilla-central/rev/05aee76d966b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 15•7 years ago
|
||
Should we consider this for uplift to Beta or can it ride the 57 train?
Flags: needinfo?(esawin)
Assignee | ||
Comment 16•7 years ago
|
||
Fixed JNI wrappers (merged the two commits).
Attachment #8894582 -
Attachment is obsolete: true
Flags: needinfo?(esawin)
Attachment #8896241 -
Flags: review+
Assignee | ||
Comment 17•7 years ago
|
||
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?
Comment 18•7 years ago
|
||
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 19•7 years ago
|
||
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+
Comment 20•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/d5f3be22810a
Comment 21•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
As per last comment - it was on Nightly I will also check in the next beta this Wed.
Reporter | ||
Comment 23•7 years ago
|
||
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.
Comment 24•7 years ago
|
||
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)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•