Closed Bug 828307 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla22
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- verified

People

(Reporter: swu, Assigned: edgar)

References

Details

Attachments

(12 files, 14 obsolete files)

1.62 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.66 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.51 KB, patch
edgar
: review+
Details | Diff | Splinter Review
11.66 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.79 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.30 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.61 KB, patch
Details | Diff | Splinter Review
1.58 KB, patch
Details | Diff | Splinter Review
11.63 KB, patch
Details | Diff | Splinter Review
3.71 KB, patch
Details | Diff | Splinter Review
3.07 KB, patch
Details | Diff | Splinter Review
2.52 KB, patch
Details | Diff | Splinter Review
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'.
Depends on: 828089
Assignee: nobody → echen
Blocks: 844679
Blocks: 844745
Attached patch Part 1: IDL change, v1 (obsolete) — Splinter Review
Attached patch WIP, Part 2: GPS change, v1 (obsolete) — Splinter Review
Attached patch WIP, Part 2: GPS change, v1 (obsolete) — Splinter Review
I found I upload a wrong file, upload again with correct one.
Attachment #717783 - Attachment is obsolete: true
Attachment #717794 - Attachment description: WIP → WIP, Part 5: Marionette tests change, v1
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.
Attachment #717785 - Flags: feedback?(doug.turner)
Attachment #717782 - Attachment description: WIP, Part 1: IDL change, v1 → Part 1: IDL change, v1
Attachment #717782 - Flags: superreview?(jonas)
Attachment #717782 - Flags: review?(vyang)
Attachment #717787 - Attachment description: WIP, Part 3: RIL implementation change, v1 → Part 3: RIL implementation change, v1
Attachment #717787 - Flags: review?(vyang)
Attachment #717792 - Attachment description: WIP, Part 4: xpcshell tests change, v1 → Part 4: xpcshell tests change, v1
Attachment #717792 - Flags: review?(vyang)
Attachment #717794 - Attachment description: WIP, Part 5: Marionette tests change, v1 → Part 5: Marionette tests change, v1
Attachment #717794 - Flags: review?(vyang)
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.
Attachment #717785 - Flags: feedback?(doug.turner) → feedback+
(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.
Attached patch Part 2: GPS change, v2 (obsolete) — Splinter Review
Attachment #717785 - Attachment is obsolete: true
Attachment #718268 - Flags: review?(kchen)
Attachment #718268 - Flags: review?(kchen) → review+
Attachment #717782 - Flags: review?(vyang) → review+
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?
Attachment #717787 - Flags: review?(vyang) → review+
Attachment #717792 - Flags: review?(vyang) → review+
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?
Attachment #717794 - Flags: review?(vyang)
(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.
Add r=kchen into title after r+
Attachment #718268 - Attachment is obsolete: true
1). Address review comment #12
2). Add r=vyang into title after r+
Attachment #717787 - Attachment is obsolete: true
(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
Attachment #718314 - Attachment is obsolete: true
Add r=vyang into title after r+
Attachment #717792 - Attachment is obsolete: true
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
Attachment #717794 - Attachment is obsolete: true
Attachment #718343 - Flags: review?(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?
Attachment #718343 - Flags: review?(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.
Correct the check condition in RILContextHelper. (comment #20)
Attachment #718338 - Attachment is obsolete: true
Attachment #718821 - Flags: review?(vyang)
Address comment #20.
Attachment #718343 - Attachment is obsolete: true
Attachment #718822 - Flags: review?(vyang)
Attachment #718822 - Flags: review?(vyang) → review+
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 :)
Attachment #718821 - Flags: review?(vyang) → review+
No longer depends on: 845629
Attachment #718821 - Attachment is obsolete: true
Oh, I found I miss one thing, we should save lastKnownMcc into preference as string type as well. Thanks.
Attachment #720532 - Flags: review?(vyang)
Attachment #720532 - Flags: review?(vyang) → review+
(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
Attachment #717782 - Flags: superreview?(jonas) → superreview+
1). Rebase.
2). Add r=vyang and sr=jonas after r+ and sr+.
Attachment #717782 - Attachment is obsolete: true
Attachment #723768 - Flags: superreview+
Attachment #723768 - Flags: review+
Attachment #718305 - Flags: review+
Rebase
Attachment #719840 - Attachment is obsolete: true
Attachment #723770 - Flags: review+
Rebase.
Attachment #718339 - Attachment is obsolete: true
Attachment #723773 - Flags: review+
Rebase.
Attachment #720532 - Attachment is obsolete: true
Attachment #723784 - Flags: review+
Marking this bug as tef? since it should be uplifted. See https://bugzilla.mozilla.org/show_bug.cgi?id=845629#c11
blocking-b2g: --- → tef?
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.
blocking-b2g: tef? → tef+
Flags: needinfo?(dcoloma)
Thanks Dietrich, tefplusing it
Flags: needinfo?(dcoloma)
patch for b2g18
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.
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.
(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.
(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.
Flags: needinfo?(anshulj)
Should we hold on the uplifting for bugs 845629, 863125, 860411 and 863126 until this bug has been included in shipping RIL?
(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.
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.
Flags: needinfo?(anshulj)
(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.
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.
(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.
Let's refine the procedure in mailing list. The original bug(MNC/MCC) was fixed yesterday. Thanks.
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.
Yep, it's on v1.0.1
(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.
Duplicate of this bug: 844679
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 )
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.
Status: RESOLVED → VERIFIED
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a3a49f8d101e
Gaia   09299399500b50de72cdd3b4365fc6455bbbb145
BuildID 20130522070210
Version 18.0

Inari
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.
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).
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...
(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...
You need to log in before you can comment on or make changes to this bug.