925761 - Reintroduce Geolocation simulation in Simulator

Status: RESOLVED WORKSFORME

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

Comment 1

[Paul Rouget [:paul]]

Alex, what do you recommend here? Should we start from scratch or should we re-enable previous code?

Could we leverage the existing code to get the simulation working in Firefox Desktop as well?

Comment 2

[Alexandre Poirot [:ochameau]]

We can leverage the existing simulator codebase.
Made of an spoofing xpcom:
  https://github.com/mozilla/r2d2b2g/blob/master/prosthesis/components/FakeGeolocationProvider.js
And its companion actor to control it:
  https://github.com/mozilla/r2d2b2g/blob/master/prosthesis/content/dbg-geolocation-actors.js

I disabled it in the simulator, as it expects to retrieve the position from firefox... I don't really know why it has been designed in that way, but now the simulator doesn't receive the position from firefox and we need to compute it from the b2g runtime itself.

I'd say the challenging part is where/how we display the geolocation UI.
It should be easy to make it work in the simulator process and have a simulator-only geolocation control.
We could also immediatly try to figure out how to share API UI controls between simulator and devices, but obviously it will require more time to at least define how we are going to do that.

Comment 5

[Jan Keromnes [:janx]]

Attachment: WIP Import prosthesis' FakeGeolocationProvider.

Comment 6

[Jan Keromnes [:janx]]

Attachment: WIP Add geolocation actor stub.

Comment 7

[Jan Keromnes [:janx]]

Attachment: WIP Reintroduce Geolocation simulation in Simulator. r=paul

Comment 8

[Jan Keromnes [:janx]]

Note to self: Maybe there is no need to reimplement a provider. Upon spoof-event, unwatch `mProvider`, and call `nsGeolocation::Update(craftedPosition)`.

Comment 9

[Daniele "Mte90" Scasciafratte]

any news about this?

Comment 10

[Jan Keromnes [:janx]]

Daniele, I haven't had time to continue work on this for a while, but I'm expecting to return to it in a week or so.

Comment 11

[Jan Keromnes [:janx]]

Attachment: Allow geolocation spoofing.

New approach, work in progress.

Comment 12

[Jan Keromnes [:janx]]

Attachment: Allow geolocation spoofing.

Comment 13

[Jan Keromnes [:janx]]

Attachment: Geolocation controls in developer menu.

Comment 14

[Jan Keromnes [:janx]]

Attachment: fxos simulator screenshot

Comment 15

[Jan Keromnes [:janx]]

Comment on attachment 8480540 [details] [diff] [review]
Allow geolocation spoofing.

Hi Johnny, could you please review this change I made?

I'm adding a way to spoof geolocation coordinates. It works by listening to a "geolocation-spoof" observer notification, with a JSON payload containing spoof parameters.

I'll add tests to it, but I'd like to make sure you agree with my approach first.

To test it, I added a bit of Gaia UI (see screenshot). Please apply the present gecko patch, and build the Firefox OS Simulator against a Gaia repo that contains the work-in-progress gaia patch. Once the simulator is running, spoofing is controlled by Settings > Developer > Geolocation > Spoof position. The fake position is currently hard-coded to Mozilla's SF office.

This will be useful in Firefox OS simulators and devices, for location testing purposes. Once the back-end spoofing part in nsGeolocation.cpp is landed, I'll work on better control panels in Gaia (for devices), in the Simulator itself, and maybe in desktop Firefox if deemed useful.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 8480540 [details] [diff] [review]
Allow geolocation spoofing.

Seems reasonable to me, but Doug and Garvan should wheigh in here.

Comment 17

[Paul Rouget [:paul]]

> I'll work on better control panels in Gaia (for devices), in the Simulator itself,
> and maybe in desktop Firefox if deemed useful.

Do we want 3 ways (gaia, simulator, devtools) to setup geolocation? Maybe we should discuss that in a different bug (frontend and actor won't happen here, right?)

Comment 18

[Jan Keromnes [:janx]]

Thanks Johnny!

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #17)
> > I'll work on better control panels in Gaia (for devices), in the Simulator itself,
> > and maybe in desktop Firefox if deemed useful.
> 
> Do we want 3 ways (gaia, simulator, devtools) to setup geolocation? Maybe we
> should discuss that in a different bug (frontend and actor won't happen
> here, right?)

Let's discuss the front-end(s) in another bug, I'll ask you for feedback once I've filed it.

Comment 19

[:garvan]

Comment on attachment 8480540 [details] [diff] [review]
Allow geolocation spoofing.

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

Very cool to have this!
I am assuming this approach works even if the providers are not returning a position? I think it will in the case of a single location request, because the code will try to return a cached position, which is overridden with a spoofed location. 

In the case of a watch request, what does this code do? 

r+ from me with the changes I requested.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +813,5 @@
>  NS_IMETHODIMP
>  nsGeolocationService::Update(nsIDOMGeoPosition *aSomewhere)
>  {
>    SetCachedPosition(aSomewhere);
> +  aSomewhere = GetCachedPosition().position; // allow spoofing

I would do
if mSpoofedPosition is valid/exists {
  // do spoofing thing
}

@@ +884,5 @@
> +  spoofedCoords = new nsGeoPositionCoords(values[0], values[1], values[2],
> +    values[3], values[4], values[5], values[6]);
> +
> +  mSpoofedPosition.position = new nsGeoPosition(spoofedCoords,
> +    static_cast<long long int>(PR_Now()));

I'd prefer PR_Now() assigned to a variable declaring the expected time unit. Time unit bugs are a footgun in this code (for which I have lost a toe). And double-check that nsGeoPosition takes the time unit from PR_Now() as an arg.

Comment 20

[Paul Rouget [:paul]]

Comment on attachment 8480540 [details] [diff] [review]
Allow geolocation spoofing.

Jan, can you address Garvan's comment are re-ask a review to Doug?

Comment 21

[Jan Keromnes [:janx]]

Attachment: Allow geolocation spoofing. r=garvank,dougt sr=jst

Thanks for your review Garvan!

> I am assuming this approach works even if the providers are not returning a
> position? I think it will in the case of a single location request, because
> the code will try to return a cached position, which is overridden with a
> spoofed location. 

Yes, I also see no reason that it shouldn't work, although I didn't try to sabotage all providers to verify.

I was worried about the same thing as you, i.e. what if a request registers a callback and no provider trigger its resolution before it times out? But I believe that [1] guarantees that while spoofing, no new requests will get to wait for providers, and if requests were pending before we started spoofing, the call to Update() upon spoof should resolve them all.

However, I'll properly verify if any suspicion arises about this.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeolocation.cpp#445

> In the case of a watch request, what does this code do? 

Single-shot and watch requests basically use the same code path. Upon each, the service will try to get a position if it has none, and then things go through the nsGeolocationService::Update() (either right away or through a pending callback) which does the position spoofing. Later, whenever the providers feel like updating a position, updates (still spoofed) will be sent to watch requests. The same happens if the spoofing code is called to change a position.

While spoofing, in the case that no provider changes position, watch requests will receive no further updates until the position is re-spoofed to another location, which makes sense to me.

> I would do
> if mSpoofedPosition is valid/exists {
>   // do spoofing thing
> }

Done, even though this duplicates the `if (mSpoofedPosition.position)` test meaning "are we spoofing?". If it ever changes, il will have to be updated in the two locations. 

> I'd prefer PR_Now() assigned to a variable declaring the expected time unit.
> Time unit bugs are a footgun in this code (for which I have lost a toe). And
> double-check that nsGeoPosition takes the time unit from PR_Now() as an arg.

Done. The targeted `nsGeoPosition` contructor expects the timestamp to be a `long long` [2].

[2] http://dxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeoPosition.h#59

Comment 22

[Jan Keromnes [:janx]]

Comment on attachment 8488629 [details] [diff] [review]
Allow geolocation spoofing. r=garvank,dougt sr=jst

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

Doug, please have a look.

I'm aware that this patch has no tests. I would like to make sure we're on the same page before writing them, and will add them to the patch before returning to Johnny for super-review.

::: b2g/chrome/content/settings.js
@@ +455,5 @@
> +// =================== Geolocation  ======================
> +SettingsListener.observe('geolocation.spoof', null, function(active) {
> +  dump('GEOLOCATION spoof ' + active + '\n');
> +  if (active) {
> +    Services.obs.notifyObservers(null, 'geolocation-spoof', '{"latitude":37.78937,"longitude":-122.38912}');

Note: The code in b2g/chrome/content/settings.js is temporary and works with the WIP gaia patch to manually test this feature in the Simulator. I will remove it from this patch once we get further into the review process.

Comment 23

[Paul Rouget [:paul]]

Doug, can you look at this, or tell us who else can review this code?

Comment 24

[Josh Matthews [:jdm]]

Comment on attachment 8488629 [details] [diff] [review]
Allow geolocation spoofing. r=garvank,dougt sr=jst

I'll take a look at this.

Comment 25

[Josh Matthews [:jdm]]

Comment on attachment 8488629 [details] [diff] [review]
Allow geolocation spoofing. r=garvank,dougt sr=jst

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

::: dom/geolocation/nsGeolocation.cpp
@@ +807,5 @@
>    }
>  
> +  if (!strcmp("geolocation-spoof", aTopic)) {
> +    SpoofPosition(aData);
> +    Update(GetCachedPosition().position); // Update locators with spoofed position

Why not just pass nullptr here for clarity?

@@ +888,5 @@
> +  const char* keys[] = { "latitude", "longitude", "altitude", "accuracy",
> +                         "altitudeAccuracy", "heading", "speed" };
> +  double values[] = { 0, 0, 0, 1, 0, 0, 0 };
> +
> +  for (int i = 0; i < 7; i++) {

mozilla::ArrayLength

Comment 26

[Jan Keromnes [:janx]]

Thanks for the review! I'll try to rename some overlapping functions to play nice with bug 1073419.

(In reply to Josh Matthews [:jdm] from comment #25)
> > +  if (!strcmp("geolocation-spoof", aTopic)) {
> > +    SpoofPosition(aData);
> > +    Update(GetCachedPosition().position); // Update locators with spoofed position
> 
> Why not just pass nullptr here for clarity?

GetCachedPosition().position is not a nullptr here, because it was just updated by SpoofPosition(aData).

If you mean the lines below, GetCachedPosition().position is still not a nullptr, because setting mSpoofedPosition.position to nullptr actually causes GetCachedPosition() to return the real cached position again (which might or might not be a nullptr).

> > +  for (int i = 0; i < 7; i++) {
> 
> mozilla::ArrayLength

Done in my branch, will be in the next upload.

Comment 27

[Jan Keromnes [:janx]]

Thanks for the r+!

Before going forward with this however, I'll try using the new domain-specific, variable-accuracy geolocation code that landed in bug 1073419. Spoofing on a per-domain / per-tab basis seems smarter than spoofing once for the whole runtime.

Comment 30

[:garvan]

(ni just so you see this comment)
Naoki you were asking about geo spoofing, if this bug lands, it gives a way to turn on spoofing in the developer menu. I forget how this bug sets the x/y of the spoofed location. Also, not sure why it is marked as "in Simulator" as the code would work in a regular build also.

Comment 31

[Jan Keromnes [:janx]]

You're right, even though intended for the Simulator, the code was designed to work in a regular build as well.

However, this patch has not landed yet because another feature landed that does approximately the same thing: Adjustable Location Accuracy. Firefox OS has a dev app to control it called the Privacy Panel (which will soon be integrated in the Settings app I believe) but it might work in a desktop build as well, so I'm thinking of adding controls for it in the DevTools. (It's just a little weird to use a privacy feature for development purposes.)

Out of curiosity, why did you ask about geo spoofing?

Comment 32

[:garvan]

QA wants it to simulate location on the device. For example, set the location to Egypt, make sure all the stuff on the device that is location specific shows up correctly.

Do you know if ALA has any setting for a user-set fixed lat/long location?

Comment 33

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Oh actually the privacy is in here and you can select a city as a custom location using the location adjustment.

Having said that when going to my geolocation test pages, it's not picking up the location:
It thinks I'm at 0,0 instead of Berlin, Germany or the position I placed in.

It's not also adding the browser nor search to the permissions for granting even though I granted access to geolocation.

Seems like the feature itself is broken; though supposedly it gives access to what we want.

Comment 34

[Jan Keromnes [:janx]]

(Flagging myself so I don't forget to reply, in the future please use needinfo for questions.)

Comment 35

[Jan Keromnes [:janx]]

Yes, the geolocation settings are in Settings > Privacy Controls > Location Accuracy > (enable Location adjustment and tweak the custom location).

However you're right Naoki, the feature seems broken. On Flame the custom location seems to be ignored and instead you're at 0,0. The GPS coordinate inputs also don't work.

I'll file bugs for that and investigate. Once the feature works, it should help QA with some tests. Eventually, I'll add DevTools support for this and other location-related features like Language, Timezone, Region, etc.

Comment 36

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Thanks, Jan!

A side note: I have heard rumors that the privacy may be a custom build option as we might be removing it from production.  I don't have an issue with that so long as we still keep the feature.  We may want the engineering build to keep this feature in the settings.  I'll try to keep track of that.

Comment 37

[Jan Keromnes [:janx]]

Hi Naoki,

I wasn't aware of such rumors, but I agree that we need this feature.

It's great if you can keep track of that, in the meantime I'm currently busy with other things, but I should be able to return to geolocation fairly soon (once bug 1028905 and bug 1090949 lay the ground work for simulation tools).

Comment 38

[Jan Keromnes [:janx]]

Marking this as fixed, because you can now spoof geolocation from the Firefox OS Privacy Panel.

Here is how to do it, in the Simulator or on a real Firefox OS devices:

  1. Open the Settings app
  2. Head to Privacy Panel, then Location Accuracy
  3. Choose and edit the Custom Location

However, since this is not ideal for developers, and doesn't work in Firefox desktop, we will also work on a per-app / per-tab geolocation spoofing tool, which will be controllable from the devtools / command line / maybe even from a device. To know more about this effort, watch bugs 891796 and 1069882.

Comment 40

[Jan Keromnes [:janx]]

(Maybe "FIXED" is a bit dishonest for a feature that was added elsewhere.)

Comment 41

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

To note, privacy panel is only in the engineering builds now.