1065751 - [AGPS]Gonk GPS provider AGPS: doesn't handle roaming cell

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[:garvan]

+++ This bug was initially created as a clone of Bug #1064266 +++

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.

From Kan-Ru's comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1064266#c5
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 1

[Josh Cheng [:josh]]

Hi Jamin,
Can you take this bug? Thanks!

Comment 3

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

Attachment: Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. (v1)

Comment 4

[Josh Cheng [:josh]]

(In reply to Jamin Liu [:jaliu] (PTO: 12/8 ~ 12/10) from comment #3)
> Created attachment 8528902 [details] [diff] [review]
> Support AGPS with roaming cell by retrieving mcc & mnc from
> nsIMobileNetworkInfo. (v1)

Hi Jamin, Do you need review for your patch? Thanks!

Comment 5

[Josh Cheng [:josh]]

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

Comment 6

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

Comment on attachment 8528902 [details] [diff] [review]
Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. (v1)

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

Hi Kan-ru,
I'd like to ask you to review the patch when you have time.

I tested this patch on flame v2.2 with a roaming SIM card of India.
However, I only tested it once and I didn't print mcc/mnc as record at that time.
The reason is that I have only one pre-paid SIM card from India with data-connection and it ran out its quota during my first roaming AGPS test. 
(P.S. It did speed up the GPS locating process in that test.)

Nevertheless, I verified mcc/mnc values with roaming SIM card in networkGeolocation when I tested the patch of Bug 1064266. 

Thank you.

Comment 7

[Josh Cheng [:josh]]

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

Comment 8

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

Comment on attachment 8528902 [details] [diff] [review]
Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. (v1)

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

LGTM

Comment 9

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

Attachment: (for master) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. (v1), r=kchen

Thank Kan-Ru for reviewing the patch.

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

Comment 10

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

It looks fine on treeherder.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98ab3bbeca9c

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad534d0193f4

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

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

Jamin, Uplift to 2.0m got conflicts, Could you rebase a patch for 2.0m? Thanks!

Comment 14

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

Attachment: (for 2.0m) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. (v1), r=kchen

Hi Kai-Zhen,
Please push this patch to 2.0m branchsince it's marked as a 2.0m+ bug.
Thank you.

Comment 15

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

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/e794ed760e5e

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Does this need to land on v2.1? If so, please request b2g34 approval on the patch.

Comment 17

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

[Blocking Requested - why for this release]:

FxOS v2.1 can't reduce the GPS TTFF(Time To First Fix) by using AGPS technology if the device uses roaming network. 
In this case, the AGPS TTFF might be even worse than general GPS since AGPS provides incorrect aiding data.

I'd like to ask for triage to review whether or not this bug is a v2.1 blocker.
The patch is ready. If it's a 2.1+ bug, I will request b2g34 approval later.

Thanks

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Brian, can you find someone to test this patch for 2.1 please?  We're worried about fall outs when uplifting.

Comment 19

[Brian Huang [:brianhuang][:brhuang]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #18)
> Brian, can you find someone to test this patch for 2.1 please?  We're
> worried about fall outs when uplifting.

Sure.

Mike, Please help to test this patch. Thank you.

Comment 20

[Mike Lien[:mlien]]

Hi Jamin, could I get the v2.1 local patch for testing?

Comment 21

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

(In reply to Mike Lien[:mlien] from comment #20)
> Hi Jamin, could I get the v2.1 local patch for testing?

Hi Mike,
Ok, I'll pull v2.1 branch and put v2.1 local patch on Bugzilla later. :)

Comment 22

[Keven Kuo [:kkuo]]

Hi! Jamin,

I changed the status as reopen and please help to land this on v2.1. Thanks

--
Keven

Comment 23

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

ni? myself to remind me provide v2.1 patch as long as I've finished my current task.

Comment 24

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

Attachment: (for v2.1) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. r=kchen

Comment 25

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

(In reply to Mike Lien[:mlien] from comment #20)
> Hi Jamin, could I get the v2.1 local patch for testing?

Hi Mike,
I put v2.1 patch at attachment 8551229 [details] [diff] [review].
Thank you for your help. :)

Comment 26

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

Hi Mike,
Would you mind to test #attachment 8551229 [details] [diff] [review] when you have feel time ?
I'll request for mozilla-b2g34_v2_1 approval if QA think the patch is safe for v2.1. :)

Comment 27

[Mike Lien[:mlien]]

Patch has been verified with v2.1 and it works well. The tested roaming SIM is T-Mobie.

[Only GPS]: it takes 5 minutes and 30 seconds to get the first position
*Test steps (Only GPS):
 1. Flash build contains patch
 2. Launch Geoloc app and start to get position

[AGPS from roaming SIM]:
Slot one with 3G: it takes less than 10 seconds to get the first position
Slot two with 2G: it takes less than 30 seconds to get the first position
*Test steps (Roaming SIM AGPS):
 1. Flash build contains patch
 2. Enable Data connection and Data roaming in Settings app -> Cellular & Data
 3. Launch Geoloc app and start to get position

Comment 28

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

Comment on attachment 8551229 [details] [diff] [review]
(for v2.1) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. r=kchen

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
(feature) Bug 715788 - Add A-GPS support for gonk

> User impact if declined: 
If user uses roaming network, the GPS TTFF(Time To First Fix) can't be reduced by using AGPS technology.

In this case, the AGPS TTFF might be even worse than general GPS since AGPS provides incorrect aiding data.

> Testing completed:
Mike Lien from Taipei QA team has tested this patch with roaming SIM.
Please refer to #comment 27

> Risk to taking this patch (and alternatives if risky): 
Currently, the use case isn't covered by any automated test case since it's hard to emulate the condition of usage on tinderbox. (Roaming card + AGPS)

> String or UUID changes made by this patch:
No.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Uplifts don't require checkin-needed :)

Comment 30

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

Attachment: (for v2.1s) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. r=kchen

- Upload a patch for v2.1s

Comment 31

[Ryan VanderMeulen [:RyanVM]]

*sigh* Except reopening the bug for uplifts totally breaks actually getting them uplifted. *PLEASE* leave bugs closed if they're fixed on master. Status flags track uplifts and our bug queries won't catch things needing uplift otherwise.

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e6c0cae7fee3

Comment 32

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/e6c0cae7fee3