1064266 - [MLS] Network location provider doesn't handle roaming correctly

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect, P1)

Description

[Hanno Schlichting]

We have code in the network location provider to extract all current cell connections, extract their unique identifiers and use them to do location lookups.

This code needs to look up the mcc and mnc values of each cell connection. But currently the code uses the mcc and mnc values of the SIM card and not those of the active connection.

Specifically in http://dxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#432:

iccInfo = radio.rilContext.iccInfo and later iccInfo.mcc and iccInfo.mnc are used.

Instead this needs to use the network.mcc and network.mnc values available from a https://developer.mozilla.org/en-US/docs/Web/API/MozMobileConnectionInfo - in this case this is probably available as voice.network.mcc / voice.network.mnc.

The impact of this bug, is that anyone on a roaming connection won't use correct cell information for geolocation lookups. Most likely not receiving any cell based result at all, but potentially also a wrong location result.

Roaming happens both inside countries as well as abroad. If someone is abroad they will most likely get wrong positions from inside their home country instead of results from the country they are actually in.

This bug is present in all versions of Firefox OS. Specifically any version of Firefox with devices using the Mozilla Location Service is affected: Tarako 1.3T, Dolphin 1.4/2.0?, Flame 2.0/2.1 and Woodduck (2.0M?).

Devices using a replaced Geo stack and those before 1.3T aren't affected.

Comment 1

[:garvan]

Attachment: bug1064266.patch

As per Hanno's suggestion, changed to use voice.network.mcc and mmc instead of the values from SIM. 

I tested with a non-roaming SIM on Flame 2.1.

In future, the test case for this is to have a SIM card in a phone that requires roaming to connect, and shut off WIFI. As long as the cell tower is in the MLS db, this test case will return the cell-based location (and if the cell tower is unknown to MLS, then GeoIP is returned, and this test cannot be used).

Comment 2

[:garvan]

Further to my previous comment:
- you would also have to shut off the GPS (there is a switch in the Gaia debug screen for this purpose [2.1 and higher])
- shutting off the wifi doesn't work, B2G scans for wifi networks with the connect to wifi checkbox. This is not QA-testable ATM.

Comment 3

[Hanno Schlichting]

Comment on attachment 8486633 [details] [diff] [review]
bug1064266.patch

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

Feedback+ from me.

Comment 4

[:garvan]

(In reply to Garvan Keeley [:garvank] from comment #2)
> Further to my previous comment:
> - you would also have to shut off the GPS (there is a switch in the Gaia
> debug screen for this purpose [2.1 and higher])

I mean, on a GPS-enabled device. Could just use a Tarako for this test, although, you can't shut off WIFI scanning, so still not QA testable.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8486633 [details] [diff] [review]
bug1064266.patch

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

looks good.

I thought we use mcc and mnc from network in the GPS provider, but I'm wrong. We need to fix this too:

https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp?rev=6f40822c1c04#503

Comment 6

[:garvan]

Cloned the bug in relation to comment 5

Comment 7

[Carsten Book [:Tomcat]]

Hi, could you provide a try run link, thanks!

Comment 8

[:garvan]

Try green linux64, full mochitest:
https://tbpl.mozilla.org/?tree=Try&rev=1e85627df2d6

B2G emulator build is also running, but I'll be waiting till xmas for it, and there is no test that hits this code.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/dd6e02fe1625

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd6e02fe1625

Comment 11

[Chris Peterson [:cpeterson]]

[Blocking Requested - why for this release]:

B2G 1.3T and later are affected. Geolocation using cell towers is broken when roaming and, in some countries, roaming is very common.

Comment 12

[Wayne Chang [:wchang]]

As we'll continue to have some new launches for 1.3T in the next weeks let's take this on 1.3T.

Chris,
Can you also set 1.4/2.0 uplift approval on the patch? Those requests can be made to :bajaj.

Thanks a lot!

Comment 13

[Chris Peterson [:cpeterson]]

^ Garvan: can you please double-check which B2G branches need this fix and set the patch's uplift approval flags?

Comment 14

[:garvan]

Comment on attachment 8486633 [details] [diff] [review]
bug1064266.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Changeset: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/d6d3224471e7
Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=977120

User impact if declined: 
When roaming, no cell tower lookups for geolocation. Will have to rely on wifi or geoip. In markets such as India, network geolocation is heavily dependent on cell towers, so network geolocation will be very poor (when roaming) without this fix.

Testing completed: 
Verified the non-roaming case, that the MCC+MNC (mobile country codes) are looked up correctly. Don't have test hardware for roaming case.

Risk to taking this patch (and alternatives if risky):
Extremely low risk, only changed code to look at the mobile country codes from the active cell connection rather than the SIM card.
 
String or UUID changes made by this patch:
None

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 8486633 [details] [diff] [review]
bug1064266.patch

[Triage Comment]

Approving on aurora for 2.1/b2g 30 for 1.4.

Its too late for 2.0(b2g32) and we've EOL 1.3(b2g 28).

TO land this on 1.3T, either self land it on https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t with a=bajaj or NI Ryanvm for help.

Comment 16

[:garvan]

Thanks, I wasn't intending for 2.0 since that is closed, I assumed the b2g32 flag was for 2.0M. What flag do I set for that one?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora (and presumably others) uplift.

Comment 18

[:garvan]

There is a problem, my tiny line change is on top of a much larger change
https://bugzilla.mozilla.org/show_bug.cgi?id=843452

This patch added the nsIMobileConnectionService which this patch uses. Chris, I'll follow up with you on suggestions for next steps.

Comment 19

[Chris Peterson [:cpeterson]]

Garvan says his patch depends on bug 843452, which only landed on Gecko 35 for B2G 2.2. So we can't uplift his patch to any releases before 2.2.

Comment 20

[Josh Cheng [:josh]]

Hi Jamin,
As we discussed. Could you help to provide 2.0 patch when you have time? Thanks!

Comment 21

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Josh Cheng [:josh] from comment #20)
> Hi Jamin,
> As we discussed. Could you help to provide 2.0 patch when you have time?
> Thanks!

OK, I would provide a patch for 2.0M. :)

Comment 22

[Josh Cheng [:josh]]

(In reply to Jamin Liu [:jaliu] (PTO: 12/8 ~ 12/10) from comment #21)
> (In reply to Josh Cheng [:josh] from comment #20)
> > Hi Jamin,
> > As we discussed. Could you help to provide 2.0 patch when you have time?
> > Thanks!
> 
> OK, I would provide a patch for 2.0M. :)

Hi Jamin,
Could you give me an ETA date? Thanks!

Comment 23

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Josh Cheng [:josh] from comment #22)
> Hi Jamin,
> Could you give me an ETA date? Thanks!

ETA for 2.0m would be Dec. 16.
Thanks. :)

Comment 24

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (for 2.0m) Use the mcc and mnc values from MobileNetworkInfo instead of IccInfo (i.e. SIM)

Hi Kan-ru,
Due to the request from our partner, we need to fix this bug on branch 2.0m.
I'd like to ask you to review the patch when you have time.

I've tested this patch on flame v2.0 with a roaming SIM card of India.
The the mcc/mnc values likes correct to me.
> network.mcc: 466 (Taiwan)
> network.mnc: 92 (Chunghwa LDM)
> iccInfo.mcc: 404 (India)
> iccInfo.mnc: 11 (Vodafone IN)

Thank you.

Comment 25

[Josh Cheng [:josh]]

Hi Kan-ru,
This fix is request urgently by partner on 2.0M. Could you help to review it? Thanks!

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8537026 [details] [diff] [review]
(for 2.0m) Use the mcc and mnc values from MobileNetworkInfo instead of IccInfo (i.e. SIM)

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

LGTM

Comment 27

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (for 2.0m) Use the mcc and mnc values from MobileNetworkInfo instead of IccInfo (i.e. SIM), r=kchen

Thank Kan-Ru for reviewing the patch.

- Convert to hg format
- Add reviewer's name to commit message

Comment 28

[Jamin Liu [:jaliu][:Jamin]]

checkin-needed for 2.0m

Comment 29

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/89001dd4f870