924740 - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ming]

Attachment: ICCIOHelper.processICCIOGetResponse exception

For USIM card ,it seems ril of gecko didn't support it correctly . When ICCIOHelper process  the  response for at command AT+CRSM  [eg : fileid EF_ICCID EF_AD ....],it will throws exception.

This can cause other problems.
Due to the exception in reading the sim records , the callback didn't be called, then the mcc code didn't be set, and then in gaia/system the apns didn't be created.

Will mozilla support the USIM card?

here is Exception :
1325 I/Gecko   (  104): RIL Worker[0]: Solicited response for request type 28, token 13, error 0
1326 I/Gecko   (  104): RIL Worker[0]: Handling parcel as REQUEST_SIM_IO
1327 I/Gecko   (  104): RIL Worker[0]: Parcel handling threw Error: Expected file ID 2fe2 but read 4121
1328 I/Gecko   (  104): processICCIOGetResponse@resource://gre/modules/ril_worker.js:10905
1329 I/Gecko   (  104): ICC_COMMAND_GET_RESPONSE@resource://gre/modules/ril_worker.js:10997
1330 I/Gecko   (  104): processICCIO@resource://gre/modules/ril_worker.js:10882
1331 I/Gecko   (  104): REQUEST_SIM_IO@resource://gre/modules/ril_worker.js:5198
1332 I/Gecko   (  104): handleParcel@resource://gre/modules/ril_worker.js:4822
1333 I/Gecko   (  104): processParcel@resource://gre/modules/ril_worker.js:146
1334 I/Gecko   (  104): processIncoming@blob:20e8e8ab-03b6-423b-b75e-af61aabad2d1:536
1335 I/Gecko   (  104): onRILMessage@resource://gre/modules/ril_worker.js:13109

see the attachment B-all for more logs

Comment 1

[James Zhang (Spreadtrum)]

ming, pls add your patch to attachment

Comment 2

[Ming]

Attachment: carrier.js.patch

Comment 3

[Ming]

during finding the bugs i made two patch:

1.carrier.js.patch : 
fix the bugs in the case that the 'apnsForIccCards' is undefined or empty, which can cause that user can't custom the apn settings .

2.ril_worker.js.patch: 
Refered to android , i found that android works fine even the operation of SIM IO for USIM card was failed too.
The difference is that the android can get the mcc code for all types of card from the response of REQUEST_GET_IMSI, but in gecko the codes in RIL[REQUEST_GET_IMSI] is under the case "if (isCdma)";

Comment 4

[Ming]

Attachment: ril-worker.js.patch

Comment 5

[Edgar Chen [:edgar]]

(In reply to Ming from comment #3)
> during finding the bugs i made two patch:
> 
> 1.carrier.js.patch : 
> fix the bugs in the case that the 'apnsForIccCards' is undefined or empty,
> which can cause that user can't custom the apn settings .
> 
> 2.ril_worker.js.patch: 
> Refered to android , i found that android works fine even the operation of
> SIM IO for USIM card was failed too.
Does it mean android fail to operate SIM IO for USIM, too?

> The difference is that the android can get the mcc code for all types of
> card from the response of REQUEST_GET_IMSI, but in gecko the codes in
> RIL[REQUEST_GET_IMSI] is under the case "if (isCdma)";

For SIM/USIM, We parse the mcc/mnc based on the result of EF_AD [1]. I think the reason you can not get mcc/mnc is because failed to read EF_AD. And for "if (isCdma)" part please see bug 866645 comment #5. Thanks

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#11086

Comment 6

[Ming]

Not failed to read，actually it is failed to parse.

the actually response data is :
D/AT      (  155): [w] Channel2: AT< +CRSM: 144,0,62178202412183026FAD8A01058B036F060180020004880118
D/AT      (  155): [w] Channel2: AT< OK
D/RILC    (  155): [w] [0016]< SIM_IO {sw1=0x90,sw2=0x0,62178202412183026FAD8A01058B036F060180020004880118}

RIL Worker[0]: Parcel handling threw Error: Expected file ID 6fad but read 4121


On unagi with the same card  gecko will get  the response like this :
D/RILC    (  111): RIL_SIM_IO_Response: sw1=144 sw2=0 data=000000046fad040000000005020000
This can be parse correctly.
It seems there are some transformations in unagi's MODEM or RILC,is it?
 

(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> (In reply to Ming from comment #3)
> > during finding the bugs i made two patch:
> > 
> > 1.carrier.js.patch : 
> > fix the bugs in the case that the 'apnsForIccCards' is undefined or empty,
> > which can cause that user can't custom the apn settings .
> > 
> > 2.ril_worker.js.patch: 
> > Refered to android , i found that android works fine even the operation of
> > SIM IO for USIM card was failed too.
> Does it mean android fail to operate SIM IO for USIM, too?
> 
> > The difference is that the android can get the mcc code for all types of
> > card from the response of REQUEST_GET_IMSI, but in gecko the codes in
> > RIL[REQUEST_GET_IMSI] is under the case "if (isCdma)";
> 
> For SIM/USIM, We parse the mcc/mnc based on the result of EF_AD [1]. I think
> the reason you can not get mcc/mnc is because failed to read EF_AD. And for
> "if (isCdma)" part please see bug 866645 comment #5. Thanks
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#11086

Comment 7

[Ming]

Yes, android failed too.
(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> (In reply to Ming from comment #3)
> > during finding the bugs i made two patch:
> > 
> > 1.carrier.js.patch : 
> > fix the bugs in the case that the 'apnsForIccCards' is undefined or empty,
> > which can cause that user can't custom the apn settings .
> > 
> > 2.ril_worker.js.patch: 
> > Refered to android , i found that android works fine even the operation of
> > SIM IO for USIM card was failed too.
> Does it mean android fail to operate SIM IO for USIM, too?
> 
> > The difference is that the android can get the mcc code for all types of
> > card from the response of REQUEST_GET_IMSI, but in gecko the codes in
> > RIL[REQUEST_GET_IMSI] is under the case "if (isCdma)";
> 
> For SIM/USIM, We parse the mcc/mnc based on the result of EF_AD [1]. I think
> the reason you can not get mcc/mnc is because failed to read EF_AD. And for
> "if (isCdma)" part please see bug 866645 comment #5. Thanks
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#11086

Comment 8

[Edgar Chen [:edgar]]

(In reply to Ming from comment #6)
> Not failed to read，actually it is failed to parse.

We follow the standard [1] to parse the result of GetResponse. Thanks.

[1] TS 51.001, clause 9.2.1

> 
> the actually response data is :
> D/AT      (  155): [w] Channel2: AT< +CRSM:
> 144,0,62178202412183026FAD8A01058B036F060180020004880118
> D/AT      (  155): [w] Channel2: AT< OK
> D/RILC    (  155): [w] [0016]< SIM_IO
> {sw1=0x90,sw2=0x0,62178202412183026FAD8A01058B036F060180020004880118}
> 
> RIL Worker[0]: Parcel handling threw Error: Expected file ID 6fad but read
> 4121
> 
> 
> On unagi with the same card  gecko will get  the response like this :
> D/RILC    (  111): RIL_SIM_IO_Response: sw1=144 sw2=0
> data=000000046fad040000000005020000
> This can be parse correctly.
> It seems there are some transformations in unagi's MODEM or RILC,is it?

Comment 9

[Ming]

Sorry ,i'm a newer to protocols , can i say that it's the responsibility of RILC or MODEM to transform the data format in order to fit the protocol of [1]? Just like what qcom did on unagi.
(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> (In reply to Ming from comment #6)
> > Not failed to read，actually it is failed to parse.
> 
> We follow the standard [1] to parse the result of GetResponse. Thanks.
> 
> [1] TS 51.001, clause 9.2.1
> 
> > 
> > the actually response data is :
> > D/AT      (  155): [w] Channel2: AT< +CRSM:
> > 144,0,62178202412183026FAD8A01058B036F060180020004880118
> > D/AT      (  155): [w] Channel2: AT< OK
> > D/RILC    (  155): [w] [0016]< SIM_IO
> > {sw1=0x90,sw2=0x0,62178202412183026FAD8A01058B036F060180020004880118}
> > 
> > RIL Worker[0]: Parcel handling threw Error: Expected file ID 6fad but read
> > 4121
> > 
> > 
> > On unagi with the same card  gecko will get  the response like this :
> > D/RILC    (  111): RIL_SIM_IO_Response: sw1=144 sw2=0
> > data=000000046fad040000000005020000
> > This can be parse correctly.
> > It seems there are some transformations in unagi's MODEM or RILC,is it?

Comment 10

[Ming]

(In reply to Ming from comment #3)

> For SIM/USIM, We parse the mcc/mnc based on the result of EF_AD [1]. I think
> the reason you can not get mcc/mnc is because failed to read EF_AD. And for
> "if (isCdma)" part please see bug 866645 comment #5. Thanks
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#11086

Also i am puzzled that why don't we drop the code of "if (isCdma)"? It we do so ,just like android , the mcc code can be set in two call path.

Comment 11

[Edgar Chen [:edgar]]

(In reply to Ming from comment #9)
> Sorry ,i'm a newer to protocols , can i say that it's the responsibility of
> RILC or MODEM to transform the data format in order to fit the protocol of
> [1]? Just like what qcom did on unagi.

Yes, I think it should be handled in RILC or MODEM side. Thanks.

Comment 12

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

On Android, the Modem has coverted the GET_RESPONSE for USIM to SIM format,
that is, when Android framework/FirefoxOS RIL calls RIL_REQUEST_SIM_IO with command is GET_RESPONSE, the response is of format TS 51.011, whether the SIM underneath is SIM or USIM.

In comment 1 you mentioned you got 4121,
that's from FCP template from USIM. 
See TS 102.221, 11.1.1.3.2: Response for an EF

FCP Template :
  - 82XX 4221 : File Descriptor
    82 : Tag
    XX : Length
    42 : File descriptor byte (cf 11.5)
      42 = 01 000 010
        - 01 : Shareable file
        - 000 : Working EF
        - 010 : Linear fixed structur
    21 : Data coding byte


You could also look into Android AOSP, Android RIL doesn't handle FCP template as well, for modem has processed this for them.

AFAIK, or you could do some search on google,
only some old models from HTC still send FCP template to upper layer.

Many modems from different vendors have done the conversion in the modem side, like QC, Samsung, LG, which FirefoxOS runs fine on their devices.

Comment 13

[Ming]

I see . Thanks to all.
:)

Comment 14

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

close as WONTFIX as this should be done in modem side.

Comment 15

[James Zhang (Spreadtrum)]

Add Bruce, please check spreadtrum modem issue.

Comment 16

[Ming]

Modem side won't handle this , reopen it.

Comment 17

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

(In reply to Ming from comment #16)
> Modem side won't handle this , reopen it.

Any reason other vendors could but spreadtrum can't?  Mozilla RIL takes advantage of Android RIL to reduce porting effort to different devices.  It's true there are still differences mainly from version fragmentation and additional, exclusive commands.  So, what on earth is the reason you cannot be compatible with Android RIL on this topic?

Comment 18

[Ming]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #17)
As mentioned above ,  android doesn't handle this correctly.

Comment 19

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

At least we both agree Android RIL doesn't have to handle FCP.  Again, Mozilla RIL is based on Android RIL, especially rild.  We're expecting behaviours revealed from Android frameworks/base/telephony/java/.  For those don't match and we don't have a way to contact the vendor or to modify the logic in rild directly, we create a quirk when necessary.  But I don't think the previous statement evaluates to true in this case.

Comment 20

[Ming]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
Maybe android just forget to handle the usim card.

Maybe ffos can do a better job for usim card.


Bruce && james ,  any ideas?

Comment 21

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

(In reply to Ming from comment #20)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
> Maybe android just forget to handle the usim card.

That's not true.

Comment 22

[James Zhang (Spreadtrum)]

Ming, pls reopen this bug.

Mchen and Styang, not only tara(8810), but also fugu(7710) and shark(7730) also have this FFOS issue.
And our android bsp version is ok. So we need clarify whether this issue is caused by qcom hard code.

Comment 23

[James Zhang (Spreadtrum)]

Add Sam.Hua and Bruce.Jiang

Comment 24

[Ming]

Need more talk over this issue

Comment 25

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

I don't accept somebody overrides my decision before giving a convincing result.

Comment 26

[Andreas Gal :gal]

We need a resolution here. Please don't close this bug until we have found a fix.

Comment 27

[Ken Chang[:kenkai]]

(In reply to Andreas Gal :gal from comment #26)
> We need a resolution here. Please don't close this bug until we have found a
> fix.
Thanks for this suggestion. I had talked with partner before. They had fixed this bug on their codes. 
We can close this bug now.

Comment 28

[James Zhang (Spreadtrum)]

Actually we patch gecko code to fix this bug, we are suffering the inconvenience of these patches when DSDS development.
We need a spreadtrum branch and flag for improving our cooperation effect, thanks.

Comment 29

[Andreas Gal :gal]

Can you please attach your Gecko bug. Lets take a look and discuss options. I would prefer to have Spreadtrum support directly in the main tree. Maybe we can special path this for SPRD hardware for now, and work on a nicer API abstraction 2nd. This bug should only be closed by Spreadtrum or myself, once we have a full resolution.

Comment 30

[Ken Chang[:kenkai]]

(In reply to James Zhang from comment #28)
> Actually we patch gecko code to fix this bug, we are suffering the
> inconvenience of these patches when DSDS development.
> We need a spreadtrum branch and flag for improving our cooperation effect,
> thanks.
Thanks for this information. I think we had discussed this before.
1. For short term solution, you can push this in your github. We will get it from your Github to replay
   our ril_work.js. And you will sync with our RIL everyday.
2. For long term solution, you will provide a fix in your Modem.
For item 1, indeed, we had discussed if it is possible to have a flag to land your patch into mozilla-central last week. But according to Shawn's email comments, it seems possible to fix this in your RILC instead of Modem/Gecko.
So do you want to close this bug after having long term solution? 
I will call you to talk more about this.

Comment 31

[Ken Chang[:kenkai]]

(In reply to Ken Chang from comment #30)
> 1. For short term solution, you can push this in your github. We will get it
> from your Github to replay
Oops! What I meant is "replace"....:(

Comment 32

[Ken Chang[:kenkai]]

Here is the conclusion after discussing with James and Sam.
They will own their ril_worker.js. However, if they would like to fix the common bugs, they still can provide their code for review and check-in.

Comment 33

[Andreas Gal :gal]

James, Sam, are you willing to contribute your changes back into our code base? I would love to have the SPRD special cases in our code as well.

Comment 34

[James Zhang (Spreadtrum)]

(In reply to Andreas Gal :gal from comment #33)
> James, Sam, are you willing to contribute your changes back into our code
> base? I would love to have the SPRD special cases in our code as well.

Andreas, of course we will, thank you !

Comment 35

[Ken Chang[:kenkai]]

(In reply to James Zhang from comment #34)
> Andreas, of course we will, thank you !
James and Sam, as we discussed in email at this morning, it will be great if you can use a neutral flag for your codes. Moreover, if you or your members can read those links what I sent you before providing a patch, it will be very helpful for landing code process. Thanks.

Comment 36

[James Ho]

(In reply to Ken Chang from comment #35)
> James and Sam, as we discussed in email at this morning, it will be great if
> you can use a neutral flag for your codes. Moreover, if you or your members
> can read those links what I sent you before providing a patch, it will be
> very helpful for landing code process. Thanks.

Ken, please guide through James, Sam and his team thru the patch review and landing process. Thanks.

Comment 37

[Ken Chang[:kenkai]]

(In reply to James Ho from comment #36)
> Ken, please guide through James, Sam and his team thru the patch review and
> landing process. Thanks.
Sure, I will do this.

Comment 38

[James Zhang (Spreadtrum)]

Ming.Li will review sprd patch and commit again. Thanks.

Comment 39

[James Zhang (Spreadtrum)]

We're debugging DSDS block issue now and this issue is not the block issue. We'll provide patch next week, thanks!

Comment 40

[Ken Chang[:kenkai]]

(In reply to James Zhang from comment #39)
> We're debugging DSDS block issue now and this issue is not the block issue.
> We'll provide patch next week, thanks!
It's good. But will you keep your fix for this bug in your server? We still need it before you land this patch.
Thanks.

Comment 41

[James Zhang (Spreadtrum)]

(In reply to Ken Chang from comment #40)
> (In reply to James Zhang from comment #39)
> > We're debugging DSDS block issue now and this issue is not the block issue.
> > We'll provide patch next week, thanks!
> It's good. But will you keep your fix for this bug in your server? We still
> need it before you land this patch.
> Thanks.

Yes, we'll keep this patch until land.

Comment 42

[Ming]

There is no time for me util next wednesday.
I'll work on this bug then.

Comment 43

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

From TS 102.221, clause 11.1.1.3.2,
The response for USIM is of FCP Template, which is of BER-TLV format(TS 101.220),
hence make this bug depends on Bug 947110.

Comment 44

[Ken Chang[:kenkai]]

Ming, before you provide a patch, please check bug 947110. I wonder if you could provide patches for these 2 bugs. That will make your patches clear and structural. Thanks.

Comment 45

[:gerard-majax]

Maybe a late and dumb reply, but the error reported reminds me A LOT of what I have been fighting with when fixing https://bugzilla.mozilla.org/show_bug.cgi?id=904038

Comment 46

[Ming]

i'm waiting for 947110

Comment 47

[shawn ku [:sku]]

Attachment: Bug 924740 - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card.

Comment 48

[shawn ku [:sku]]

It's a little wierd to me by checking the getResponse data on Unagi.

Per TS 51.011 clause 9.2.1 SELECT, the first two bytes are defined as RFU, however we got [0x00, 0x00, ...] via Unagi.
The correct values should be [0xff, 0xff, ...]

Anyway, it does *not* impact what we try to fix regarding FCP template.

12-20 10:43:00.257   111   187 D RILC    : RIL_SIM_IO_Response: sw1=144 sw2=0 data=0000000a2fe2040000000005020000

Comment 49

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

Comment on attachment 8350450 [details] [diff] [review]
Bug 924740 - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card.

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

::: dom/system/gonk/ril_worker.js
@@ +11317,5 @@
>    processICCIOGetResponse: function processICCIOGetResponse(options) {
>      let strLen = Buf.readInt32();
> +    // Keep the position before parsing FCP template in case we need it for
> +    // non-BerTLV format getResponse reverting.
> +    let previousReadAvailable = Buf.getReadAvailable();

Since we will seek back to the original position if it's of SIM format, seems peeking next octet is easier.

let peek = GsmPDUHelper.readHexOctet();
Buf.seekIncoming(-1 * PARCEL_SIZE_SIZE);
if (peek === FCP_TEMPLATE_TAG) {
  this.processUsimGetResponse(options);
} else {
  this.processSimGetResponse(options);
}

@@ +11318,5 @@
>      let strLen = Buf.readInt32();
> +    // Keep the position before parsing FCP template in case we need it for
> +    // non-BerTLV format getResponse reverting.
> +    let previousReadAvailable = Buf.getReadAvailable();
> +    let berTlvs;

berTlv;

@@ +11343,5 @@
> +        options.recordSize = tlv.value.recordLength;
> +      }
> +      if (tlv.value.numOfRecords) {
> +        options.totalRecords = tlv.value.numOfRecords;
> +      }

Can't we do the same thing as GetResponse for SIM ?

let fileStruct = tlv.value.fileStructure;
if (fileStruct == STRUCTURE_LINEARED || fileStruct == STRUCTURE_CYCLIC) {
  options.recordSize = ...;
  options.totalRecords = ...;
}

Comment 50

[shawn ku [:sku]]

Thanks Yoshi, please check my inline reply.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #49)
> Comment on attachment 8350450 [details] [diff] [review]
> Bug 924740 - [fugu]It throws exception when run
> ICCIOHelper.processICCIOGetResponse for USIM card.
> 
> Review of attachment 8350450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +11317,5 @@
> >    processICCIOGetResponse: function processICCIOGetResponse(options) {
> >      let strLen = Buf.readInt32();
> > +    // Keep the position before parsing FCP template in case we need it for
> > +    // non-BerTLV format getResponse reverting.
> > +    let previousReadAvailable = Buf.getReadAvailable();
> 
> Since we will seek back to the original position if it's of SIM format,
> seems peeking next octet is easier.
> 
> let peek = GsmPDUHelper.readHexOctet();
> Buf.seekIncoming(-1 * PARCEL_SIZE_SIZE);
> if (peek === FCP_TEMPLATE_TAG) {
>   this.processUsimGetResponse(options);
> } else {
>   this.processSimGetResponse(options);
> }
> 

Will address this in next patch.

> @@ +11318,5 @@
> >      let strLen = Buf.readInt32();
> > +    // Keep the position before parsing FCP template in case we need it for
> > +    // non-BerTLV format getResponse reverting.
> > +    let previousReadAvailable = Buf.getReadAvailable();
> > +    let berTlvs;
> 
> berTlv;
> 

Copy that.

> @@ +11343,5 @@
> > +        options.recordSize = tlv.value.recordLength;
> > +      }
> > +      if (tlv.value.numOfRecords) {
> > +        options.totalRecords = tlv.value.numOfRecords;
> > +      }
> 
> Can't we do the same thing as GetResponse for SIM ?
> 
> let fileStruct = tlv.value.fileStructure;
> if (fileStruct == STRUCTURE_LINEARED || fileStruct == STRUCTURE_CYCLIC) {
>   options.recordSize = ...;
>   options.totalRecords = ...;
> }

Yes, we definitely can do it, however, it's a little redundant since we already do it in retrieveFileDescriptor@BerTlvHelper.
That means there will be no recordLength/numOfRecords attributes if non-linear fixe or non-cyclic files.

What's your opinion?

// Code snippet:
  retrieveFileDescriptor: function retrieveFileDescriptor(length) {
    let fileDescriptorByte = GsmPDUHelper.readHexOctet();
    let dataCodingByte = GsmPDUHelper.readHexOctet();
    // See TS 102 221 Table 11.5, we only care the least 3 bits for the
    // structure of file.
    let fileStructure = fileDescriptorByte & 0x07;

    let fileDescriptor = {
      fileStructure: fileStructure
    };
    // byte 5 ~ 7 are mandatory for linear fixed and cyclic files, otherwise
    // they are not applicable.
    if (fileStructure === UICC_EF_STRUCTURE[EF_TYPE_LINEAR_FIXED] ||
        fileStructure === UICC_EF_STRUCTURE[EF_TYPE_CYCLIC]) {
      fileDescriptor.recordLength = (GsmPDUHelper.readHexOctet() << 8) +
                                     GsmPDUHelper.readHexOctet();
      fileDescriptor.numOfRecords = GsmPDUHelper.readHexOctet();
    }

Comment 51

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

(In reply to shawn ku [:sku] from comment #50)
> Yes, we definitely can do it, however, it's a little redundant since we
> already do it in retrieveFileDescriptor@BerTlvHelper.
> That means there will be no recordLength/numOfRecords attributes if
> non-linear fixe or non-cyclic files.
> 

If the structure is transparent, there won't be recordLength nor numOfRecords.
Not sure what do you mean 'redundant'.

Comment 52

[shawn ku [:sku]]

Sorry for not explain the reply well, and please correct me if I do not get your first reply correctly.

I mean:
for transparent structure, there is *no* recordLength nor numOfRecords.
for linear fixed/cyclic structure, there are recordLength and numOfRecords existed.

We only need to check tlv.value.recordLength and tlv.value.numOfRecords are valid or not in processUsimGetResponse(options).

(this part has been checked by retrieveFileDescriptor@BerTlvHelper, 
 I don't want this to be re-checked in processUsimGetResponse.)

Comment 53

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

From your original patch, we need to look back to BerTlvHelper to know that only LINEAR_FIXED or CYCLIC will have recordLength and numOfRecords.

For people reading your patch in processICCIOGetResponse, (Like me right now),
It seems it's saying "In USIM, all structures could have optional recordLength or numOfRecords".
Since you write

if (tlv.value.recordLength) {
..
} 

if (tlv.value.numOfRecords) {
..
}

However in SIM, the code is saying only LINEAR_FIXED or CYCLIC could have those two properties.

if (efType == EF_TYPE_LINEAR_FIXED || efType == EF_TYPE_CYCLIC) {
...
}

The two approaches are doing the same thing but the latter is easier to understand.

Comment 54

[shawn ku [:sku]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #53)
> From your original patch, we need to look back to BerTlvHelper to know that
> only LINEAR_FIXED or CYCLIC will have recordLength and numOfRecords.
> 
> For people reading your patch in processICCIOGetResponse, (Like me right
> now),
> It seems it's saying "In USIM, all structures could have optional
> recordLength or numOfRecords".
> Since you write
> 
> if (tlv.value.recordLength) {
> ..
> } 
> 
> if (tlv.value.numOfRecords) {
> ..
> }
> 
> However in SIM, the code is saying only LINEAR_FIXED or CYCLIC could have
> those two properties.
> 
> if (efType == EF_TYPE_LINEAR_FIXED || efType == EF_TYPE_CYCLIC) {
> ...
> }
> 
> The two approaches are doing the same thing but the latter is easier to
> understand.

I got it, thx!

Comment 55

[shawn ku [:sku]]

Attachment: Bug 924740 Part1: RIL patch - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card. v2.

Comment 56

[shawn ku [:sku]]

Comment on attachment 8350520 [details] [diff] [review]
Bug 924740 Part1: RIL patch - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card. v2.

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

Test case part will be updated after Bug 947110 is landed.

Comment 57

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

Comment on attachment 8350520 [details] [diff] [review]
Bug 924740 Part1: RIL patch - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card. v2.

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

::: dom/system/gonk/ril_worker.js
@@ +11185,5 @@
>  
> +    let peek = GsmPDUHelper.readHexOctet();
> +    Buf.seekIncoming(-1 * Buf.PDU_HEX_OCTET_SIZE);
> +    if (peek === BER_FCP_TEMPLATE_TAG) {
> +      this.processUSimGetResponse(options, strLen);

passing octetLen will be easier.
processUsimGetResponse only cares about the total length of octets.

Comment 59

[Steven Yang [:styang]]

I think the patch should go to m-c as well?

Comment 60

[shawn ku [:sku]]

(In reply to Steven Yang [:styang] from comment #59)
> I think the patch should go to m-c as well?

Yes, m-c first, I will also provide 1.2f patch after test case part is reviewed.

Comment 61

[shawn ku [:sku]]

Attachment: Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card.

Comment 62

[shawn ku [:sku]]

Attachment: Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card.

Comment 63

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

Comment on attachment 8355111 [details] [diff] [review]
Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card.

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

What's this?

You attached wrong file ?

Comment 64

[shawn ku [:sku]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #63)
> Comment on attachment 8355111 [details] [diff] [review]
> Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run
> ICCIOHelper.processICCIOGetResponse for USIM card.
> 
> Review of attachment 8355111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What's this?
> 
> You attached wrong file ?

Hi Yoshi:
 Attachment 8355111 [details] [diff] just remove the override functions inside test case. This file should be right one.
Could you please help me double check again?

Thanks!!
sku


-  berHelper.searchForNextTag = function searchForNextTag(tag, iter) {
-    for (let [index, tlv] in iter) {
-      if (tlv.tag === tag) {
-        return tlv;
-      }
-    }
-    return null;
-  };
-

Comment 65

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

Comment on attachment 8355111 [details] [diff] [review]
Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card.

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

You should write a test to verify ICCIOHelper.processICCIOGetResponse could parse SIM and FCP template.

Comment 66

[shawn ku [:sku]]

Attachment: Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card.

Comment 67

[shawn ku [:sku]]

Attachment: Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card. v2.

Comment 68

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

Comment on attachment 8355399 [details] [diff] [review]
Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card. v2.

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +2840,5 @@
> +  let buf = worker.Buf;
> +  let iccioHelper = worker.ICCIOHelper;
> +  let pduHelper = worker.GsmPDUHelper;
> +
> +

nit, remove extra line.

Comment 69

[shawn ku [:sku]]

Attachment: Bug 924740 Part1: RIL patch - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card. v2. r=yoshi.

Comment 70

[shawn ku [:sku]]

Attachment: Bug 924740 Part2: Test Cases Part - [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card. v2. r=yoshi.

Comment 71

[shawn ku [:sku]]

Try is green - https://tbpl.mozilla.org/?tree=Try&rev=76b774febb43

Comment 72

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

https://hg.mozilla.org/integration/b2g-inbound/rev/9fe7e96c35df
https://hg.mozilla.org/integration/b2g-inbound/rev/6415d450bd6f

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fe7e96c35df
https://hg.mozilla.org/mozilla-central/rev/6415d450bd6f

Comment 74

[shawn ku [:sku]]

fugu DSDS use v1.3 branch. nominate 1.3? here.

Comment 75

[Ken Chang[:kenkai]]

It is a 1.3+ bug. Since this bugs will effect the DSDS functions.

Comment 76

[Ken Chang[:kenkai]]

Shawn, you may need to provide a patch for 1.3 branch.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1edbd949c5a7
https://hg.mozilla.org/releases/mozilla-aurora/rev/cdbb6973f650

Comment 78

[KTucker [:KTucker][Inactive 3/4/2016]]

The test case has been added in Moztrap here: https://moztrap.mozilla.org/manage/case/13752/