Closed Bug 924740 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

Other
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: ming.li, Assigned: sku)

References

Details

Attachments

(2 files, 9 obsolete files)

4.90 KB, patch
Details | Diff | Splinter Review
4.59 KB, patch
Details | Diff | Splinter Review
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
Attachment #814710 - Attachment mime type: text/x-vhdl → text/plain
ming, pls add your patch to attachment
Attached patch carrier.js.patch (obsolete) — Splinter Review
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)";
Attached patch ril-worker.js.patch (obsolete) — Splinter Review
(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
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
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
(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?
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?
(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.
(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.
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.
I see . Thanks to all.
:)
close as WONTFIX as this should be done in modem side.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Add Bruce, please check spreadtrum modem issue.
Modem side won't handle this , reopen it.
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---
(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?
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #17)
As mentioned above ,  android doesn't handle this correctly.
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.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → WONTFIX
(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?
(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.
Summary: [tara]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card → [fugu]It throws exception when run ICCIOHelper.processICCIOGetResponse for USIM card
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.
Flags: needinfo?(ming.li)
Add Sam.Hua and Bruce.Jiang
Need more talk over this issue
Status: RESOLVED → UNCONFIRMED
Flags: needinfo?(ming.li)
Resolution: WONTFIX → ---
I don't accept somebody overrides my decision before giving a convincing result.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → WONTFIX
We need a resolution here. Please don't close this bug until we have found a fix.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
(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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → WONTFIX
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.
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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.
(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"....:(
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.
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.
(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 !
(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.
(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.
(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.
Ming.Li will review sprd patch and commit again. Thanks.
Flags: needinfo?(ming.li)
We're debugging DSDS block issue now and this issue is not the block issue. We'll provide patch next week, thanks!
(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.
(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.
blocking-b2g: --- → fugu+
There is no time for me util next wednesday.
I'll work on this bug then.
Whiteboard: [Fugu needs]
Whiteboard: [Fugu needs]
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.
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.
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
i'm waiting for 947110
Flags: needinfo?(ming.li)
Attachment #8350450 - Flags: review?(allstars.chh)
Assignee: nobody → 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 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 = ...;
}
Attachment #8350450 - Flags: review?(allstars.chh)
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();
    }
(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'.
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.)
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.
(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 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.
Attachment #8350520 - Flags: review?(allstars.chh)
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.
Attachment #8350520 - Flags: review?(allstars.chh) → review+
I think the patch should go to m-c as well?
Flags: needinfo?(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.
Flags: needinfo?(sku)
Attachment #8355109 - Attachment is obsolete: true
Attachment #8355111 - Flags: review?(allstars.chh)
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 ?
Attachment #8355111 - Flags: review?(allstars.chh)
(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 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.
Attachment #8355111 - Flags: review-
Attachment #8355399 - Flags: review?(allstars.chh)
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.
Attachment #8355399 - Flags: review?(allstars.chh) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fe7e96c35df
https://hg.mozilla.org/mozilla-central/rev/6415d450bd6f
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
fugu DSDS use v1.3 branch. nominate 1.3? here.
blocking-b2g: fugu+ → 1.3?
It is a 1.3+ bug. Since this bugs will effect the DSDS functions.
blocking-b2g: 1.3? → 1.3+
Shawn, you may need to provide a patch for 1.3 branch.
Flags: needinfo?(sku)
Flags: in-moztrap-
The test case has been added in Moztrap here: https://moztrap.mozilla.org/manage/case/13752/
Flags: in-moztrap- → in-moztrap+
You need to log in before you can comment on or make changes to this bug.