Closed Bug 1191205 Opened 7 years ago Closed 7 years ago

[v2.5] USSD responses not working

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:2.5+, firefox44 fixed)

RESOLVED FIXED
FxOS-S9 (16Oct)
blocking-b2g 2.5+
Tracking Status
firefox44 --- fixed

People

(Reporter: benoit, Assigned: benoit)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
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
I confirm this is a (major) regression, and a blocker for users with some prepaid plans.

Let’s check if bug 1159591 is involved…
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
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.
Component: Gaia::Dialer → RIL
Depends on: 1159591
[Blocking Requested - why for this release]: Functional regression that can lead to a certification blocker.
blocking-b2g: --- → 2.5?
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.
Flags: needinfo?(echen)
Blocks: 1159591
No longer depends on: 1159591
Flags: needinfo?(echen)
(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. :)
Flags: needinfo?(benoit)
(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.
Flags: needinfo?(benoit)
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.
Assignee: nobody → benoit
blocking-b2g: 2.5? → 2.5+
Attached file ss (obsolete) —
Attached patch Git diff of the patch (obsolete) — Splinter Review
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
Attachment #8648103 - Attachment is obsolete: true
Attachment #8648107 - Flags: feedback?(htsai)
Attachment #8648107 - Flags: feedback?(echen)
Attached patch Git diff of the patch (obsolete) — Splinter Review
Sorry once again, messed up the last attachment and posted an old diff !
This is the good one.
Attachment #8648107 - Attachment is obsolete: true
Attachment #8648107 - Flags: feedback?(htsai)
Attachment #8648107 - Flags: feedback?(echen)
Attachment #8648114 - Flags: feedback?(htsai)
Attachment #8648114 - Flags: feedback?(echen)
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/
Attachment #8648114 - Flags: feedback?(htsai)
Attachment #8648114 - Flags: feedback?(echen)
Attachment #8648114 - Flags: feedback+
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 :)
Attachment #8648114 - Attachment is obsolete: true
Attachment #8651661 - Flags: feedback?(htsai)
Attachment #8651661 - Flags: feedback?(echen)
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.
Attachment #8651661 - Flags: feedback?(htsai)
Attachment #8651661 - Flags: feedback?(echen)
Attachment #8651661 - Flags: feedback+
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
These are not duplicates, please see what I wrote on the other bug :
https://bugzilla.mozilla.org/show_bug.cgi?id=1198676#c10
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!
Flags: needinfo?(benoit)
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.
(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!
Flags: needinfo?(benoit)
Target Milestone: --- → FxOS-S8 (02Oct)
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?
(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.
Who could do a review of this patch?
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 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?
Attachment #8651661 - Flags: review?(htsai)
Attachment #8651661 - Flags: review?(echen)
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.
Attachment #8651661 - Flags: review?(htsai)
Attachment #8651661 - Flags: review?(echen)
Attachment #8651661 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c506db11be14
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: FxOS-S8 (02Oct) → FxOS-S9 (16Oct)
You need to log in before you can comment on or make changes to this bug.