Privacy concerns: - exposing last known network is not a problem I think, it can be looked up with some effort via the IP - exposing the home network is more dicey, however, for 99% of people current == home network, so probably not that big of a deal either

(In reply to Andreas Gal :gal from comment #1) > Created attachment 742544 [details] [diff] [review] patch I'd prefer a more rigid interface than adding another squishy setting like "ril.lastKnownNetwork".

I think for most people this isn't going to be privacy-invasive. As you said, Andreas, it's something that can be inferred from the IP. But I can imagine there are some cases where this is potentially iffy, especially if someone uses a VPN or proxy to access stuff. Imagine someone who has lots of sim cards (not out of the question) and travels a lot. An app or site who can read network IDs and is regularly used (think twitter) can learn a bit about my travel habits that maybe I don't want. Granted, this is not a majority case and the privacy risks here probably aren't severe. But I think we need some discussion in the forums about this to tease out what might not be obvious.

privacy people: we only expose this information to privileged apps with the "mobilenetwork" permission

Andreas, "Preferences are as the name implies intended for preferences. There is no sane use case for storing data in preferences. I would give any patch I come across doing that an automatic sr- for poor taste and general insanity" ;)

(In reply to Andreas Gal :gal from comment #6) > privacy people: we only expose this information to privileged apps with the > "mobilenetwork" permission Sounds like minimal risk, though this is different than the subject of the bug. "expose unprivileged access..."

comment 7: I am duplicating what the code is currently doing, and I signed up for https://bugzilla.mozilla.org/show_bug.cgi?id=866238 to end the insanity

We need marketplace to give this a try.

(In reply to Andreas Gal :gal from comment #12) > We need marketplace to give this a try. Chris is pulling in your patch and compiling

Comment on attachment 742573 [details] [diff] [review] patch for inbound Review of attachment 742573 [details] [diff] [review]: ----------------------------------------------------------------- I'm by no means suited to give real feedback on this code, but here are a few things I noticed. Thanks! ::: dom/apps/src/PermissionsTable.jsm @@ +118,5 @@ > app: DENY_ACTION, > privileged: DENY_ACTION, > certified: ALLOW_ACTION > }, > + mobilenetwork: { Can we change these tabs to soft spaces? ::: dom/network/src/MobileConnection.cpp @@ +11,5 @@ > #include "nsIDOMCFStateChangeEvent.h" > #include "nsIDOMICCCardLockErrorEvent.h" > #include "GeneratedEvents.h" > +#include "mozilla/Preferences.h" > +#include "nsIPermissionManager.h" Does order matter here? @@ +436,5 @@ > > NS_IMETHODIMP > MobileConnection::NotifyVoiceChanged() > { > + if (!CheckPermission("mobileconnection") || !CheckPermission("mobileconnection")) { Is this a typo? Checking `!CheckPermission` twice? ::: dom/system/gonk/RadioInterfaceLayer.js @@ +1083,5 @@ > } catch (e) {} > } > } > > + // Update lastKnownNetwork For consistency with the line below we could end this comment with a period. @@ +1085,5 @@ > } > > + // Update lastKnownNetwork > + if (message.mcc && message.mnc) { > + try { Tabs again here. @@ +1797,5 @@ > > + // Update lastKnownHomeNetwork. > + if (message.mcc && message.mnc) { > + try { > + services.prefs.setCharPref("ril.lastKnownHomeNetwork", message.mcc + message.mnc); Shouldn't `services` be capital `Services`?

I'm on Mac OS X 10.8.2 and I'm having difficulties building the emulator because of this error: external/qemu/distrib/sdl-1.2.12/src/video/maccommon/SDL_lowvideo.h:59:2: error: unknown type name 'PaletteHandle' My log is here: http://pastebin.mozilla.org/2357970 This seems to be the same issue as described in bug 814928 and http://www.andrewsavory.com/blog/2012/2463 - if anyone can point me in the right direction, that'd be awesome. Anyhow, CC'ing Basta to see if he can spin up a build ASAP and apply the patch above on his Snow Leopard machine.

Hey Chris, I'll take a closer look at this patch tomorrow.

(In reply to Fabrice Desré [:fabrice] from comment #16) > Hey Chris, > > I'll take a closer look at this patch tomorrow. I think Andreas wanted to uplift this today. Andreas - what's the priority here?

(In reply to Wil Clouser [:clouserw] from comment #17) > (In reply to Fabrice Desré [:fabrice] from comment #16) > > Hey Chris, > > > > I'll take a closer look at this patch tomorrow. > > I think Andreas wanted to uplift this today. Andreas - what's the priority > here? Look at my comment date...

Removing the permission check in navgiator.cpp seems like it would be the opposite of what bug 859554 wants to achieve. Would it be possible to check in navigator.cpp and add an OR statement for this use case?

How would that OR statement look like?

(In reply to Andreas Gal :gal from comment #20) > How would that OR statement look like? My mistake, the check should really be an && in navigator.cpp if (!CheckPermission("mobileconnection") && !CheckPermission("mobilenetwork") { return NS_OK; } If the page has either mobileconnection or mobilenetwork, then it gets the mobileconnection object. The per API checks would remain as in the current patch since having either the mobileconnection or mobilenetwork permission doesn't imply a principal has the other.

Updated patch, fixing nits and Navigator.cpp to do the right thing.

Is this working? Ready to land?

Nope, sadly. I'm working on adding what's needed to not be killed at https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#387

This patch makes the "mobilenetwork" mozMobileConnection a very stripped down object (eg, no event listeners will work). That's probably enough for our immediate needs. If we need the event listeners, I can work on a updated version with more intrusive changes.

(In reply to Fabrice Desré [:fabrice] from comment #25) > Created attachment 744020 [details] [diff] [review] > inbound patch, working > > This patch makes the "mobilenetwork" mozMobileConnection a very stripped > down object (eg, no event listeners will work). That's probably enough for > our immediate needs. If we need the event listeners, I can work on a updated > version with more intrusive changes. Who can review? Is this change risky at all? If so, we'll need to target 5/10 to hit our launch date.

Comment on attachment 744020 [details] [diff] [review] inbound patch, working This is a pretty hacky fix obviously. We need a better patch that exposes a part of the object but allows the listeners associated with it. We can do that for 1.2. We need to uplift this to b2g18.

Actually this is wrong: 6.60 + services.prefs.setCharPref("ril.lastKnownHomeNetwork", 6.61 + message.mcc + "-" + message.mnc); Fabrice can you land a follow-on?

typo fix: https://hg.mozilla.org/projects/birch/rev/1ae920a04d36

Backed out for Marionette failures. https://hg.mozilla.org/projects/birch/rev/dea9d8108b8e https://tbpl.mozilla.org/php/getParsedLog.php?id=22536425&tree=Birch

Triage - Renom for more discussion. See https://bugzilla.mozilla.org/show_bug.cgi?id=867793#c3 for reasoning.

Got clarity on this now - we can tef+ again. But we still need to address the risks brought up in https://bugzilla.mozilla.org/show_bug.cgi?id=867793#c4.

Try build with the a small fix to pass the Mn tests: https://tbpl.mozilla.org/?tree=Try&rev=58599c8a7c6b

This needs a b2g18-specific patch.

Hi Fabrice, Why are we setting LastKnownNetwork twice in RadioInterfaceLayer.js? Is that intentional? I don't see that in the original patch or the patch in comment 35. Nivi. (In reply to Fabrice Desré [:fabrice] from comment #38) > https://hg.mozilla.org/releases/mozilla-b2g18/rev/0c71cbc5fe0c

Paul and Antonio: Can you make sure that the TEF apps for v1.0 that needed to have per-country specific behavior use this API rather than the other solutions that we had looked at. Paul: Can you make sure that this API ends up in the permission matrix and make sure to document that this only has permission checks in the child process. We need to make sure we add them to the parent process too, which will mean using another solution than preferences here.

Jonas, don't you think that what we need there is a readonly access mode on the mobile connection api rather than keeping this extra permission?

I definitely think that what was done here was a hack and we should use a different solution. I don't think a readonly mode for mobileconnection is the way to go. There's much more information on mobileconnection than just mcc+mnc. mcc+mnc has been asked for quite a few times so I think we should only expose that information on a separate API. Additionally mobileconnection is changing a lot between releases still so we should try to not use that at all for 3rd party apps. So what I'd like to see is an API specifically for mcc+mnc and then expose that under the new permission.

An alternative solution to the common mcc+mnc request is to enable some global mechanism for using different sets of preinstalled apps for different countries. And then the phone on first startup detects which apps to install based on internal knowledge about mcc+mnc during first startup. That way we don't have to worry about the user travelling to a different country, or inserting a friends sim card, and suddenly getting a different UX. My understanding is that that is not a desirable thing for most users.

Based on https://en.wikipedia.org/wiki/Mobile_country_code, Are we trying to get the which vendor the SIM was purchased from ? Currently the implementation seems to getting the information from the network not the SIM.

Er sorry. I think I commented in the wrong bug. This only shows the last mcc+mnc pair. ignore the last statement.

ran adb logcat grepping for MCC and MNC both when 1.)turning on the device with ENTEL SIM card on chilean network and 2.) trying to launch marketplace. I've annotated the log.

John, have you gotten an update of Marketplace (version 0.0.4)? Bug 868571 makes it so 0.0.4 is the version of the Marketplace shipped with Gaia.

I can retest with a 5/17 vendor build if this helps. I'm working remotely on an Ikura running a partner build (I'm at ZTE in Chile) so no possibility here to install Mozilla build. I also do not have an opportunity to build from tip here, so I expect the fix has landed in 5/17 build. How to check the version of Marketplace on this build?

I can retest with a 5/17 vendor build if this helps. I'm working remotely on an Ikura running a partner build (I'm at ZTE in Chile) so no possibility here to install Mozilla build. I also do not have an opportunity to build from tip here, so I expect the fix has landed in 5/17 build. How to check the version of Marketplace on this build?

Sorry about the double comment - terrible network here.

Thanks for the clarification, cvan. However, if I understand correctly then, the fix uplifted for https://bugzilla.mozilla.org/show_bug.cgi?id=868571 didn't land until afternoon PST. That means it's most likely not in the partner build I am running. Since I won't build from tip on this very slow network, we'll need to wait a bit to verify this fix.

I checked the mmc|mnc pair for two prepaid SIM cards down here in Chile; Movistar and Entel. Phone was Ikura. One was on 5/13 partner build, other was 5/17. In the 5/17 build I could no longer run adb as root (it was a production build) therefore, RIL debugging was not enabled for that one. However I got the same information for both, and according to the Mobile Country code spec on wikipedia: https://en.wikipedia.org/wiki/Mobile_country_code it was correct.

I did notice something else. For my Movistar prepaid SIM (on my 5/17 build), I switched Roaming on and reran logcat. But it's showing roaming as false: I/Gecko ( 112): -*- QCContentHelper_QC_B2G: sendMessage to content process: RIL:DataInfoChanged{connected : false,emergencyCallsOnly : false,roaming : false,type : 'umts',signalStrength : -67,rel File another bug?

John, yes please file a new bug and provide the android logs using the following command: adb logcat -b radio -b main -v time

Roaming may be enabled in the UI but if the device isn't actually roaming then that log message looks fine to me.

John, you can run adb logcat | grep -Ei 'MCC|MNC' In you see logs for the MCC and MNC and requests being made to https://marketplace.firefox.com/?mcc=<MCC>&mnc=<MNC> then you have the correct, working version of the Marketplace.

Sorry, not in Chile anymore - wasn't possible to retest. Somebody should follow up on Comment 56.

Clearing secrevew as this was reviewed ages ago.(In reply to Jonas Sicking (:sicking) needinfo? or feedback? encouraged. from comment #41) > Paul: Can you make sure that this API ends up in the permission matrix and > make sure to document that this only has permission checks in the child > process. We need to make sure we add them to the parent process too, which > will mean using another solution than preferences here. Permission is documented on https://developer.mozilla.org/en-US/Apps/Developing/App_permissions and bug 947784 is raised to implement this functionality in a sandbox safe way.