950237 - Fennec's geolocation "stumbling" code needs to use the new JSON report format

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

In particular, reports include BSSIDs instead of hashes:

https://mozilla-ichnaea.readthedocs.org/en/latest/api/submit.html

rtilder said he was working on this. :)

Comment 1

[Hanno Schlichting]

This only blocks the general MLS bug, but not the privacy text UI change.

Comment 2

[Ryan Tilder [:rtilder]]

It was on the list but my hopes to do work on the MozStumbler client have disappeared in the face of other higher priority items.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: 950237.patch

Fennec has been reporting location data that is incompatible with the current server format. Migrate reporter changes from MozStumbler to Fennec:

https://github.com/mozilla/MozStumbler/blob/master/src/org/mozilla/mozstumbler/Reporter.java

• Do not omit ad hoc SSIDs (because they are not necessarily mobile devices and the server already has to detect other moving access points).

• Round lat/long measurements to 6 decimal places (for consistency and to avoid the illusion that these Android devices' GPS hardware has millimeter precision).

• Report the current date instead of a timestamp in seconds to reduce PII that might be used to track a user. The server does not need one-second precision to scrub stale measurements from the database months from now.

• Report BSSIDs instead of SHA1(BSSID + SSID) hashes.

• Workaround an HTTP connection pooling bug in ICS and JB that causes many report uploads to fail.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8372081 [details] [diff] [review]
950237.patch

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

::: mobile/android/base/GeckoApp.java
@@ +2459,5 @@
>              }
>  
> +            // Round lat/lon to 6 decimal places.
> +            locInfo.put("lat", Math.floor(location.getLatitude() * 1.0E6) / 1.0E6);
> +            locInfo.put("lon", Math.floor(location.getLongitude() * 1.0E6) / 1.0E6);

use a Formatter (formatter.format("%.2f", location.getLatitude()));

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> use a Formatter (formatter.format("%.2f", location.getLatitude()));

Using a Formatter seems a bit heavyweight for this operation. A Formatter will produce a formatted string, which JSONObject will emit as a quoted string property (e.g. "lat":"37.871936") instead of a number (e.g. "lat":37.871936), so we would need to parse that formatted string to obtain a number again. And the Formatter can't be reused, so I would need two:

> locInfo.put("lat", Double.parseDouble(new Formatter().format("%.6f", location.getLatitude()).toString()));
> locInfo.put("long", Double.parseDouble(new Formatter().format("%.6f", location.getLongitude()).toString()));

I can just leave the old code without rounding. The server will drop the extra bits of pseudo-precision anyways. :)

Comment 6

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41b171702ded

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/41b171702ded

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 8372081 [details] [diff] [review]
950237.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Fennec's geolocation data collection has been broken for a few months, using the wrong report format so the server has been receiving useless data.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk because this code is already broken. This code is preffed to only be enabled on Nightly and Aurora.
String or IDL/UUID changes made by this patch: N/A

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8372081 [details] [diff] [review]
950237.patch

It is not too late for 28 if needed.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f676d238a96d

Comment 11

[Chris Peterson [:cpeterson]]

• Ryan: thanks for uplifting this fix.

• Sylvestre: we won't need to uplift this fix to Beta 28 because this code is preffed off in the Beta and Release channels (atm).