STK command to remove STK menu does not work

RESOLVED FIXED in B2G C2 (20nov-10dec)

Status

Firefox OS
Gaia::Settings
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cyang, Assigned: frsela)

Tracking

unspecified
B2G C2 (20nov-10dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Blocks: 802677
Group: qualcomm-confidential
Flags: needinfo?(cyang)

Comment 1

5 years ago
Carol could you provide which SIM/application did you use for this test?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(cyang)
blocking-basecamp: ? → +
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)
Assignee: nobody → 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
I suggest you look at the GCF test case for these details.

Comment 5

5 years ago
Fernando, any eta here?
(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.
Depends on: 808919
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
Created attachment 685559 [details]
WIP. This should hide the menu after on next updateMenu() call

Could you test again with this patch?
Attachment #685559 - Flags: feedback?(mvines)
Attachment #685559 - Flags: feedback?(cyang)
(Reporter)

Comment 9

5 years ago
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
(Reporter)

Comment 10

5 years ago
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
(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
(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
Flags: needinfo?(allstars.chh)
(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.
Flags: needinfo?(allstars.chh)
Whiteboard: [needs update from nivi per comment #10]

Comment 14

5 years ago
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.
Attachment #685559 - Flags: feedback?(mvines) → feedback+
(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
Attachment #685559 - Flags: review?(21)
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.
Attachment #685559 - Flags: review?(21) → review+
Whiteboard: [needs update from nivi per comment #10]
Sure. Fixed and merged:

https://github.com/mozilla-b2g/gaia/pull/6776
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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)
Status: RESOLVED → REOPENED
Depends on: 816835
Resolution: FIXED → ---
Created attachment 688123 [details] [diff] [review]
Changed the if to check if an empty menu is set

Yoshi, you mean this?
Attachment #688123 - Flags: feedback?(allstars.chh)
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.
Attachment #688123 - Flags: feedback?(allstars.chh) → feedback+
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
Attachment #688123 - Attachment is obsolete: true
Attachment #688130 - Flags: feedback?(allstars.chh)
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,
Attachment #688130 - Flags: feedback?(allstars.chh) → feedback+
(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 :)
(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?
(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.
Attachment #688130 - Flags: review?(21)

Comment 26

5 years ago
minor comment on the latest patch : typo 

condition should be : if (!menu || (menu.items.length == 1 && menu.items[0] === null)) {
(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
Created attachment 688774 [details] [diff] [review]
Changed the if to check if an empty menu is set with null item
Attachment #688130 - Attachment is obsolete: true
Attachment #688130 - Flags: review?(21)
Attachment #688774 - Flags: review?(21)
Attachment #688774 - Flags: review?(21) → review+
Is this ready to land?
(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
That bug need not block this from landing.  We have already verified the fix w/ the commercial ril.   Please land.
(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
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Attachment #685559 - Flags: feedback?(cyang)
You need to log in before you can comment on or make changes to this bug.