1095802 - Piped STK messages results in no Terminal Responses

Status: RESOLVED FIXED

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

Description

[Anshul]

[Blocking Requested - why for this release]: This is a regression caused by bug 1082936

There are two set of issues with the fix for bug 1082936
1. For commands like DTMF, play tone etc, STK expects a terminal response right away as these commands are just notification to the user and expects no response from user. With the addition of icc.canProcessMessage function these commands aren't even processed if a previous command has occupied the view and so no TR is sent from Gaia

2. For commands like display text where a TR is expected if a user say didn't respond to the request within a timeout defined by Gaia. However if there is another STK command currently being displayed on the UI, icc_worker.js doesn't process the display text command and so doesn't even install the timeout timer. Therefore the TR might be delayed indefinitely until user closes the currently displayed STK screen.

Comment 1

[howie [:howie]]

Hi Manuel, just want to check if you are working on this?

Comment 2

[Manuel Casas Barrado [:mancas]]

Yes, I'm working to fix this issue by taking into account the timeouts and the STK messages responses.

Comment 3

[Anshul]

Manuel, wondering if instead of delaying the subsequent STK requests, can the subsequent STK dialogs replace the previous dialog?

Comment 4

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

Hi Anshul,

We're worried with corner cases. For example, we receive a command that should be early answered and then we display the information to the user, but in that time, we receive another command with timeout, so if the user doesn't see the message, it will be lost.

Should we prioritize these commands?
Should we fire the timeout and remove the dialog from the stack?

So we're working with these topics in mind.

Some STK dialogs, manage callback methods which fires/removes these timeouts, and other STK commands.

WDYT?

Comment 5

[Manuel Casas Barrado [:mancas]]

Attachment: Proposed patch

Comment 6

[Manuel Casas Barrado [:mancas]]

Anshul, could you take a look at the patch when you get a chance?

Thanks!

Comment 7

[Anshul]

Hello Manuel, I left a few comments for you on github. Besides that I am seeing following issues.

- Display text is sending a TR of ok right away if the screen is not busy as you aren't checking for options.respondeNeeded again. Left a comment on github on how to fix it.
 
- Launch browser command can optionally have confirmMessage to be displayed to the user. Currently you are displaying the message without checking if it can be processed or not so I see message overlap again as reported in bug 1082936.

With the above changes our stk test script works fine the first time around. If I run it again, a lot of tests fail again. I am still investigating it but it seems like there is a lot of timing issues with the concept of piping the STK messages.

On analyzing the spec TS 11.14 it seemed to me that only one STK session can be active at a time. So modem would not send another STK command when the TR for the first one is not received. What this means is that we may not need to pipe the messages at all. We could however finish the previous command by sending a TR of "ME currently unable to process command" (section 6.11 in TS 11.14) for the current STK command if TR hasn't already been sent, close the active window by calling this.hideView and continue to process the received STK command.

I feel that this would greatly simplify the code. Let me know what you think.

Comment 8

[Anshul]

Comment on attachment 8522157 [details] [review]
Proposed patch

This patch uncovers more issues with the approach to pipe STK messages. I am beginning to think that we should, to avoid more code churn on 2.1, revert the bug  1082936 on 2.1 as a solution to this issue and continue to discuss on better solutions for 2.2.

Comment 9

[Anshul]

Bhavna, please see comment #8.

Comment 10

[bhavana bajaj [:bajaj]]

:mancas, can we backout https://bugzilla.mozilla.org/show_bug.cgi?id=1082936 easily and handle these in 2.2 at this point ?

Comment 11

[Manuel Casas Barrado [:mancas]]

Of course we can. I'm taking into account the comments that Anshul said, in comment 7, so the approach of piped messages does not have any sense.

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Manuel Casas Barrado [:mancas] from comment #11)
> Of course we can. I'm taking into account the comments that Anshul said, in
> comment 7, so the approach of piped messages does not have any sense.

Thanks in that case please backout 1082936 from master and 2.1 (with a=bajaj on the commit message fro 2.1) and we can continue to resolve this on 2.2

Comment 13

[Manuel Casas Barrado [:mancas]]

Who can perform the backout of Bug 1082936?

Comment 14

[bhavana bajaj [:bajaj]]

Anshul, can you please recommend if we still need a backout of Bug 1082936 or we are fine leaving it in that state in 2.1 and pursue this in 2.2?

Comment 15

[Anshul]

bhavana, I think we should back it out in 2.1 and pursue it on 2.2.

Comment 16

[bhavana bajaj [:bajaj]]

(In reply to Manuel Casas Barrado [:mancas] from comment #13)
> Who can perform the backout of Bug 1082936?

you/fresla may be the right people to do the backout as you are the author of the patch. So feel free to perform the backout from master/2.1 repo. I'll NI :fresla to see if he can help given he landed the change in https://bugzilla.mozilla.org/show_bug.cgi?id=1082936#c11.

Comment 17

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

(In reply to bhavana bajaj [:bajaj] from comment #16)
> (In reply to Manuel Casas Barrado [:mancas] from comment #13)
> > Who can perform the backout of Bug 1082936?
> 
> you/fresla may be the right people to do the backout as you are the author
> of the patch. So feel free to perform the backout from master/2.1 repo. I'll
> NI :fresla to see if he can help given he landed the change in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1082936#c11.

Done.
Reverted on commit 6a24c8e4564432c804931ed85346f73425cf5290

Comment 18

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

Reverted also in branch 2.1: 8495c5ebac772bba2b3b95f6f1e5c46b753ef7f4

Comment 19

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8522157 [details] [review]
Proposed patch

Hey Anshul, could you take a look at the new patch when you get a chance?

Thanks!

Comment 20

[Anshul]

Manuel, the patch doesn't apply cleanly for me on moz central or on 2.0.

Comment 21

[Anshul]

Sorry I meant v2.1

Comment 22

[Manuel Casas Barrado [:mancas]]

Anshul, the patch is for branch master so you have to test it in v2.2. Although if you prefer I can develop a patch for v2.1 too.

Comment 23

[Anshul]

Comment on attachment 8522157 [details] [review]
Proposed patch

Manuel, the patch seems to be working well. Thanks for taking in the suggestion and working on it.

Comment 24

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8522157 [details] [review]
Proposed patch

Hey Anshul, could you review the patch and check if all cases are covered? Take into account that we have landed another patch which handles the stk menu and the visibility events.

Thanks

Comment 25

[Anshul]

Sure, my build is a few days old and your patch isn't applying cleanly. Let me move to the tip and try in a day or two.

Comment 26

[Anshul]

Comment on attachment 8522157 [details] [review]
Proposed patch

This patch is passing all our unit tests. Please note that bug 1033974 and bug 1088611 have removed TRs for some of the commands. So please make sure to not add those back with this patch.

Comment 27

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8522157 [details] [review]
Proposed patch

Hey Kevin, could you check the code of this patch, please?

Thanks!

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8522157 [details] [review]
Proposed patch

Normally I'd assign the review to Tim because he did the review on bug 1082936. Since you've already gotten feedback here, and I assume you've talked to :frsela about it judging from the comments, I'm fine leaving a review stamp here.

I did leave some minor comments on github that you can choose to address if you want. Thanks for the patch.

Comment 29

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

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