* Build info: Gaia d8ff994bd96c37ba9a93c343932a5441a78a0eec Gecko 6db37b3f76b4fe2aa6f8fb5ae9e036ed99344772 BuildID 20140327060053 Version 28.1 ro.build.version.incremental=69 ro.build.date=Thu Mar 27 06:07:07 CST 2014 * Steps to reproduce 1. Lock screen 2. Receive a SMS from another contact => a notification appears 3. Unlock screen and Launch SMS APP 4. read the new message 5. Check the Notification on Status bar --> The notification is not removed.

The original bug on this one didn't reproduce, this one does. Also, this is a different STR, so we (Jason and I) argue for reopening this bug. It's also a functional regression. QA Wanted to see if we can reproduce this on 1.3.

The notification implementation is significantly different in 1.3 and in 1.4. 1.3 is like 1.2 and 1.1, nothing changed much AFAIK. Seeing the build id in comment 0, I think it's more a 1.4 issue than a 1.3 issue.

There's a few bugs including in for 1.3 : bug 977533 which was marked a dup of bug 855165 We should probably check to see if https://github.com/mozilla-b2g/gaia/commit/4efcdad4637d6af1654dcd441409b187a00b81ce is in 1.3T

Turns out commit https://github.com/mozilla-b2g/gaia/commit/4efcdad4637d6af1654dcd441409b187a00b81ce is not in 1.3, nor 1.3T... After looking further in why this wasn't pushed : https://bugzilla.mozilla.org/show_bug.cgi?id=855165#c74

I spoke with Julien and he mentioned that all the SMS are removed on launch on 1.3 buri. Retesting that was true for buri: Gaia 523196662f4d19c7898d5f4773da020c39cfe57e Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/aa1d48ab3715 BuildID 20140328004002 Version 28.0 ro.build.version.incremental=324 ro.build.date=Thu Dec 19 14:04:55 CST 2013 Not on Tarako. This is a Tarako specific bug it seems. Need to check to see which patch is missing.

Gaia ecbd79aab76e119a7987092b59cbafc97f7ba72f Gecko f23735074dc673457426afa5d87a67f934a18a9b BuildID 20140328060053 Version 28.1 ro.build.version.incremental=70 ro.build.date=Fri Mar 28 06:17:40 CST 2014 Tarako

I can't do this now but I would add a log at [1] to see whether we get the correct event from Gecko. [1] https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/system/js/window.js#L235

Also check whether this happens with the following (separate) cases: * replace 3. with: unlock screen, check in the app manager that the sms app is not running and kill it if it's running, then launch it * before 1.: launch the SMS app and minimize it and then lock the phone * before 1.: launch the SMS app and lock the phone (keep it maximized).

Can we confirm that this isn't reproducing on 1.4? That will confirm this is a tarako-specific bug.

logcat of the issue attached; around 03-28 11:59:30.374 is when I launched SMS For test case : * replace 3. with: unlock screen, check in the app manager that the sms app is not running and kill it if it's running, then launch it =The sms app when killed / relaunched the notification is not removed. For both these cases: * before 1.: launch the SMS app and minimize it and then lock the phone * before 1.: launch the SMS app and lock the phone (keep it maximized). = Once the SMS app comes in the foreground the SMS notification clears. I noticed also if you're quick enough (ie as soon as the SMS message comes in, unlock and launch SMS, the notification clears, because the SMS app is still running in the background)

okay, so re: comment 8 we probably get the right event from gecko as this event relates to the working use cases. I don't know where is the code that would send "appforeground" when we launch the app. Alive, can you investigate this issue in v1.3t ?

due to bug 988979, I had to use an earlier version of 1.4 than today's. I believe Julien mentioned in comment 3, that the results are suppose to be different in 1.3 than in 1.4 1.4 has a different result. When you launch the app, it does not clear the SMS message. When you view the specific message, the notification clears.

Actually, I figured a work around to get the SIM working. Turn airplane mode on and off; I also checked on : Gaia e07a5915737727ba22c06f270ee7af6572f746f3 Gecko https://hg.mozilla.org/releases/mozilla-aurora/rev/33e7fd745c1b BuildID 20140328000202 Version 30.0a2 ro.build.version.incremental=324 ro.build.date=Thu Dec 19 14:04:55 CST 2013 And got the same result.

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #12) > okay, so re: comment 8 we probably get the right event from gecko as this > event relates to the working use cases. > > I don't know where is the code that would send "appforeground" when we > launch the app. > > Alive, can you investigate this issue in v1.3t ? appforeground is: mozbrowservisibilitychange, https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/system/js/window.js#L234 If it's not caught in v1.3t it should be gecko issue. Gecko doesn't send visibilitychange event via mozbrowser event for some reason.

Alive, I know we get "mozbrowservisibilitychange" when the app is already launched and we pass it in foreground, but is it also the path when we launch it the first time? (Sorry for being extra-careful, just want to be sure ;) )

Hi Julien, since you have been active on this bug, do you think you will be able to take this bug? thanks

I'm really not the right person, not to mention my lack of time. The System team is probably big enough to find a talented person there ;)

Joe, Tim, are you taking this tarako bug or should we take care of it?

This is almost certainly a gecko bug, and it most likely exists on m-c too. I flashed both 1.3 and 1.3t over m-c versions of gecko, and I can reproduce. I agree with comment 15 that mozbrowservisibilitychange is not always being sent, and this is the problem. (In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #16) > Alive, I know we get "mozbrowservisibilitychange" when the app is already > launched and we pass it in foreground, but is it also the path when we > launch it the first time? I'm no expert, but the issue has been intermittent for me. Even after I kill the app, sometimes opening the app does clear notifications and sometimes not. So I think this is the right event to be listening for, it's just doesn't always fire :/

can we understand the reproducible rate on this from QA? thanks

Taking a look...

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #16) > Alive, I know we get "mozbrowservisibilitychange" when the app is already > launched and we pass it in foreground, but is it also the path when we > launch it the first time? > > (Sorry for being extra-careful, just want to be sure ;) ) No, I don't think you will get change event since the mozbrowser iframe is created as 'foreground'. I am also curious why the code doesn't use appopen(v1.3)/appopened(v1.4+) event but appforeground event.

This comes from bug 868816 (this was "foreground" instead of "appforeground" back at the time). I think "appopen" wasn't working when the app was already maximized behind the lockscreen. But maybe at this time we had the "mozbrowservisibilitychange" event also when we launched an app? Please do a regression window, this will probably make things clearer. Here is the simplest STR I have: 1. launch the Messages app 2. send a SMS to yourself 3. immediately press the "home" button so that the app is minimized when the SMS is received 4. wait for the notification 5. long press home, kill the Messages app 6. launch the Messages app Expected in 1.3 and before: * the notification is removed Actual in 1.3: * the notification is kept If the first 1.3 build have this bug, please go on with 1.2. Thanks !

A window is not possible here. This is working on 1.3 & 1.4, so this is tarako-specific issue.

No it's not, I reproduced it myself on 1.3, see comment 24. The difference with the initial STR is that on Tarako the app is always killed automatically whereas on other devices with more memory it might stay launched if you don't launch other applications, so you need to kill it yourself to reproduce the issue reliably. Hence my updated STR in comment 24.

(In reply to Julien Wajsberg [:julienw] from comment #26) > No it's not, I reproduced it myself on 1.3, see comment 24. > > The difference with the initial STR is that on Tarako the app is always > killed automatically whereas on other devices with more memory it might stay > launched if you don't launch other applications, so you need to kill it > yourself to reproduce the issue reliably. Hence my updated STR in comment 24. comment 24 is not the same bug as this issue. That issue deals with app launch removing the notifications associated with the app, but that's not what this bug is about. This issue deals with reading the message directly & making sure the notification disappears post that behavior.

(In reply to Joe Cheng [:jcheng] from comment #21) > can we understand the reproducible rate on this from QA? thanks Can we get a reconfirmation as well of whether this is present on 1.3 or not with the original STR given? There appears to be confusion about this.

Jason, the STR in comment 24 are another way of reproducing the exact same underlying bug. I agree the STR look different, but this is how to reproduce the same thing using all devices, instead of using the Tarako only, that few of us have. The original STR will reproduce on 1.3 on Tarako or Fugu, but not on devices with more memory, because the SMS app is not killed using these STR on these devices. Step 4 of comment 0 is unnecessary in 1.3 and 1.3t to remove a notification, that's probably what tricks you here. Now please let's get a regression window for the STR in comment 24 to fix the bug we have in comment 0.

There's no value to pursue a window here if this is already present on 1.3. So I want a confirmation from actual testing that this is still present on 1.3 or not. If it is, then this will no longer block Tarako & we'll evaluate the need for 1.3.

(In reply to Jason Smith [:jsmith] from comment #27) > comment 24 is not the same bug as this issue. That issue deals with app > launch removing the notifications associated with the app, but that's not > what this bug is about. This issue deals with reading the message directly & > making sure the notification disappears post that behavior. In 1.4, sms notifications are only deleted once the thread they belong to is actually read in the app. But in 1.3, notifications are supposed to clear any time the app is launched/brought to the foreground. Since this bug is 1.3 specific and doesn't affect 1.4, it should be about about the app launch not clearing the notifications. I can confirm that this issue reproduces on vanilla 1.3 using a keon. The issue however is more prominent on a tarako since the sms app is more likely to have been killed when receiving an SMS. I can also confirm that mozbrowservisibilitychange event does fire when launching an app sometimes. For instance if I reboot the phone, send an sms, and then launch the app, the notification correctly goes away. But if I kill the app after receiving the sms, and then relaunch (as stated in comment 24 STR), the notification stays. So almost certainly we have a gecko issue here with consistently firing the mozbrowservisibilitychange event when an app launches. It's not clear to me that this is a regression though, since this might have always happened in 1.3, and the memory constraints of tarako surfaced the problem. In any case, I am investigating the gecko side of things today.

There are two parts to this problem. 1.) When an sms arrives and the messages app is not running, the system launches it in the background and displays the notification. The problem is that our BrowserElementChild assumes it is visible when it is first created (http://hg.mozilla.org/mozilla-central/file/1417d180a1d8/dom/browser-element/BrowserElementChildPreload.js#l133), so when we do call setVisible(true) on it after the user launches it, the change event never gets fired since gecko thinks it was already visible (http://hg.mozilla.org/mozilla-central/file/1417d180a1d8/dom/browser-element/BrowserElementChildPreload.js#l895). 2.) Even if we kill the backgrounded messages app after receiving the notification and then relaunch it, the mozbrowservisibilitychange event still will not fire given that the browser child starts as visible. I see two approaches to fix. 1.) Do not default to "visible" when creating browser element children, so when we foreground the app after it has been running in the background only (or launch it after it has been killed), we see the visibility change event. 2.) Listen for a different event in the system app. Perhaps 'appopen(ed)' as suggested in comment 23. Approach 2 is less risky if we can find the appropriate event. But given that the system can launch apps in the background, but yet the mozbrowser frame thinks they are "visible", there is a problem here worth fixing. I will speak with vivien tomorrow about this to get some advice.

re: comment 30: yes there is value in having a regression window, to find the regressing commit, to know what changed and thus what to fix. re: comment 32: I don't get your first point, since we actually get the event when the app is still running in background and the user launches it. I'll check today on my 1.1 phone, maybe this reproduces there too. I agree with you that even if this issue exists since forever, the Tarako makes it a lot more apparent. Removing the regression-window request until I see it's really a regression.

OK, this is not a regression, the same thing happens on my 1.1 phone, so this actually never worked. It's just made more visible in Tarako for the reasons we already outlined.

(In reply to Julien Wajsberg [:julienw] from comment #33) > re: comment 32: I don't get your first point, since we actually get the > event when the app is still running in background and the user launches it. If the app has been in the foreground before, we do indeed receive the event. But if the app has never been in the foreground, we won't receive the event. STR this: 1.) Kill messages app if it is running. 2.) Send sms to the phone. 3.) Run `adb shell b2g-ps` and observe that messages is indeed running in the background. 4.) Launch the message app using the dock. Actual result: notification stays, and we never receive mozbrowservisibilitychnage event from gecko. Expected result: notification gets cleared after receiving aforementioned event.

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.][OOO until 4/7] from comment #23) > No, I don't think you will get change event since the mozbrowser iframe is > created as 'foreground'. > I am also curious why the code doesn't use appopen(v1.3)/appopened(v1.4+) > event but appforeground event. The reason why we use appforeground is bug 868816. Basically, when unlocking the phone, we dont get the appopen event, and so notifications don't clear. I think we will need to listen for both events and clear notifications. After speaking with Alive and Vivien on IRC today, I think the part where an app is opened in the background and then doesn't fire a mozbrowservisibility change when it is foregrounded is a different bug and won't block 1.3.

The event name changed between 1.3 and 1.4 (appopen to appopened), so I will do too separate pull requests. This one is for 1.3.

Master patch, so we have code parity with 1.3, but going forward we shouldn't be using that old api. Also, integration test from 1.3 patch is not applicable because sms app no longer uses it.

1.3 is actually affected, but is less noticeable on devices with more memory.

Comment on attachment 8402091 [details] 1.3 Github PR [Approval Request Comment] SMS notifications aren't automatically closing when app is killed in the background. This is very prevalent on low memory devices [Bug caused by] (feature/regressing bug #): bug 868816 [User impact] if declined: On low memory devices, users will have to manually click/close sms notifications to get them to clear. [Testing completed]: Tested on keon and tarako. Added 1.3 specific integration test. [Risk to taking this patch] (and alternatives if risky): Adds an additional event listener to clear notifications. Biggest risk is clearing notifications when we don't want to, but that risk is low. [String changes made]: none.

Comment on attachment 8402110 [details] Master GH PR Thanks for investigating, could we have the unit test + the integration test in master to prevent this happen again?

FYI we have a python-based integration test for displaying notification at [1]. I don't mind whether you do a JS or python-based integration test though :) [1] https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/messages/test_sms_notification.py

Re: comment 42: I think this test is run only once per day because it needs an actual sms to be sent to the device. Feel free to find another way to test this, but I don't know a good way to fake that for integration tests yet. :(

(In reply to Julien Wajsberg [:julienw] from comment #43) > Re: comment 42: I think this test is run only once per day because it needs > an actual sms to be sent to the device. Feel free to find another way to > test this, but I don't know a good way to fake that for integration tests > yet. :( In my 1.3 integration test, I directly invoke the sms activity handler [1] to simulate this. Unfortunately, this won't work in master to test the desktop notification clearing functionality since in master we use the new notification api. I think the integration test on master will have to manually invoke mozNotification.createNotification for some app to test this. But I think we can land the 1.3 test as is here, and do the master test in a follow-up. 1.) https://github.com/mozilla-b2g/gaia/pull/17996/files#diff-6635f921a6e891a929a36ae9aacbe5f5R25

Comment on attachment 8402091 [details] 1.3 Github PR Forgot to add Fabrice to the approval request.

Updated the PR on master to include a desktop helper integration test. However, this test will fail until bug 992383 gets fixed. Since there is no rush on the master branch for this fix, I think we can wait for that. In the meantime, we should go ahead with the 1.3 landing.

Bug 992383 landed. Retrying the travis job for master PR: https://travis-ci.org/mozilla-b2g/gaia/builds/22430448

landed in master with integration test: https://github.com/mozilla-b2g/gaia/commit/1130c4ed846c14a0af17449bf4789e3d16275702

(In reply to Michael Henretty [:mhenretty] from comment #40) > Comment on attachment 8402091 [details] > 1.3 Github PR > > [Approval Request Comment] > SMS notifications aren't automatically closing when app is killed in the > background. This is very prevalent on low memory devices > > [Bug caused by] (feature/regressing bug #): > bug 868816 > > [User impact] if declined: > On low memory devices, users will have to manually click/close sms > notifications to get them to clear. > > [Testing completed]: > Tested on keon and tarako. Added 1.3 specific integration test. > > [Risk to taking this patch] (and alternatives if risky): > Adds an additional event listener to clear notifications. Biggest risk is > clearing notifications when we don't want to, but that risk is low. > > [String changes made]: none. Clearing the approval request as this patch is needed on 1.3T branch "alone". Given this patch is already a blocker for 1.3T, this does not require an explicit approval to land on that branch. Here are the guidelines : https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.3T.

Adding a NI on :jhammink so this issue gets verified once it lands on the 1.3T branch.

(In reply to bhavana bajaj [:bajaj] from comment #49) > Clearing the approval request as this patch is needed on 1.3T branch > "alone". Given this patch is already a blocker for 1.3T, this does not > require an explicit approval to land on that branch. Here are the guidelines > : https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.3T. This is actually not a Tarako only bug, see comment 39, comment 34, comment 31 and comment 24. It is most noticeable because the bug reproduces with memory pressure, but this can happen on any 1.3 device. This needs to be fixed everywhere imo. Since the patch is ready for all branches, can we get the approval for 1.3?

status-b2g-v1.4:unaffected ?

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #52) > status-b2g-v1.4:unaffected ? Sorry about the confusion, it is affected, and this patch should indeed be uplifted.

Comment on attachment 8402110 [details] Master GH PR [Approval Request Comment] Desktop notifications (old api) aren't automatically closing when app is killed in the background and then relaunched). This is very prevalent on low memory devices. Note: on 1.4, this no longer affects the SMS app, since they use the new notification API. [Bug caused by] (feature/regressing bug #): bug 868816 [User impact] if declined: Apps that use the old desktop notification api might have notifications that stick around longer than expected (ie, require launching the app twice before they get cleared). [Testing completed]: New integration test. [Risk to taking this patch] (and alternatives if risky): Adds an additional event listener to clear notifications. Biggest risk is clearing notifications when we don't want to, but that risk is low. [String changes made]: none.

Please ask Fabrice for 1.3 approval.

Comment on attachment 8402091 [details] 1.3 Github PR [Approval Request Comment] SMS notifications (or any app using desktop notifications) aren't automatically closing when app is killed in the background. This is very prevalent on low memory devices. [Bug caused by] (feature/regressing bug #): bug 868816 [User impact] if declined: On low memory devices, users will have to manually click/close sms notifications to get them to clear. [Testing completed]: Tested on keon and tarako. Added 1.3 specific integration test. [Risk to taking this patch] (and alternatives if risky): Adds an additional event listener to clear notifications. Biggest risk is clearing notifications when we don't want to, but that risk is low. [String changes made]: none.

1.3: https://github.com/mozilla-b2g/gaia/commit/177e5797f99da3a9bef8b0e012d983da0cec545b

Any reason you didn't land this on v1.4? Per the landing guidelines, you should have (you don't need double-approval as spelled out there). https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_3

I thought it was a 1.3-specific patch and that 1.4 is not affected, but Michael is saying otherwise in comment 53, so now I'm confused :)

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #58) > Any reason you didn't land this on v1.4? Per the landing guidelines, you > should have (you don't need double-approval as spelled out there). > https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_3 Ah, gotcha. I didn't quite understand this, so thanks for the clarification. (In reply to Julien Wajsberg [:julienw] from comment #59) > I thought it was a 1.3-specific patch and that 1.4 is not affected, but > Michael is saying otherwise in comment 53, so now I'm confused :) This bug was initially marked `1.4: unaffected` because it was thought to be an SMS specific bug, and in 1.4 SMS uses the new notification API. However, since this bug actually deals with any app that uses the old notification api (like calendar), it also applies to master/1.4. I submitted two patches, one for master and one for 1.3, each with slightly different implementation and different integration tests. I landed the master patch first, then the 1.3. Now I just have to uplift to 1.4 ... 1.4: https://github.com/mozilla-b2g/gaia/commit/32af2759f67252fa490d1f36f912b73a33481965

oh yeah, right, I forgot we still need to support the old notification API. Thanks for the explanation!

Backed out for causing an intermittent failure on tbpl. https://tbpl.mozilla.org/php/getParsedLog.php?id=37629125&tree=Mozilla-Aurora#error0 1.4: https://github.com/mozilla-b2g/gaia/commit/c50e0e299af14f69e15493e26387ff51a5a8dc21

This was not causing the failure mentioned. Re-landed. v1.4: https://github.com/mozilla-b2g/gaia/commit/8dff633372022723e2ebad17fe3c826436b3b258

Verified as fixed v1.3, this issue does NOT reproduce in the latest Buri v1.3 build: 4/14 v1.3 Environmental Variables: Device: Buri v1.3 MOZ RIL BuildID: 20140414004002 Gaia: 8b92c56267fe50772095f1f25d6cc1f9c9966eb4 Gecko: 3e26908fca71 Version: 28.0 Firmware Version: V1.2-device.cfg - Verified as fixed v1.3T, this issue does NOT reproduce in the latest Tarako v1.3T build: 4/14 v1.3T Environmental Variables: Device: Tarako v1.3T MOZ BuildID: 20140414004001 Gaia: 0e7f21e61625b75a9149480cd5a259211549f020 Gecko: 3b02811c314a Version: 28.1 Firmware Version: sp6821 - Verified as fixed v1.4, this issue does NOT reproduce in the latest Buri v1.4 build: 4/14 v1.4 Environmental Variables: Device: Buri v1.4 MOZ RIL BuildID: 20140414000201 Gaia: 8dff633372022723e2ebad17fe3c826436b3b258 Gecko: bc14179fc49c Version: 30.0a2 Firmware Version: V1.2-device.cfg - Verified as fixed v1.5, this issue does NOT reproduce in the latest Buri v1.5 build: 4/14 v1.5 Environmental Variables: Device: Buri v1.5 MOZ RIL BuildID: 20140404040204 Gaia: d9a574284d672f532f7c562a091bb01f531202b1 Gecko: 6c924a018540 Version: 31.0a1 Firmware Version: V1.2-device.cfg - 1) Changing status of bug to verified as this was checked against master.