Closed Bug 1064266 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla35
blocking-b2g 2.0M+
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- fixed
b2g-v2.1 --- wontfix
b2g-v2.2 --- fixed

People

(Reporter: hschlichting, Assigned: garvan)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch bug1064266.patchSplinter Review
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).
Attachment #8486633 - Flags: review?(kchen)
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 on attachment 8486633 [details] [diff] [review]
bug1064266.patch

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

Feedback+ from me.
Attachment #8486633 - Flags: feedback+
(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 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
Attachment #8486633 - Flags: review?(kchen) → review+
See Also: → 1065751
Cloned the bug in relation to comment 5
Keywords: checkin-needed
Hi, could you provide a try run link, thanks!
Assignee: nobody → gkeeley
Flags: needinfo?(gkeeley)
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.
Flags: needinfo?(gkeeley)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dd6e02fe1625
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
[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.
blocking-b2g: --- → 1.4?
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!
blocking-b2g: 1.4? → 1.3T+
Flags: needinfo?(cpeterson)
^ Garvan: can you please double-check which B2G branches need this fix and set the patch's uplift approval flags?
Flags: needinfo?(cpeterson) → needinfo?(gkeeley)
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
Attachment #8486633 - Flags: approval-mozilla-b2g32?
Attachment #8486633 - Flags: approval-mozilla-b2g30?
Attachment #8486633 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(gkeeley)
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.
Attachment #8486633 - Flags: approval-mozilla-b2g32?
Attachment #8486633 - Flags: approval-mozilla-b2g32-
Attachment #8486633 - Flags: approval-mozilla-b2g30?
Attachment #8486633 - Flags: approval-mozilla-b2g30+
Attachment #8486633 - Flags: approval-mozilla-b2g28?
Attachment #8486633 - Flags: approval-mozilla-b2g28-
Attachment #8486633 - Flags: approval-mozilla-aurora+
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?
Needs rebasing for Aurora (and presumably others) uplift.
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.
Flags: needinfo?(gkeeley)
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.
Attachment #8486633 - Flags: approval-mozilla-b2g32-
Attachment #8486633 - Flags: approval-mozilla-b2g30+
Attachment #8486633 - Flags: approval-mozilla-b2g28-
Attachment #8486633 - Flags: approval-mozilla-aurora+
See Also: → 1100297
Depends on: 843452
Hi Jamin,
As we discussed. Could you help to provide 2.0 patch when you have time? Thanks!
blocking-b2g: 1.3T+ → 2.0M+
Flags: needinfo?(jaliu)
(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. :)
Flags: needinfo?(jaliu)
Blocks: Woodduck_P2
(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!
Flags: needinfo?(jaliu)
(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. :)
Flags: needinfo?(jaliu)
Priority: -- → P1
No longer blocks: Woodduck_P2
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.
Attachment #8537026 - Flags: review?(kchen)
Hi Kan-ru,
This fix is request urgently by partner on 2.0M. Could you help to review it? Thanks!
Flags: needinfo?(kchen)
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
Attachment #8537026 - Flags: review?(kchen) → review+
Flags: needinfo?(kchen)
Thank Kan-Ru for reviewing the patch.

- Convert to hg format
- Add reviewer's name to commit message
Attachment #8537026 - Attachment is obsolete: true
checkin-needed for 2.0m
Keywords: checkin-needed
Summary: Network location provider doesn't handle roaming correctly → [MLS] Network location provider doesn't handle roaming correctly
Flags: in-moztrap?(slyu)
Flags: in-moztrap?(slyu) → in-moztrap-
You need to log in before you can comment on or make changes to this bug.