B2G RIL - Error fetching records for ICC cards with RIL_APPTYPE_SIM = 1.

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jaoo, Unassigned)

Tracking

Trunk
mozilla18
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

The RIL doesn't fetch properly ICC records for ICC cards with RIL_APPTYPE_SIM = 1. It seems a problem related to the `pathId` property that identifies where these records are stored in the ICC card. The RIL handles these records properly for USIM ICC cards but not for SIM ICC cards. The latter is the case of the SIMs from Vivo (Telefonica's branch in Brasil).
Posted patch WIP v1. (obsolete) — Splinter Review
This patch doesn't fix the problems but it give us an idea about where the problem is. There are still some problems while fetching MSISDN and MBDN records.
Attachment #654641 - Flags: feedback?(allstars.chh)
Attachment #654641 - Attachment is patch: true
Nom'ing for basecamp.
Blocks: b2g-ril
blocking-basecamp: --- → ?
FWIW, fetching MBDN was working on my US T-Moble SIM until recently. I'm not sure if that's related, but I'll find out soon hopefully.
Comment on attachment 654641 [details] [diff] [review]
WIP v1.

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

Hi, jaoo
Your patch will not work in USIM, as I discovered this in Bug 771440.

I suggest you can see the code in [1], it will detect the type of sim,
then using the corresponding Icc File Handler,

Then you can also see the file for APP_SIM in [2],
current code in m-c is only handling APP_USIM [3].

Also since its aid is null, 
I might suggest that you could still add
this.add = app.aid || null; 
in my first suggestion in email.

[1]:http://bit.ly/OmeHVW
[2]:http://bit.ly/OmeUbS
[3]:http://bit.ly/MYIyVv
Attachment #654641 - Flags: feedback?(allstars.chh) → feedback-
Posted patch WIP v2. (obsolete) — Splinter Review
Getting the `pathId` value for the specific ICC record depending on which type of ICC card we are using. Still seen problems while retrieving IMSI and MBDN records. Some feedback would be also nice about what would happen with importing contact feature from ICC card after proposed changes.
Attachment #654641 - Attachment is obsolete: true
Attachment #655733 - Flags: feedback?(allstars.chh)
Comment on attachment 655733 [details] [diff] [review]
WIP v2.

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

Cool. :)
Thanks for your hard work.

::: dom/system/gonk/ril_worker.js
@@ +2030,5 @@
>      this.sendDOMMessage({rilMessageType: "cardstatechange",
>                           cardState: this.cardState});
>    },
>  
> +  /** 

Extra space here

@@ +2032,5 @@
>    },
>  
> +  /** 
> +   * Helper function for getting the pathId for the specific ICC record
> +   * depeding on which type of ICC card we are using.

add documentation for fileId

@@ +2036,5 @@
> +   * depeding on which type of ICC card we are using.
> +   */
> +  _getPathIdOfICCRecord: function _getPathIdOfICCRecord(fileId) {
> +    let index = this.iccStatus.gsmUmtsSubscriptionAppIndex;
> +    let app = this.iccStatus.apps[index];

Perhaps 
let appType = this.iccStatus.apps[index].app_type ?
since you don't use app below.

@@ +2053,5 @@
> +          case ICC_EF_AD:
> +            return EF_PATH_MF_SIM + EF_PATH_DF_GSM;
> +          case ICC_EF_MBDN:
> +            return EF_PATH_MF_SIM + EF_PATH_DF_GSM;
> +        }

Since some are repeat, maybe you want to fall through?

@@ +2069,5 @@
> +            return EF_PATH_MF_SIM + EF_PATH_ADF_USIM;
> +          case ICC_EF_AD:
> +            return EF_PATH_MF_SIM + EF_PATH_ADF_USIM;
> +          case ICC_EF_MBDN:
> +            return EF_PATH_MF_SIM + EF_PATH_ADF_USIM;

ditto

@@ +2071,5 @@
> +            return EF_PATH_MF_SIM + EF_PATH_ADF_USIM;
> +          case ICC_EF_MBDN:
> +            return EF_PATH_MF_SIM + EF_PATH_ADF_USIM;
> +        }
> +    }  

Extra space
Attachment #655733 - Flags: feedback?(allstars.chh) → feedback+
Comment on attachment 655733 [details] [diff] [review]
WIP v2.

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

::: dom/system/gonk/ril_worker.js
@@ +2034,5 @@
> +  /** 
> +   * Helper function for getting the pathId for the specific ICC record
> +   * depeding on which type of ICC card we are using.
> +   */
> +  _getPathIdOfICCRecord: function _getPathIdOfICCRecord(fileId) {

s/Of/For/ would be slightly better IMHO

@@ +2036,5 @@
> +   * depeding on which type of ICC card we are using.
> +   */
> +  _getPathIdOfICCRecord: function _getPathIdOfICCRecord(fileId) {
> +    let index = this.iccStatus.gsmUmtsSubscriptionAppIndex;
> +    let app = this.iccStatus.apps[index];

Guard against index being null/-1 or `app` being null-ish/undefined?

@@ +2040,5 @@
> +    let app = this.iccStatus.apps[index];
> +
> +    switch(app.app_type) {
> +      case CARD_APPTYPE_SIM:
> +        switch(fileId) {

Nit: space between `switch` and parenthesis

@@ +2053,5 @@
> +          case ICC_EF_AD:
> +            return EF_PATH_MF_SIM + EF_PATH_DF_GSM;
> +          case ICC_EF_MBDN:
> +            return EF_PATH_MF_SIM + EF_PATH_DF_GSM;
> +        }

Like Yoshi said, lots of reptition... I wonder if we can just precompute the prefix (EF_PATH_MF_SIM) and whether to use EF_PATH_DF_GSM or EF_PATH_DF_USIM in RIL._processICCStatus(). It would make this function much simpler and also slight more efficient.
Posted patch v1 (obsolete) — Splinter Review
Addressed all the comments above and tested the patch with both SIM and USIM ICC cards. Some comments about the errors I was having are that ICC cards from Vivo don't include MBDN record and therefore voicemail number is null in content. As there's no MBDN record in the SIM the ICC IO operation fails but It finishes gracefully. Regarding the MSIDN record Vivo doesn't include it either. The ICC IO operation retrieves `+000000000000` as Android does. It seems the changes don't brake anything and make the fetching work for SIM ICC cards.
Attachment #654637 - Attachment is obsolete: true
Attachment #655733 - Attachment is obsolete: true
Attachment #656014 - Flags: review?(philipp)
Attachment #656014 - Flags: feedback?(allstars.chh)
Comment on attachment 656014 [details] [diff] [review]
v1

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

::: dom/system/gonk/ril_worker.js
@@ +2041,5 @@
> +      return;
> +    }
> +    let app = this.iccStatus.apps[index];
> +    if (!app) {
> +      return;

Style/strict mode nit: you're using an empty `return` for error cases. You should explicitly return something. Maybe `null`, maybe 0, maybe -1, maybe some default value? Not sure, you pick what makes most sense :). You should also return that at the bottom of the function in case none of the `case` statements applied.

For extra credit, document this in the comment above the function e.g.

  * @return the pathId or XYZ in case of an error or invalid input.
  */


r=me with that and also the trailing whitespace removed.
Attachment #656014 - Flags: review?(philipp) → review+
Attachment #656014 - Flags: feedback?(allstars.chh) → feedback+
Posted patch v2Splinter Review
Addressing latest comments.
Attachment #656014 - Attachment is obsolete: true
Attachment #656103 - Flags: checkin?(philipp)
Attachment #656103 - Flags: checkin?(philipp)
Target Milestone: mozilla17 → mozilla18
https://hg.mozilla.org/mozilla-central/rev/020c834a8991
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Seems like a blocker and even though it's fixed we don't want it to regress.
blocking-basecamp: ? → +
You need to log in before you can comment on or make changes to this bug.