828307 - B2G RIL: Change to store MNC/MCC values from integer to string

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Shian-Yow Wu [:swu]]

The current MCC/MNC value are stored as integer.  For operator, it's use exact string for matching, so it will cause problems when if MNC value starts with '0'.

Comment 1

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL change, v1

Comment 2

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2: GPS change, v1

Comment 3

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2: GPS change, v1

I found I upload a wrong file, upload again with correct one.

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation change, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests change, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 5: Marionette tests change, v1

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 717785 [details] [diff] [review]
WIP, Part 2: GPS change, v1

Hi Doug:

This bug is about change mcc/mnc type to string in nsIRadioInterfaceLayer, so there will be some change in GPS. I am not quite sure GPS needs to change mcc/mnc type to string or not, so create bug 844745 for tracking.

Could you help to provide some feedback about this patch?

Thanks in advance.

Comment 8

[Edgar Chen [:edgar]]

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=71a3d1a73216

Comment 9

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

Comment on attachment 717785 [details] [diff] [review]
WIP, Part 2: GPS change, v1

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

Stealing.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +448,5 @@
>    if (rilCtx) {
>      nsCOMPtr<nsIDOMMozMobileICCInfo> iccInfo;
>      rilCtx->GetIccInfo(getter_AddRefs(iccInfo));
>      if (iccInfo) {
> +      // TODO: Bug 844745 - B2G GPS: mcc/mnc type is int in AGpsRefLocationCellID

We can't change the AGpsRefLocationCellID type. So parsing the string back to integer is the right thing to do here.

@@ +457,5 @@
> +      iccInfo->GetMcc(mcc);
> +      iccInfo->GetMnc(mnc);
> +
> +      location.u.cellID.mcc = mcc.ToInteger(&result);
> +      location.u.cellID.mnc = mnc.ToInteger(&result);

You may want to check the parsing result.

Comment 10

[Edgar Chen [:edgar]]

(In reply to Kan-Ru Chen [:kanru] from comment #9)
> Comment on attachment 717785 [details] [diff] [review]
> WIP, Part 2: GPS change, v1
> 
> Review of attachment 717785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stealing.
> 
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +448,5 @@
> >    if (rilCtx) {
> >      nsCOMPtr<nsIDOMMozMobileICCInfo> iccInfo;
> >      rilCtx->GetIccInfo(getter_AddRefs(iccInfo));
> >      if (iccInfo) {
> > +      // TODO: Bug 844745 - B2G GPS: mcc/mnc type is int in AGpsRefLocationCellID
> 
> We can't change the AGpsRefLocationCellID type. So parsing the string back
> to integer is the right thing to do here.

Thanks for your feedback. :)

> 
> @@ +457,5 @@
> > +      iccInfo->GetMcc(mcc);
> > +      iccInfo->GetMnc(mnc);
> > +
> > +      location.u.cellID.mcc = mcc.ToInteger(&result);
> > +      location.u.cellID.mnc = mnc.ToInteger(&result);
> 
> You may want to check the parsing result.

I will add in formal patch.

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 2: GPS change, v2

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 717787 [details] [diff] [review]
Part 3: RIL implementation change, v1

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

Are there settings or preference entries to fix as well?

::: dom/system/gonk/ril_worker.js
@@ +2984,5 @@
>      }
>  
>      let [longName, shortName, networkTuple] = operatorData;
> +    let thisTuple = (this.operator.mcc && this.operator.mnc) ?
> +                    (this.operator.mcc + this.operator.mnc) : null;

I think you can simply have:

  let thisTuple = this.operator.mcc + this.operator.mnc;

@@ +3219,5 @@
>      for (let i = 0; i < strings.length; i += 4) {
>        let network = {
>          longName: strings[i],
>          shortName: strings[i + 1],
> +        mcc: null, mnc: null,

New line please.

@@ +3253,5 @@
>    _processNetworkTuple: function _processNetworkTuple(networkTuple, network) {
>      let tupleLen = networkTuple.length;
> +
> +    network.mcc = null;
> +    network.mnc = null;

Move into |else| block?

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 717794 [details] [diff] [review]
Part 5: Marionette tests change, v1

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

::: dom/network/tests/marionette/test_mobile_networks.js
@@ -143,5 @@
> -  });
> -
> -  throwsException(function() {
> -    connection.selectNetwork({});
> -  });

Why shouldn't these cases fail?

Comment 14

[Edgar Chen [:edgar]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)
> Comment on attachment 717794 [details] [diff] [review]
> Part 5: Marionette tests change, v1
> 
> Review of attachment 717794 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/tests/marionette/test_mobile_networks.js
> @@ -143,5 @@
> > -  });
> > -
> > -  throwsException(function() {
> > -    connection.selectNetwork({});
> > -  });
> 
> Why shouldn't these cases fail?

oh~ I remove 'null' test by accident. Will add back. 
throwsException(function() {
  connection.selectNetwork(null);
});

For '{}' case, because I use below code in selectNetwork, so pass an empty object will not throw exception any more.
let thisTuple = (this.operator.mcc && this.operator.mnc) ?
                            (this.operator.mcc + this.operator.mnc) : null;

But if you prefer use 'let thisTuple = this.operator.mcc + this.operator.mnc;' then this test needs to be added back.

I will update new patches to address this, thanks.

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 2: GPS change, v2, r=kchen

Add r=kchen into title after r+

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation change, v2, r=vyang

1). Address review comment #12
2). Add r=vyang into title after r+

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation change, v3, r=vyang

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #12)
> Comment on attachment 717787 [details] [diff] [review]
> Part 3: RIL implementation change, v1
> 
> Review of attachment 717787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are there settings or preference entries to fix as well?
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2984,5 @@
> >      }
> >  
> >      let [longName, shortName, networkTuple] = operatorData;
> > +    let thisTuple = (this.operator.mcc && this.operator.mnc) ?
> > +                    (this.operator.mcc + this.operator.mnc) : null;
> 
> I think you can simply have:
> 
>   let thisTuple = this.operator.mcc + this.operator.mnc;

Oh, we may get unexpected result in following code:

   let thisTuple = this.operator.mcc + this.operator.mnc;

For example, if mcc and mnc are both null, thisTuple becomes 0. Or if mcc and mnc are both 'undefined', thisTuple becomes NaN.

So I think we should keep original code:

    let thisTuple = (this.operator.mcc && this.operator.mnc) ?
                    (this.operator.mcc + this.operator.mnc) : null;

Thanks

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests change, v1, r=vyang

Add r=vyang into title after r+

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 5: Marionette tests change, v2

1). Add 'null' case back.
2). For '{}' case, selectNetwork() doesn't need to parse int to string. So we will not get exception any more if we pass an empty object. (Please ignore my previous comment in comment #14)

Thanks

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 718343 [details] [diff] [review]
Part 5: Marionette tests change, v2

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

::: dom/network/tests/marionette/test_mobile_networks.js
@@ -143,5 @@
>    });
>  
> -  throwsException(function() {
> -    connection.selectNetwork({});
> -  });

Well, I mean why should we remove all these test cases? Passing a null network or an empty js object should trigger an exception/error. I know you remove the case because there is no more exceptions thrown in this case, but why? Is there something wrong?

I find some checks in RILContentHelper. Didn't they function as expected?

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Edgar Chen [:edgar][:echen] from comment #17)
> Part 3: RIL implementation change, v3, r=vyang
> We may get unexpected result in following code:

|thisTuple| is only for matching newly got network tuple. So anything differs from the new one will still trigger necessary further processing.

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation change, v4

Correct the check condition in RILContextHelper. (comment #20)

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 5: Marionette tests change, v3

Address comment #20.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 718821 [details] [diff] [review]
Part 3: RIL implementation change, v4

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

Thank you for clarifying all the problems :)

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation change, v5, r=vyang

Comment 26

[Edgar Chen [:edgar]]

Attachment: Part 6: Save lastKnownMcc as string type, v1

Oh, I found I miss one thing, we should save lastKnownMcc into preference as string type as well. Thanks.

Comment 27

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #1)
> Created attachment 717782 [details] [diff] [review]
> Part 1: IDL change, v1

Hi Jonas,

This patch is for changing MNC/MCC type from integer to string, because '01' and '001' are different operator code. Needs your help to review when you are available.

Thanks in advance

Comment 28

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=348239256ff5

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL change, v2, r=vyang, sr=jonas

1). Rebase.
2). Add r=vyang and sr=jonas after r+ and sr+.

Comment 30

[Edgar Chen [:edgar]]

Attachment: Part 3: RIL implementation change, v6, r=vyang

Rebase

Comment 31

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests change, v2, r=vyang

Rebase.

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 6: Save lastKnownMcc as string type, v2, r=vyang

Rebase.

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/205360b12ae3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d630cab30499
https://hg.mozilla.org/integration/mozilla-inbound/rev/cea302feb67f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f469f29041f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6dced5d2ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba5ebc462e5

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/205360b12ae3
https://hg.mozilla.org/mozilla-central/rev/d630cab30499
https://hg.mozilla.org/mozilla-central/rev/cea302feb67f
https://hg.mozilla.org/mozilla-central/rev/0f469f29041f
https://hg.mozilla.org/mozilla-central/rev/4a6dced5d2ea
https://hg.mozilla.org/mozilla-central/rev/9ba5ebc462e5

Comment 35

[José Antonio Olivera Ortega [:jaoo]]

Marking this bug as tef? since it should be uplifted. See https://bugzilla.mozilla.org/show_bug.cgi?id=845629#c11

Comment 36

[Dietrich Ayala (:dietrich)]

This sounds like it is a behavior regression, and would likely fail certification by multiple parties, so marking tef+, and ni? for daniel to confirm.

Comment 37

[Daniel Coloma:dcoloma]

Thanks Dietrich, tefplusing it

Comment 38

[Edgar Chen [:edgar]]

Attachment: [b2g18] Part 1: IDL change, v2, r=vyang, sr=jonas

patch for b2g18

Comment 39

[Edgar Chen [:edgar]]

Attachment: [b2g18] Part 2: GPS change, v2, r=kchen

patch for b2g18

Comment 40

[Edgar Chen [:edgar]]

Attachment: [b2g18] Part 3: RIL implementation change, v6, r=vyang

Comment 41

[Edgar Chen [:edgar]]

Attachment: [b2g18] Part 4: xpcshell tests change, v2, r=vyang

Comment 42

[Edgar Chen [:edgar]]

Attachment: [b2g18] Part 5: Marionette tests change, v3, r=vyang

Comment 43

[Edgar Chen [:edgar]]

Attachment: [b2g18] Part 6: Save lastKnownMcc as string type, v2, r=vyang

patch for b2g18

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b231beb4937a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/de90a08821a2
https://hg.mozilla.org/releases/mozilla-b2g18/rev/74f28bd1ed06
https://hg.mozilla.org/releases/mozilla-b2g18/rev/77a643f61d60
https://hg.mozilla.org/releases/mozilla-b2g18/rev/52419d926f5e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e19f0babf188

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2c1ea04d75e8
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/bf6c3bf15223
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8cae642254d1
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/bf9f3c95c7a8
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8408a72fa8f2
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2c37df73e3e9

This was tef+, not leo+, BTW.

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

I'm not sure whether these patches had been verified on 1.0.1 branch or not, so I left it affected, and wanted to ask Edgar tomorrow.

Comment 47

[Anshul]

Guys, this is another instance where commercial RIL broke on v1 because of this change. The proper process would have been to mark it as NO_UPLIFT until commercial RIL is ready. 

We are working on adding the support for the changes in the RIL interface in commercial RIL urgently.

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Anshul from comment #47)
> Guys, this is another instance where commercial RIL broke on v1 because of
> this change. The proper process would have been to mark it as NO_UPLIFT
> until commercial RIL is ready. 

In fact, at the time I asked you during Madrid work week, you don't filter NO_UPLIFT bugs and have no preparation for them. So I see no point marking changes with NO_UPLIFT before landing it.

Comment 49

[Ken Chang[:kenkai]]

(In reply to Anshul from comment #47)
> Guys, this is another instance where commercial RIL broke on v1 because of
> this change. The proper process would have been to mark it as NO_UPLIFT
> until commercial RIL is ready. 
> 
> We are working on adding the support for the changes in the RIL interface in
> commercial RIL urgently.

Anshul, because this bug is very emergent bug blocking the certification test, we were
requested to fix it as soon as possible yesterday. That is why this NO_UPLIFT wasn't set. 
But now can you please provide the new commercial RIL for fixing this problem?
We really need this new commercial RIL for certification. Thanks.

Comment 50

[José Antonio Olivera Ortega [:jaoo]]

Should we hold on the uplifting for bugs 845629, 863125, 860411 and 863126 until this bug has been included in shipping RIL?

Comment 51

[Ken Chang[:kenkai]]

(In reply to Ken Chang from comment #49)
> (In reply to Anshul from comment #47)
> > Guys, this is another instance where commercial RIL broke on v1 because of
> > this change. The proper process would have been to mark it as NO_UPLIFT
> > until commercial RIL is ready. 
> > 
> > We are working on adding the support for the changes in the RIL interface in
> > commercial RIL urgently.
Anshul, we also need the new commercial RIL for 1.1(Leo). Thanks.

Comment 52

[Anshul]

Ken, we are working on providing the changes for both 1.0 and 1.1 and should be available today.

Vicamo, I am not sure how you got the impression that we don't look at NO_UPLIFT tag. MMS was an exception as you guys wanted to land all your patches right away without having to wait for the corresponding fixes in commercial RIL.

I would like to ensure that we do very much still require that you notify us anytime you are planning to make an interface change. If you tell us a day in advance then we would be ready by the time you are ready and so your changes won't have to wait on us. If you don't let us know then your changes would break our builds which means OEM won't be able to get your fix anyways until we add support for the same in commercial RIL.

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Anshul from comment #52)
> Ken, we are working on providing the changes for both 1.0 and 1.1 and should
> be available today.
> 
> Vicamo, I am not sure how you got the impression that we don't look at
> NO_UPLIFT tag.

Simple. I asked you whether you're ready for MMS interface changes, and you asked me what had changed back. I had marked every MMS changes with NO_UPLIFT, and commented under each of them with a link to the uplift branch, and you still didn't know. That's how I knew it.

> MMS was an exception as you guys wanted to land all your
> patches right away without having to wait for the corresponding fixes in
> commercial RIL.

You can well say that for everything. Everything is an exception.

> I would like to ensure that we do very much still require that you notify us
> anytime you are planning to make an interface change.

We did, but in vain.

> If you tell us a day
> in advance then we would be ready by the time you are ready and so your
> changes won't have to wait on us.

What actually happened is: we had marked changes with NO_UPLIFT for weeks and still have to ignore commercial ril build failure for days so that we can land MMS changes first.

> If you don't let us know then your changes
> would break our builds which means OEM won't be able to get your fix anyways
> until we add support for the same in commercial RIL.

Bugzilla is open, github is open. How do I "don't let you know"?

The tag is totally useless. Please fix the way you fetch B2G sources.

Comment 54

[Michael Vines [:m1] [:evilmachines]]

We should be able to solve this by ensuring that Anshul is added as a reviewer for ALL future RIL interface changes.  

Vicamo, ensure that Anshul is r? on all interface changes that you are requested to review from this point forward.

Comment 55

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #54)
> We should be able to solve this by ensuring that Anshul is added as a
> reviewer for ALL future RIL interface changes.

The problem is landing to another tree, not review. Please fix the root problem instead.

Comment 56

[Ken Chang[:kenkai]]

Let's refine the procedure in mailing list. The original bug(MNC/MCC) was fixed yesterday. Thanks.

Comment 57

[Tony Chung [:tchung]]

Can someone confirm if this patch is uplifted to 1.0.1 or not?  the flag says yes, but the comments confuse me.   

This is blocking QA from testing our nightly builds + commercial RIL on v1.0.1 according to:  https://bugzilla.mozilla.org/show_bug.cgi?id=863395#c15.

Comment 58

[Michael Vines [:m1] [:evilmachines]]

Yep, it's on v1.0.1

Comment 59

[Tony Chung [:tchung]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #58)
> Yep, it's on v1.0.1

then is there another reason why i'm still hitting the problem with https://bugzilla.mozilla.org/show_bug.cgi?id=863395#c14?   the build has AU 087.

Comment 61

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

In order to test, you would have to use a Mock MCC/MNC starting with 0 or you would need to connect using a SIM card from a MNC starting with 0 and connect to that network.  ( see https://en.wikipedia.org/wiki/Mobile_country_code )

Comment 62

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

Turns out that the movistar would be one of those SIM.  Even testing in the US, I am getting the correct values:

D/ICCIO_QC_B2G(  112): MCC = 214, MNC = 07
D/ICCIO_QC_B2G(  112): MCC = 214, MNC = 07


So the Ril is working fine.  I am however seeing T-Mobile as the carrier in settings.  I think this might be more of a gaia issue.

Comment 63

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

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a3a49f8d101e
Gaia   09299399500b50de72cdd3b4365fc6455bbbb145
BuildID 20130522070210
Version 18.0

Inari

Comment 64

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

Oh interesting:
05-23 13:28:05.129: I/Gecko(112): -*- QCContentHelper_QC_B2G: sendMessage to content process: RIL:DataInfoChanged{connected : false,emergencyCallsOnly : false,roaming : true,type : 'edge',signalStrength : -99,relSignalStrength : 22,network : {mcc : '310', mnc : '260', longName : 'T-Mobile', shortName : 'T-Mobile'},cell : {gsmLocationAreaCode : 275, gsmCellId :23991},state : 'registered'}

yet : adb logcat | grep -E 'MCC|MNC'
had showed me the correct values as seen in comment 62.  The QCContentHelper_QC_B2G is getting the network operator, where as the API is getting the SIM carrier.

Comment 65

[José Antonio Olivera Ortega [:jaoo]]

Naoki, the pair MCC and MNC codes in the ICC card identifies the subscriber's home network and they don't change even in roaming (these codes are stored in the ICC card, they *never* change). The log you see from the RIL content helper (in this case, because you are in roaming with the Movistar ICC card) shows the MCC and MNC pair from the foreign network (in this case the T-Mobile network where the device is camping in the US).

Comment 66

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

Jose, shouldn't the MCC then not change and only the APN should change?  We're showing 2 different values based on where you check for the MCC.  This could cause a lot of bugs to easily appear based on mcc/mnc...

Comment 67

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #66)
> Jose, shouldn't the MCC then not change and only the APN should change?

The MCC and MNC codes stored in the SIM never change. The APNs for data calls, MMS, SUPL, etc. are always the ones that corresponds to that pair of codes so the APNs never change as well.

> We're showing 2 different values based on where you check for the MCC.

How is that possible?, which are those places? Maybe you are seeing the MCC and MNC pair from the voice connection or the one from the data connection.

Even in roaming, both the MCC and MNC pair from the voice connection and data connection change but the APN for data calls must not change.

> This could cause a lot of bugs to easily appear based on mcc/mnc...