Bingqing Li Reporter
	Description • 10 years ago

Steps to reproduce:
-----------------------------------------
1. Install Youtube app from Market Place.
2. Launch Youtube and play a video
3. USSD message pops up while playing the video


Actual result:
-----------------------------------------
YouTube is pushed to background and gets killed due to LMK

	Marvin Khoo [:Marvin_Khoo]
	Comment 1 • 10 years ago

it's hard to view YouTube video using prepaid SIM, since the USSD msg comes actively and reports how much mins / data left to use. nom to 1.3? is a blocker.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 2 • 10 years ago

I'm preparing a fix for bug 989713 that might help with this too.

	Jason Smith [:jsmith]
	Comment 3 • 10 years ago

(In reply to Marvin Khoo [:Marvin_Khoo] from comment #1)
> it's hard to view YouTube video using prepaid SIM, since the USSD msg comes
> actively and reports how much mins / data left to use. nom to 1.3? is a
> blocker.

I'm assuming you meant to nominate this for Tarako.

	bhavana bajaj [:bajaj]
	Comment 4 • 10 years ago

Bing, can you confirm if this is a recent regression or have we had this issue forever on the youtube app ?

	Marvin Khoo [:Marvin_Khoo]
	Comment 5 • 10 years ago

Thanks Jason, yes, is 1.3T?.. my bad. :(

also, after tapping the X on upper left and close the USSD dialog, the next UI we see is dialer pad. i think we shouldn't show dialer pad unless the USSD command is typed from the pad.

may i know what's the behavior or UI display in US / China if user receives "remaining minutes / data" USSD info when using pre-paid SIM?

	Bingqing Li Reporter
	Comment 6 • 10 years ago

(In reply to bhavana bajaj [:bajaj] from comment #4)
> Bing, can you confirm if this is a recent regression or have we had this
> issue forever on the youtube app ?
I'm truly sorry that we are unable to reproduce it at our site, need partner to confirm this.

Hi Marvin and James, could you please help with this?

	James Zhang (Spreadtrum)
	Comment 7 • 10 years ago

Youtube killed by LMK when USSD app popup.
You can also try other app popup case.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 8 • 10 years ago

per discussion with partner, partner will see if they can workaround this 
ni? James

	James Zhang (Spreadtrum)
	Comment 9 • 10 years ago

Loop Siiaceon.

	Marvin Khoo [:Marvin_Khoo]
	Comment 10 • 10 years ago

attached a flow for better understanding. You may found this issue when using whatever tools to send device an USSD while playback a YouTube content.

	siiaceon
	Comment 11 • 10 years ago

try to group ussd, and need check if can make ussd not prompt but only one tone(ussd notify unhide while exit video). (Many handset use this solution for such as MMS/SMS case)
loop arvin also.

	siiaceon
	Comment 12 • 10 years ago

Youtobe need background while ussd coming. So just make USSD group also can make Youtobe crash . So solution is
Need moz cooperator change the design, which need seperate the dialer and USSD. At least cannot launch dailer when recieved notify USSD (ussd which no need dailer)

anyone else solution?

	James Zhang (Spreadtrum)
	Comment 13 • 10 years ago

Group USSD notify to system app?

	siiaceon
	Comment 14 • 10 years ago

just make USSD group also can make Youtobe crash except group Youtobe(not a good solution).
need seperate the dialer and USSD by mozilla designed (no need dailer when ussd just notify). 

agree ?

	James Zhang (Spreadtrum)
	Comment 15 • 10 years ago

Find USSD owner and discuss with him.

	James Zhang (Spreadtrum)
	Comment 16 • 10 years ago

But I don't think change design is a good idea at this time.

	siiaceon
	Comment 17 • 10 years ago

 dailer/USSD owner to aprove, pls
change designed or not launch dailer in this case?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 18 • 10 years ago

I've done some investigation on this and the solution looks more like a design change / window management issue rather than an LMK problem, let me describe why: I've tried running both the YouTube app and the dialer at the same time and both work fine. I can switch between the two w/o either one getting killed and more importantly I can simulate a USSD interaction w/o the YouTube app getting killed. For example this sequence works fine:

- Launch the YouTube app
- Start playing a video
- Go back to the homescreen
- Launch the dialer
- Type an MMI code and send it (such as *#30#)
- Wait for the USSD panel to show up and close it
- Go back to the homescreen
- Go back to the YouTube app

Once we're back in the YouTube app it's still running fine.

Now solving this is not trivial however as two different changes are required:

- We need to close the dialer after the USSD interaction has finished and the user has closed the USSD screen. This shouldn't be really hard if we add some tracking to the app to distinguish between solicited and unsolicited USSD messages.

- We need a way to bring back the YouTube app to the foreground when the dialer is closed. This should behave somewhat similarly to what we do when we close the callscreen and the former background app comes back into the foreground.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 19 • 10 years ago

Thanks Gabriele for your comment 18
What do you think of the risk and estimate of effort on this bug? this will help to evaluate whether this bug should stay as 1.3T+. Thanks

	Gabriele Svelto [:gsvelto] Assignee
	Comment 20 • 10 years ago

(In reply to Joe Cheng [:jcheng] from comment #19)
> What do you think of the risk and estimate of effort on this bug? this will
> help to evaluate whether this bug should stay as 1.3T+. Thanks

I've studied the code a bit more and the required change is to move the USSD UI into an attention screen and change the related code accordingly. This has both risks and drawbacks:

- The affected code is relatively isolated so the change is unlikely to affect the rest of the application however the change is fairly radical so it might cause breakage the existing MMI/USSD functionality.

- Additionally moving the USSD display into an attention screen changes it's behavior. Currently when we display an USSD message we do it in the context of the dialer app. Tapping the home button when viewing an USSD message will send the dialer to the background and bringing it back will still display the current USSD message. In an attention screen this will work differently: tapping the home button will shrink the attention screen and show a reduced version of it in the top bar and the only way to dismiss it will be to explicitly close it using the close button. Would this be acceptable from a UX POV? What should appear in the minimized attention screen then? What happens if a new USSD message arrives while the attention screen is shrunk? All this probably requires some additional UX.

- Last but not least the upside is that we don't need to move the USSD code anywhere else (like in the system app). It can still live in the dialer and this shouldn't cause problems with a foreground app such as YouTube.

	Thomas Ho [:tho]
	Comment 21 • 10 years ago

Carrie, please review the proposed UX change for USSD. Thank you very much.

	Carrie Wang [New account: carrie75521@gmail.com]
	Comment 22 • 10 years ago

Hi  Gabriele, 

I don't really understand the "Shrunk UI" part. Is it like something from Dialer that if users tap Home while they are in a call and they can tap the bar again to get back to the call screen (in this case, that will be USSD page)?
However, I'd prefer to keep this USSD info in Notification. In this way, users tap Home and they can still access the page whenever they need (by dragging down the notification tray and tapping it to access the page), but if they tap "x" to close the page, it will be cleared from notification tray. Is this doable? Thanks!

	Joe Cheng [:jcheng] (please needinfo)
	Comment 23 • 10 years ago

also Ni? Rik for comment 22 as he took the bug

	Anthony Ricaud (:rik)
	Comment 24 • 10 years ago

We may be killing the Youtube app because we request a high-priority wake lock when we receive a MMI. This was introduced to fix bug 999356. I need to try some code to verify this. If I do verify it, I'm not sure how we can fix it though.

For the UI part, moving the code outside of the Dialer app or moving it to an attention screen will introduce a lot of regressions in behaviour. What about this solution: If the dialer is not the front app, we show a notification. In the dialer, we store all the mmi received and when a user taps a notification, we display the ussd window for that ussd. That may help us solve bug 861754 at the same time.

	Carrie Wang [New account: carrie75521@gmail.com]
	Comment 25 • 10 years ago

I'd go with the "notification" proposal as what Rik mentioned here.

	James Zhang (Spreadtrum)
	Comment 26 • 10 years ago

Our Indian QA sain dolphin also has 30% reproduce rate.

	siiaceon
	Comment 27 • 10 years ago

1025457 related issue

	Kai-Chih Hu [:khu]
	Comment 28 • 10 years ago

I am wondering that is this issue reproducible on other devices as well?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 29 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #24)
> We may be killing the Youtube app because we request a high-priority wake
> lock when we receive a MMI. This was introduced to fix bug 999356. I need to
> try some code to verify this. If I do verify it, I'm not sure how we can fix
> it though.

Some clearing on this: we're not killing the YouTube app, we're just sending it in the background as the USSD display is part of the dialer so once that is closed you'll find yourself in the dialer instead of the YouTube app.

> For the UI part, moving the code outside of the Dialer app or moving it to
> an attention screen will introduce a lot of regressions in behaviour. What
> about this solution: If the dialer is not the front app, we show a
> notification. In the dialer, we store all the mmi received and when a user
> taps a notification, we display the ussd window for that ussd. That may help
> us solve bug 861754 at the same time.

+1

The MMI code is already tracking the state of a single MMI request and expanding it to handle more should be doable w/o too many changes.

	Anthony Ricaud (:rik)
	Comment 30 • 10 years ago

While working on this, I'm sometimes getting this error:
[JavaScript Error: "requestWindow is undefined" {file: "jar:file:///system/b2g/omni.ja!/components/RILContentHelper.js" line: 2105}]

Hsin-Yi: I'm not yet sure if it's a problem but could you take a look in case we need this fixed for the solution to work?

Here is the patch I use to receive USSD messages while developing. Pressing volume down sends a USSD request.
diff --git a/apps/system/js/hardware_buttons.js b/apps/system/js/hardware_buttons.js
index 9c77db3..c96de57 100644
--- a/apps/system/js/hardware_buttons.js
+++ b/apps/system/js/hardware_buttons.js
@@ -493,6 +493,9 @@
       case 'volume-down-button-release':
         if (this.direction === 'volume-down-button-press') {
           if (!this.repeating) {
+            console.log('XXX: MMI sent');
+            navigator.mozMobileConnections[0].sendMMI('#123#');
+
             this.hardwareButtons.publish('volumedown');
           }
           this.hardwareButtons.setState('base', type);

	Anthony Ricaud (:rik)
	Comment 31 • 10 years ago

I'm running into bug 1028039 while working on this.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 32 • 10 years ago

I am looking at this. Will keep you posted!

	Anthony Ricaud (:rik)
	Comment 33 • 10 years ago

Here is my current state: https://github.com/Rik/gaia/tree/youtube-ussd-1.3t-1016885

Although we'd like to land this on master, I had to work on 1.3t because of bug 1028039. The current patch works but it is not ready for review yet. I'll need someone else to continue the work while I am out (unassigning myself to reflect that).

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 34 • 10 years ago

Thanks for the sharp eye, :rik. File a gecko bug 1028062.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 35 • 10 years ago

Bug 1028062 doesn't block this issue IMO. It is caught when Rik is working on this. Bug 1028082 makes firing request error for mobileConnection.cancelMMI().

	Gabriele Svelto [:gsvelto] Assignee
	Comment 36 • 10 years ago

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> Bug 1028062 doesn't block this issue IMO. It is caught when Rik is working
> on this. Bug 1028082 makes firing request error for
> mobileConnection.cancelMMI().

Agreed.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 37 • 10 years ago

Here's a more complete version of Anthony's patch now that we've got the document.hidden issue fixed. The only major change is that I've converted both the handleMMIReceived() and sendNotification() functions to use promises so that we can properly release the high-priority wakelock when we're finished. I've given this patch only light testing and I'm not 100% happy with it yet so I'm asking for feedback first. BTW it also lacks tests so this is barely above a WIP.

	Wayne Chang [:wchang]
	Comment 38 • 10 years ago

Hi Gabriele,

Thank you for supporting here, can you suggest a comfortable ETA for this and the risk assessment on this change?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 39 • 10 years ago

(In reply to Wayne Chang [:wchang] from comment #38)
> Thank you for supporting here, can you suggest a comfortable ETA for this
> and the risk assessment on this change?

A comfortable ETA including reviews and a couple of extra iterations would be sometimes later this week like Thursday or Friday at the latest. The risk for this change should be modest as it affects only unsolicited USSD messages making them pop-up notifications when the dialer is not visible. The associated logic received only minor changes and only for the unsollicited case.

	Etienne Segonzac (:etienne)
	Comment 40 • 10 years ago

Comment on attachment 8447745 [details] [diff] [review]
[PATCH] Send a notification instead of displaying the dialer when receiving an unsolicited USSD notification

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

Let me know if my comments are unclear, had a hard time formulating them.

::: apps/communications/dialer/js/dialer.js
@@ +413,5 @@
>            }
>  
>            MmiManager.handleMMIReceived(evt.message, evt.sessionEnded,
> +                                       evt.serviceId)
> +                    .then(releaseWakeLock);

I'm afraid in the cases where we're doing a getSelf.launch(), this might release the wake lock _before_ the visibility change. (since handleMMIReceived resolves outside of the getSelf block)

::: apps/communications/dialer/js/mmi.js
@@ +356,5 @@
> +          };
> +          var notification =
> +            new Notification('USSD', {body: message, icon: iconURL});
> +          notification.addEventListener('click', clickCB);
> +          resolve();

Since we're resolving as soon as the notification is sent (which is a good thing), we're releasing the wake lock and the dialer could get killed.
Should we add support for those notifications in dialer.js/handleNotification? (called via system message when the app gets killed taking the clickCB with it).

	Gabriele Svelto [:gsvelto] Assignee
	Comment 41 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #40)
> I'm afraid in the cases where we're doing a getSelf.launch(), this might
> release the wake lock _before_ the visibility change. (since
> handleMMIReceived resolves outside of the getSelf block)

Good point. We should release the wakelock immediately only after having posted the notification. I'm splitting that case of handleMMIReceived() to make the code cleaner so I'll use that to handle the unlocking properly too.

> Since we're resolving as soon as the notification is sent (which is a good
> thing), we're releasing the wake lock and the dialer could get killed.
> Should we add support for those notifications in
> dialer.js/handleNotification? (called via system message when the app gets
> killed taking the clickCB with it).

Yes, I forgot about that. That's definitely needed otherwise we might lose some messages and that would be especially likely so on the Tarako.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 42 • 10 years ago

This is a fully working patch for gaia/master with the issues you had highlighted addressed plus a little test coverage. A few notes:

- I'm passing information using the iconURL field of a notification; the tag would be better but we need to uplift this to the v1.3t branch and we don't have tags there so I didn't want to make my life more complicated than it already is :-p

- On the topic of notifications, the dialer is a bit of a mess in this regard still abusing iconURL to distinguish between the origin of notifications and closing all notifications when one is tapped. I've made it play nice with my changes but we should open a nice follow-up bug to clean up the mess.

- I've had to modify the fake origin of certain mock messages from 'app://communications.gaiamobile.org' to 'http://communications.gaiamobile.org:8080' because the test failed otherwise.

	Etienne Segonzac (:etienne)
	Comment 44 • 10 years ago

Comment on attachment 8448690 [details] [diff] [review]
[PATCH] Send a notification instead of displaying the dialer when receiving an unsolicited USSD notification

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

(In reply to Gabriele Svelto [:gsvelto] from comment #42)
> Created attachment 8448690 [details] [diff] [review]
> [PATCH] Send a notification instead of displaying the dialer when receiving
> an unsolicited USSD notification
> 
> This is a fully working patch for gaia/master with the issues you had
> highlighted addressed plus a little test coverage. A few notes:
> 
> - I'm passing information using the iconURL field of a notification; the tag
> would be better but we need to uplift this to the v1.3t branch and we don't
> have tags there so I didn't want to make my life more complicated than it
> already is :-p

Make sense, let's make sure we have a follow up filed (and referenced in the code).

> 
> - On the topic of notifications, the dialer is a bit of a mess in this
> regard still abusing iconURL to distinguish between the origin of
> notifications and closing all notifications when one is tapped. I've made it
> play nice with my changes but we should open a nice follow-up bug to clean
> up the mess.

The cost of being a pioneer :)

> 
> - I've had to modify the fake origin of certain mock messages from
> 'app://communications.gaiamobile.org' to
> 'http://communications.gaiamobile.org:8080' because the test failed
> otherwise.

Ok. let's keep an eye open to make sure this doesn't fail on some test infrastructure.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 45 • 10 years ago

Thanks for the quick review!

(In reply to Etienne Segonzac (:etienne) from comment #44)
> Make sense, let's make sure we have a follow up filed (and referenced in the
> code).

I've filed bug 1033254 and referenced it in the code; let's get this code cleaned up ASAP.

> Ok. let's keep an eye open to make sure this doesn't fail on some test
> infrastructure.

My first Travis run contained some changes to the fake app origin but did fail, now I've reverted those changes again and the tests fail locally. I'll be waiting to see what happens on Travis; annoying :-|

	Gabriele Svelto [:gsvelto] Assignee
	Comment 46 • 10 years ago

This is the Travis run: https://travis-ci.org/mozilla-b2g/gaia/builds/28946591

It's red but the failing tests are completely unrelated so I'll just go ahead and push it.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 47 • 10 years ago

Pushed to gaia/master ddbf0e11b494fd9665545141485d0b70cab6d514

https://github.com/mozilla-b2g/gaia/commit/ddbf0e11b494fd9665545141485d0b70cab6d514

I'll post the v1.3t patch shortly.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 48 • 10 years ago

While testing the v1.3t patch on the Tarako I noticed that a mistake had slipped in my original patch that makes the changes work only when one SIM is used. When multiple SIMs are used localization fails and the notification is not delivered when receiving an unsolicited message so I'm reopening the bug. Fix pending.

	Wayne Chang [:wchang]
	Comment 49 • 10 years ago

Hi Gabriel,

When you have a patch good to go, please also request for 1.4 approval... partner is also encountering the same problem on Dolphin.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 50 • 10 years ago

This patch initializes the MmiManager before sending a notification, not doing so made notifications fail on multi-SIM devices as the message couldn't be localized properly. The change is conceptually small but the patch is large because I've added test coverage for this specific case. BTW I should really file a follow up to clean up the MMI unit-tests as they're in dire shape (and a bunch of them are disabled to make things worse).

	Etienne Segonzac (:etienne)
	Comment 52 • 10 years ago

Comment on attachment 8450069 [details] [diff] [review]
[PATCH] Always initialize the MmiManager before sending a notification

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

Good catch!

	Gabriele Svelto [:gsvelto] Assignee
	Comment 53 • 10 years ago

Pushed to gaia/master c664153cd702ae1db12a485e539bae88b9bdcdea

https://github.com/mozilla-b2g/gaia/commit/c664153cd702ae1db12a485e539bae88b9bdcdea

The Travis run was partially red but the failures were unrelated:

https://travis-ci.org/mozilla-b2g/gaia/builds/29041867

I'll be uplifting the patches right away.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 54 • 10 years ago

Uplifting this patch has been an uphill battle against a race in the LazyL10n which I finally managed to fix. I'm asking for review again before landing because the patches had to undergo some important changes, namely:

- New style notifications have been replaced with old style ones, this had been planned ahead and was thus relatively painless.

- The LazyL10n object was suffering from the following race: initialization would go through two asynchronous steps, one to wait for the 'localized' event and one to lazy load further code. If another caller would invoke LazyL10n.get() in between the two calls its callback would be lost as it would never receive the localized event nor would it be able to proceed further. I've modified the code to mark the beginning and end of the initialization procedure and allow only one caller to go through it. Successive callers arriving in the middle of the initialization phase will have their callbacks delayed until the end the object is fully initialized by stacking them into an array.

Besides doing some testing of the scenarios at hand I also tested the other users of LazyL10n to ensure this change didn't break anything but I want to stress that this hasn't been a simple, straightforward uplift and thus we have to proceed with some caution before merging.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 55 • 10 years ago

I've updated the PR because the new unit-tests were failing; hopefully they should be fixed now.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 56 • 10 years ago

Good news, Travis is green for the uplift too: https://travis-ci.org/mozilla-b2g/gaia/builds/29123015

	Etienne Segonzac (:etienne)
	Comment 57 • 10 years ago

Comment on attachment 8450584 [details] [review]
[PULL REQUEST] v1.3t uplift

r=me with the test added (see github).

Stupid question but better safe than sorry, we do have Promise support in the gecko we're releasing with 1.3t right?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 58 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #57)
> r=me with the test added (see github).

Good point, I'm going to add that.

> Stupid question but better safe than sorry, we do have Promise support in
> the gecko we're releasing with 1.3t right?

Mmm... now that you mention it the documentation says we've got promises enabled by default in Gecko 29 and hidden behind a flag starting with Gecko 25. v1.3t is Gecko 28 so it's a matter of figuring out if they've been enabled or not. I'll double-check, worst case I'll turn that specific instance into a callback (ugly but functional).

	Gabriele Svelto [:gsvelto] Assignee
	Comment 59 • 10 years ago

I've updated the PR once more:

- Now we use a callback instead of a promise to release the wakelock after having sent the notification, I checked that it's released correctly.

- I've adjusted the tests to cope with the callback.

- I've slightly modified the LazyL10n code: I push the queued up callbacks and then shift them so they're called in order of arrival. Before I popped them out of the array causing them to be called in inverse order of arrival. I think this is cleaner and less "surprising".

- There's an additional test for the LazyL10n code that verifies that all the queued callbacks are invoked properly.

If Travis turns green I'm ready to merge: https://travis-ci.org/mozilla-b2g/gaia/builds/29150394

	James Zhang (Spreadtrum)
	Comment 60 • 10 years ago

Please also merge to v1.4. Dolphin has the same issue.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 61 • 10 years ago

(In reply to James Zhang (Spreadtrum) from comment #60)
> Please also merge to v1.4. Dolphin has the same issue.

I will as soon as I get approval for this and its dependency.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 62 • 10 years ago

Uplifted to gaia/v1.3t 50578b579c378018bf19e966a8cda5b0bcd04a1d

https://github.com/mozilla-b2g/gaia/commit/50578b579c378018bf19e966a8cda5b0bcd04a1d

	James Zhang (Spreadtrum)
	Comment 63 • 10 years ago

Hi Gabriele,

   Can you also merge this patch to v1.4? We met this issue on dolphin and 1.4 is dolphin branch now.

	Wayne Chang [:wchang]
	Comment 64 • 10 years ago

Gabriele,

Is the 1.3t patch good for 1.4 or does it need rebasing?

Thanks

	Gabriele Svelto [:gsvelto] Assignee
	Comment 65 • 10 years ago

(In reply to Wayne Chang [:wchang] from comment #64)
> Is the 1.3t patch good for 1.4 or does it need rebasing?

Some rebasing is needed, I'm almost done with. Will post the PR for approval soon.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 66 • 10 years ago

Here's a PR for the v1.4 branch. It required some light modifications, mostly to the unit-tests. I'm not asking for approval just yet because I haven't had the time to test it on a device.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 67 • 10 years ago

Comment on attachment 8452163 [details] [review]
[PULL REQUEST] v1.4 uplift

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: Unsolicited USSD messages will bring the dialer application to the front hiding whatever the user was doing in the meantime (e.g. watching a video on YouTube). According to our partners this is quite detrimental in the target markets where unsolicited USSD messages are sent to inform the user of its remaining credit / data cap.
[Testing completed]: Tested on a Flame & covered with unit-tests
[Risk to taking this patch] (and alternatives if risky): USSD/MMI code may be adversely affected. A minor issue that cropped up during testing is bug 1034279.
[String changes made]: None

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 68 • 10 years ago

Comment on attachment 8452163 [details] [review]
[PULL REQUEST] v1.4 uplift

Approved for 1.4.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 69 • 10 years ago

Thanks for the approval, landed attachment 8452163 [details] [review] on gaia/v1.4 b0e9b4bdb39c5eb93a6783a34624ffc84f62b126

https://github.com/mozilla-b2g/gaia/commit/b0e9b4bdb39c5eb93a6783a34624ffc84f62b126

Now let's move on to v2.0.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 70 • 10 years ago

See comment 67, the patches here are unmodified and can be cherry-picked cleanly from the master branch.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 72 • 10 years ago

Uplifted to gaia/v2.0 1bd6e8957ccf310b2f75ba5695b058a2e284df3a

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

	helloarvin
	Comment 73 • 10 years ago

Hi Gabriele,

In v1.4, there's a little problem occurred that it won't show title info(sim index and operator name) on USSD message received. I filed a new bug to track this, let's check it in Bug 1051841.

Thanks.

	Wei Gao (Spreadtrum)
	Comment 74 • 10 years ago

Dear Gabriele

Our QA in India found a new bug. When he clicked STK menu which meaned sending *121# in settings, there was neither ussd page nor ussd notification shows.
After I checked the log, I found the ussd had been received, but the ussd page was on background.

I think that is because "sessionEnded" flag, After clicking STK menu to send *121#, "sessionEnded" flag in received ussd message is false. So there is no ussd notification.

https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/dialer/js/dialer.js#L299

	Wei Gao (Spreadtrum)
	Comment 75 • 10 years ago

I think this patch can resolve this issue.
Could you give some advice?
Thanks.

	Wei Gao (Spreadtrum)
	Comment 76 • 10 years ago

(In reply to Wei Gao (Spreadtrum) from comment #75)
> Created attachment 8484789 [details] [diff] [review]
> ussd_sessionEnd.patch
> 
> I think this patch can resolve this issue.
> Could you give some advice?
> Thanks.

Maybe we can also launch('dialer') instead of showing notification in "MmiManager.handleMMIReceived" to resolve this issue.
https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/dialer/js/dialer.js#L307

	Gabriele Svelto [:gsvelto] Assignee
	Comment 77 • 10 years ago

(In reply to Wei Gao (Spreadtrum) from comment #74)
> Our QA in India found a new bug. When he clicked STK menu which meaned
> sending *121# in settings, there was neither ussd page nor ussd notification
> shows.
> After I checked the log, I found the ussd had been received, but the ussd
> page was on background.
> 
> I think that is because "sessionEnded" flag, After clicking STK menu to send
> *121#, "sessionEnded" flag in received ussd message is false. So there is no
> ussd notification.

Yes, but that was done on purpose to solve the issue in this bug. We need to find a better way. Let's file another bug for this.

	Wei Gao (Spreadtrum)
	Comment 78 • 10 years ago

(In reply to Gabriele Svelto [:gsvelto] from comment #77)
> Yes, but that was done on purpose to solve the issue in this bug. We need to
> find a better way. Let's file another bug for this.

Ok, thanks, I will file a new bug.

	Alissa（leave from Mozilla）
	Comment 80 • 10 years ago

This issue has been successfully verified on Flame 2.1, 2.0
See attachment:verify.3gp
Reproducing rate: 0/5

Flame 2.1 new versions:
Gaia-Rev        dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebce587d2194
Build-ID        20141203001205
Version         34.0

Flame 2.0 new versions:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/29222e215db8
Build-ID        20141203000201
Version         32.0