1093688 - [PP] Adjustable Location Accuracy - Exception not working

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect, P1)

Description

[erik.zitto]

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

[Wojtek Lakomy]

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

[Wojtek Lakomy]

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

Comment 3

[erik.zitto]

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

Comment 4

[Dave Huseby [:huseby]]

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.

Comment 5

[Dave Huseby [:huseby]]

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

Comment 6

[Dave Huseby [:huseby]]

Attachment: Bug_1093688.patch

This patches fixes both this bug and bug 1097636.

Comment 8

[erik.zitto]

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

Comment 9

[Dave Huseby [:huseby]]

Attachment: Bug 1093688.patch

I forgot to initialize the JS compartment.  I just need a quick r+ to land this.

Comment 10

[Dave Huseby [:huseby]]

Re-opening so that the patch can be landed.

Comment 11

[Martin Thomson [:mt:]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Dave Huseby [:huseby]]

Attachment: Bug 1093688.patch

Updated patch with better commit message.

Comment 14

[Dave Huseby [:huseby]]

Try pass: http://mzl.la/1yiO127

Comment 15

[Dave Huseby [:huseby]]

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

Comment 16

[Dave Huseby [:huseby]]

Attachment: Bug 1093688.patch

Comment 17

[Dave Huseby [:huseby]]

None of the failures in the last try pass are related to this patch.

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Dave Huseby [:huseby]]

I believe it is unrelated.  It is a test for browser save image.

Comment 20

[Carsten Book [:Tomcat]]

(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

Comment 21

[Dave Huseby [:huseby]]

Alright, I'm on it.  I'll update soon.

Comment 22

[Zac C (:zac)]

(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.

Comment 23

[Josh Cheng [:josh]]

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!

Comment 24

[marta]

NI? Huseby, not sure if he's working on it

Comment 25

[Josh Matthews [:jdm]]

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.

Comment 26

[David Scravaglieri [:scravag]]

Dave, please could you finish this work as it is a 2.2 blocker please.

Comment 27

[Martin Shuman [:Marty]]

Adding privacy keyword.

Comment 29

[Josh Cheng [:josh]]

Hi Dave,
Do yu have any update for the bug? Thanks!

Comment 30

[Dave Huseby [:huseby]]

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.

Comment 31

[Josh Cheng [:josh]]

Dear Gerry,
Could yo help to check whether this issue still happen after Bug 111537, Bug 1116189 and Bug 1114467 all fixed? Thanks!

Comment 32

[Gerry Chang [: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.

Comment 33

[Gerry Chang [:gchang]]

Attachment: The log of disappeared apps in exception list

Comment 34

[Gerry Chang [:gchang]]

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

Comment 35

[Dave Huseby [:huseby]]

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".

Comment 36

[Josh Cheng [:josh]]

Dear Marta,
Could you please help to check comment 35 from Dave?
Thanks!

Comment 37

[marta]

I'm looking at it

Comment 38

[GitHub Autolander Bot]

Attachment: [gaia] martasect:Bug_1093688 > mozilla-b2g:master

Comment 39

[marta]

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

Dave could you look if that solves the problem?

Comment 40

[Josh Cheng [:josh]]

Hi Dave,
Could you help to check comment 39 from Marta? Thanks!

Comment 41

[Dave Huseby [:huseby]]

I'll try to take a look today.

Comment 42

[Dave Huseby [:huseby]]

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.

Comment 43

[Dave Huseby [:huseby]]

Now I'm seeing the exceptions list broken again and fuzzing doesn't work at all. :-/

Comment 44

[Josh Cheng [:josh]]

Removed from 2.2 blocker as Privacy Control will not be FxOS 2.2 committed feature.

Comment 45

[Dave Huseby [:huseby]]

See Bug 1172129

Comment 46

[Dave Huseby [:huseby]]

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

currently rethinking all of this code.