Matt Brubeck (:mbrubeck) Assignee
	Description • 13 years ago

In bug 667980 we added the ability for chrome code to get information about the type of network connection on an Android device.  The rationale was that "a number of people asked that we turn off telemetry when on 3G" -- but we don't actually provide that ability yet.  Currently no one actually uses this new capability.

This change adds "Read phone state and identity" to the permissions Firefox requests at install time.  This is a permission that is very commonly used for user tracking by ad networks, and it is not clear to people why the browser is requesting it.

Even if we explain somewhere why we need these permissions (bug 672352), this will continue to be a big turn-off for Firefox users since the Android Market presents the permissions at every install or upgrade, far more prominently than any explanation we can provide.

I think we should have a general policy of requesting the bare minimum permissions.  Since we are not actually getting any benefit from this code or permission, we should remove it.

Requesting tracking-firefox8 because this is a new feature/regression in Firefox 8 and I think we should fix it on beta.  We can leave the interface in place if necessary for code freeze reasons, but we should back out the Android implementation of the interface.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 1 • 13 years ago

Sample feedback from just two days after Firefox Beta 8.0 release:

via email: "What possible reason could a browser want with that information?"

Twitter: https://twitter.com/#!/bcpk/status/120137443428139009

Android Market: Out of the 10 most recent Firefox Beta reviews, 5 mention the new permission:

"phone identity? wtf? please remove this ugly permission. wont update otherwise and keep the 1* rating. don't hurt this nice software!"

"Why does a browser need phone identity permissions?"

"FF8 and Phone ID Will not update to FF8 until this permission is removed. The browser does not need to know my phone number or serial number. :("

"Spy app? Why does a browser need to know my identity? Uninstalling, android stock browser is the best anyway."

	Wesley Johnston (:wesj)
	Comment 2 • 13 years ago

Note that ever implement the Network Info api:

http://www.w3.org/TR/netinfo-api/

(which I've heard webdevs asking for a few times), we'll need this information. And, from what I've heard, automatic updates are not downloaded from the Android market when we change the permissions of the app. The user must open the market and manually select "update".

	Matt Brubeck (:mbrubeck) Assignee
	Comment 3 • 13 years ago

(In reply to Wesley Johnston (:wesj) from comment #2)
> Note that ever implement the Network Info api:
> 
> http://www.w3.org/TR/netinfo-api/
> 
> (which I've heard webdevs asking for a few times), we'll need this
> information.

That's true.  I have my own reservations about whether the network link type is the right thing to expose (it gets used as a proxy for network speed, which is often wrong).  Regardless of our future plans though, we should consider removing this permission request from Firefox 8, 9, and 10 because we don't have any code that uses it.  Right now it increases user anxiety with no user benefit.

> And, from what I've heard, automatic updates are not downloaded
> from the Android market when we change the permissions of the app. The user
> must open the market and manually select "update".

That's correct.  The latest version of the Android Market also highlights any permissions that are new since the last version.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 4 • 13 years ago

Because the surrounding code has changed, we'll need slightly different patches on different branches.  This one is for beta.  I'll request approval after the team has decided whether to take this patch, take a different patch, or do nothing.

This patch completely backs out bug 667980.  If we want to freeze interface changes on beta, then we will need a slightly different patch that leaves the interfaces in place.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 5 • 13 years ago

To play devil's advocate for a moment, it might be better to add this sooner rather than later.  The permission is more prominent when it's added by an update (and has a "NEW" badge during the update process) then when it's just one in a long list of permissions.

If we add it now, it may scare away users updating from 7 to 8, but it'll be less scary for new users installing 8 or later releases for the first time.  If we remove it now but end up adding it back later when we have more users, then more users will be affected by the scary update process.

(An ideal fantasy resolution would be to get Google to add a more fine-grained permission that can check network types but not phone ID.  However, that would only work after all users had upgraded to a new version of Android that does not currently exist.)

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 6 • 13 years ago

Is it the case that we're not even using the permission for anything? If so, I think we definitely shouldn't ask for the permission now. I am not even sure that the initial motivation mentioned in comment 0 (controlling telemetry I/O based on network link type) is reasonable, since it may negatively effect the accuracy of telemetry data itself (these users will always appear to be using fast connections because their numbers on slow connections never get reported).

Also, couldn't the backout patch be simply the removal of the permission from the manifest? That seems reasonable, especially if there are no callers yet, but we intend to have callers in the future.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 7 • 13 years ago

We have an implementation of (In reply to Brian Smith (:bsmith) from comment #6)
> Also, couldn't the backout patch be simply the removal of the permission
> from the manifest? That seems reasonable, especially if there are no callers
> yet, but we intend to have callers in the future.

We have *code* in Gecko that calls the protected TelephonyManager APIs, but *that* code doesn't have any actual callers in the browser.  Theoretically there could be add-ons using that code, although I don't know of any such add-ons in the wild.  (They would need to call nsINetworkLinkService methods that are new in Firefox 8 and implemented only on Android.)

Without the correct permissions, I believe that calling these APIs will cause Fennec to crash with a Java exception.  It's better to back the code out than to leave it in in that state.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 8 • 13 years ago

Comment on attachment 563973 [details] [diff] [review]
backout patch for beta

Ian Melven pointed out elsewhere that TelephonyManager.listen and TelephonyManager.getNetworkType do not actually require the READ_PHONE_STATE permission.  Do we call any methods that actually do need the permission?  I'm testing to see whether we can just remove the permission request without changing any code.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 9 • 13 years ago

Just another devil's advocate here. I don't want to knee jerk on this permission. I have seen other apps, like Facebook, Seesmic and Opera Mobile, also request this permission. 

If we think we want this level of functionality, I'd rather take the permission (and the grumbles) now.

	Ian Melven :imelven
	Comment 10 • 13 years ago

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Just another devil's advocate here. I don't want to knee jerk on this
> permission. I have seen other apps, like Facebook, Seesmic and Opera Mobile,
> also request this permission. 
> 
> If we think we want this level of functionality, I'd rather take the
> permission (and the grumbles) now.

i'm not opposed to taking this now, but if we do i think we absolutely should try to make sure https://bugzilla.mozilla.org/show_bug.cgi?id=672352 happens asap.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 11 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #8)
> Ian Melven pointed out elsewhere that TelephonyManager.listen and
> TelephonyManager.getNetworkType do not actually require the READ_PHONE_STATE
> permission.  Do we call any methods that actually do need the permission? 

It looks like the Android docs for TelephonyManager.listen are wrong.  On my HTC T-Mobile G2 (stock Android 2.3), if I remove the permission check then I get:

E/AndroidRuntime(13752): Caused by: java.lang.SecurityException: Neither user 10075 nor current process has android.permission.READ_PHONE_STATE.
E/AndroidRuntime(13752):        at android.os.Parcel.readException(Parcel.java:1322)
E/AndroidRuntime(13752):        at android.os.Parcel.readException(Parcel.java:1276)
E/AndroidRuntime(13752):        at com.android.internal.telephony.ITelephonyRegistry$Stub$Proxy.listen(ITelephonyRegistry.java:201)
E/AndroidRuntime(13752):        at android.telephony.TelephonyManager.listen(TelephonyManager.java:861)
E/AndroidRuntime(13752):        at org.mozilla.gecko.GeckoApp.onResume(GeckoApp.java:513)
E/AndroidRuntime(13752):        at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1150)
E/AndroidRuntime(13752):        at android.app.Activity.performResume(Activity.java:3832)
E/AndroidRuntime(13752):        at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2110)
E/AndroidRuntime(13752):        ... 12 more

	Matt Brubeck (:mbrubeck) Assignee
	Comment 12 • 13 years ago

Looking at the Android source code, the telephony service checks for READ_PHONE_STATE permission when you listen to any event specified here:
http://www.google.com/codesearch#uX1GffpyOZk/services/java/com/android/server/TelephonyRegistry.java&q=PHONE_STATE_PERMISSION_MASK&l=100

The documentation here lists the required permission for all of those events *except* LISTEN_DATA_CONNECTION_STATE, which is the one that we use:
http://developer.android.com/reference/android/telephony/PhoneStateListener.html

	Matt Brubeck (:mbrubeck) Assignee
	Comment 13 • 13 years ago

The error in the documentation was reported 19 months ago with no response yet from Google:
http://code.google.com/p/android/issues/detail?id=6940

	Matt Brubeck (:mbrubeck) Assignee
	Comment 14 • 13 years ago

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Just another devil's advocate here. I don't want to knee jerk on this
> permission. I have seen other apps, like Facebook, Seesmic and Opera Mobile,
> also request this permission. 

Frankly, I don't think we're in the same position of strength on mobile that those apps are.  We can afford to pull the same stuff they do.

> If we think we want this level of functionality, I'd rather take the
> permission (and the grumbles) now.

If we had a useful feature that needed this permission, I might agree with this.  But right now it provides no benefits to us or our users, only negatives.  We had some hypothetical ideas for how to use this when we added it to Firefox 8, but none of them have been useful enough for us to actually implement them in Firefox 8, 9, or 10.

Also, 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.

Since we don't need this feature yet, I think we should investigate whether there are less-scary ways of implementing it *before* we ship it to users.

	Paul [pwd]
	Comment 15 • 13 years ago

I once suggested keeping the phone screen on while a page is loading (bug 655560), could that not justify this permission?

	Matt Brubeck (:mbrubeck) Assignee
	Comment 16 • 13 years ago

(In reply to Paul [sabret00the] from comment #15)
> I once suggested keeping the phone screen on while a page is loading (bug
> 655560), could that not justify this permission?

Keeping the screen on is a separate permission. It does not require READ_PHONE_STATE.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 17 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #16)
> (In reply to Paul [sabret00the] from comment #15)
> > I once suggested keeping the phone screen on while a page is loading (bug
> > 655560), could that not justify this permission?
> 
> Keeping the screen on is a separate permission. It does not require
> READ_PHONE_STATE.

(and knowing whether a page is loading or whether a network is available can also be done without additional permissions, if that's what you were asking.)

	Doug Turner (:dougt)
	Comment 18 • 13 years ago

Comment on attachment 563973 [details] [diff] [review]
backout patch for beta

per irc - we need to keep the interface.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 19 • 13 years ago

This patch backs out only the Android widget/embedding changes.  It does not back out the inteface change in netwerk.  Instead it changes the Android implementation to always return LINK_TYPE_UNKNOWN, which is what all the other platforms already do.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 20 • 13 years ago

Pushed attachment 564291 [details] [diff] [review] to Try: https://tbpl.mozilla.org/?tree=Try&rev=431da4ae868c

	Doug Turner (:dougt)
	Comment 21 • 13 years ago

Comment on attachment 564291 [details] [diff] [review]
partial backout patch for beta

land on m-b m-a and m-c please

	Matt Brubeck (:mbrubeck) Assignee
	Comment 22 • 13 years ago

Comment on attachment 564291 [details] [diff] [review]
partial backout patch for beta

Requesting approval for Beta (8) and Aurora (9).  This patch backs out changes that landed in Firefox 8 that requested a new scary-looking permission that we ended up not using.  The new permission request is a top cause of user complaints and negative reviews in beta.  Since we don't actually use the permission, we would like to back it out.

The patch is very low risk.  It touches only Android-specific files.  It changes the new nsINetworkLinkService::GetLinkType method to always return LINK_TYPE_UNKNOWN on Android.  Aside from that one change, it only backs out code added by bug 667980.

This does not affect any Firefox code or UX.  It does not affect binary compatibility.  If any add-ons call this new method, they will now get the result LINK_TYPE_UNKNOWN on all platforms.  (Previously they would get a valid type on Android and LINK_TYPE_UNKNOWN on all other platforms.)  MXR does not find any current add-ons that actually call this method, which was added in Firefox 8 and implemented only on Android.  Nevertheless I will add the addon-compat and dev-doc-needed keywords so that we can notify any add-on authors who are considering using the new method.

	LegNeato
	Comment 23 • 13 years ago

Comment on attachment 564291 [details] [diff] [review]
partial backout patch for beta

Approved all around. Is this already backed out on central?

	Matt Brubeck (:mbrubeck) Assignee
	Comment 24 • 13 years ago

(In reply to Christian Legnitto [:LegNeato] from comment #23)
> Approved all around. Is this already backed out on central?

No, I was waiting for Try results to complete.  I'll push the backout to mozilla-central today, then to Aurora and Beta tomorrow.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 25 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/5cf6c081e112

	Matt Brubeck (:mbrubeck) Assignee
	Comment 26 • 13 years ago

For some reason, the latest nightly build, built from 149fc4a6efca which includes this fix, is *still* prompting for "Read phone state and identity" permission on installation:
http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2011-10-04-03-08-58-mozilla-central-android/fennec-10.0a1.multi.android-arm.apk

but this local build from mozilla-central changeset 5cf6c081e112 (which differs from the nightly by only two unrelated changesets) does not prompt for "Read phone state and identity" permissions on install:
http://people.mozilla.com/~mbrubeck/fennec-5cf6c081e112.apk

I'm investigating why this fix did not seem to work in the nightly build but works in my local build.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 27 • 13 years ago

The tinderbox build of 5cf6c081e112 (same changeset as my local build) also still prompts for the READ_PHONE_STATE permission:
https://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-central-android/1317685112/fennec-10.0a1.en-US.android-arm.apk

Reopening this bug while I investigate.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 28 • 13 years ago

jhammel verified that this had the desired effect (no "Read phone state" permission request) when installed a phone without a previous Nightly install, and I verified that the permission request was gone when installing Nightly on one of my devices after a factory reset.  Just uninstalling Nightly was not enough, though.  The permission still seems to be cached or remembered somewhere.

So it appears this is fixed for new installs, but not for devices which previously installed a version with the permission request.

This is good enough for our release users, since we never shipped a version on the release channel with the READ_PHONE_STATE permission.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 29 • 13 years ago

Pushed to Aurora 9 and Beta 8.  (Already pushed to Nightly 10 in comment 25.)
https://hg.mozilla.org/releases/mozilla-aurora/rev/724b6e0c57e8
https://hg.mozilla.org/releases/mozilla-beta/rev/0e269b0cf409

See comment 28 if you are trying to verify this fix.

	Kevin Brosnan [Ex-Mozilla]
	Comment 30 • 13 years ago

The workaround for it to clear the permission was to uninstall all versions of Firefox on the phone. [Nightly, Aurora, Beta, Release and custom builds]

Verified

Mozilla/5.0 (Android; Linux armv7l; rv:8.0) Gecko/20111006 Firefox/8.0 Fennec/8.0 ID:20111006003041

Mozilla/5.0 (Android; Linux armv7l; rv:9.0a2) Gecko/20111006 Firefox/9.0a2 Fennec/9.0a2 ID:20111006042015

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111006 Firefox/10.0a1 Fennec/10.0a1 ID:20111006030949

	Florian Scholz (Open Web Docs)
	Comment 31 • 13 years ago

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