1102023 - IccId contains unexpected value (e.g. '*' and '#')

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P2)

Description

[sync-1]

Created an attachment (id=1021956)
 pic
 
 DEFECT DESCRIPTION:
 ->The ICCID display error.
 
 REPRODUCING PROCEDURES:
 ->Settings ->Device information ->More information ->ICCID
 KO:If ICCID have 'A' character, the mobile show it as '*'; If ICCID have 'b' character, the mobile show it as '#'.
 
 EXPECTED BEHAVIOUR:
 ->Display normally.
 
 ANALYSE:
 ril_worker.js function semiOctetToBcdChar() treat 'A' as '*', 'B' as '#'.
 
 REPRODUCING RATE:
 100%
 0752-2639344/61344
 
 For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Attachment: pic

Comment 2

[Josh Cheng [:josh]]

Hi Gary,
Could you please help to check the problem? Thanks!

Comment 3

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

redirect to sku, should be on his radar.
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js 

Hi Sku,
   Why do we have this design?

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Moving to the RIL component.

Comment 5

[shawn ku [:sku]]

Hi Edgar:
 Could we have your comment here first?

Thanks!!
Shawn

Comment 6

[Edgar Chen [:edgar]]

The '*' and '#' are from extended BCD coding [1]. But from the spec [2], the coding of IccId should be BCD, not extended BCD coding. We should correct this.

However, 0xa and 0xb are not a valid BCD value and they will be mapped to "" [3] if we follow the same error handling that we have now.

Please file a new bug if this isn't meet your expectation.

[1] See TS 131.102, table 4.4 Extended BCD coding
[2] See TS 102.221, clause 13.2 EF ICCID (ICC Identification)
[3] We had a similar issue before, see bug 976491.

Comment 7

[Edgar Chen [:edgar]]

This is a generic bug, change the title accordingly

Comment 8

[Josh Cheng [:josh]]

HI Edgar,
Could you help to provide ETA date? Thanks for your help!

Comment 9

[Edgar Chen [:edgar]]

Attachment: WIP, patch, v1

Comment 10

[Edgar Chen [:edgar]]

Attachment: Patch, v2

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8534933 [details] [diff] [review]
Patch, v2

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

Separate BCD and extended BCD helper to two, semiOctetToBcdChar/readSwappedNibbleBcdString and semiOctetToExtendedBcdChar/readSwappedNibbleExtendedBcdString.
And correct decoding icc to use BCD helper, instead of extended BCD helper.

Hi hsinyi, may I have your review, thank you.

Comment 12

[Josh Cheng [:josh]]

Dear Tao Li,
The patch is currently under review. If you need to verify the issue on your side before it land, please try patch https://bugzilla.mozilla.org/attachment.cgi?id=8534933
Thanks!

Comment 13

[land]

Dear Josh,
   It is not OK. Maybe the following function have bug.
@@ -12679,21 +12733,22 @@ ICCRecordHelperObject.prototype = {
 
   /**
    * Read the ICCID.
    */
   readICCID: function() {
     function callback() {
       let Buf = this.context.Buf;
       let RIL = this.context.RIL;
+      let GsmPDUHelper = this.context.GsmPDUHelper;
 
       let strLen = Buf.readInt32();
       let octetLen = strLen / 2;
       RIL.iccInfo.iccid =
-        this.context.GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
+        GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
       // Consumes the remaining buffer if any.
       let unReadBuffer = this.context.Buf.getReadAvailable() -
                          this.context.Buf.PDU_HEX_OCTET_SIZE;
       if (unReadBuffer > 0) {
         this.context.Buf.seekIncoming(unReadBuffer);
       }
       Buf.readStringDelimiter(strLen);

If I modify "GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);" to "GsmPDUHelper.readSwappedNibbleExtendedBcdString(octetLen, true);". The bug disappear.

Comment 14

[Edgar Chen [:edgar]]

(In reply to land from comment #13)
> Dear Josh,
>    It is not OK. Maybe the following function have bug.
> @@ -12679,21 +12733,22 @@ ICCRecordHelperObject.prototype = {
>  
>    /**
>     * Read the ICCID.
>     */
>    readICCID: function() {
>      function callback() {
>        let Buf = this.context.Buf;
>        let RIL = this.context.RIL;
> +      let GsmPDUHelper = this.context.GsmPDUHelper;
>  
>        let strLen = Buf.readInt32();
>        let octetLen = strLen / 2;
>        RIL.iccInfo.iccid =
> -        this.context.GsmPDUHelper.readSwappedNibbleBcdString(octetLen,
> true);
> +        GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
>        // Consumes the remaining buffer if any.
>        let unReadBuffer = this.context.Buf.getReadAvailable() -
>                           this.context.Buf.PDU_HEX_OCTET_SIZE;
>        if (unReadBuffer > 0) {
>          this.context.Buf.seekIncoming(unReadBuffer);
>        }
>        Buf.readStringDelimiter(strLen);
> 
> If I modify "GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);" to
> "GsmPDUHelper.readSwappedNibbleExtendedBcdString(octetLen, true);". The bug
> disappear.

It's strange. I have verified the patch locally, It works good.
Hi Tao Li, could you help to capture the log for me (start capturing at the beginning of device
boot-up)? Thank you.

Comment 15

[land]

Attachment: mtklog.rar

Patch log as the attachment.

Comment 16

[land]

Dear Edgar,

The function readICCID() always call the function readSwappedNibbleBcdString(). I think it should call the function readSwappedNibbleExtendedBcdString(). Maybe it is a slip of the pen.

Before your patch:
   /**
    * Read the ICCID.
    */
   readICCID: function() {
     function callback() {
       ......
       RIL.iccInfo.iccid =
        this.context.GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);

After your patch:
   /**
    * Read the ICCID.
    */
   readICCID: function() {
     function callback() {
       ......
       let GsmPDUHelper = this.context.GsmPDUHelper;
 
       ......
       RIL.iccInfo.iccid =
         GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8534933 [details] [diff] [review]
Patch, v2

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

Looks good to me, thank you!

Comment 18

[Edgar Chen [:edgar]]

Hi Tao, I tested this patch again, but it works good.
Could you help to double confirm again?

(In reply to land from comment #16)
> Dear Edgar,
> 
> The function readICCID() always call the function
> readSwappedNibbleBcdString(). I think it should call the function
> readSwappedNibbleExtendedBcdString(). Maybe it is a slip of the pen.

I modified readSwappedNibbleBcdString() in this patch, now it decodes data as BCD, not extended BCD. Calling readSwappedNibbleBcdString() in readICCID() is expected, actually.

Comment 19

[Edgar Chen [:edgar]]

try looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=63e2606acee8

Comment 20

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0d3dfb9b73b6

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d3dfb9b73b6

Comment 22

[land]

If IccId is "898600751914a9800454". Your patch will delete the charactor "a", show IccId as "8986007519149800454".

Pls see the following code-flow for the root cause:
1. 
   readICCID: function() {
     function callback() {
       RIL.iccInfo.iccid =
         GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
2.
   readSwappedNibbleBcdString: function(pairs, suppressException) {
     let str = "";
     for (let i = 0; i < pairs; i++) {
       ......
       str += this.semiOctetToBcdChar(nibbleL, suppressException);
       if (nibbleH != 0x0F) {
         str += this.semiOctetToBcdChar(nibbleH, suppressException);
       }
     }
 
     return str;
   },

3. Here, if IccId charactor don't belong to bcdChars, It will be treat as "".
   bcdChars: "0123456789",
   semiOctetToBcdChar: function(semiOctet, suppressException) {
     if (semiOctet >= this.bcdChars.length) {
       if (suppressException) {
         return "";
       } else {
         throw new RangeError();
       }
     }
 
     return this.bcdChars.charAt(semiOctet);
   },

Comment 23

[Edgar Chen [:edgar]]

(In reply to land from comment #22)
> If IccId is "898600751914a9800454". Your patch will delete the charactor
> "a", show IccId as "8986007519149800454".

Hi Tao, this is an expected behavior of this patch, please see commit #6.
0xa isn't a valid BCD value, so we map it to "" for error handling. Thank you.

Comment 24

[land]

I know it is not a valid BCD value. BUT some SIM card's IccId field include this character.

Comment 25

[land]

Maybe you can treat BCD value [a | b | c | d | e] as '*' to show for user.

Comment 26

[Josh Cheng [:josh]]

Hi Land,
According to http://www.etsi.org/deliver/etsi_ts/131100_131199/131102/04.15.00_60/ts_131102v041500p.pdf
Table 4.4: Extended BCD coding 

and

http://www.etsi.org/deliver/etsi_ts/102200_102299/102221/08.02.00_60/ts_102221v080200p.pdf
Clause 13.2 EF ICCID (ICC Identification)

The ICCID is presented by BCD instead of Extended BCD, that's why we remove A, B.

If you need to deal with some SIM card's IccId field. That would be customization effort and TCL can implement this base on our code. Thanks!

Comment 27

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Can you also land this patch on 2.0M? Thanks!

Comment 28

[land]

Dear Josh,
  OK, I will try it, thanks.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Does this need a v2.1 approval request as well?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/2f185a5bed54

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8534933 [details] [diff] [review]
Patch, v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Settings ->Device information ->More information ->ICCID shows unexpected value (e.g. *, # ... etc).
Testing completed: Patch includes xpcshell test.
Risk to taking this patch (and alternatives if risky): Low 
String or UUID changes made by this patch: None.

Comment 32

[Kai-Zhen Li [:seinlin][:kli]]

Ryan, Thanks!

Comment 33

[Shally Li（leave from Mozilla）]

This issue has been successfully verified on Flame v2.2 and Woodduck v2.0.
See attachment: verified_v2.0m.png and verified_v2.2.png
Reproduce rate: 0/4

Woodduck 2.0 build:
Gaia-Rev        1f11c58d7ad7b1eb2e08db98154ff4f20aa760ed
Gecko-Rev       c9a727cb1d1ea0b5a82c64a360c03dbc35421d59
Build-ID        20141223050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2
FW-Incremental  1419282327
FW-Date         Tue Dec 23 05:05:50 CST 2014

Flame 2.2 build:
Gaia-Rev        ca6e91e09ef3ab417a0f6b6d6668d43597d85700
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/b915a50bc6be
Build-ID        20141222040204
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141222.072850
FW-Date         Mon Dec 22 07:29:01 EST 2014
Bootloader      L1TC00011880

Comment 34

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.0m.png

Comment 35

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.png

Comment 36

[Edgar Chen [:edgar]]

Hi Steven, do we need to uplift this fix to 2.1s?

Comment 37

[sync-1]

TCL bug already fixed, pls close it.

Comment 38

[bhavana bajaj [:bajaj]]

Comment on attachment 8534933 [details] [diff] [review]
Patch, v2

Given this is already fixed on TCl side and its too late to take RIL changes like these on 2.1 I am minusing these. We can add land this on specific partner branches as necessary.

Comment 39

[Steven Yang [:styang]]

Vincent, let's pick it up for 2.1S.

Comment 40

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/342ee4302326