667980 - Only submit Telemetry data on mobile via wifi (Android)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[(dormant account)]

A number of people asked that we turn off telemetry when on 3G. Currently there is no api to identify connection type.

Comment 1

[Doug Turner (:dougt)]

as for types, the DAP is currently uses these strings:

    "unknown" if the type of this connection is unknown.
    "ethernet" if this is an Ethernet connection.
    "usb" if this is a connection over a USB port.
    "wifi" if this is a Wi-Fi connection (i.e. IEEE802.11).
    "wimax" if this is a WiMax connection (i.e. IEEE802.16).
    "2g" if this is a 2G connection (e.g. GSM, GPRS, EDGE).
    "3g" if this is a 3G connection (e.g. UMTS, CDMA).
    "4g" if this is a 4G connection (e.g. LTE, UMB).


Also, it is fair to note that this doesn't guarantee throughput.  (For example, at my office, the WiFI sometimes is slower than my 3g network).

Comment 2

[Doug Turner (:dougt)]

i am thinking this should just be a simple event:

window.addEventListener("network-connection", function(event) {
  if (event.type == "wifi")
   dosomething();
}

Comment 3

[(dormant account)]

A listener is good t have, but for my particular case it would be more convenient to be able to call a method somewhere. Either way works for me.

Comment 4

[Doug Turner (:dougt)]

okay.

Comment 5

[Doug Turner (:dougt)]

the first part is to get the gecko system to report the network connection in a consistent manner.  I think we should put the current connection type in the SystemInfo property bag, and broadcast when it changes using the nsIObserverService.  (Taras, that would probably work for you).

The next step would be to make this a more DOMie API.  That can be a follow up.

Comment 6

[Doug Turner (:dougt)]

Attachment: android hack

Comment 7

[(dormant account)]

(In reply to comment #5)
> the first part is to get the gecko system to report the network connection
> in a consistent manner.  I think we should put the current connection type
> in the SystemInfo property bag, and broadcast when it changes using the
> nsIObserverService.  (Taras, that would probably work for you).

sounds good

Comment 8

[Doug Turner (:dougt)]

Attachment: android hack

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 542575 [details] [diff] [review]
android hack

needs, at least:

* a mapping from the android subtype to the names we know about
* storing the current value in the system info property bag.

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Patch that adds network type attribute and notification

Patch based on Doug's android hack - adds an attribute to nsINetworkLinkService that lets you query the network type, and notifies the network type when it changes.

Does this provide what you needed?

Things to think about:

- What types of network do we want to expose? Do we want a coarse set of definitions (as in this patch), or something a lot more fine-grained?
- Expose via some kind of DOM api?

Comment 11

[Chris Lord [:cwiiis]]

Whoops, missed the bit about system-info - taking a look at that now.

Comment 12

[Chris Lord [:cwiiis]]

Looking at system info, does this information really belong there? Seems to only contain static information about the system (cpu + memory info, hardware types, etc.)

I missed comment #2, will update the patch and use consistent names.

Comment 13

[Doug Turner (:dougt)]

you could hang the state information off of nsINetworkLinkService

Comment 14

[Chris Lord [:cwiiis]]

Just a note, I have what is hopefully a better version of this patch in the pipeline, but it's crashing at the moment (so I guess it isn't better yet..) - will upload tomorrow once I've sorted that out.

Also, the patch does make the state information available via nsINetworkLinkService. (the current one does this too)

Comment 15

[Chris Lord [:cwiiis]]

Attachment: Reworked patch that adds network connection type + notification

This patch goes a bit further than the initial patch - it adds the same property to nsINetworkLinkService and does the same notification, but it uses the categories listed in comment #1, and has code to determine what type of mobile connection is being used.

This unfortunately requires an extra permission (read-phone-state).

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Patch that includes stub implementations for all platforms

This patch refreshes the uuid in the idl file and includes stub implementations for all the platforms.

Only Android is actually implemented, all other platforms return 'unknown' for the connection type.

An implementation on the NetworkManager NetworkLinkStatus object would be completely do-able, but quite tedious. You'd need to read the NM_DEVICE_TYPE property of the org.freedesktop.NetworkManager.Device object (which you'd have to retrieve asynchronously on state-change).

Much like on Android where you have to read the capabilities from the TelephonyManager to know what kind of 'mobile' connection it is, likewise you'd need to read the NM_DEVICE_MODEM_CAPABILITIES property of the org.freedesktop.NetworkManager.Device.Modem object to figure out what kind of mobile connection it is in NetworkManager (thankfully this list of capabilities maps much more easily to 2g/3g/4g than Android's multitudinous collection of types).

This is pretty tedious because you ought not to do synchronous calls on dbus, and we're using dbus directly rather than a convenience library to interface with NetworkManager.

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 543302 [details] [diff] [review]
Patch that includes stub implementations for all platforms

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

Looks good.  one more patch, and I think we are done.

::: embedding/android/GeckoApp.java
@@ +357,5 @@
> +            GeckoApp.mAppContext.getSystemService(Context.TELEPHONY_SERVICE);
> +        tm.listen(mPhoneStateListener, PhoneStateListener.LISTEN_DATA_CONNECTION_STATE);
> +
> +        // Notify if network state changed since we paused
> +        GeckoAppShell.onNetworkStateChange(true);

why do we have to add this onNetworkStatusChanged() call?

::: embedding/android/GeckoAppShell.java
@@ +94,5 @@
>      static private File sCacheFile = null;
>      static private int sFreeSpace = -1;
>  
> +    static private String sNetworkState;
> +    static private String sNetworkType;

Initialize these.

@@ +658,5 @@
>          GeckoApp.setLaunchState(GeckoApp.LaunchState.GeckoRunning);
>          sendPendingEventsToGecko();
> +
> +        // Refresh the network connectivity state
> +        onNetworkStateChange(false);

Why is this needed?

@@ +1017,5 @@
>  
>      public static boolean isNetworkLinkUp() {
> +        if (sNetworkState == "up")
> +            return true;
> +        else

non-sequitur else

just do:

if (whatever)
  return true
return false

@@ +1024,5 @@
> +
> +    public static boolean isNetworkLinkKnown() {
> +        if (sNetworkState == "unknown")
> +            return false;
> +        else

same

::: netwerk/base/public/nsINetworkLinkService.idl
@@ +65,5 @@
> +
> +  /**
> +   * The type of network connection.
> +   */
> +  readonly attribute string linkType;

although this is perfectly fine the way it is -- i wonder if script developers would like constants they can use?

::: netwerk/system/android/nsAndroidNetworkLinkService.cpp
@@ +74,5 @@
> +NS_IMETHODIMP
> +nsAndroidNetworkLinkService::GetLinkType(char **aLinkType)
> +{
> +  NS_ENSURE_ARG_POINTER(aLinkType);
> +  NS_ENSURE_TRUE(mozilla::AndroidBridge::Bridge(), NS_ERROR_NOT_IMPLEMENTED);

Wrong error.  if the bridge is not found, we are hosed.  NS_ERROR_UNEXPECTED?

@@ +81,5 @@
> +  if (mozilla::AndroidBridge::Bridge()->GetNetworkLinkType(linkType)) {
> +      *aLinkType = ToNewUTF8String(linkType);
> +      if (!(*aLinkType))
> +          return NS_ERROR_OUT_OF_MEMORY;
> +      else

drop the else.

maybe the logic should be:

if (*aLinkType)
  return NS_OK;

then have everything fall out to the last return

::: netwerk/system/maemo/nsMaemoNetworkLinkService.cpp
@@ +75,5 @@
> +nsMaemoNetworkLinkService::GetLinkType(char **aLinkType)
> +{
> +  NS_ENSURE_ARG_POINTER(aLinkType);
> +
> +  *aLinkType = NS_strdup(NS_NETWORK_LINK_TYPE_UNKNOWN);

add a comment like "stub implementation, on this platform we don't know what to do yet".

::: widget/src/android/AndroidBridge.cpp
@@ +681,5 @@
> +                                                             jGetNetworkLinkType));
> +    if (!jstrType)
> +        return PR_FALSE;
> +
> +    nsJNIString jniStr(jstrType);

jniStr(jstrType, mJNIEnv)

Comment 18

[Chris Lord [:cwiiis]]

(In reply to comment #17)
> Comment on attachment 543302 [details] [diff] [review] [review]
> Patch that includes stub implementations for all platforms
> 
> Review of attachment 543302 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  one more patch, and I think we are done.
> 
> ::: embedding/android/GeckoApp.java
> @@ +357,5 @@
> > +            GeckoApp.mAppContext.getSystemService(Context.TELEPHONY_SERVICE);
> > +        tm.listen(mPhoneStateListener, PhoneStateListener.LISTEN_DATA_CONNECTION_STATE);
> > +
> > +        // Notify if network state changed since we paused
> > +        GeckoAppShell.onNetworkStateChange(true);
> 
> why do we have to add this onNetworkStatusChanged() call?

When the app is paused, we stop listening to network status changes. This is in the situation that the network status changes while we're paused - it won't notify if it hasn't changed.

> ::: embedding/android/GeckoAppShell.java
> @@ +94,5 @@
> >      static private File sCacheFile = null;
> >      static private int sFreeSpace = -1;
> >  
> > +    static private String sNetworkState;
> > +    static private String sNetworkType;
> 
> Initialize these.

Ok

> @@ +658,5 @@
> >          GeckoApp.setLaunchState(GeckoApp.LaunchState.GeckoRunning);
> >          sendPendingEventsToGecko();
> > +
> > +        // Refresh the network connectivity state
> > +        onNetworkStateChange(false);
> 
> Why is this needed?

So we have the correct values in sNetworkState and sNetworkType on startup. We always return the cached values, so we need to update them on startup.

> @@ +1017,5 @@
> >  
> >      public static boolean isNetworkLinkUp() {
> > +        if (sNetworkState == "up")
> > +            return true;
> > +        else
> 
> non-sequitur else
> 
> just do:
> 
> if (whatever)
>   return true
> return false

Ok

> @@ +1024,5 @@
> > +
> > +    public static boolean isNetworkLinkKnown() {
> > +        if (sNetworkState == "unknown")
> > +            return false;
> > +        else
> 
> same

Ok

> ::: netwerk/base/public/nsINetworkLinkService.idl
> @@ +65,5 @@
> > +
> > +  /**
> > +   * The type of network connection.
> > +   */
> > +  readonly attribute string linkType;
> 
> although this is perfectly fine the way it is -- i wonder if script
> developers would like constants they can use?

Yes - shall I use string constants or numbers? If numbers, how do you normally go about passing numbers in aData of notifications?

> ::: netwerk/system/android/nsAndroidNetworkLinkService.cpp
> @@ +74,5 @@
> > +NS_IMETHODIMP
> > +nsAndroidNetworkLinkService::GetLinkType(char **aLinkType)
> > +{
> > +  NS_ENSURE_ARG_POINTER(aLinkType);
> > +  NS_ENSURE_TRUE(mozilla::AndroidBridge::Bridge(), NS_ERROR_NOT_IMPLEMENTED);
> 
> Wrong error.  if the bridge is not found, we are hosed.  NS_ERROR_UNEXPECTED?

This is what happens elsewhere in the file - I agree, I'd have thought NS_ERROR_FAILED or UNEXPECTED would make more sense... Should I change this for the other methods?

> @@ +81,5 @@
> > +  if (mozilla::AndroidBridge::Bridge()->GetNetworkLinkType(linkType)) {
> > +      *aLinkType = ToNewUTF8String(linkType);
> > +      if (!(*aLinkType))
> > +          return NS_ERROR_OUT_OF_MEMORY;
> > +      else
> 
> drop the else.
> 
> maybe the logic should be:
> 
> if (*aLinkType)
>   return NS_OK;
> 
> then have everything fall out to the last return

Ok, will switch around the logic to get rid of else's

> ::: netwerk/system/maemo/nsMaemoNetworkLinkService.cpp
> @@ +75,5 @@
> > +nsMaemoNetworkLinkService::GetLinkType(char **aLinkType)
> > +{
> > +  NS_ENSURE_ARG_POINTER(aLinkType);
> > +
> > +  *aLinkType = NS_strdup(NS_NETWORK_LINK_TYPE_UNKNOWN);
> 
> add a comment like "stub implementation, on this platform we don't know what
> to do yet".

Ok

> ::: widget/src/android/AndroidBridge.cpp
> @@ +681,5 @@
> > +                                                             jGetNetworkLinkType));
> > +    if (!jstrType)
> > +        return PR_FALSE;
> > +
> > +    nsJNIString jniStr(jstrType);
> 
> jniStr(jstrType, mJNIEnv)

Again, this was the same as several other places in the file - Is there a reason to use this different structure?

Comment 19

[Chris Lord [:cwiiis]]

Of course, the attribute on the interface doesn't need to correspond with the signal, so perhaps ignore my comment about aData and I'll assume we want number constants, as is customary in other idl's

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Revised patch based on review

Hopefully this addresses all the concerns above. As GetLinkType now returns an int, that removes the issue with the string and I changed the return type from NOT_IMPLEMENTED to UNEXPECTED (we can change the others some other time).

Seems to build/work on android and Linux, need to ping IT about my account to push to try...

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 543985 [details] [diff] [review]
Revised patch based on review

http://tbpl.mozilla.org/?tree=Try&rev=dae2ceb818c0

Comment 22

[Chris Lord [:cwiiis]]

Darn, this doesn't build because some of the network types were added in later versions of the API - Details;

NETWORK_TYPE_EVDO_B was added in API level 9,
NETWORK_TYPE_EHRPD was added in API level 11,
NETWORK_TYPE_LTE was added in API level 11.

Seeing as these are just constants, I'll replace them with numbers and a comment with explanation.

Surprised that LTE is only in API level 11, I wonder where that leaves current Gingerbread phones that support LTE?

Comment 23

[Chris Lord [:cwiiis]]

Attachment: Don't use fields from above API level 8

Same patch that just uses the numbers along with comments explaining why for the fields that only appear in API level > 8.

Comment 24

[Jason Duell]

Comment on attachment 544168 [details] [diff] [review]
Don't use fields from above API level 8

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

Just a few drive-by comments:

> // XXX This function has not yet been implemented for this platform

We should file followup bugs to implement GetLinkType for our other platforms:  It's likely to come in handy for many things.  I'm not sure who the usual suspects for the various platforms are: if anyone does know and can cc them on the bug for their platform(s), that'd be awesome.  If you're feeling pressed for time, email me any names and I can open the bugs.  Thanks!

::: embedding/android/GeckoAppShell.java
@@ +1098,5 @@
> +        if (notifyChanged && (state != sNetworkState || typeCode != sNetworkTypeCode)) {
> +            Log.i("GeckoAppShell", "Network state changed: (" + state + ", " + type + ") ");
> +            sNetworkState = state;
> +            sNetworkType = type;
> +            sNetworkTypeCode = typeCode;

Just want to make sure it's intentional that you only update sNetworkState/Type/Code when you're doing notification.  I.e. that this function should notify if the network state has changed since the last onNetworkState(true) call, and intermediate onNetworkState(false) calls don't "count" as noticing a state change.

::: netwerk/base/public/nsINetworkLinkService.idl
@@ +102,5 @@
>  #define NS_NETWORK_LINK_DATA_UNKNOWN "unknown"
> +
> +/**
> + * We send notifications through nsIObserverService with topic
> + * NS_NETWORK_LINK_TYPE_TOPIC whenever the network conection type

s/conection/connection/

Comment 25

[Chris Lord [:cwiiis]]

(In reply to comment #24)
> Comment on attachment 544168 [details] [diff] [review] [review]
> Don't use fields from above API level 8
> 
> Review of attachment 544168 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Just a few drive-by comments:
> 
> > // XXX This function has not yet been implemented for this platform
> 
> We should file followup bugs to implement GetLinkType for our other
> platforms:  It's likely to come in handy for many things.  I'm not sure who
> the usual suspects for the various platforms are: if anyone does know and
> can cc them on the bug for their platform(s), that'd be awesome.  If you're
> feeling pressed for time, email me any names and I can open the bugs. 
> Thanks!

Yes, we'll do this - if you know which people to contact for which platforms, that'd be great, otherwise I'll just look at who's been editing the relevant files and go on that.

> ::: embedding/android/GeckoAppShell.java
> @@ +1098,5 @@
> > +        if (notifyChanged && (state != sNetworkState || typeCode != sNetworkTypeCode)) {
> > +            Log.i("GeckoAppShell", "Network state changed: (" + state + ", " + type + ") ");
> > +            sNetworkState = state;
> > +            sNetworkType = type;
> > +            sNetworkTypeCode = typeCode;
> 
> Just want to make sure it's intentional that you only update
> sNetworkState/Type/Code when you're doing notification.  I.e. that this
> function should notify if the network state has changed since the last
> onNetworkState(true) call, and intermediate onNetworkState(false) calls
> don't "count" as noticing a state change.

That's correct - I assume we don't want to send the changed notification on every startup (we didn't do so before, so maintaining existing behaviour), but we need to update the variables with the correct state in case they're read.

> ::: netwerk/base/public/nsINetworkLinkService.idl
> @@ +102,5 @@
> >  #define NS_NETWORK_LINK_DATA_UNKNOWN "unknown"
> > +
> > +/**
> > + * We send notifications through nsIObserverService with topic
> > + * NS_NETWORK_LINK_TYPE_TOPIC whenever the network conection type
> 
> s/conection/connection/

Thanks, will correct typo and re-attach.

Comment 26

[Chris Lord [:cwiiis]]

Attachment: Correct typo

Comment 27

[Chris Lord [:cwiiis]]

Comment on attachment 544432 [details] [diff] [review]
Correct typo

Passes on try: http://tbpl.mozilla.org/?tree=Try&rev=28df653f079c

Comment 28

[Doug Turner (:dougt)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/49d539befa07

Comment 29

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/49d539befa07

Comment 30

[Matt Brubeck (:mbrubeck)]

This will add "Read phone state and identity" to the permissions Firefox requests at install time, right? I think we need a followup bug to consider adding an explanation in our Android Market description of how we use this and other permissions, since it is a common cause of complaints and confusion.

Is the functionality gain from this change worth the loss of automatic updates from Fx7->8, and the perceived privacy issues?

Comment 31

[Doug Turner (:dougt)]

> Is the functionality gain from this change worth the loss of automatic updates from Fx7->8, and the perceived privacy issues?

You are correct. I hate the idea of being blocked on adding any new functionality that requires new rights because automatic updates will break.

Comment 32

[Chris Lord [:cwiiis]]

I think being able to alter behaviour depending on the type of connection is quite desirable, although it's only the fine-grained type that requires this permission (without, you can determine either 'mobile' or 'wireless', but not what type of mobile or wireless (i.e. wimax, lte, 3g, 2g, etc.)).

It's kind of a shame that the network type isn't retrievable without this permission, as that's a pretty useful piece of information and we don't really need any of the other information it grants access to (yet, at least).

Likelihood is we'll have to add some more permissions in the future for things like Device APIs though, so I don't think this should be held up because of it. Shall I open the bug to add information to our market entry?

Comment 33

[Mike Hommey [:glandium]]

(In reply to comment #32)
> I think being able to alter behaviour depending on the type of connection is
> quite desirable, although it's only the fine-grained type that requires this
> permission (without, you can determine either 'mobile' or 'wireless', but
> not what type of mobile or wireless (i.e. wimax, lte, 3g, 2g, etc.)).

I think the main concern of the people Taras mention in comment 0 is that they don't want telemetry to initiate data transfers that will be billed to them.
If there's really a concern about requiring the extra permission, I think the goal of this bug could be very much achieved by only distinguishing between 'mobile' and 'wireless'.

Comment 34

[Doug Turner (:dougt)]

I'd like, at some point, to be able to expose network information to webapps.

Comment 36

[Matt Brubeck (:mbrubeck)]

(In reply to comment #30)
> This will add "Read phone state and identity" to the permissions Firefox
> requests at install time, right? I think we need a followup bug to consider
> adding an explanation in our Android Market description of how we use this
> and other permissions, since it is a common cause of complaints and
> confusion.

Filed bug 672352.

Comment 37

[Jorge Villalobos [:jorgev] (he/him)]

This would be useful to know for add-on developers.

Comment 38

[Trevor]

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsINetworkLinkService

Comment 39

[Matt Brubeck (:mbrubeck)]

We are discussing backing this change out; see bug 691054 for details.

Comment 40

[(dormant account)]

(In reply to Matt Brubeck (:mbrubeck) from comment #39)
> We are discussing backing this change out; see bug 691054 for details.

I'm ok with a backout. I was planning to use this for telemetry but the decision of when to not send a telemetry packet on 3g is a complicated one, so we do not need this until we have a solid plan for telemetry.

Comment 41

[Matt Brubeck (:mbrubeck)]

This was backed out on all branches because the cost/benefit for adding the new permission was too high (bug 691054).  From bug 691054 comment 14, some possible ways we could re-add this feature (or at least part of it) without the READ_PHONE_STATE permission:

> the original use case (change settings when on a mobile network) could be done with
> ConnectivityManager.getActiveNetworkInfo().getType(), which does not require any scary
> permissions.
> 
> Actually, it looks from the source code like NetworkInfo.getSubType will return the same
> info as TelephonyManager.getNetworkType (although its documentation isn't as good and
> ConnectivityManager might not notify listeners every time the subtype changes), and also
> does not require scary permissions.

Comment 42

[Chris Lord [:cwiiis]]

Unassigning in case someone else wants to pick this up.

Comment 43

[Philipp von Weitershausen [:philikon]]

JP, can you elaborate on the basecamp nom? AFAICT from the discussion, this bug has been punted on for now. Clearing it for now, please re-nom if appropriate. Thanks.

(Also removing the link to the B2G Network Manager since at least the patch in this bug seems to be Android only.)

Comment 44

[Mounir Lamouri (:mounir)]

bug 697355 isn't b2g related AFAICT.

Comment 45

[Lawrence Mandel [:lmandel] (use needinfo)]

basecamp+ because we don't want to submit Telemetry data over cellular data connections on B2G. Note that any way to prevent this should be acceptable for v1, including disabling Telemetry on B2G if required. (We can leave the pref but simply not submit Telemetry data until we can limit it to wifi.) Dougt notes that we did not pursue this patch in Android due to the requirement to add a network query permission on app install. Please keep the Android use case in mind when fixing this bug.

Comment 46

[Lawrence Mandel [:lmandel] (use needinfo)]

After further discussion during triage, I have created a separate B2G specific bug for this behaviour (bug 813837). I have therefore cleared the basecamp+ flag on this bug.

Comment 47

[Mounir Lamouri (:mounir)]

(In reply to Matt Brubeck (:mbrubeck) from comment #41)
> This was backed out on all branches because the cost/benefit for adding the
> new permission was too high (bug 691054).  From bug 691054 comment 14, some
> possible ways we could re-add this feature (or at least part of it) without
> the READ_PHONE_STATE permission:

BTW, I completely forgot to write a comment here about that but in GeckoNetworkManager.java, I am getting the network type and I haven't been asking any permission. It should be easy to change GeckoNetworkManager to return the connection type. I guess all the boilerplate (having the value going from the Java code to the chrome) could be copied from the patch attached in this bug.
If someone is interested to take this bug, I would be glad to give a hand.

Comment 48

[immerfox]

I would like to take this bug! Is it possible?

Comment 49

[Mounir Lamouri (:mounir)]

You should look at the backed out patches and modify them so they would use mobile/android/base/GeckoNetworkManager.java to get the current connection status.

Comment 51

[Patrick McManus [:mcmanus]]

nsinetworklinkservice