Open Bug 1529591 Opened 11 months ago Updated 4 months ago

Permissions API for geo not reflecting OS X 'Enable Location Services' setting

Categories

(Core :: DOM: Geolocation, defect, P3)

1.0 Branch
defect

Tracking

()

People

(Reporter: ben.brostoff, Assigned: garvan)

References

Details

Attachments

(3 files, 10 obsolete files)

98.13 KB, image/png
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.109 Safari/537.36

Steps to reproduce:

In the JS console:

navigator.permissions.query({ name: 'geolocation' }).then(console.log);
navigator.geolocation.getCurrentPosition(a => console.log(a), b => console.log('Err', b));

The first log shows permission was granted, but the second's error message is that the user denied a location prompt. I detected this through a website that was claiming I denied the prompt even though I could see through the Mozilla UI I had granted permissions.

Actual results:

Error message shows user denied permission even if they have accepted.

Expected results:

Error message should get closer to the actual source of the error (which I don't know off hand, but does not seem related to permissions).

One additional comment; the issue here for me was I had not enabled Location Services for Firefox (I'm using a Mac). So while the browser permissions were set correctly, I'm thinking this didn't matter because of the OS permissions.

Component: Untriaged → Geolocation
Product: Firefox → Core

I tried reproducing this issue on

Version 65.0.1
Build ID 20190211233335
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:65.0) Gecko/20100101 Firefox/65.0

I don't get error message in console.

navigator.permissions.query({ name: 'geolocation' }).then(console.log);
navigator.geolocation.getCurrentPosition(a => console.log(a), b => console.log('Err', b));
undefined
PermissionStatus

onchange: null

ownerGlobal: ChromeWindow chrome://browser/content/browser.xul

state: "granted"

<prototype>: PermissionStatusPrototype { state: Getter, onchange: Getter & Setter, … }

Kanchan - in order to replicate you need to turn Location Services off if using a Mac; my OS is Mojave 10.14. I've linked to a screenshot of the code executing I described in the console

With OS X system pref for Enable Location Services off for Firefox I get
navigator.geolocation.getCurrentPosition reports user denied which is correct.
What is incorrect is:

  1. page permission prompt shown for geo, correct behaviour would be to not show the prompt. (Note that interaction with the prompt is correct in that ok or deny both result in getCurrentPosition error callback for user denied)
  2. navigator.permissions.query should report the denied state.
Summary: Permissions API and geolocation.getCurrentPosition give conflicting output for whether permission was granted → Permissions API for geo not reflecting OS X 'Enable Location Services' setting
Duplicate of this bug: 1536481
Priority: -- → P3
Attached patch check-cllocation-perm.patch (obsolete) — Splinter Review

This almost-working patch checks OS X location permissions prior to making a Firefox permission request. However, it only works in non-E10s mode, and I have no idea why just yet. My guess is that only the chrome process is allowed to check CLLocation permissions, and E10s mode has the child process checking it. In E10s mode the OS X system permission check always reports false, when it should be true.
I just thought this would be easy to fix, and alas, it is not.

See Also: → 1554431
Duplicate of this bug: 1442411
Duplicate of this bug: 1554431

Any idea why the child processes can't access OS X CLLocationManager? (I am assuming that is the problem).
(see comment #6)

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(sledru)

I don't know anything about this code, sorry :/

Flags: needinfo?(sledru)

I confirmed that CLLocationManager is only accessible from the main process. Someone who knows IPDL better than I (who knows zero) perhaps can set up a cross-process message for checking this. Actually, the cross-process call is not the difficult part, it will be making sync code behave async where this call is inserted.

jdm: Can you drop any tips for someone fixing this bug? My attempted patch in comment 6 only works in non-E10s mode.

Flags: needinfo?(josh)

I propose the following:

Flags: needinfo?(josh)

Thanks for the recipe Josh!
The RegisterRequestWithPrompt happens in the child process before any geo engine setup calls happen in the parent process.
Here is the flow of fn calls and their respective process:

Is content process: 1, nsGeolocationService::Init
Is content process: 1, Geolocation::GetCurrentPosition 
Is content process: 1, Geolocation::RegisterRequestWithPrompt 
Is content process: 1, nsGeolocationRequest::Allow
Is content process: 1, nsGeolocationService::StartDevice
Is content process: 0, nsGeolocationService::Init   
Is content process: 0, nsGeolocationRequest::Allow

What would be a good place to put the CoreLocation main process setup code? Any ideas on how to defer this only to when geolocation is used?
Worst case, some static init could take place in the parent process that sets up the OS X initial permission state and installs the observer for this state change.

Flags: needinfo?(josh)
Attached patch garvan-geo-patch.diff (obsolete) — Splinter Review

Here is a working patch that uses IPDL promise-like lambda syntax to call to the parent process to check the mac system permission.
I need to vet it some more, and see if I am missing any cases.

Attachment #9067437 - Attachment is obsolete: true
Flags: needinfo?(josh)
Attached patch garvan-geo-patch.diff (obsolete) — Splinter Review

updated version

Assignee: nobody → gkeeley
Attachment #9071964 - Attachment is obsolete: true
Attached patch garvan-geo-patch.diff (obsolete) — Splinter Review
Attachment #9071966 - Attachment is obsolete: true

Before showing the Firefox geolocation permission prompt, check if the Mac system prefs has geolocation disabled.

Attachment #9072196 - Attachment is obsolete: true
Attachment #9072235 - Attachment description: Bug 1529591 - Check Mac system geo permission prior to showing request permission dialog → Bug 1529591 - Check Mac system geo permission prior to showing request permission dialog.
Attachment #9072235 - Attachment is obsolete: true
Attachment #9072235 - Attachment is obsolete: false
Keywords: checkin-needed

Diff does not have proper author information in Phabricator. See the Lando FAQ for help with this error.
:garvan, please take a look over this FAQ webpage: https://wiki.mozilla.org/Phabricator/FAQ#Lando

Flags: needinfo?(gkeeley)
Keywords: checkin-needed
Attachment #9073654 - Attachment is obsolete: true
Flags: needinfo?(gkeeley)
Attachment #9072235 - Attachment is obsolete: true

The docs say the Web UI will not format the patch in a landable way. I re-submitted the patch using phlay, I hope it is correctly formatted now.

Keywords: checkin-needed

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7510c8179dff
Check Mac system geo permission prior to showing request permission dialog. r=jdm

Keywords: checkin-needed

The error is dom/geolocation/nsGeolocation.cpp:1258:8: error: Refcounted variable 'request' of type 'nsGeolocationRequest' cannot be captured by a lambda

This builds fine locally. I assume the build machine has an additional level of checks, I'll have to investigate how to get this to capture.

EDIT: use mach static-analysis check dom/geolocation/nsGeolocation.cpp to reveal the error

You may want to try a RefPtr<nsGeolocationRequest> instead. I assume the concern here is that the raw pointer may end up pointing to a freed value by the time the closure is invoked.

Thanks Josh! That is what was needed :), updated PR forthcoming .

Flags: needinfo?(gkeeley)
Attachment #9073654 - Attachment is obsolete: false
Attachment #9073654 - Attachment is obsolete: true

I added the lines to the patch to wrap the captured objects in smart pointers. Clang is happy again.

 RefPtr<Geolocation> self = this;
 RefPtr<nsGeolocationRequest> req = request;

I am not sure how to request re-review. Need info'ing for either review, or a tip on how to mark this for review again. Thanks!

Flags: needinfo?(josh)
Flags: needinfo?(josh)
Keywords: checkin-needed

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14faf5e9443e
Check Mac system geo permission prior to showing request permission dialog. r=jdm

Keywords: checkin-needed

The test failure is in non-e10s mode, not sure yet what the correct approach is here.

Perhaps doing a special case for non-e10s mode:

bool Geolocation::RegisterRequestWithPrompt(nsGeolocationRequest* request) {
#ifdef MOZ_WIDGET_COCOA
  if (!BrowserTabsRemoteAutostart()) { // non e10s mode
    if (!isMacGeoSystemPermissionEnabled()) {
      return false;
    }
    return RegisterRequestWithPromptImpl(request);
  }

New patch is up with changes from Comment #31

Flags: needinfo?(gkeeley)
Attachment #9074205 - Attachment is obsolete: true
Attachment #9074205 - Attachment is patch: true
Attachment #9074205 - Attachment mime type: text/x-phabricator-request → text/plain
Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/165672f8816b
Check Mac system geo permission prior to showing request permission dialog. r=jdm

Keywords: checkin-needed

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=165672f8816bb856ac2d34e79967242840282607&tochange=6b6d05d2d55007ae3f470ee7b1db848651a2e4ac&searchStr=mochitest&selectedJob=262376869

https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=262376898&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=165672f8816bb856ac2d34e79967242840282607&tochange=6b6d05d2d55007ae3f470ee7b1db848651a2e4ac&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-browser-chrome-e10s-4%2Cm%28bc4%29

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=262376869&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=262376898&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/6b6d05d2d55007ae3f470ee7b1db848651a2e4ac

[task 2019-08-20T00:05:34.047Z] 00:05:34 INFO - TEST-START | browser/components/resistfingerprinting/test/mochitest/test_geolocation.html
[task 2019-08-20T00:05:34.048Z] 00:05:34 INFO - GECKO(1682) | ++DOMWINDOW == 10 (0x121402400) [pid = 1683] [serial = 10] [outer = 0x11f2287a0]
[task 2019-08-20T00:05:34.690Z] 00:05:34 INFO - GECKO(1682) | [Child 1683, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 664
[task 2019-08-20T00:05:35.111Z] 00:05:35 INFO - [1674, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 664
[task 2019-08-20T00:05:35.115Z] 00:05:35 INFO - GECKO(1682) | ++DOMWINDOW == 11 (0x122e4c000) [pid = 1683] [serial = 11] [outer = 0x11f2287a0]
[task 2019-08-20T00:05:35.319Z] 00:05:35 INFO - TEST-INFO | started process screencapture
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - TEST-INFO | screencapture: exit 0
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - TEST-UNEXPECTED-FAIL | browser/components/resistfingerprinting/test/mochitest/test_geolocation.html | Should be able to call success callback, Got error. code = 1
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - SimpleTest.ok@https://example.com/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - doTest_getCurrentPosition/<@https://example.com/tests/browser/components/resistfingerprinting/test/mochitest/test_geolocation.html:38:11
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - PositionErrorCallbackdoTest_getCurrentPosition@https://example.com/tests/browser/components/resistfingerprinting/test/mochitest/test_geolocation.html:32:27
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - Async
window.onload/<@https://example.com/tests/browser/components/resistfingerprinting/test/mochitest/test_geolocation.html:20:21
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - focusedOrLoaded/<@https://example.com/tests/SimpleTest/SimpleTest.js:803:67
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - TEST-UNEXPECTED-FAIL | browser/components/resistfingerprinting/test/mochitest/test_geolocation.html | Should be able to call success callback, Got error. code = 1
[task 2019-08-20T00:05:35.495Z] 00:05:35 INFO - SimpleTest.ok@https://example.com/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - doTest_watchPosition/wid<@https://example.com/tests/browser/components/resistfingerprinting/test/mochitest/test_geolocation.html:52:11
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - PositionErrorCallbackdoTest_watchPosition@https://example.com/tests/browser/components/resistfingerprinting/test/mochitest/test_geolocation.html:45:37
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - doTest_getCurrentPosition/<@https://example.com/tests/browser/components/resistfingerprinting/test/mochitest/test_geolocation.html:39:29
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - PositionErrorCallback
doTest_getCurrentPosition@https://example.com/tests/browser/components/resistfingerprinting/test/mochitest/test_geolocation.html:32:27
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - Async*window.onload/<@https://example.com/tests/browser/components/resistfingerprinting/test/mochitest/test_geolocation.html:20:21
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - focusedOrLoaded/<@https://example.com/tests/SimpleTest/SimpleTest.js:803:67
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - GECKO(1682) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - GECKO(1682) | MEMORY STAT | vsize 6750MB | residentFast 152MB | heapAllocated 26MB
[task 2019-08-20T00:05:35.496Z] 00:05:35 INFO - TEST-OK | browser/components/resistfingerprinting/test/mochitest/test_geolocation.html | took 1316ms

Check also these failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Cwithout%2Ce10s%2Ctest-macosx1014-64%2Fdebug-mochitest-chrome-1proc-1%2Cm-1proc%28c1%29&tochange=6b6d05d2d55007ae3f470ee7b1db848651a2e4ac&fromchange=6755c1cf8a42371683322da33c8db48df287b3a6&selectedJob=262376868

This intermittent too: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=262376866&repo=autoland&lineNumber=13865

Flags: needinfo?(gkeeley)

My patch has insufficient environment checks; distinguishing between mac/not-mac is not enough, it needs to distinguish whether the geolocation provider is CoreLocation, or another type (specifically MLS or testing provider).
Only the CoreLocation geo provider needs to check if the system permission is set to allow.

Flags: needinfo?(gkeeley)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:garvan, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(gkeeley)
Attachment #9086462 - Attachment is obsolete: true
Attachment #9090802 - Attachment is obsolete: true
Attachment #9090802 - Attachment is obsolete: false
Attachment #9091419 - Attachment is obsolete: true
Attachment #9090802 - Attachment is obsolete: true
Flags: needinfo?(gkeeley)
Keywords: checkin-needed

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/493061169650
Check Mac system geo permission prior to showing request permission dialog. Differential Revision: https://phabricator.services.mozilla.com/D44881 r=jdm

Keywords: checkin-needed

Oops, mac-specific code was not wrapped in #ifdef. I'll fix that.

Flags: needinfo?(gkeeley)
You need to log in before you can comment on or make changes to this bug.