[PP] Adjustable Location Accuracy - Exception not working

RESOLVED WONTFIX

Status

defect
P1
major
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: erik.zitto, Assigned: huseby)

Tracking

({privacy})

Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 affected, b2g-master affected)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141013195847

Steps to reproduce:

In Privacy Panel -> Location Accuracy I set an exception for an application. It should show blurred current location.


Actual results:

Displayed location remained the same as before setting the exception.


Expected results:

Displayed location should vary from the previous location according the set radius.

Comment 1

5 years ago
Agree with Erik, test failed. 

To reproduce this bug please try:

- gecko from  branch https://github.com/dhuseby/gecko/commits/privacy-panel-master, commit: a8ed2c287f
- gaia from https://github.com/kukomigo/gaia/tree/freeze-master, commit 197b30a814a 
(it is gaia master version from bebef1987 with latest patch from Privacy Panel - https://github.com/kukomigo/gaia/tree/privacy-panel-master-apps, commit 252ab5f4)

Comment 2

5 years ago
Today we had test failed again.

Tested versions:
- gecko from https://git.mozilla.org/releases/gecko.git, commit: 8bae192f289
- gaia from https://github.com/Wojtek-Lakomy/gaia/tree/privacy-panel-master-apps, commit: dc77aea67b6c
Reporter

Comment 3

5 years ago
Present also with new build of Gecko - https://git.mozilla.org/releases/gecko.git - master branch, commit 8bae192f289ece626288df9b2f448d5b77dade49, Gaia remained same as before tested.
Assignee

Comment 4

5 years ago
While testing this one I'm getting persistent crashes of b2g.  I've been investigating all day.  I built using the tip of gecko master and the latest version of Wojtek-Lakomy/privacy-panel-master-apps.
Summary: [PP] Adjustable Locacation Accuracy - Exception not working → [PP] Adjustable Location Accuracy - Exception not working
Assignee

Comment 5

5 years ago
so here's more info on the random crashes.  it looks like it isn't geolocation's fault.  i'm bisecting it now:

> W/GPS     ( 5114): ../../../dom/geolocation/nsGeolocationSettings.cpp:160   mozsettings changed: geolocation.app_settings
> I/ServiceManager(  279): service 'android.security.keystore' died
> I/ServiceManager(  279): service 'media.resource_manager' died
> I/ServiceManager(  279): service 'SurfaceFlinger' died
> I/ServiceManager(  279): service 'permission' died
> I/ServiceManager(  279): service 'display.qservice' died
> D/nfcd    (  322):  8 of bytes to be sent... data=0x0 ret=0
> F/libc    ( 5334): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 5334 (Homescreen)
> F/libc    ( 5334): Unable to open connection to debuggerd: Permission denied
Assignee

Comment 6

5 years ago
Posted patch Bug_1093688.patch (obsolete) — Splinter Review
This patches fixes both this bug and bug 1097636.
Assignee: nobody → huseby
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8523396 - Flags: review?(martin.thomson)
Assignee

Updated

5 years ago
Duplicate of this bug: 1097636
Reporter

Comment 8

5 years ago
Fixed and working. Gecko: master, apply patch https://bugzilla.mozilla.org/attachment.cgi?id=8523396
Gaia: https://github.com/martasect/gaia/tree/privacy-panel-master-apps commit: 541d8201f7ccec25f582ba1ee115f7c484477e48
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Assignee

Comment 9

5 years ago
Posted patch Bug 1093688.patch (obsolete) — Splinter Review
I forgot to initialize the JS compartment.  I just need a quick r+ to land this.
Attachment #8523396 - Attachment is obsolete: true
Attachment #8523396 - Flags: review?(martin.thomson)
Attachment #8528676 - Flags: review?(martin.thomson)
Assignee

Comment 10

5 years ago
Re-opening so that the patch can be landed.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment on attachment 8528676 [details] [diff] [review]
Bug 1093688.patch

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

I really don't know that part of the code very well, but this looks right.
Attachment #8528676 - Flags: review?(martin.thomson) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Can you please run this through Try first and update the patch to include a useful commit message?
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
Assignee

Comment 13

5 years ago
Posted patch Bug 1093688.patch (obsolete) — Splinter Review
Updated patch with better commit message.
Attachment #8528676 - Attachment is obsolete: true
Assignee

Comment 14

5 years ago
Try pass: http://mzl.la/1yiO127
Assignee

Comment 15

5 years ago
Another try pass to see if the errors in the lass try are transient: http://mzl.la/1B3plgt
Assignee

Comment 16

5 years ago
Attachment #8529372 - Attachment is obsolete: true
Assignee

Comment 17

5 years ago
None of the failures in the last try pass are related to this patch.
Keywords: checkin-needed
dave, do you know if the failure in https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3486743&repo=try is related to the patch seems this was in the last 2 try runs at least
Flags: needinfo?(huseby)
Keywords: checkin-needed
Assignee

Comment 19

5 years ago
I believe it is unrelated.  It is a test for browser save image.
Flags: needinfo?(huseby)
Assignee

Updated

5 years ago
Keywords: checkin-needed
(In reply to Dave Huseby [:huseby] from comment #19)
> I believe it is unrelated.  It is a test for browser save image.

seems it was not unrelated test failed after this checkin like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=938642&repo=b2g-inbound so i had to back this out
Flags: needinfo?(huseby)
Assignee

Comment 21

5 years ago
Alright, I'm on it.  I'll update soon.
Flags: needinfo?(huseby)

Comment 22

5 years ago
(In reply to Dave Huseby [:huseby] from comment #17)
> None of the failures in the last try pass are related to this patch.

+1, there is a long outstanding bug to resolve these intermittents, it is bug 1084288.
Assignee

Updated

5 years ago
Depends on: 1107673
Assignee

Updated

5 years ago
Depends on: 1107681
Assignee

Updated

5 years ago
Blocks: 1057676
Assignee

Updated

5 years ago
Blocks: 1111486
Assignee

Updated

5 years ago
Blocks: 1114667
Assignee

Updated

5 years ago
Depends on: 1116189
Assignee

Updated

5 years ago
Blocks: 1111992
Blocks: Privacy
Assignee

Updated

5 years ago
No longer blocks: 1114667
Depends on: 1114667

Updated

5 years ago
Status: REOPENED → NEW
blocking-b2g: --- → 2.2+

Updated

5 years ago
See Also: → 1111992
Dear Marta,
I am not sure Dave still working on this. Can you help to check this issue or anyone can help here?
Thank you!
Flags: needinfo?(marta)

Comment 24

5 years ago
NI? Huseby, not sure if he's working on it
Flags: needinfo?(marta) → needinfo?(huseby)
Comment on attachment 8530735 [details] [diff] [review]
Bug 1093688.patch

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

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +350,5 @@
>  void
>  GeolocationSetting::HandleTypeChange(const JS::Value& aVal)
>  {
>    AutoJSAPI jsapi;
> +  jsapi.Init(&aVal.toObject());

This doesn't make sense. One of isObject and isString will be false; you cannot convert between them indiscriminately. It should assert in debug builds.

@@ +428,5 @@
>  void
>  GeolocationSetting::HandleFixedCoordsChange(const JS::Value& aVal)
>  {
>    AutoJSAPI jsapi;
> +  jsapi.Init(&aVal.toObject());

Same problem.
Dave, please could you finish this work as it is a 2.2 blocker please.
Flags: needinfo?(huseby)
Adding privacy keyword.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: privacy

Updated

4 years ago
Status: NEW → ASSIGNED
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)

Updated

4 years ago
Duplicate of this bug: 1118155
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][COM=Privacy Panel]
Hi Dave,
Do yu have any update for the bug? Thanks!
Assignee

Comment 30

4 years ago
I just sent out patches for review for Bug 1115375 and Bug 1116189 and Bug 1114467 is waiting to be checked in.  The current build I have on my phone with all three of these patches has a working exceptions list.
Flags: needinfo?(huseby)
Dear Gerry,
Could yo help to check whether this issue still happen after Bug 111537, Bug 1116189 and Bug 1114467 all fixed? Thanks!
Flags: needinfo?(gchang)
Check on below build. 
Gaia-Rev        389542b71c89253c0d176d3b0bfb54e275c19bf1
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9fd3441c8983
Build-ID        20150223002503
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2


1. I add "Geoloc" app in exception list. It seems to work. But, when I want to add another app in exception list, I found another issue.
The app list in "Add exceptions" will become only 1 app. The rest apps disappear and I don't have any ways to bring them back but reset the whole device.

Please see the URL.
http://youtu.be/Bjj84aLSnyk

2. Because bug 1118155 is marked as duplicated as this bug, I also check the problem in bug 1118155. I still can't locate position in google map.
Flags: needinfo?(gchang) → needinfo?(huseby)
Per second finding in comment #32, the custom location can work now. The location can reflect to what I change.
I tried today's build and custom location can work.

Gaia-Rev        ca64f2fe145909f31af266b1730874051ba76c78
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/16804008c29f
Build-ID        20150224162516
Version         37.0
Device-Name     flame
FW-Release      4.4.2
Assignee

Comment 35

4 years ago
I am now seeing that the exceptions list is broken with the latest code.  I'm seeing two things:

1. when I first go to the exceptions list, it properly lists all of the apps.  then I set an exception for geoloc.  then I close the settings app.  when I open the settings app and go back to the exceptions list, it only shows the camera app.  this is a problem with the privacy panel and maybe with gaia.

2. if I set a radius for an app exception, regardless of what I set, it treats it like I set the radius to 0, which is the same as "precise location".
Flags: needinfo?(huseby) → needinfo?(marta)
Dear Marta,
Could you please help to check comment 35 from Dave?
Thanks!
Severity: normal → major
Priority: -- → P1

Comment 37

4 years ago
I'm looking at it
Flags: needinfo?(marta)

Comment 39

4 years ago
Comment on attachment 8579361 [details] [review]
[gaia] martasect:Bug_1093688 > mozilla-b2g:master

Dave could you look if that solves the problem?
Attachment #8579361 - Flags: feedback?(huseby)
Hi Dave,
Could you help to check comment 39 from Marta? Thanks!
Flags: needinfo?(huseby)
Assignee

Comment 41

4 years ago
I'll try to take a look today.
Assignee

Comment 42

4 years ago
I just tested this with the latest v2.2 and marta's patch.  The exception list works great now.  The only thing broken is fuzzed locations.  Precise, no location, fixed location, and global settings all work.  I'll debug this ASAP.
Flags: needinfo?(huseby)
Assignee

Comment 43

4 years ago
Now I'm seeing the exceptions list broken again and fuzzing doesn't work at all. :-/
Removed from 2.2 blocker as Privacy Control will not be FxOS 2.2 committed feature.
blocking-b2g: 2.2+ → ---
Assignee

Comment 45

4 years ago
See Bug 1172129
Status: ASSIGNED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → WONTFIX
Assignee

Comment 46

4 years ago
Comment on attachment 8579361 [details] [review]
[gaia] martasect:Bug_1093688 > mozilla-b2g:master

currently rethinking all of this code.
Attachment #8579361 - Flags: feedback?(huseby) → feedback-
You need to log in before you can comment on or make changes to this bug.