Piped STK messages results in no Terminal Responses

RESOLVED FIXED in 2.2 S2 (19dec)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: anshulj, Assigned: mancas)

Tracking

({regression})

unspecified
2.2 S2 (19dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed)

Details

(Whiteboard: [caf priority: p2][CR 754162])

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
anshulj
: review+
kgrandon
: review+
anshulj
: feedback+
Details | Review
(Reporter)

Description

5 years ago
[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.
Assignee: nobody → b.mcb
blocking-b2g: 2.1? → 2.1+
Keywords: regression
Blocks: 1082936
Whiteboard: [CR 754162]
Whiteboard: [CR 754162] → [caf priority: p2][CR 754162]

Comment 1

5 years ago
Hi Manuel, just want to check if you are working on this?
Flags: needinfo?(b.mcb)
(Assignee)

Comment 2

5 years ago
Yes, I'm working to fix this issue by taking into account the timeouts and the STK messages responses.
Flags: needinfo?(b.mcb)
(Reporter)

Comment 3

5 years ago
Manuel, wondering if instead of delaying the subsequent STK requests, can the subsequent STK dialogs replace the previous dialog?
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?
(Assignee)

Comment 5

5 years ago
Posted file Proposed patch
Attachment #8522157 - Flags: feedback?(anshulj)
(Assignee)

Comment 6

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

Thanks!
(Reporter)

Comment 7

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

Comment 8

5 years ago
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.
Flags: needinfo?(b.mcb)
Attachment #8522157 - Flags: feedback?(anshulj) → feedback-
(Reporter)

Comment 9

5 years ago
Bhavna, please see comment #8.
Flags: needinfo?(bbajaj)
:mancas, can we backout https://bugzilla.mozilla.org/show_bug.cgi?id=1082936 easily and handle these in 2.2 at this point ?
Flags: needinfo?(bbajaj) → needinfo?(b.mcb)
(Assignee)

Comment 11

5 years ago
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.
Flags: needinfo?(b.mcb)
(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
(Assignee)

Comment 13

5 years ago
Who can perform the backout of Bug 1082936?
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?
Flags: needinfo?(anshulj)
(Reporter)

Comment 15

5 years ago
bhavana, I think we should back it out in 2.1 and pursue it on 2.2.
Flags: needinfo?(anshulj)
(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.
Flags: needinfo?(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
Flags: needinfo?(frsela)
Reverted also in branch 2.1: 8495c5ebac772bba2b3b95f6f1e5c46b753ef7f4
blocking-b2g: 2.1+ → 2.2+
(Assignee)

Comment 19

5 years ago
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!
Attachment #8522157 - Flags: feedback- → feedback?(anshulj)
(Reporter)

Comment 20

5 years ago
Manuel, the patch doesn't apply cleanly for me on moz central or on 2.0.
(Reporter)

Comment 21

5 years ago
Sorry I meant v2.1
(Assignee)

Comment 22

5 years ago
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.
Flags: needinfo?(anshulj)
(Reporter)

Comment 23

5 years ago
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.
Flags: needinfo?(anshulj)
Attachment #8522157 - Flags: feedback?(anshulj) → feedback+
(Assignee)

Comment 24

4 years ago
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
Attachment #8522157 - Flags: review?(anshulj)
(Reporter)

Comment 25

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

Comment 26

4 years ago
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.
Attachment #8522157 - Flags: review?(anshulj) → review+
(Assignee)

Comment 27

4 years ago
Comment on attachment 8522157 [details] [review]
Proposed patch

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

Thanks!
Attachment #8522157 - Flags: review?(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.
Attachment #8522157 - Flags: review?(kgrandon) → review+
Landed. https://github.com/mozilla-b2g/gaia/commit/1a171fcf1901656144f81dc24aaff01f21113fbd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S2 (19dec)
You need to log in before you can comment on or make changes to this bug.