Last Comment Bug 814111 - STK command to remove STK menu does not work
: STK command to remove STK menu does not work
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::Settings (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: P1 normal (vote)
: B2G C2 (20nov-10dec)
Assigned To: Fernando R. Sela (no CC, needinfo please) [:frsela]
:
Mentors:
Depends on: 808919 816835
Blocks: 802677
  Show dependency treegraph
 
Reported: 2012-11-21 11:41 PST by Carol Yang [:cyang]
Modified: 2013-03-25 09:27 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+


Attachments
WIP. This should hide the menu after on next updateMenu() call (361 bytes, text/html)
2012-11-27 03:42 PST, Fernando R. Sela (no CC, needinfo please) [:frsela]
21: review+
mvines: feedback+
Details
Changed the if to check if an empty menu is set (2.07 KB, patch)
2012-12-03 22:35 PST, Fernando R. Sela (no CC, needinfo please) [:frsela]
allstars.chh: feedback+
Details | Diff | Splinter Review
Changed the if to check if an empty menu is set with null item (2.03 KB, patch)
2012-12-03 22:58 PST, Fernando R. Sela (no CC, needinfo please) [:frsela]
allstars.chh: feedback+
Details | Diff | Splinter Review
Changed the if to check if an empty menu is set with null item (2.06 KB, patch)
2012-12-05 07:32 PST, Fernando R. Sela (no CC, needinfo please) [:frsela]
21: review+
Details | Diff | Splinter Review

Description Carol Yang [:cyang] 2012-11-21 11:41:51 PST
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 2012-11-22 08:55:34 PST
Carol could you provide which SIM/application did you use for this test?
Comment 2 Michael Vines [:m1] [:evilmachines] 2012-11-23 10:51:48 PST
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] 2012-11-26 03:26:48 PST
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] 2012-11-26 06:51:39 PST
I suggest you look at the GCF test case for these details.
Comment 5 Andreas Gal :gal 2012-11-26 13:21:26 PST
Fernando, any eta here?
Comment 6 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-11-27 00:39:16 PST
(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] 2012-11-27 02:14:50 PST
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] 2012-11-27 03:42:53 PST
Created attachment 685559 [details]
WIP. This should hide the menu after on next updateMenu() call

Could you test again with this patch?
Comment 9 Carol Yang [:cyang] 2012-11-28 15:17:52 PST
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] 2012-11-28 17:32:42 PST
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] 2012-11-28 22:16:53 PST
(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] 2012-11-28 22:17:58 PST
(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 Huang[:allstars.chh] 2012-11-28 22:56:15 PST
(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 2012-11-30 15:39:49 PST
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] 2012-12-01 01:57:22 PST
(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) 2012-12-02 08:03:55 PST
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] 2012-12-02 23:08:46 PST
Sure. Fixed and merged:

https://github.com/mozilla-b2g/gaia/pull/6776
Comment 18 Yoshi Huang[:allstars.chh] 2012-12-03 22:06:12 PST
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] 2012-12-03 22:35:37 PST
Created attachment 688123 [details] [diff] [review]
Changed the if to check if an empty menu is set

Yoshi, you mean this?
Comment 20 Yoshi Huang[:allstars.chh] 2012-12-03 22:49:25 PST
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] 2012-12-03 22:58:05 PST
Created attachment 688130 [details] [diff] [review]
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 Huang[:allstars.chh] 2012-12-03 23:06:17 PST
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] 2012-12-03 23:13:46 PST
(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) 2012-12-04 08:41:21 PST
(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] 2012-12-04 09:44:19 PST
(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 2012-12-04 13:27:09 PST
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] 2012-12-05 07:31:39 PST
(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] 2012-12-05 07:32:40 PST
Created attachment 688774 [details] [diff] [review]
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 2012-12-05 22:49:18 PST
Is this ready to land?
Comment 30 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-12-06 00:12:50 PST
(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] 2012-12-06 00:27:10 PST
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] 2012-12-06 00:48:55 PST
(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

Note You need to log in before you can comment on or make changes to this bug.