Closed Bug 880369 Opened 7 years ago Closed 6 years ago

navigator.mozMobileConnection.lastKnownHomeNetwork reports an invalid MCC/MNC pair for Polish T-Mobile SIM

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: cvan, Assigned: edgar)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(6 files, 2 obsolete files)

1) Insert a Polish T-Mobile SIM card.

2) Look at navigator.mozMobileConnection.lastKnownHomeNetwork or navigator.mozMobileConnection.lastKnownNetwork, both of which are reported as

   260-02137341765

An easy way to get this information is to install this packaged app from the Marketplace:

    https://marketplace.firefox.com/app/carrier-info/

Or logcat the Marketplace app:

    adb logcat | grep -Ei 'MCC|MNC|lastknown|mobilenetwork'

3) The MCC (260) for Poland is correct (per http://mcclist.com/mobile-network-codes-country-codes.asp and http://en.wikipedia.org/wiki/Mobile_Network_Code#Poland_-_PL). But the MNC (02137341765) is not.

The maximum length of a MNC is 3 digits, and I'm not even sure how to parse that MNC. It looks like 260-02 would be a valid MCC/MNC pair, so that's probably what the API needs to return.
Affected:
Boot2Gecko 1.0.1.0-prerelease
Operator shelf for marketplace will be incorrect if we don't fix this in Poland.
blocking-b2g: --- → tef?
Hi :cvan,

I can not reproduce this issue with Chunghwa SIM card. In your case, it seems device can not parse MNC correctly, this may a SIM card dependent issue. Could you help to capture log with RIL debug enabled?

---------------------------------------
Enable RIL debug log:
adb remount
adb pull /system/b2g/defaults/pref/user.js user.js
[update user.js with adding/modifying 'pref("ril.debugging.enabled", true);']
adb push user.js /system/b2g/defaults/pref/user.js
adb shell reboot

---------------------------------------
Please capture log by below command and start at the beginning of device boot-up:
adb logcat -b main -b radio -v threadtime > logcat.txt

Thanks
ni?(cvan) to provide radio logs for comment #3.
Flags: needinfo?(cvan)
Flags: needinfo?(cvan)
From the log, the EF_AD doesn't contain the length of MNC in Polish T-Mobile SIM card. So that MOZ RIL could not parse MNC correctly.

06-10 14:05:15.652   106   269 I Gecko   : RIL Worker: AD: 0, 255, 255, 
06-10 14:05:15.652   106   269 I Gecko   : RIL Worker: MCC: 260 MNC: 021373417656

From the recent spec. [1], the length of MNC is a mandatory field in EF_AD. But from the old spec. [2], it is an optional field. Although most SIM card contains the length of MNC in EF_AD, but I think we still need to have some way to get MNC correctly for the case that EF_AD doesn't contain the length of MNC.

Thanks.

[1] Please see TS 31.102 section 4.2.18
[2] Please see TS 51.011 section 10.3.18
Assignee: nobody → echen
If EF_AD dose not contain the length of MNC, check the MCC table to decide the length.
Attachment #760813 - Flags: review?(allstars.chh)
Attachment #760816 - Flags: review?(allstars.chh)
Comment on attachment 760813 [details] [diff] [review]
Part 1: Check mcc table if EF_AD dose not contain the length of mnc, v1

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

::: dom/system/gonk/ril_consts.js
@@ +2554,5 @@
>  this.PDU_CDMA_MSG_CODING_IS_91_TYPE_CLI = 0x84;
>  this.PDU_CDMA_MSG_CODING_IS_91_TYPE_SMS = 0x85;
>  
>  // Allow this file to be imported via Components.utils.import().
>  this.EXPORTED_SYMBOLS = Object.keys(this);

We'd better leave this line in the last, so I suggest you move your code before this.

::: dom/system/gonk/ril_worker.js
@@ +10202,5 @@
>        if (imsi) {
>          // MCC is the first 3 digits of IMSI.
>          RIL.iccInfo.mcc = imsi.substr(0,3);
>          // The 4th byte of the response is the length of MNC.
> +        let mncLength = ad[3];

You should add another check for ad,
if ad is undefined, (Some SIM from China seems doesn't have EF_AD at all)
ad[3] will throw TypeError.

@@ +10207,5 @@
> +        if (!mncLength) {
> +          // If response dose not contain the length of MNC, check the MCC table
> +          // to decide the length of MNC.
> +          let index = MCC_TABLE_FOR_MNC_LENGTH_IS_3.indexOf(RIL.iccInfo.mcc);
> +          mncLength = (index != -1) ? 3 : 2;

nit, !==
Attachment #760813 - Flags: review?(allstars.chh) → review+
Attachment #760816 - Flags: review?(allstars.chh) → review+
Address review comment #10
Attachment #760813 - Attachment is obsolete: true
Attachment #761471 - Flags: review+
Keywords: checkin-needed
Comment on attachment 761471 [details] [diff] [review]
Part 1: Check mcc table if EF_AD dose not contain the length of mnc, v2, r=allstars.chh

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

::: dom/system/gonk/ril_worker.js
@@ +10202,5 @@
>        if (imsi) {
>          // MCC is the first 3 digits of IMSI.
>          RIL.iccInfo.mcc = imsi.substr(0,3);
>          // The 4th byte of the response is the length of MNC.
> +        let mncLength = (ad) ? ad[3] : 0;

ad && ad[3];
seems shorter and better.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #13)
> Comment on attachment 761471 [details] [diff] [review]
> Part 1: Check mcc table if EF_AD dose not contain the length of mnc, v2,
> r=allstars.chh
> 
> Review of attachment 761471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +10202,5 @@
> >        if (imsi) {
> >          // MCC is the first 3 digits of IMSI.
> >          RIL.iccInfo.mcc = imsi.substr(0,3);
> >          // The 4th byte of the response is the length of MNC.
> > +        let mncLength = (ad) ? ad[3] : 0;
> 
> ad && ad[3];
> seems shorter and better.

okay, I use this to check ad, thanks.
Keywords: checkin-needed
Blocks: 866645
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7eb2fe2b8551
https://hg.mozilla.org/mozilla-central/rev/eeceac81b3e3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment on attachment 760816 [details] [diff] [review]
Part 2: Xpcshell tests for mcc/mnc parsing, v1

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +2606,5 @@
>  
> +/**
> + * Verify MCC and MNC parsing
> + */
> +add_test(function test_mcc_mnc_parsing()) {

/home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js:2624: SyntaxError: syntax error:
/home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js:2624: add_test(function test_mcc_mnc_parsing()) {

/home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js:2624: ........................................^
All of our partners are TA, so this will have to land for the first time in 1.1. leo+
blocking-b2g: tef? → leo+
Needs a b2g18 patch for uplift. I assume you'll pick up the fix for bug 887674 while you're at it.
Yes, I will pick up the fix for bug 887674 when I provide the b2g18 patch here.
Flags: needinfo?(echen)
You need to log in before you can comment on or make changes to this bug.