787967 - B2G RIL: report roaming if SPN is different from operator name

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[:gerard-majax]

French network operator "Free" is using roaming with "Orange" while it sets up his network. Having two Free SIM card, one in Android device and one in B2G device, I noticed that only B2G reports roaming.

I suspect we should do as in Android:
https://github.com/android/platform_frameworks_base/blob/jb-release/telephony/java/com/android/internal/telephony/gsm/GsmServiceStateTracker.java#L1264
As justified in:
https://github.com/android/platform_frameworks_base/blob/jb-release/telephony/java/com/android/internal/telephony/gsm/GsmServiceStateTracker.java#L665

Comment 1

[:gerard-majax]

Attachment: Requesting SPN from SIM card

A first need is to request the service provider from the sim card.

Comment 2

[:gerard-majax]

Attachment: Part 1: Code for requesting SPN from SIM card

Introducing the getSPN() function.

Comment 3

[:gerard-majax]

Attachment: Part 2: Probbing the SPN with other ICC fields

Now, we probe the SPN when probbing the other ICC fields

Comment 4

[:gerard-majax]

Attachment: Part 3: Heuristic for detecting when not really roaming

This introduces a heuristic that verifies if we are really roaming or of RIL is just joking. Idea is simple: check sim/network name and country code. If they match, then we are not really roaming.

Comment 5

[:gerard-majax]

Attachment: Part 4: Forcing roaming status when needed

Now we make use of the roaming heuristic and we force it false if someone is fooling is.

Comment 6

[:gerard-majax]

With debug enabled:

I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.rilMessageType=iccinfochange
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.imsi=20815XXXXXXXX
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.spn=Free
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.msisdn=+336XXXXXXX
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.mnc=15
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.mcc=208
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: spn=Free
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: onsl=Free
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: onss=Free
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: equalsOnsl=true
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: equalsOnss=true
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: equalsMcc=true
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: retval=false

Comment 7

[Mounir Lamouri (:mounir)]

Alexande, if those patch are ready, you should ask a review to a RIL peer. See: https://wiki.mozilla.org/Modules/All#Radio_Interface_Layer

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 657920 [details] [diff] [review]
Part 1: Code for requesting SPN from SIM card

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

Yoshi, you know the ICC I/O code very well, can you review this please?

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 657922 [details] [diff] [review]
Part 3: Heuristic for detecting when not really roaming

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +531,5 @@
> +   * Fix the roaming. RIL can report roaming in some case it is not
> +   * really the case. See bug 787967
> +   *
> +   * @param message  The voiceMessage or dataMessage from which the
> +   *                 roaming state will be changed (maybe).

I don't think this is true, is it? Perhaps you initially wanted `isRoamingBetweenOperators` to modify the message in place but then changed it to return a value?

Also, we could call this parameter `regInfo` or even `registration` for a bit more clarity.

@@ +538,5 @@
> +  isRoamingBetweenOperators: function isRoamingBetweenOperators(message, operator) {
> +    let icc = this.rilContext.icc;
> +    let retval = message.roaming;
> +
> +    if (icc) {

Bail out early could make this a little easier to read:

  let icc = this.rilContext.icc;
  if (!icc) {
    return registration.roaming;
  }

  ...
  return message.roaming && !(equalsMcc && (equalsOnsl || equalsOnss));

@@ +544,5 @@
> +
> +        let onsl = operator.longName;
> +        let onss = operator.shortName;
> +
> +        let equalsOnsl = (onsl != undefined) && (spn == onsl);

Better:

  onsl && (spn == onsl);

@@ +549,5 @@
> +        let equalsOnss = (onss != undefined) && (spn == onss);
> +
> +        let equalsMcc = icc.mcc == operator.mcc;
> +
> +        retval = message.roaming && !(equalsMcc && (equalsOnsl || equalsOnss));

It feels like we're checking things twice here, given how you invoke this helper in part 4:

  if (data.connected && data.roaming && !this.isRoamingBetweenOperators(data, operatorMessage)) {

I would prefer to have this helper modify `message.roaming` in place and do these checks only once in one place.

@@ +554,5 @@
> +
> +        if (DEBUG) {
> +          debug("isRoamingBetweenOperators: icc=" + JSON.stringify(icc));
> +          debug("isRoamingBetweenOperators: icc.rilMessageType=" + icc.rilMessageType);
> +          debug("isRoamingBetweenOperators: icc.imsi=" + icc.imsi);

Please don't include this block in the final patch. All this information should already be logged as part of the postMessage() calls that we make between ril_worker and RadioInterfaceLayer.

Comment 10

[:gerard-majax]

Attachment: Part 3 v2: Heuristic for detecting when not really roaming

Applying the changes you suggested:
 - in place modification
 - message -> registration
 - remove debug

Comment 11

[:gerard-majax]

Attachment: Part 4v2: Forcing roaming status when needed

Now only calling the isRoamingBetweenOperators() that will do the change itself.

Comment 12

[:gerard-majax]

Attachment: Part 3 v3: Heuristic for detecting when not really roaming

Removing the operator argument, we already have it as part of the 'network' field in registration message.

Comment 13

[:gerard-majax]

Attachment: Part 4 v3: Forcing roaming status when needed

Only passing the voice or data message.

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 658257 [details] [diff] [review]
Part 3 v3: Heuristic for detecting when not really roaming

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +533,5 @@
> +    *
> +    * @param registration  The voiceMessage or dataMessage from which the
> +    *                      roaming state will be changed (maybe, if needed).
> +    */
> +   isRoamingBetweenOperators: function isRoamingBetweenOperators(registration) {

Now that we've made the function do in-place changes, maybe `checkRoamingBetweenOperators` is a better name.

As for the function body, it looks good except for a few nits:

* there's some trailing whitespace, a lot of empty lines, an overlong line (80 chars max please)
* it's maybe not as concise as it could be while using somewhat cryptic variable names

I'd probably write it as

  let icc = this.rilContext.icc;
  if (!icc || !registration.connected) {
    return;
  }
		 
  let spn = icc.spn;
  let operator = registration.network;
  let longName = operator.longName;
  let shortName = operator.shortName;
		
  let equalsOnsl = longName && (spn == longName);
  let equalsOnss = shortName && (spn == snortName);
  let equalsMcc = icc.mcc == operator.mcc;
		
  registration.roaming = registration.roaming &&
                         !(equalsMcc && (equalsOnsl || equalsOnss));


r=me with those addressed. Once you get Yoshi's review for Part 1, I suggest you squish all patches into one and upload a final patch for checkin. Thanks!

Comment 15

[:gerard-majax]

Attachment: Part 3 v4: Heuristic for detecting when not really roaming

Addressing latest comments:
 - changing function name (makes a 86-long line though)
 - smaller-easier to read code as suggested
 - removing too cryptics variables
 - removed whitespaces as far as I've found some.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 657920 [details] [diff] [review]
Part 1: Code for requesting SPN from SIM card

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

Hi, Lissy
Thanks for your work, however I think this patch is incomplete,
I cannot find ICC_EF_SPN in _getPathIdForICCRecord through all your patches ,nor m-c.
so I gave you r- here.

Can you check that again?

Thanks

::: dom/system/gonk/ril_worker.js
@@ +1172,5 @@
> +      let length = Buf.readUint32();
> +      // Each octet is encoded into two chars.
> +      // Minus 1 because first is used to store display condition
> +      let len = (length / 2) - 1;
> +      let spnDisplayCondition = Buf.readUint32();

let spnDisplayCondition = GsmPDUHelper.readHexOctet();

@@ +1174,5 @@
> +      // Minus 1 because first is used to store display condition
> +      let len = (length / 2) - 1;
> +      let spnDisplayCondition = Buf.readUint32();
> +      this.iccInfo.spn = GsmPDUHelper.readAlphaIdentifier(len);
> +

add Buf.readStringDelimiter(length); here

@@ +1184,5 @@
> +
> +    this.iccIO({
> +      command:   ICC_COMMAND_GET_RESPONSE,
> +      fileId:    ICC_EF_SPN,
> +      pathId:    this._getPathIdForICCRecord(ICC_EF_SPN),

I cannot find ICC_EF_SPN in _getPathIdForICCRecord, 
where did you put it ?

Comment 17

[:gerard-majax]

Attachment: Part 1 v2: Code for requesting SPN from SIM card

Addressing comments:
 - added ICC_EF_SPN in _getPathIdForICCRecord as a sibling of EF_AD
 - fixed display condition reading
 - added Buf.readStringDelimiter() call

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 658399 [details] [diff] [review]
Part 1 v2: Code for requesting SPN from SIM card

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

Great.
Thanks for your hard work.

Comment 19

[:gerard-majax]

ping?

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Alexandre LISSY :gerard-majax from comment #19)
> ping?

See philikon's comment in comment 14.

Also it seems your patch is not from hg, can you also check [1], [2], and [3]?

Thank you

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2]: https://developer.mozilla.org/en-US/docs/Creating_a_patch
[3]: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 21

[:gerard-majax]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #20)
> (In reply to Alexandre LISSY :gerard-majax from comment #19)
> > ping?
> 
> See philikon's comment in comment 14.

Yes, I did the change suggested, but I expected it should be reviewed once again, just for the sake of correctness, we both could forgot something.

> 
> Also it seems your patch is not from hg, can you also check [1], [2], and
> [3]?

It's from git format-patch, so I think it matches the requirements in [1]:

    It has a correctly formatted author name.
    It has a correctly formatted commit message.
    It is generated in git style.

> 
> Thank you
> 
> [1]:
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> [2]: https://developer.mozilla.org/en-US/docs/Creating_a_patch
> [3]: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 22

[:gerard-majax]

Attachment: Merging all patches in one

Merging all patches in one, after rebasing on current master.

Comment 23

[Philipp von Weitershausen [:philikon]]

Comment on attachment 661524 [details] [diff] [review]
Merging all patches in one

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

Nice work!

Comment 24

[Philipp von Weitershausen [:philikon]]

Comment on attachment 661524 [details] [diff] [review]
Merging all patches in one

Unfortunately this patch no longer applies on top of m-c tip. Could you please rebase? Thanks!

Comment 25

[:gerard-majax]

(In reply to Philipp von Weitershausen [:philikon] from comment #24)
> Comment on attachment 661524 [details] [diff] [review]
> Merging all patches in one
> 
> Unfortunately this patch no longer applies on top of m-c tip. Could you
> please rebase? Thanks!

My gecko master is at 0b6677a9d5ed3cd8f0dd95f39cd3a0a204129d76, and the branch rebase successfully :(. Are there pending changed between mozilla-central dans the git repo that affects this code ?

Comment 26

[Philipp von Weitershausen [:philikon]]

(In reply to Alexandre LISSY :gerard-majax from comment #25)
> (In reply to Philipp von Weitershausen [:philikon] from comment #24)
> > Comment on attachment 661524 [details] [diff] [review]
> > Merging all patches in one
> > 
> > Unfortunately this patch no longer applies on top of m-c tip. Could you
> > please rebase? Thanks!
> 
> My gecko master is at 0b6677a9d5ed3cd8f0dd95f39cd3a0a204129d76,

I don't know which repository this is from. There are several git ones. The Mercurial repository at http://hg.mozilla.org/mozilla-central/ is the canonical source code repository for Gecko so at the end of the day, no matter how you work on Gecko, this is where patches have to apply.

> and the
> branch rebase successfully :(. Are there pending changed between
> mozilla-central dans the git repo that affects this code ?

Maybe. I was actually trying to apply it to mozilla-inbound [1] which is where patches get landed first before they bubble over to mozilla-central if they pass CI.

Comment 27

[:gerard-majax]

Re-updated my gecko from git://github.com/mozilla/releases-mozilla-central, still no issue :(

Comment 28

[:gerard-majax]

Attachment: Fixing roaming status.

Okay, finally got this conflict, so here we go with the updated patch. Should be harmless, but I'm currently rebuilding gecko and testing on my Nexus S in a couple of minutes.

Comment 29

[:gerard-majax]

Gecko flashed, phone rebooted, PIN code entered, roaming went away :)

Comment 30

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/509340e36e44

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/509340e36e44