1001211 - Enable more continuous wireless data collection

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Barnes [:rbarnes]]

Right now, Firefox for Android can collect geo-tagged information about WiFi access points and cell towers that the phone can see (with user consent).  However, it only collects and sends this data when the user loads a page that uses the DOM geolocation API.  The browser should instead listen for events from the Android PASSIVE_PROVIDER, so that it collects and sends data whenever any app on the phone activates the geolocation capabilities of the phone.

Comment 1

[:garvan]

Attachment: bug1001211_add_passive_gps_provider.diff

Listen for location using LocationManager.PASSIVE_PROVIDER, and call the preexisting method collectAndReportLocInfo to send info to MLS.

Tested on Nexus 5, with Google Maps, various other 3rd-party GPS apps, updates arrived reliably. Tested enabling and disabling the app.geo.reportdata, no problems found.

Would be better object design to put both this code and the GeckoApp.collectAndReportLocInfo were put into their own class together, that the GeckoApp has-a containment to, and instatiates in GeckoApp.onCreate().

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

What affect on power consumption does this patch have?

Comment 3

[Richard Barnes [:rbarnes]]

Comment on attachment 8412955 [details] [diff] [review]
bug1001211_add_passive_gps_provider.diff

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

Couple of minor comments below, but overall, looks OK.

::: mobile/android/base/GeckoApp.java
@@ +2528,5 @@
> +        LowPowerGpsListenerController() {
> +            try {
> +                mLocationManager = (LocationManager)getSystemService(Context.LOCATION_SERVICE);
> +            } catch (NoSuchFieldError e) {
> +                Log.e(LOGTAG, "LOCATION_SERVICE not found?!", e);

Punctuation probably not required here.

@@ +2532,5 @@
> +                Log.e(LOGTAG, "LOCATION_SERVICE not found?!", e);
> +                return;
> +            }
> +
> +            final String PREF_GEOREPORTING_ENABLED = "app.geo.reportdata";

Might be nice to move these parameters (also the final longs below) up to be private final class fields, just to have them all in one place.

@@ +2537,5 @@
> +
> +            mPrefObserverId = PrefsHelper.getPref(PREF_GEOREPORTING_ENABLED,
> +                                                  new PrefsHelper.PrefHandlerBase() {
> +                @Override
> +                public void prefValue(String pref, int isGeolocationEnabled) {

Wouldn't hurt to verify that pref.equals(PREF_GEOREPORTING_ENABLED)

Does this event fire on subscription as well as on changes?  If not, then do you need to manually check it and start up the first time?

@@ +2585,5 @@
> +                PrefsHelper.removeObserver(mPrefObserverId);
> +            }
> +        }
> +
> +        private class LowPowerGpsListener implements LocationListener {

Seems like you could save one layer of abstraction by just having LowPowerGpsListenerController implement LocationListener.

Comment 4

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

Comment on attachment 8412955 [details] [diff] [review]
bug1001211_add_passive_gps_provider.diff

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

::: mobile/android/base/GeckoApp.java
@@ +1384,5 @@
>          });
>  
>          GeckoAppShell.setNotificationClient(makeNotificationClient());
>          NotificationHelper.init(getApplicationContext());
> +        mLowPowerGpsListenerController = new LowPowerGpsListenerController();

this should be in initialize() and not  onCreate(). Note that onCreate() doesn't match up to onDestroy() in the Android life cycle, so you could get onCreate() -> onStop() -> onCreate() -> onStop() -> onDestroy().

http://developer.android.com/images/activity_lifecycle.png

Also, all of this should probably be in BrowserApp. GeckoApp is the base class of BrowserApp (i.e. Fennec) and WebApp.

@@ +2587,5 @@
> +        }
> +
> +        private class LowPowerGpsListener implements LocationListener {
> +            public void onLocationChanged(Location location) {
> +                GeckoApp.this.collectAndReportLocInfo(location);

this makes an HTTP connection on every update. That was fine-ish when we were only doing this for explicit updates, but now that we're collecting passive data, we need to batch the data up and only send every now and then.

Comment 5

[:garvan]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> What affect on power consumption does this patch have?

It should not affect power consumption. That said, the manner in which the patch is opening an http connection every update (potentially as frequent as every 2 seconds), would have some power cost. As Brad commented, we should be aggregating the uploads so as to not open connections so frequently -I think this would eliminate any potential power concerns. 

This patch uses LocationManager.PASSIVE_PROVIDER
http://developer.android.com/reference/android/location/LocationManager.html#PASSIVE_PROVIDER
This mode receives location updates when other apps have requested a GPS location. Any time the status bar on android shows a GPS fix (due to another app), Firefox will get sent GPS locations. If you were to switch to Google Maps app, and press the in-app button to get a GPS fix, as locations are sent from the OS to Google Maps, they are also sent to Firefox. 
Firefox must be running in the background for this update to be received.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Thanks for the detailed response. I am no longer worried about the passive provider and continuous collection. The HTTP connection is the only hole we need to plug.

Comment 7

[:garvan]

Where this bug stands, is to change the approach to do background stumbling at all times, not simply when Fennec is backgrounded.

Current areas of investigation:

1) Upload Data Volume (WiFi vs. Cellular Data)

A quick investigation shows that a single stumble is 1 kbyte in size, which on MozStumbler we gzip to 1/4 of the size (worst case from what I see). If if someone is running a mapping app:
250 bytes * 30 collections/min * 60 mins = 450 kbytes an hour. A heavier user, 2-3 hours of map usage, which would be 1 MB per day. 
In Canada, data costs are commonly $.1 / MB, or >$20 / MB roaming (add reference). We could potentially cost a user $600 in a month.
I need to do a more thorough investigation on all these numbers of course.

I think it is clear we need to have a pref to have the user optionally enable Cellular data uploading. I am sure getting an additional pref put in is a difficult endeavor, UX folk don't take that stuff lightly (nor should they).
The reason we must have cellular upload was made clear to me by this MozStumbler post:
https://github.com/mozilla/MozStumbler/issues/555
Cellular data might be primarily how users in India connect to the internet, and have inexpensive data plans to do so.

2) Privacy

Do we have limits on how stale the data can be? If the creation date on the current batch gets older than a week/month, we delete it?

Why don’t we encrypt the data we are storing? As in, get the server to send us a public key, and get a new public key after each batch upload (and a UUID so the server can look up the priv key).

Do we have to timestamp the data? Currently we use a monthly timestamp, should we look at avoiding this entirely?

Comment 8

[Ravi Dandu]

If for some reason the data is not uploaded after a certain time, we should delete it from the device (eg: after 30 days). This is to prevent data from being forever, if the device never gets an opportunity to connect.

The data should be temporarily saved in a manner unaccessible to other apps. Encryption can work: Data is encrypted with MLS-server-public key. Only MLS-server-private key can decrypt it.

Comment 9

[Hanno Schlichting]

For encryption, I'd avoid it in the initial work. If we data is only stored for a short time on the device, the potential leakage of private data is minimal. And other apps on the same device can already collect the same data. There are enough apps where users are either willing to grant location access to them, or many users who will just blindly accept any permissions an app asks for.

There's also a question of transparency vs. privacy here. A user should be able to inspect and verify the data we collect here. For MozStumbler we want to make sure users can export this data and analyze it themselves or contribute it to other projects. That use-case isn't quite as important for the more minimal data gathered in Fennec, but transparency vs. privacy is still a valid concern.

Comment 10

[Hanno Schlichting]

One note on power consumption: Making sure we only do infrequent batch uploads is one important concern. But there's also a concern in querying the state of the cell/wifi modems whenever the passive provider is triggered.

I'm not sure, but certain API's especially anything that triggers "scanning" might wake the cell and wifi modems from sleep. As an example someone might use a driving directions app which has the map / track data downloaded locally. The position updates are coming from the GPS sensor which is powered on. But the cell and wifi modems might be powered down, as no outside internet connection is required. If we aren't careful, we get positions updates quite often while the user is driving and could more or less force the cell and wifi modems into being constantly on, draining the battery more than we should.

Comment 11

[:garvan]

Fennec stumbling has landed, https://bugzilla.mozilla.org/show_bug.cgi?id=1038843
Closing.