995907 - [Tarako][SMS][Notification] The notification of new SMS does not appear while playing a large size video.

Status: VERIFIED FIXED

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

Description

[Bingqing Li]

Environment:
------------------------------------------------------
Gaia      b2802627a974795ccba989cede0540f20fadc633
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/c8491a42d4e8
BuildID   20140413164001
Version   28.1
ro.build.version.incremental=eng.cltbld.20140413.210357
ro.build.date=Sun Apr 13 21:04:05 EDT 2014


Steps to reproduce:
-----------------------------------------------------
1. Launch Video
2. Select a large size video (over 100M) to play
3. Send a text message to the test device while the video is being played
   ---> The notification does not show while the video is being played.

4. Wait for a feww minutes (e.g. 3 min), exit Video, and launch SMS

Actual results:
--------------------------------------------------
1. The new message is already in the list, and the received time is about a few minutes ago (e.g. 3 min ago).

2. The notification appears after exiting Video, but the notification says the message is received just now.


Expected result:
-------------------------------------------------
The notification can be shown while the video is being played.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Can we get a logcat, b2g-info, etc here please?

Comment 2

[Bingqing Li]

Attachment: logcat

Comment 3

[Bingqing Li]

Attachment: slog

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

triage: this does not seem like an absolute blocker for tarako at this moment, put this to backlog. ni? Candice, are you fine with this? Thanks

Comment 5

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #4)
> triage: this does not seem like an absolute blocker for tarako at this
> moment, put this to backlog. ni? Candice, are you fine with this? Thanks

No, we aren't fine with this. SMS notifications must always work. Period. There's no time when you shouldn't get a notification that a SMS came in. That's basic phone functionality. Resetting the blocking flag, as we already concluded that this is a cut and dry blocker.

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

The problem is this: while a video is playing, the sms app is launched in the background to handle the `sms-received` system message. However, before the app gets a chance to send sms notification it gets killed through memory pressure.

There are two possible approaches to fixing this.
1.) Define a different entry point for 'sms' received that doesn't load everything, and perhaps we can keep the memory footprint low enough to prevent it from getting killed.
2.) Background the Video app, and foreground the sms app immediately. Not the best user experience, but better than not getting informed of sms.

I'm currently investigating the feasibility of option 1.

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Github PR v1.3t

Julien, what are you thoughts on this approach? This would give us a lightweight entry-point for the 'sms-received' system message. This fixes this problem, but perhaps it could cause other issues I am not aware of.

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

Two Problems found:

1.) Clicking on the notification causes the SMS app to be foregrounded with the blank entry point still up. This displays a black screen.

2.) While the SMS app is in the foreground, if we receive an sms we will not get an sms.

I think I can fix both these problems, so leaving the fb? for Julien.

Comment 9

[Julien Wajsberg [:julienw]]

So, this is a direction I'd like to eventually take, but clearly the feasibility is how to resolve the remaining issues you saw in comment 8.

The second point is not the most difficult: the likely solution is use the 'received' event (in MessageManager) to call a "onMessageReceived" function that would be in ThreadListUI, and that would do what we currently do in ActivityHandler when the app is already launched.

For the first point, I think we should remove the "onclick" event on the sent notification and rely exclusively on the "notification" system message. Hopefully Gecko sees that the entry point is different and will launch the app with the other entry point.

In the end, there is also no reason to keep the "sms-received" handler in this ActivityHandler file and we could definitely export this handler to a separate file.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

Thanks for doing this work, this is definitely something I wanted to do eventually.

Comment 11

[James Zhang (Spreadtrum)]

We should land this patch before 4.25, then we can enter spreadtrum PTR2 test cycle.

Comment 12

[[:fabrice] Fabrice Desré]

Michael, Julien: probably a stupid question, but why aren't we displaying the notification from the system app and then opening the sms app when we tap the notification?

Comment 13

[Julien Wajsberg [:julienw]]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> Michael, Julien: probably a stupid question, but why aren't we displaying
> the notification from the system app and then opening the sms app when we
> tap the notification?

It does not sound right that the system app listens to the system message 'sms-received' to send a notification. Or did you mean something else ?

Moreover we do different things depending whether the app is launched, or which thread is displayed, this sounds like a business for the Messages app more than the System app.

Comment 14

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

(In reply to Michael Henretty [:mhenretty] from comment #8)
> 1.) Clicking on the notification causes the SMS app to be foregrounded with
> the blank entry point still up. This displays a black screen.

Fixed by doing a manual redirect to the main entry point in the notification click handler.
 
> 2.) While the SMS app is in the foreground, if we receive an sms we will not
> get an sms.

Fixed by manually invoking ActivityHandler.onSmsReceived from the message manager. I believe this is what your were suggesting to do Julien.

Comment 15

[Julien Wajsberg [:julienw]]

(In reply to Michael Henretty [:mhenretty] from comment #14)
> Comment on attachment 8407945 [details] [review]
> Github PR v1.3t
> 
> (In reply to Michael Henretty [:mhenretty] from comment #8)
> > 1.) Clicking on the notification causes the SMS app to be foregrounded with
> > the blank entry point still up. This displays a black screen.
> 
> Fixed by doing a manual redirect to the main entry point in the notification
> click handler.

Should work, but I just want to know if you tried removing the onclick handler? Do you know why Gecko is not launching the app with the other entry point? I mean, is it expected, per spec, or something?

>  
> > 2.) While the SMS app is in the foreground, if we receive an sms we will not
> > get an sms.
> 
> Fixed by manually invoking ActivityHandler.onSmsReceived from the message
> manager. I believe this is what your were suggesting to do Julien.

I was suggesting to do this, but also to move this to ThreadListUI, but I'll have a look to the actual patch.

Comment 16

[Julien Wajsberg [:julienw]]

Found an issue:
* ensure the app is killed
* receive a SMS, wait for the notification to be displayed
* launch the app
=> white screen
Not sure how to fix this ;)

I don't like the "hash" hack but I don't find a simple enough other solution for this.
Can you also do a PR for master? Note that bug 936729 will land today so you'll want to base your patch on this if it's not landed yet.

Also, can you try to explain why we need to workaround these 2 problems? Is these gecko issues?

(In reply to Michael Henretty [:mhenretty] from comment #8)
> Two Problems found:
> 
> 1.) Clicking on the notification causes the SMS app to be foregrounded with
> the blank entry point still up. This displays a black screen.

=> looks like the same issue than the one that I saw; I still don't understand why gecko don't launch the other endpoint.

> 
> 2.) While the SMS app is in the foreground, if we receive an sms we will not
> get an sms.

same thing here actually: why gecko does not detect the "wrong" endpoint is launched?

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> (In reply to Michael Henretty [:mhenretty] from comment #14)
> > Comment on attachment 8407945 [details] [review]
> > Github PR v1.3t
> > 
> > (In reply to Michael Henretty [:mhenretty] from comment #8)
> > > 1.) Clicking on the notification causes the SMS app to be foregrounded with
> > > the blank entry point still up. This displays a black screen.
> > 
> > Fixed by doing a manual redirect to the main entry point in the notification
> > click handler.
> 
> Should work, but I just want to know if you tried removing the onclick
> handler? Do you know why Gecko is not launching the app with the other entry
> point? I mean, is it expected, per spec, or something?

Removing the onclick handler does not make the "notification" system message fire. I spoke with Fabrice about this and he said it was a bug, so I filed that here: bug 1001278. I don't think we should block on this though since we have a workaround. What do you think?


(In reply to Julien Wajsberg [:julienw] from comment #16)
> Found an issue:
> * ensure the app is killed
> * receive a SMS, wait for the notification to be displayed
> * launch the app
> => white screen
> Not sure how to fix this ;)

This actually looks like a different bug with the app launch_path. Right now when launching an app manually, we don't check if the launch entry point is different than the current URL [1] (which we indeed do for system messages [2]). In master, this code has been refactored, but we should check if the problem exists there too. For 1.3t, I can fix it in this bug.

1.) https://github.com/mozilla-b2g/gaia/blob/f348044cf8beaa35cf82818d47f0a65c7a0f7c7a/apps/system/js/window_manager.js#L983
2.) https://github.com/mozilla-b2g/gaia/blob/f348044cf8beaa35cf82818d47f0a65c7a0f7c7a/apps/system/js/window_manager.js#L1006


> I don't like the "hash" hack but I don't find a simple enough other solution
> for this.
> Can you also do a PR for master? Note that bug 936729 will land today so
> you'll want to base your patch on this if it's not landed yet.

Yup, we can do something similar for master. But I think this should be a separate bug that doesn't block Tarako.

> Also, can you try to explain why we need to workaround these 2 problems? Is
> these gecko issues?
> 
> (In reply to Michael Henretty [:mhenretty] from comment #8)
> > Two Problems found:
> > 
> > 1.) Clicking on the notification causes the SMS app to be foregrounded with
> > the blank entry point still up. This displays a black screen.
> 
> => looks like the same issue than the one that I saw; I still don't
> understand why gecko don't launch the other endpoint.

This is actually different than the issue you found. The problem with clicking a notification is that the system message is not sent for the notification click with a running app, and so the entry point for notification click will not be launched. The issue you found is that when manually launching SMS, the entry point is not launched. Both are fixable/workaroundable issues :)

> > 2.) While the SMS app is in the foreground, if we receive an sms we will not
> > get an sms.
> 
> same thing here actually: why gecko does not detect the "wrong" endpoint is
> launched?

Yup, same problem. When an app is launch, we don't receive the 'sms-received' system message, and therefore the system never tries to relaunch the app.

Comment 18

[Julien Wajsberg [:julienw]]

The current behavior makes it necessary to register the system message handler in all pages even if one of these pages is specified in manifest.webapp for this system message. This is really wrong IMO.

A simple command line:
  find apps -name manifest.webapp -exec grep -A 5 '"messages" *:' {} \+

shows me that only costcontrol has a different message handler page than the main page. Reading their code, I see they do something weird to workaround the issue: [1].

[1] https://github.com/mozilla-b2g/gaia/blob/4457f14677f3a5ef626b7905dca584681401acc3/apps/costcontrol/js/widget.js#L67-L79

Comment 19

[Julien Wajsberg [:julienw]]

Discussed with Vivien:

In 1.3t:
* we should call "window.close()" after sending the notification so that clicking the notification launches the right path
* we should have an iframe like costcontrol does so that we don't lose the system messages

In master/2.0:
* we should fix the system app to allow specifying a different path for system messages and for the launch path. It seems we fixed in Gecko the issue where it used to send system messages to the wrong window in case we had several.

Michael, how does that sound?

Comment 20

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

(In reply to Julien Wajsberg [:julienw] from comment #19)
> Discussed with Vivien:
> 
> In 1.3t:
> * we should call "window.close()" after sending the notification so that
> clicking the notification launches the right path

Good idea!

> * we should have an iframe like costcontrol does so that we don't lose the
> system messages

Which system messages are you concerned with losing? Right now we have 'sms-received' and 'notification'. 'sms-received' will causes the 'sms.html' page to load, fire notification, then close. If the app is open, we use mozMobileMessage to handle the message. So we shouldn't lose anything there. For notifications, we don't have much control over this. If the app is running, we need to use click handler, if not running we get system message to 'index.html'. I don't think an iframe will help us here either. Perhaps I am misunderstanding?

> 
> In master/2.0:
> * we should fix the system app to allow specifying a different path for
> system messages and for the launch path. It seems we fixed in Gecko the
> issue where it used to send system messages to the wrong window in case we
> had several.

Agreed. It is also worth noting that if an app fires an notification, gets killed, gets relaunched, and then someone clicks the original notification, the app will not get an onclick event or system message for this. I would say this is also a bug, but probably not related to this bug.



In any case, I have updated the PR to use window.close at the end of the sms handler, and that seems to have solved the problems mentioned above. I thought there would be a race condition, where you could launch the sms app while "sms.html" was handling the system message, and we would get the familiar blank screen. However, it looks like the system message code will run its course (thereby calling window.close) before the appLaunch code happens. I've been playing around with this patch, and I think it solves our use cases. Please let me know if you can find issues with it.

Comment 21

[Julien Wajsberg [:julienw]]

(In reply to Michael Henretty [:mhenretty] from comment #20)
> Comment on attachment 8407945 [details] [review]
> Github PR v1.3t
> 
> (In reply to Julien Wajsberg [:julienw] from comment #19)
> > Discussed with Vivien:
> > 
> > In 1.3t:
> > * we should call "window.close()" after sending the notification so that
> > clicking the notification launches the right path
> 
> Good idea!
> 
> > * we should have an iframe like costcontrol does so that we don't lose the
> > system messages
> 
> Which system messages are you concerned with losing? Right now we have
> 'sms-received' and 'notification'. 'sms-received' will causes the 'sms.html'
> page to load, fire notification, then close. If the app is open, we use
> mozMobileMessage to handle the message.

Yeah, the iframe was actually an alternative to calling ActivityHandler from MessageManager. I thought it was maybe cleaner to just add the iframe for sms.html at the end of index.html. But now that I think more of it, we'd need to do special code to find whether we're in iframe mode or in background mode, and I don't like this. Let's stick to your solution.

> 
> > 
> > In master/2.0:
> > * we should fix the system app to allow specifying a different path for
> > system messages and for the launch path. It seems we fixed in Gecko the
> > issue where it used to send system messages to the wrong window in case we
> > had several.
> 
> Agreed. It is also worth noting that if an app fires an notification, gets
> killed, gets relaunched, and then someone clicks the original notification,
> the app will not get an onclick event or system message for this. I would
> say this is also a bug, but probably not related to this bug.

This sounds bad... but I just tested on master and it seems to work?


I'll review your patch now.

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

works well, some comments + I'd like some simple unit tests if possible

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

Ok, I think we are pretty close now. I addressed your comments, and added unit tests to test the window.close functionality (should close on sms entry point, shouldn't close on main entry point).

The one question I do have is about class 0 notifications. It looks like these notifications cause the SMS app to open and then display an alert. In this case, we don't want to call window.close, is that correct? My only concern would be what happens to the app after the class 0 sms. I'm afraid we might be left in the "blank screen" state again. A simple solution could be to redirect to the main entry point before displaying the alert. But I'm not sure how to test class 0 notifications. Can you help me here?

Comment 24

[Julien Wajsberg [:julienw]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

Added some more comments.

About testing class 0 messages... well, currently, you need to build the emulator and send a command using telnet.

But you can try fetching my very-WIP work in [1], add a messageClass in [2], create a DEBUG=1 DESKTOP=0 profile, run Firefox with this profile, load http://sms.gaiamobile.org:8080/simulator/simulator.html, load the application, and send a message. And maybe it will be able to to exercise your code in index.html ;)

[1] https://github.com/julienw/gaia/compare/sms-simulator
[2] https://github.com/julienw/gaia/compare/sms-simulator#diff-351084faefa777be577c2c44a54bf8c2R45

Comment 25

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

Updated PR to address your comments.

It looks like your simulator invokes the sms system message handler when running gaia in firefox (which is good) but I am mostly concerned about how this will behave on the device. It sounds like the best way to test this is with the emulator, so I will build that overnight. In the meantime, please give my patch another review :)

Thinking more, class-0 notifications come from the operator usually, is that correct? For instance, does setting the call forwarding number on the SIM cause the operator to send a class-0 sms to confirm the new setting? I feel like I have seen that before. I will try this.

Comment 26

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Michael Henretty [:mhenretty] from comment #25)
> Thinking more, class-0 notifications come from the operator usually, is that
> correct? For instance, does setting the call forwarding number on the SIM
> cause the operator to send a class-0 sms to confirm the new setting? I feel
> like I have seen that before. I will try this.

NM, that is not a class-0 sms, that is something else.

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

This is why I should not set flags at midnight.

Comment 28

[James Zhang (Spreadtrum)]

The notification of alarm also has this issue.
see http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=307826

Comment 29

[Julien Wajsberg [:julienw]]

We won't fix the alarm notification in this patch.

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

added comments on github

basically the code looks fine but there are weird behaviors.

Comment 31

[Michael Henretty [:mikehenrty][:mhenretty]]

Ok Julien, I found the problem with the weird behaviors you described. It seems we were missing the libraries we needed for the MMS flow (l10n and SMIL). I've corrected this and tested both the auto-download and non-auto-download MMS sending while playing a video, and we do indeed see the notification. Note: auto-download can take a loooong time, so you will need a long video to test :)

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8407945 [details] [review]
Github PR v1.3t

r=me with the localization fix from my commit

If you have an idea to fix the "several sms-received system messages in a few seconds" case (that will happen when you receive messages while in flight mode or while the phone is off), please request another review. I'd like to fix this remaining issue but not spending too much time on it, so if you don't see how to fix, let's just land this.

Comment 33

[James Zhang (Spreadtrum)]

v1.3t https://github.com/mozilla-b2g/gaia/commit/128e558f0159c38d3ef60fc38d5a09910b62233e

Comment 34

[Julien Wajsberg [:julienw]]

revert: 407b862f6bad4000ebd2f9e9a0283a6a56d494aa

Please don't merge yourself as the pull request is not ready. As I've said in comment 32, it's r=me _with an additional commit_ that I'd like Michael to review as well. Plus we have a remaining issue that I'd like to discuss with him.

Comment 35

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to James Zhang from comment #28)
> The notification of alarm also has this issue.
> see http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=307826

James, is there a public mozilla bug for this? I can't access that link.

Comment 36

[James Zhang (Spreadtrum)]

(In reply to Michael Henretty [:mhenretty] from comment #35)
> (In reply to James Zhang from comment #28)
> > The notification of alarm also has this issue.
> > see http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=307826
> 
> James, is there a public mozilla bug for this? I can't access that link.

Use your mozilla email to register,then you can access.
If see some bug comment in Chinese, use google translate.

Comment 37

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Github PR, 1.3t - updated based on julien's feedback

Comment 38

[Julien Wajsberg [:julienw]]

Comment on attachment 8418389 [details] [review]
Github PR, 1.3t - updated based on julien's feedback

r=me but let's wait for a green travis !

Comment 39

[Michael Henretty [:mikehenrty][:mhenretty]]

v1.3t: https://github.com/mozilla-b2g/gaia/commit/d0c1737e8c086e85df17e210ad1f28c926b852aa

Comment 40

[Roland Kunkel [:RolandK]]

SMS notification was displayed while viewing a large video (103 MB), verified as fixed on latest v1.3T Tarako build:

v1.3T Environmental Variables:
Device: Tarako v1.3T MOZ RIL
BuildID: 20140523014001
Gaia: e76fc9fc64a027d84b2ec311fc624f4c3459dca9
Gecko: 52c1f97caee9
Version: 28.1
Firmware Version: SP6821A-Gonk-4.0-5-12