975299 - [Sora][STK]Can not enter to 'Refresh' Menu again after execute Refresh command.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P1)

Description

[sync-1]

Created an attachment (id=635779)
 adb log
 
 DEFECT DESCRIPTION:
 Can not enter to 'Refresh' Menu again after execute Refresh command.
 
  REPRODUCING PROCEDURES:
 1.MS load simulated SIM card and SATK application which can send "Refresh" procative SIM command.
 2.Enter to 'Refresh' Menu,run the test case STM07002->SPN,after execute refresh command,the screen exit 'Refresh' Menu.
 3.Try to enter to 'Refresh' Menu again,will find the screen no response and can not enter to it ->KO.
 
  EXPECTED BEHAVIOUR:
 It should can enter to 'Refresh' Menu again normally.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 3GPP TS 11.14
 
  TOOLS AND PLATFORMS USED:
 V108
 
  USER IMPACT:
 Important.
 
  REPRODUCING RATE:
 2/2.
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Attachment: QXDM log

Comment 2

[sync-1]

Attachment: adb log

Comment 3

[Jason Smith [:jsmith]]

Does this reproduce on a 1.1 Buri device?

Comment 4

[xiaokang.chen]

(In reply to Jason Smith [:jsmith] from comment #3)
> Does this reproduce on a 1.1 Buri device?

1.1 Buri is OK.

Comment 5

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Hi,

Checking the log traces I saw a strange empty command:


I/Gecko   (  303): -*- [SUB0] QCContentHelper_QC_B2G: sendMessage to content process: icc-stkcommand{ iccId : '89441000000400617452', command : {  } }

I/Gecko   (  303): -*- [SUB0] QCContentHelper_QC_B2G: sendMessage to content process: RIL:StkCommand{  }

Comment 6

[xiaokang.chen]

Dear Fernando,
I will check this with QA and will upload traces if v1.1 is ok. Thanks for your strong support!
BR
Chen

Comment 7

[Jason Smith [:jsmith]]

Waiting on additional traces here before making a blocking decision.

Comment 8

[xiaokang.chen]

Dear Fernando,
v1.1's traces don't have the empty command. Can I just ignore the empty command in gaia??
Thanks a lot!

Br
Chen

Comment 9

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to xiaokang.chen from comment #8)
> Dear Fernando,
> v1.1's traces don't have the empty command. Can I just ignore the empty
> command in gaia??
> Thanks a lot!
> 
> Br
> Chen

Hi Chen,
Currently, Gaia STK code expects more information into the command object:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/icc.js#L133

We should add a check here to avoid Gaia crashes and ignore these command; but my question is Why is sending an empty command?

Comment 10

[Noemí Freire (:noemi)]

The empty command shouldn't be received, ni to carol who might shed some light about this. Thanks!

Comment 11

[xiaokang.chen]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #9)
> (In reply to xiaokang.chen from comment #8)
> > Dear Fernando,
> > v1.1's traces don't have the empty command. Can I just ignore the empty
> > command in gaia??
> > Thanks a lot!
> > 
> > Br
> > Chen
> 
> Hi Chen,
> Currently, Gaia STK code expects more information into the command object:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/icc.js#L133
> 
> We should add a check here to avoid Gaia crashes and ignore these command;
> but my question is Why is sending an empty command?

Hi fernando,
You're right, I have filed an SR in qualcomm:01465335 for this bug.

Comment 12

[xiaokang.chen]

Dear Fernando,
I found where send the empty stkcommand in qualcomm's code. I will contact their developer for an solution. Thanks a lot!

Comment 13

[Noemí Freire (:noemi)]

ok, so de-nominating and including POVB tag!

Comment 14

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Protection to malformed STK Commands

This patch only protects GAIA from crashes if RIL send malformed STK messages. So this patch doesn't fix the main issue, but I think this is necessary to avoid bigger problems in the future.

Can you test if this patch allows you to enter again into the 'Refresh' Menu again?

Comment 15

[Carol Yang [:cyang]]

We will track this bug via SR.

Comment 16

[xiaokang.chen]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #14)
> Created attachment 8382948 [details] [review]
> Protection to malformed STK Commands
> 
> This patch only protects GAIA from crashes if RIL send malformed STK
> messages. So this patch doesn't fix the main issue, but I think this is
> necessary to avoid bigger problems in the future.
> 
> Can you test if this patch allows you to enter again into the 'Refresh' Menu
> again?
Thanks again for your help. Sure it's OK for gaia, I will let QA test it on our daily version cause I
can't do the refresh test in my office. I'll add comment if I got the result.

Comment 17

[xiaokang.chen]

Dear Fernando,
I got this traces:
I/GeckoDump( 7095): [system] STK Proactive Command bad formed: {"commandNumber":1,"typeOfCommand":37,"commandQualifier":0,"options":{"title":"USIM卡应用","items":[{"identifier":17,"text":"精品推荐"},{"identifier":49,"text":"天气预报"},{"identifier":50,"text":"航班查询"},{"identifier":51,"text":"出行指南"},{"identifier":52,"text":"体坛快讯"},{"identifier":81,"text":"新闻早晚报"},{"identifier":82,"text":"手机音乐"},{"identifier":83,"text":"手机阅读"},{"identifier":84,"text":"手机邮箱"},{"identifier":85,"text":"短信助理"},{"identifier":86,"text":"手机营业厅"}],"presentationType":0}}


As I know, the STK command don't have the "iccid" and "command" property. Shall we just ignor them in the protection you add ?

Comment 18

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Hi,

Current Gaia version is changed to support DSDS son Gecko version with DSDS is also required.

If you want to patch previous versions you should add the same check considering that no iccId is given and that the command is sent directly.

In other words, in DSDS we receive a message object with the iccId and the STK command; without DSDS we only receive the command.

So for previous versions check you should use:

if (!command.typeOfCommand || !command.options) {
 ...
}

Regards

Comment 19

[xiaokang.chen]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #18)
> Hi,
> 
> Current Gaia version is changed to support DSDS son Gecko version with DSDS
> is also required.
> 
> If you want to patch previous versions you should add the same check
> considering that no iccId is given and that the command is sent directly.
> 
> In other words, in DSDS we receive a message object with the iccId and the
> STK command; without DSDS we only receive the command.
> 
> So for previous versions check you should use:
> 
> if (!command.typeOfCommand || !command.options) {
>  ...
> }
> 
> Regards

Hi Fernando,
Did you communicate with qualcomm about this? If they didn't add the ICCID and COMMAND in the command, there will always be errors. 
At present, they don't add these two properties in StkService.cpp yet.

Comment 20

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to xiaokang.chen from comment #19)
> 
> Hi Fernando,

Hi,

> Did you communicate with qualcomm about this? If they didn't add the ICCID

Carol commented into this bug, so QC is informed ;)

> and COMMAND in the command, there will always be errors. 


> At present, they don't add these two properties in StkService.cpp yet.

Last Gecko versions are working this way. Moreover, in the traces attached to this bug you can see it:
icc-stkcommand{ iccId : '89441000000400617452', command : {  } }

It's sending iccId and command into the message. But the issue is that the command is empty !!!!

If you want to patch old gaia versions, we shall consider that before gecko only sents the command.

What's really important is to avoid mixing old gecko with new gaia and viceversa.

Comment 21

[Carol Yang [:cyang]]

Adding Nivi to comment.

Comment 22

[Nivi]

Hi Fernando,

For part of the problem about sending iccId in the message, here's my suggestion.

Ideally we don't need the iccid in the message as we already send the clientId and we can get the iccid from the clientId by doing the following - 

icc = navigator.mozIccManager.getIccById(iccManager.iccIds[clientId]);

The question is do we need to send duplicate info in the messages when we can easily derive one from the other.

For the other part of the problem, I will test on my side and verify why we are sending the empty command in response. Will post my findings soon.

Thanks,
Nivi.

Comment 23

[xiaokang.chen]

(In reply to Nivi from comment #22)
> Hi Fernando,
> 
> For part of the problem about sending iccId in the message, here's my
> suggestion.
> 
> Ideally we don't need the iccid in the message as we already send the
> clientId and we can get the iccid from the clientId by doing the following - 
> 
> icc = navigator.mozIccManager.getIccById(iccManager.iccIds[clientId]);
> 
> The question is do we need to send duplicate info in the messages when we
> can easily derive one from the other.
> 
> For the other part of the problem, I will test on my side and verify why we
> are sending the empty command in response. Will post my findings soon.
> 
> Thanks,
> Nivi.

Dear Nivi,
The case is about refresh, so when the case run successfully, there will be a refresh command and then the function "ProcessRefresh" will send an empty command. So the gaia stk didn't work.

note: v1.1 don't have this issue. Cause the function "ProcessRefresh" didn't get executed.

Comment 24

[xiaokang.chen]

Comment on attachment 8382948 [details] [review]
Protection to malformed STK Commands

This is OK as we talked.

Comment 25

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8382948 [details] [review]
Protection to malformed STK Commands

Looks good to me. Flagging alive for reviewing.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8382948 [details] [review]
Protection to malformed STK Commands

We seriously need tests on icc.js even though it is self-contained.

Comment 27

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #26)
> Comment on attachment 8382948 [details] [review]
> Protection to malformed STK Commands
> 
> We seriously need tests on icc.js even though it is self-contained.

Hi Tim, I'll work on unit test this week

Comment 28

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Unit tests added. If you agree I'll land

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Looks good, thank you very much.

Comment 30

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Landed: https://github.com/mozilla-b2g/gaia/commit/ffb05674b7c493980efa1e28b90e9f308903684c

Comment 31

[jinghua.xing]

Hi Fernando,

When we test the case 27.22.4.7.1 REFRESH SEQ 1.1, it failed and the TR returned 0x12. I found the message of REFRESH is 
{"iccId":"89860112811023010685","command":{"commandNumber":1,"typeOfCommand":1,"commandQualifier":3,"rilMessageType":"stkcommand","options":null,"rilMessageClientId":0}}

We discard the message because the options property is null, so there isn't any response for this case and it failed.

Can you help me to check it and see is the || !command.options really needed? Thank you.

Comment 32

[siiaceon]

side effect: https://bugzilla.mozilla.org/show_bug.cgi?id=1117619

Comment 33

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Answered in bug 1117619

(In reply to jinghua.xing from comment #31)
> Hi Fernando,
> 
> When we test the case 27.22.4.7.1 REFRESH SEQ 1.1, it failed and the TR
> returned 0x12. I found the message of REFRESH is 
> {"iccId":"89860112811023010685","command":{"commandNumber":1,"typeOfCommand":
> 1,"commandQualifier":3,"rilMessageType":"stkcommand","options":null,
> "rilMessageClientId":0}}
> 
> We discard the message because the options property is null, so there isn't
> any response for this case and it failed.
> 
> Can you help me to check it and see is the || !command.options really
> needed? Thank you.

Comment 34

[siiaceon]

Thanks Fernando