1021357 - NetworkGeolocationProvider.js: if <2 wifi APs, don't send wifi in JSON request

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[:garvan]

As found in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1009592

With no sim and wifi on

The first time the timer fires, we don't have wifi scan results yet, so we end up sending the "cellTowers":[] body as above. This time there is a network connection, so the location request gets sent but fails (I've added logging) with status 400 and this body: {"error":{"code":400,"message":"Parse Error","errors":[{"domain":"global","message":"Parse Error","reason":"parseError"}]}}  I suspect that the location server doesn't like an empty array of cell towers and would prefer an empty body or something.  We should probably just avoid sending a request in this no-data case and this problem will go away.

After that first failed request, we get some wifi data and the next time the timer fires, we make a valid request (with an empty cell towers array) and get a location response.  After that we keep using the cached value.

Comment 1

[:garvan]

David also noted we could address the no sim, no wifi case as an aside while fixing this bug. 

His note on this from https://bugzilla.mozilla.org/show_bug.cgi?id=1009592

Next I removed my sim card (it was in slot 2) and tried again with no sim and no wifi.  The location provider started up, and repeatedly tried to send this payload:

   sending {"cellTowers":[]}

But without a sim card or a wifi connection, it is not possible to contact the location service, so this always fails and the onerror handler of the xhr object is invoked. I'd think it is worth handling this case and punting before setting up the xhr request, but probably not a big deal.

Comment 2

[Hanno Schlichting]

My comment from https://bugzilla.mozilla.org/show_bug.cgi?id=1009592#c29 applies here, in full:

The service will answer a request with the exact payload of '{}' and return a GeoIP based result. But if you send either cellTowers or wifiAccessPoints, one of those arrays has to actually contain an entry to be valid or you get a 400 ParseError.

As a test, this works:

curl -i -k -XPOST -H "Content-Type: application/json" https://location.services.mozilla.com/v1/geolocate?key=test -d '{}'

Comment 3

[Robert Kaiser]

IMHO, we should at least try to get the geoIP like Hanno describes in comment #2, that's better than nothing (but we should not send repeated identical, in this case empty, requests, wasn't there a different bug on that?)

Comment 4

[:garvan]

I believe the bug here is just these 2 cases:  
- no sim and wifi on, first request malformed, later request is fine
- no sim, no wifi case, request is malformed, but has no negative effect that I am aware of

In all other cases we request '{}' correctly that I know of.

Sending repeated requests is in-progress: https://bugzilla.mozilla.org/show_bug.cgi?id=1018383

Comment 5

[:garvan]

I believe bug 1018383 fixes this, there is some additional checks for empty data

http://hg.mozilla.org/mozilla-central/annotate/869971ad9fd6/dom/system/NetworkGeolocationProvider.js#l451

Comment 6

[Hanno Schlichting]

(In reply to Garvan Keeley [:garvank] from comment #5)
> I believe bug 1018383 fixes this, there is some additional checks for empty
> data
> 
> http://hg.mozilla.org/mozilla-central/annotate/869971ad9fd6/dom/system/
> NetworkGeolocationProvider.js#l451

The wifi check is still "if (wifiData) {" - maybe that should also check for wifiData.length > 0.

wifiData is only set if there are accessPoints and than set by "wifiData = accessPoints.filter(isPublic).sort(sort).map(encode);"

I think if you only see non-public access points, this would construct an empty list. Total edge case, but should be easy to fix.

Comment 7

[:garvan]

A minimum of 2 wifi APs is needed to geolocate (privacy requirement of both GLS and MLS), so we should clean up this code so that if < 2 APs, don't send any k/v pair for wifis in the JSON.

Comment 8

[Josh Matthews [:jdm]]

This sounds like a good candidate for a mentored bug.

Comment 9

[:garvan]

True!

Comment 10

[Pierre Rudloff]

I would like to work on this if I can be mentored.

Comment 11

[Josh Matthews [:jdm]]

The relevant code is NetworkGeolocationProvider.js, and you might want to look at the bits that interact with the hasCells property.

Comment 12

[:garvan]

Hi Pierre, I can mentor you through this bug. This bug mutated a bit, so I clarified the title.

Comment 13

[:garvan]

The JSON that is sent is documented here: https://mozilla-ichnaea.readthedocs.org/en/latest/api/index.html#geolocate

Which links to here since we follow the same JSON request format:
https://developers.google.com/maps/documentation/business/geolocation

Not that you need to read all that :), it is just background info. The important part is that the JSON request can optionally have a "wifiAccessPoints": [ list of APs ] section.

The bug is that, if this is less than 2 APs,  it should not be sent as part of the JSON request.

Comment 14

[Pierre Rudloff]

Hello,
It seems I can't add files to this bug, so here is my patch: https://gist.github.com/Rudloff/621923b2d97e54a72800
This seems to be enough to fix the problem (at least on my computer). Please tell me if I forgot something.

Comment 15

[:garvan]

Thanks! This looks good to me, both the fix and the patch formatting.
Can you try again to add it as an attachment? I know you said you can't add files to the bug, but I haven't heard of the 'Add an attachment' link being restricted in any way. (I am checking on #introduction right now if that is something I should be aware of).

When you add it as an attachment, change the review combo box to '?', and put in :garvank in the adjacent text box.

If you add r=garvank at the end of this line (the commit comment):
https://gist.github.com/Rudloff/621923b2d97e54a72800#file-gistfile1-diff-L4
you will have truly covered every detail that I can think of. This is a nice-to-have on the commit comments.

Comment 16

[Pierre Rudloff]

Attachment: Patch

Sorry, I didn't see the Add an attachment link (I was looking near the comment field).

Comment 17

[:garvan]

This patch is correct. What is does reveal is a JS warning we should have caught earlier:

line 477 "ReferenceError: reference to undefined property data.wifiAccessPoints" 

Because it is now less likely to create the data.wifiAccessPoints property, I see this JS warning a lot in the log.

Changing
http://lxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#464
to 
 let data = { cellTowers: undefined, wifiAccessPoints: undefined }

will fix this. 
Do you mind adding that to your patch? 

After that I'll mark the patch r+ and you can mark the bug 'checkin-needed' (I'll explain that if you are new to that last step).

Comment 18

[Pierre Rudloff]

Attachment: Patch v2

Here is the updated patch

Comment 19

[:garvan]

Comment on attachment 8538080 [details] [diff] [review]
Patch v2

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

Everything looks good, thanks!

Comment 20

[:garvan]

I added the checkin-needed keyword (I could have left this for you to do if I felt you might want to review things further, but I think this is ready to roll). In 1-3 days, this will land on nightly (mozilla-central), and this bug will be updated to note that.

Comment 21

[:garvan]

Try run: we have nothing on try that touches this code :( sorry Sherrifs.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3ca34c900024

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ca34c900024