814111 - STK command to remove STK menu does not work

Status: RESOLVED FIXED

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

Description

[Carol Yang [:cyang]]

In GCF TC 27.22.4.8.1, the test is ran as follows:

- Start GCF test which prompts for device power up
- After power up, going into Settings->SIM Toolkit, menu with 4 items are displayed (Item1, Item2, Item3, Item4)
- GCF test will prompt to select Item2
- After selecting Item2, now a menu with 2 items are displayed (One and Two)
- GCF test will prompt to select Two
- After selecting Two, the STK command to SETUP_MENU comes with no title and an empty item list comes

At this point, the SIM Toolkit menu still exists even after exiting the Settings app. It isn't until Settings app is killed (hold onto home button for 3 seconds and swipe up on Settings app) that entering into SIM Toolkit shows an empty menu.

Comment 1

[mbarone]

Carol could you provide which SIM/application did you use for this test?

Comment 2

[Michael Vines [:m1] [:evilmachines]]

This test case was run using test equipment.  The description provided by Carol should be sufficient for the dev that owns the stk gaia functionality to start debugging.

Comment 3

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

Why GCF is sending an empty menu? ... each time we receive a STK_CMD_SET_UP_MENU message, we refresh it so if this is empty, the STK apps disappears

Comment 4

[Michael Vines [:m1] [:evilmachines]]

I suggest you look at the GCF test case for these details.

Comment 5

[Andreas Gal :gal]

Fernando, any eta here?

Comment 6

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

(In reply to Andreas Gal :gal from comment #5)
> Fernando, any eta here?

As you know, We are changing the WoW of STK System messages.

The main menu refresh (and hide it on empty menus) can be added after landing 808919.

ETA: If it's what I'm thinking, this will be really fast. Anyway, before tell you ETA, I want to review the GCF test plan to be sure about of this. I asked @brg for this doc.

Comment 7

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

Hi,

I've the test description, but we don't have access to the tests equipments so it will very useful to have some ICC traces in order to know the received command.

I think is a SET_UP_MENU command with empty title:


{"commandNumber":1,"typeOfCommand":37,"commandQualifier":0,"rilMessageType":"stkcommand","options":{"title":"","items":[],"presentationType":0}}

Is possible to repeat the test with the debug traces enabled?

Gaia file: apps/settings/js/utils.js line 9 set DEBUG to true
And: apps/system/js/icc_cache.js line 10 set DEBUG to true

And run the test with adb logcat to see the debug traces.

Thanks in advance

Comment 8

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

Attachment: WIP. This should hide the menu after on next updateMenu() call

Could you test again with this patch?

Comment 9

[Carol Yang [:cyang]]

Hi Fernando,

I saw an unsolicited proactive command come in: D00D81030125008202818285008F00

and then the following logs:

11-27 23:31:53.379   126   251 I Gecko   : RIL Worker: Handling parcel as UNSOLICITED_STK_PROACTIVE_COMMAND
11-27 23:31:53.389   126   251 I Gecko   : RIL Worker: Parcel handling threw Found invalid nibble during PDU parsing: 
11-27 23:31:53.389   126   251 I Gecko   : RIL Worker: Parcel handler didn't consume whole parcel, 2 bytes left over
11-27 23:31:53.389   126   251 I Gecko   : RIL Worker: Next parcel size unknown, going to sleep.

So it doesn't look like there was even a chance to build this message:
{"commandNumber":1,"typeOfCommand":37,"commandQualifier":0,"rilMessageType":"stkcommand","options":{"title":"","items":[],"presentationType":0}}

Do let us know if you need anything else.

Thanks,
Carol

Comment 10

[Carol Yang [:cyang]]

Hi Fernando,

I have a slight comment on attachment 685559 [details]. Should a lack of menu title be the only way to determine that the menu is empty? I don't know for sure but could it be possible to have empty title but there are still items available?

As for testing the patch, I have not been able to test on a GCF setup today. However, just testing locally, the STK menu is no longer available under Settings app when an empty STK menu command comes in, so, so far so good.

Nivi will update tomorrow after she tests this patch on GCF setup.

Thanks,
Carol

Comment 11

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

(In reply to Carol Yang from comment #10)
> Hi Fernando,
> 
> As for testing the patch, I have not been able to test on a GCF setup today.
> However, just testing locally, the STK menu is no longer available under
> Settings app when an empty STK menu command comes in, so, so far so good.
> 

Cool :)

> Nivi will update tomorrow after she tests this patch on GCF setup.
> 

I'll be waiting :)

> Thanks,
> Carol


You're wellcome!
Fernando

Comment 12

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

(In reply to Carol Yang from comment #9)
> Hi Fernando,
> 
> I saw an unsolicited proactive command come in:
> D00D81030125008202818285008F00
> 
> and then the following logs:
> 
> 11-27 23:31:53.379   126   251 I Gecko   : RIL Worker: Handling parcel as
> UNSOLICITED_STK_PROACTIVE_COMMAND
> 11-27 23:31:53.389   126   251 I Gecko   : RIL Worker: Parcel handling threw
> Found invalid nibble during PDU parsing: 
> 11-27 23:31:53.389   126   251 I Gecko   : RIL Worker: Parcel handler didn't
> consume whole parcel, 2 bytes left over
> 11-27 23:31:53.389   126   251 I Gecko   : RIL Worker: Next parcel size
> unknown, going to sleep.
> 
> So it doesn't look like there was even a chance to build this message:
> {"commandNumber":1,"typeOfCommand":37,"commandQualifier":0,"rilMessageType":
> "stkcommand","options":{"title":"","items":[],"presentationType":0}}
> 
> Do let us know if you need anything else.
> 
> Thanks,
> Carol

Hi, this is at platform level ... @Yoshi, the RIL is telling an invalid PDU parsing. Could you check this? Thanks

Comment 13

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

(In reply to Fernando R. Sela [:frsela] from comment #12)
> (In reply to Carol Yang from comment #9)
> > Hi Fernando,
> > 
> > I saw an unsolicited proactive command come in:
> > D00D81030125008202818285008F00
> > 
D0
0D // BER TLV with length 0d

81
03
012500  // Set Up Menu

82
02
8182  // Device ID

8500 // Alpha ID (Title in this case) with length 0

8F00 // Item with length 0

I'll check the parsing when length is 0,
but seems the test data might not be correct
1. if it doesn't have alpha ID, it doesn't have to send this TLV. No need to send 8500 at all.
2. From spec," Each item comprises a short identifier (used to indicate the selection), a text string", so I think Item at least should not be of length 0.

Comment 14

[Nivi]

Hi Fernando,

I tested your patch and it looks good. When running the test now, I can see that the STK menu is removed.

Thanks,
Nivi.

Comment 15

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

(In reply to nsarkar from comment #14)
> Hi Fernando,
> 
> I tested your patch and it looks good. When running the test now, I can see
> that the STK menu is removed.
> 
> Thanks,
> Nivi.

\o/ cool. I'll ask for review?

Thanks Nivi

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 685559 [details]
WIP. This should hide the menu after on next updateMenu() call

r+ but please fix the small nit about the if expression.

Comment 17

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

Sure. Fixed and merged:

https://github.com/mozilla-b2g/gaia/pull/6776

Comment 18

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

Hi, Fernando 
I would like to re-open this issue,
since from the Stk Specification I think your patch is not complete.
thanks for Siddartha reminding this.

First the proactive command provided by Carol in comment 9 is a valid one,
See TS 102.223 , clause 6.6.7

"If the "Item data object for item 1" is a null data object (i.e. length = '00' and no value part), this is an indication to the
terminal to remove the existing menu from the menu system in the terminal."

So back to your patch,
the check should be 
if (items.size == 1 && items[0] === null)

Comment 19

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

Attachment: Changed the if to check if an empty menu is set

Yoshi, you mean this?

Comment 20

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

Comment on attachment 688123 [details] [diff] [review]
Changed the if to check if an empty menu is set

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

we also need to patch from Bug 816835 to verify this since currently gecko won't add the 'null' item

::: apps/settings/js/icc.js
@@ +368,5 @@
>  
>        document.getElementById('icc-stk-exit').classList.remove('hidden');
>        document.getElementById('icc-stk-app-back').classList.add('hidden');
>  
> +      if (!menu || (!menu.title || (items.size == 1 && items[0] === null))) {

no need to check menu.title,
since from spec doesn't mention title is null when removing menu.

Comment 21

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

Attachment: Changed the if to check if an empty menu is set with null item

Without menu.title check.
Pending to be checked with patch from bug #816835
As soon as you land and check it I'll ask for review and land in gaia

Comment 22

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

Comment on attachment 688130 [details] [diff] [review]
Changed the if to check if an empty menu is set with null item

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

Sorry for not spotting this in the first place,

Comment 23

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #22)
> Comment on attachment 688130 [details] [diff] [review]
> Changed the if to check if an empty menu is set with null item
> 
> Review of attachment 688130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for not spotting this in the first place,

Don't worry, It's only a two lines hack ... so no a terrible change :)

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fernando R. Sela [:frsela] from comment #23)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #22)
> > Comment on attachment 688130 [details] [diff] [review]
> > Changed the if to check if an empty menu is set with null item
> > 
> > Review of attachment 688130 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sorry for not spotting this in the first place,
> 
> Don't worry, It's only a two lines hack ... so no a terrible change :)

Do you need to ask for a review? Or is something else missing?

Comment 25

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

(In reply to Vivien Nicolas (:vingtetun) from comment #24)
> (In reply to Fernando R. Sela [:frsela] from comment #23)
> > (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #22)
> > > Comment on attachment 688130 [details] [diff] [review]
> > > Changed the if to check if an empty menu is set with null item
> > > 
> > > Review of attachment 688130 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Sorry for not spotting this in the first place,
> > 
> > Don't worry, It's only a two lines hack ... so no a terrible change :)
> 
> Do you need to ask for a review? Or is something else missing?

Hi Vivien,

This patch cann't be landed since bug 816835 (comment #21) is landed since currently the platform is sending different content.

That's the cause I didn't ask for review ... anyway the patch is done to be used with the Yoshi's patch.

My idea was to ask for review when landed, but if you prefer to review it now, for me is Ok.

Comment 26

[psiddh]

minor comment on the latest patch : typo 

condition should be : if (!menu || (menu.items.length == 1 && menu.items[0] === null)) {

Comment 27

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

(In reply to Siddartha P from comment #26)
> minor comment on the latest patch : typo 
> 
> condition should be : if (!menu || (menu.items.length == 1 && menu.items[0]
> === null)) {

That's true !

Thank you

Comment 28

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

Attachment: Changed the if to check if an empty menu is set with null item

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Is this ready to land?

Comment 30

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> Is this ready to land?

As soon as https://bugzilla.mozilla.org/show_bug.cgi?id=816835 lands

Comment 31

[Michael Vines [:m1] [:evilmachines]]

That bug need not block this from landing.  We have already verified the fix w/ the commercial ril.   Please land.

Comment 32

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

(In reply to Michael Vines [:m1] from comment #31)
> That bug need not block this from landing.  We have already verified the fix
> w/ the commercial ril.   Please land.

Thanks Michael,
If it's correctly tested, I'll land:

https://github.com/mozilla-b2g/gaia/pull/6849