1191205 - [v2.5] USSD responses not working

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Benoit Chabod]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150629114848

Steps to reproduce:

1. Open Dialer
2. Type an USSD code that displays a menu where you can respond and that doesn't end the session immediately
3. Send the USSD code and read the operator message
4. Try to reply accordingly using the input field available


Actual results:

Whatever is your response and even if you reply fast, the Dialer will display a "session expired" error.


Expected results:

The USSD session should have kept going, and I should have been able to navigate through the menus.

Comment 1

[Benoit Chabod]

This feature worked fine before I updated my Flame, so the regression window is between 2.2 and 2.5.
My mobile network carrier is Orange and I'm using USSD code #123# to test this bug, although it happens with every USSD menu.

Additional information :

OS Version : 2.5.0.0-prerelease
Build number : emg.benoit.20150804.142516
Build identifier : 201508044143520
Update Channel : default
Git Commit Info : 2015-08-04 06:38:05 23e2e115

Comment 2

[Fabien Cazenave [:kaze]]

I confirm this is a (major) regression, and a blocker for users with some prepaid plans.

Let’s check if bug 1159591 is involved…

Comment 3

[Benoit Chabod]

Ok, changes made to address bug 1159591 definitely caused this regression.
I used a Gaia nightly build from http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/ (Central Flame-KK of 2015-07-16) and tried two different Gecko :

* add1c0d - Bumping gaia.json [...]                 <-- First Gecko version used, after the patch
* f39d43b - Bug 1159591 - Part 12 [...]
* 457a0a8 - Bug 1159591 - Part 11 [...]
* 84fc95b - Bug 1159591 - Part 10 [...]
* 325672a - Bug 1159591 - Part 9 [...]
* c418ccb - Bug 1159591 - Part 8 [...]
* 15938bd - Bug 1159591 - Part 7 [...]
* 44e6208 - Bug 1159591 - Part 6 [...]
* 589822e - Bug 1159591 - Part 5 [...]
* 1cc9e83 - Bug 1159591 - Part 4 [...]
* 0f7cfd2 - Bug 1159591 - Part 3 [...]
* 27d1451 - Bug 1159591 - Part 2 [...]
* 826b4e9 - Bug 1159591 - Part 1[...]
* deab23f - Bumping manifests [...]                 <-- Second Gecko version used, before the patch

My user story works fine before the patch, and encounters the bug described above if I use it.

Comment 4

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: Functional regression that can lead to a certification blocker.

Comment 5

[Benoit Chabod]

I'm currently working on a patch with a better way to handle USSD sessions.
I think the bug might be caused by the design of _sendUSSDInternal() in TelephonyService.js :

>_sendUSSDInternal: function(aClientId, aUssd, aCallback) {
>    if (!this._ussdSessions[aClientId]) {
>      this._sendToRilWorker(aClientId, "sendUSSD", { ussd: aUssd }, aResponse => {
>        this._ussdSessions[aClientId] = !aResponse.errorMsg;
>        aCallback(aResponse);
>      });
>      return;
>    }
> 
>    // Cancel the previous ussd session first.
>    this._cancelUSSDInternal(aClientId, aResponse => {
>      // Fail to cancel ussd session, report error instead of sending ussd
>      // request.
>      if (aResponse.errorMsg) {
>        aCallback(aResponse);
>        return;
>      }
> 
>      this._sendUSSDInternal(aClientId, aUssd, aCallback);
>    });
>  },

Currently, every active USSD session is cancelled when a new USSD message is sent.
In my opinion, the session should be cancelled only if there is a valid reason to do so: it expired, user explicitly starts a new session, etc.

Comment 6

[Edgar Chen [:edgar]]

(In reply to Benoit Chabod from comment #5)
> Currently, every active USSD session is cancelled when a new USSD message is
> sent.
> In my opinion, the session should be cancelled only if there is a valid
> reason to do so: it expired, user explicitly starts a new session, etc.

Totally agree, if the request is from existing ussd session, we should not cancel it. This is a flaw from bug 1159591. Will you help to provide a patch to fix this regression? Or if you are not available, I can also take this bug. :)

Comment 7

[Benoit Chabod]

(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> Totally agree, if the request is from existing ussd session, we should not
> cancel it. This is a flaw from bug 1159591. Will you help to provide a patch
> to fix this regression? Or if you are not available, I can also take this
> bug. :)

I think I can manage it !
I will try to add a unit test for USSD responses and sessions longer than one exchange, so that this regression doesn't happen again in the future.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Benoit, per your comment 7 I set you as assignee. Feel free to re-assign or let me/Edgar know if you need any help. Thank you!


it's 2.5+ as it's a regression and breaks a basic function.

Comment 9

[Benoit Chabod]

Attachment: ss

Comment 10

[Benoit Chabod]

Attachment: Git diff of the patch

Here is a patch that would work to fix this regression :)
I just moved the cancel logic, so that it's performed only when a new USSD request is coming. If a USSD session was used to reply, we don't send a cancel request.
PS: don't mind the previous attachment, sent it by mistake

Comment 11

[Benoit Chabod]

Attachment: Git diff of the patch

Sorry once again, messed up the last attachment and posted an old diff !
This is the good one.

Comment 12

[Edgar Chen [:edgar]]

Comment on attachment 8648114 [details] [diff] [review]
Git diff of the patch

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

Overall, it looks good. Thanks for the patch.
BTW, if you could help to create a patch in [1] format, that will be great. 

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#I%27m_all_used_to_Git_but_how_can_I_provide_Mercurial-ready_patches

::: dom/telephony/gonk/TelephonyService.js
@@ +1072,5 @@
> +            if (aResponse.errorMsg) {
> +              aCallback.notifyDialMMIError(aResponse.errorMsg);
> +              return;
> +            }
> +            this._sendUSSDInternal(aClientId, aMmi.fullMMI, this._defaultUSSDHandler.bind(this, aCallb

Somehow this line was truncated.

@@ +1076,5 @@
> +            this._sendUSSDInternal(aClientId, aMmi.fullMMI, this._defaultUSSDHandler.bind(this, aCallb
> +          });
> +          return;
> +        }
> +        this._sendUSSDInternal(aClientId, aMmi.fullMMI, this._defaultUSSDHandler.bind(this, aCallback)

And this.

@@ +1684,4 @@
>      }
>    },
>  
> +  _defaultUSSDHandler: function(aCallback, aResponse) {

s/_defaultUSSDHandler/_defaultMMICallbackHandler/

Comment 13

[Benoit Chabod]

Attachment: Bug-1191205-Cancel-USSD-sessions-only-when-needed.patch

No problem, does this look better ?
Wrapped the two lines that got truncated, and replaced _defaultUSSDHandler with _defaultMMICallbackHandler. And this should be a Mercurial-ready patch :)

Comment 14

[Edgar Chen [:edgar]]

Comment on attachment 8651661 [details] [diff] [review]
Bug-1191205-Cancel-USSD-sessions-only-when-needed.patch

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

I have verified this patch with z3c, it works perfectly. Thank you, Benoit.

Comment 15

[mac]

Please also look at: https://bugzilla.mozilla.org/show_bug.cgi?id=1198676 and mark it as duplicate if it is a duplicate of the bug

Comment 16

[Benoit Chabod]

These are not duplicates, please see what I wrote on the other bug :
https://bugzilla.mozilla.org/show_bug.cgi?id=1198676#c10

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Benoit,
Haven't seen progress for a while, any update? Also, as this is a 2.5 blocker, please help set target milestone accordingly, thank you!

Comment 18

[pelloux]

I'm taking over responsability for this bug since Benoit can't work on it anymore.

I indeed intend to get this fixed for 2.5.

Comment 19

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to pierre-eric from comment #18)
> I'm taking over responsability for this bug since Benoit can't work on it
> anymore.
> 
> I indeed intend to get this fixed for 2.5.

Thanks for taking the effort, Pierre-eric :)
I just set the targetmile stone to the next sprint. If it doesn't meet your plan, feel free to change it. Thank you!

Comment 20

[pelloux]

Benoit provided a patch in comment 13 and Edgar confirmed it work properly so I'm wondering what is needed here to merge his work and close the bug?

Comment 21

[Gabriele Svelto [:gsvelto]]

(In reply to pierre-eric from comment #20)
> Benoit provided a patch in comment 13 and Edgar confirmed it work properly
> so I'm wondering what is needed here to merge his work and close the bug?

The patch has been f+'d but not reviewed, :echen didn't ask for changes in comment 14 so I would expect that not many changes would be needed for landing. Besides that, once the patch is r+'d we'd only need a green try run and the checkin-needed flag.

Comment 22

[pelloux]

Who could do a review of this patch?

Comment 23

[Gabriele Svelto [:gsvelto]]

Both :echen and :hsinyi who've commented in this bug are the RIL owners and could review it. If they're too busy you can find other peers of the module here:

https://wiki.mozilla.org/Modules/All#Radio_Interface_Layer

Comment 24

[Fabien Cazenave [:kaze]]

Comment on attachment 8651661 [details] [diff] [review]
Bug-1191205-Cancel-USSD-sessions-only-when-needed.patch

Edgar, Hsin-Yi, can one of you review this patch please?

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 8651661 [details] [diff] [review]
Bug-1191205-Cancel-USSD-sessions-only-when-needed.patch

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

I trigger a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd1a093996d1&exclusion_profile=false&group_state=expanded
And will help to land it once the try result is positive.
Thank you.

Comment 26

[Edgar Chen [:edgar]]

Another try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeaf3a3f33d8&exclusion_profile=false&group_state=expanded

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/c506db11be14

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c506db11be14