816835 - B2G STK: Remove Menu causes Parcel error

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

From https://bugzilla.mozilla.org/show_bug.cgi?id=814111#c13,
some test data is using 0-length Comprehension-TLV and this caused failure in decoding parcels.

Comment 1

[psiddh]

Attachment: To address usecases when setup menu is issued with item tag

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 ME
to remove the existing menu from the menu system in the ME.

Comment 2

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

(In reply to Siddartha P from comment #1)
> Created attachment 687341 [details] [diff] [review]
> To address usecases when setup menu is issued with item tag
> 
> 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 ME
> to remove the existing menu from the menu system in the ME.
Thanks for reminding this.
See TS 102.223 , clause 6.6.7

Comment 3

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

Comment on attachment 687341 [details] [diff] [review]
To address usecases when setup menu is issued with item tag

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

Hi, Siddartha
thanks for your patch.

Not sure if patch is ready for review or not.
Next time you could send request for review. For example, r? in Bugzilla.

Also I think we need a marionette test for this,
Can you also write a test script?
I am also working on some Stk tests so if you need any help please feel free to ask.

thanks

::: dom/system/gonk/ril_worker.js
@@ +7224,5 @@
>     * | (Y-1)+X+2    |                        |        |
>     */
>    retrieveItem: function retrieveItem(length) {
> +    if (!length) {
> +      return {

you could 'return null;' directly.
Since it's javascript, we could use it more dynamically.

Also can we put more comments here? for example, quoted from spec,
"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."

Comment 4

[psiddh]

sure.. comments will be addressed in the next patch. Could you elaborate on the requesting for review . Not sure if I have understood your comment on 'review process'.

Marionette, Use-case should be fairly straight forward.

Comment 5

[psiddh]

Attachment: SETUP MENU first Item length is zero

Addressed the comments

Comment 6

[psiddh]

Attachment: TestCase to test the removal of already existing SETUP_MENU

Here is the tentative marionette test case. Both the *updated patch and test script have been tested. (Note : There is still a TBD: in the script)

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

I assume you're take the bug. Thank you :)

Comment 8

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

Comment on attachment 687341 [details] [diff] [review]
To address usecases when setup menu is issued with item tag

Obsolete Siddartha's old patch.

Comment 9

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

Comment on attachment 688525 [details] [diff] [review]
SETUP MENU first Item length is zero

Hi, Siddartha
Thanks for your patch,

Also you need to update comments in your patch
1. Add reviewer's name in the bug comment, 
2. Remove the "[PATCH]",

see https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

and seems you used line break in your comment so I cannot see those final words 'its length is 0'

Comment 10

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

(In reply to Siddartha P from comment #4)
> sure.. comments will be addressed in the next patch. Could you elaborate on
> the requesting for review . Not sure if I have understood your comment on
> 'review process'.
> 
> Marionette, Use-case should be fairly straight forward.

See https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

Basically after you uploaded the patch,  Details -> Flags -> review -> choose ?, and fill-in the mail of the reviewer, if the reviewer is me, type ":allstars.chh", it should be auto-completed to fill out my email address.

Comment 11

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

(In reply to Siddartha P from comment #6)
> Created attachment 688533 [details] [diff] [review]
> TestCase to test the removal of already existing SETUP_MENU
> 
Hi Siddartha,
Can you make it a patch? so I can add my comments line by line.
For the test script you can find sample from https://bugzilla.mozilla.org/attachment.cgi?id=688149,
but it's still being reviewed, might be subject to change.

Comment 12

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

Moving to C2 since this bug blocks a C2 blocker.

Comment 13

[psiddh]

Attachment: PATCH v2

Addressed the comments

Comment 14

[psiddh]

Comment on attachment 688525 [details] [diff] [review]
SETUP MENU first Item length is zero

Marking this patch as obsolete. refer latest patch submitted 'PATCH v2'

Comment 15

[psiddh]

Attachment: Marionette test case to remove a setup menu

Comment 16

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

Comment on attachment 689316 [details] [diff] [review]
PATCH v2

I have gave you a r+, so you don't have to ask for review again.

Comment 17

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

Comment on attachment 689514 [details] [diff] [review]
Marionette test case to remove a setup menu

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

Hi, Siddartha
Since the file looks similar to test_stk_proactive_command.js
can you add your patch into that file?

thanks

::: dom/telephony/test/marionette/test_stk_remove_setup_menu.js
@@ +9,5 @@
> +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> +
> +function getResult(cmd) {
> +  is(cmd.typeOfCommand, icc.STK_CMD_SET_UP_MENU);
> +  let result = ((JSON.stringify(cmd.options.items.length) == 1) && !(cmd.options.items[0]));

why JSON.stringify?
can you explain?

Comment 18

[psiddh]

Isn't 'cmd.options.items.length' a JS object ? "is( JS obj , 1)" always yields a failure. On converting to JSON string, we can simply do a string comparison. I could be wrong though.

I thought of adding it to test_stk_proactive_command.js itself, but I wasn't sure. I really liked the way 'test_stk_proactive_command.js' is structured

Comment 19

[psiddh]

I will add it to test_stk_proactive_command.js

Comment 20

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

One thing I'd like to remind you, recently marionette needs to add a '--load-early' option, 

in your B2G,
$>./test.sh marionette --load-early gecko/dom/icc/tests/marionette/test_stk_proactive_command.js

Comment 21

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

Comment on attachment 689514 [details] [diff] [review]
Marionette test case to remove a setup menu

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

::: dom/telephony/test/marionette/test_stk_remove_setup_menu.js
@@ +7,5 @@
> +
> +let icc = navigator.mozMobileConnection.icc;
> +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> +
> +function getResult(cmd) {

Do you think we could make the naming better?
Or add some comments in this function?
it might be difficult to know get 'what' result in the first place.

@@ +9,5 @@
> +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> +
> +function getResult(cmd) {
> +  is(cmd.typeOfCommand, icc.STK_CMD_SET_UP_MENU);
> +  let result = ((JSON.stringify(cmd.options.items.length) == 1) && !(cmd.options.items[0]));

I just test your patch and I think 
(cmd.options.items.length == 1 && !(cmd.options.items[0])) works fine.

Also you could simply 
return (cmd.options.items.length == 1 && !(cmd.options.items[0])); 
without introducing 'result'.

Comment 22

[psiddh]

Attachment: Added a marionette test case to remove an existing setup menu

Comment 23

[psiddh]

Comment on attachment 689514 [details] [diff] [review]
Marionette test case to remove a setup menu

Refer to the latest patch submitted

Comment 24

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

Comment on attachment 689984 [details] [diff] [review]
Added a marionette test case to remove an existing setup menu

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

Great, thank you : )
Also in the patch comment, since you wrote two patches, (1 for test case)
so we usually write
"Bug 816835 - Part 1: ...."
"Bug 816835 - Part 2: Marionette tests for ..."

Comment 25

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

Also you need to rebase your patch for the test case since Bug 804671 also updated the marionette test case for proactive command.

Comment 26

[psiddh]

Attachment: Part 2: Marionette test case to remove a setup menu.

Comment 27

[psiddh]

Attachment: Part 1: PATCH v3

Comment 28

[psiddh]

Thanks Yoshi for the comments and feedback. I have now uploaded the latest patch set. Pls. let me know if anything else is needed to land these patches

Comment 29

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

Great, \O/
Your first patch
I'll help you check-in these.

Comment 30

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf33b009d411
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d530edaaa6a

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf33b009d411
https://hg.mozilla.org/mozilla-central/rev/8d530edaaa6a