Closed Bug 1156464 Opened 10 years ago Closed 10 years ago

[Window Management] Accessing a message thread via notification after killing Messages app creates adverse effects on future notifications while in app

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Status:
VERIFIED FIXED
Milestone:
2.2 S12 (15may)
Project Flags:
blocking-b2g 2.2+
Tracking Flags:
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: pcheng, Assigned: azasypkin)

References

Details

(Whiteboard: [3.0-Daily-Testing][p=1])

Attachments

(4 files)

logcat of issue
184.41 KB, text/plain
Details
[gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2
46 bytes, text/x-github-pull-request
steveck
: review+
steveck
: feedback+
jocheng
: approval-gaia-v2.2+
Details | Review
[gaia] azasypkin:bug-1156464-notification-hash > mozilla-b2g:master
46 bytes, text/x-github-pull-request
steveck
: review+
Details | Review
verified_v2.2&3.0.mp4
4.81 MB, video/mp4
Details
Attached file logcat of issue —
Description: If user receives an SMS notification > kill Messages app > tap on the notification > go back to thread view, the thread will still be marked as unread. Furthermore, in this state, user won't be able to receive SMS notifications while in the Messages app. This issue could be resolved if user returns to homescreen then go back to Messages again, then future notifications will resume displaying. STR: 1) Have Messages app running (either in foreground or background), and receive an SMS on DUT 2) Long press Home button and kill the Messages app 3) Pull down notification tray and tap on the notification 4) After being brought to view the SMS, tap on back arrow button Expected: The thread is marked as read Actual: The thread is marked as unread with a blue dot in front 5) Mark the thread as read, and receive another Message on DUT while still in Messages app Expected: SMS notification shows Actual: No SMS notification. The thread silently updates. Repro rate: 5/5 See video: https://www.youtube.com/watch?v=yxYBrJ6Yz0A Attaching a logcat. Device: Flame 3.0 Master (full flashed 319MB KK) BuildID: 20150420010204 Gaia: cb41d8421da5dc4f16ea566ea2917a9b7f828154 Gecko: 50b95032152c Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b Version: 40.0a1 (3.0 Master) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
This issue reproduces on 2.2 Device: Flame 2.2 BuildID: 20150420002502 Gaia: c15a2b6d3a783813959c2b3bffd2a131f4270b9e Gecko: cc02ee38b252 Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429 Version: 37.0 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 ---- 2.1 is using a different card view and possibly major changes in notification, so I'm not sure if this is a valid comparison. The issue does not occur on 2.1. Thread is correctly marked as read after killing the app and tapping on notification, and future notifications can be received while still in Messages app. Device: Flame 2.1 BuildID: 20150420001205 Gaia: bbe983b4e8bebfec26b3726b79568a22d667223c Gecko: b85d4f4a6d61 Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429 Version: 34.0 (2.1) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 Not marking this a regression for now.
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
Flags: needinfo?(ktucker)
Whiteboard: [3.0-Daily-Testing]
This breaks a major function of the phone. Even though it is a bit of an edge case, the user should always be notified when a new SMS arrives. Nominating this 2.2?
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
The first bug ("mark as read") should be resolved in bug 1153808. The bug exists for a long time so it's not a blocker. But the second bug is strange and it looks like we don't get the system notification. But let's investigate more before leaving the issue to the System peers.
Whiteboard: [3.0-Daily-Testing] → [3.0-Daily-Testing][systemsfe]
Can you find an owner for more investigation?
Flags: needinfo?(felash)
I think this is a regression from bug 1109745.
Blocks: 1109745
Component: Gaia::System::Window Mgmt → Gaia::SMS
Flags: needinfo?(felash)
Whiteboard: [3.0-Daily-Testing][systemsfe] → [3.0-Daily-Testing]
triage: regression
blocking-b2g: 2.2? → 2.2+
(In reply to Julien Wajsberg [:julienw] from comment #5) > I think this is a regression from bug 1109745. passing this to oleg for now in that case, please reassign as needed.
Assignee: nobody → azasypkin
I'll investigate what's going on here.
Status: NEW → ASSIGNED
See Also: → 818000
After my work to produce testcases for bug 818000, I'm quite sure this is the hash we added to the system message entry point. Should be really easy to fix.
Blocks: sms-sprint-2.2S13
Whiteboard: [3.0-Daily-Testing] → [3.0-Daily-Testing][p=1]
So it's not _that_ easy to fix after all. We use the hash to distinguish the cases of "the user started the app using the notification" and "the user started the app using the icon", because mozHasPendingMessages is supposedly slow at startup. A possible solution could be to use the hash but remove it once we used this information, but this is not working either. I added this case in the testcase app from bug 818000. Fabrice, do you think fixing this specific testcase could be possible? That or make mozHasPendingMessages faster (we still need to really measure how slow it is, maybe we're better now?) ? If we can't do that, we'll need to decide between backing out bug 1109745 which would be a UX regression IMO, or using mozHasPendingMessages and making the startup slower (still to be measured).
Flags: needinfo?(fabrice)
I've just added v2.2 PR with a bit hackish solution (using mozHasPendingMessages, no tests for now). (In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #10) > > If we can't do that, we'll need to decide between backing out bug 1109745 > which would be a UX regression IMO We don't have to backout patch entirely, it's still useful for the case when Messages run as activity, we'll just have to remove "#notification" from manifest that will fix the issue, but wipe out our performance improvement for run-from-notication case only. > or using mozHasPendingMessages and making the startup slower (still to be measured). Here are results I got from Raptor (RUNS=10) for v2.2 branch: Current v2.2: Metric Mean Median Min Max StdDev p95 -------------------------------- -------- -------- -------- -------- ------ -------- coldlaunch.navigationLoaded 904.800 899.500 839.000 979.000 38.791 979.000 coldlaunch.willRenderThreads 936.600 928.500 866.000 1017.000 41.164 1017.000 coldlaunch.navigationInteractive 941.100 932.000 874.000 1022.000 40.582 1022.000 coldlaunch.visuallyLoaded 1043.700 1042.500 967.000 1105.000 38.468 1105.000 coldlaunch.fullyLoaded 1136.000 1135.000 1058.000 1197.000 38.186 1197.000 coldlaunch.contentInteractive 1400.300 1399.500 1313.000 1471.000 44.855 1471.000 coldlaunch.pss 14.590 14.600 14.400 14.900 0.137 14.900 coldlaunch.uss 10.230 10.200 10.000 10.700 0.179 10.700 coldlaunch.rss 27.790 27.800 27.600 27.900 0.094 27.900 v2.2 with the patch: Metric Mean Median Min Max StdDev p95 -------------------------------- -------- -------- -------- -------- ------ -------- coldlaunch.navigationLoaded 897.400 899.500 848.000 962.000 37.364 962.000 coldlaunch.willRenderThreads 931.600 937.500 877.000 992.000 40.431 992.000 coldlaunch.navigationInteractive 935.200 940.500 880.000 996.000 40.264 996.000 coldlaunch.visuallyLoaded 1034.900 1031.500 967.000 1105.000 40.317 1105.000 coldlaunch.fullyLoaded 1127.600 1119.000 1062.000 1204.000 40.406 1204.000 coldlaunch.contentInteractive 1383.800 1383.500 1322.000 1446.000 34.626 1446.000 coldlaunch.pss 14.620 14.600 14.400 14.800 0.125 14.800 coldlaunch.uss 10.260 10.250 10.000 10.600 0.156 10.600 coldlaunch.rss 27.840 27.800 27.500 28.100 0.150 28.100 "mozHasPendingMessage" call is done somewhere between "coldlaunch.navigationLoaded" and "coldlaunch.willRenderThreads". Not sure if this data clarifies a lot, it maybe just the margin of error.
(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #10) > Fabrice, do you think fixing this specific testcase could be possible? That > or make mozHasPendingMessages faster (we still need to really measure how > slow it is, maybe we're better now?) ? This slowness has been fixed, and I think that landed on 2.2, so I'm not surprised by the results from the previous comment.
Flags: needinfo?(fabrice)
That's a very good news indeed! Let's do that then :) Thanks to you both. (In reply to Oleg Zasypkin [:azasypkin] from comment #12) > > We don't have to backout patch entirely, it's still useful for the case when > Messages run as activity, we'll just have to remove "#notification" from > manifest that will fix the issue, but wipe out our performance improvement > for run-from-notication case only. Wondering if we have the same issue with activities? Mmm likely not because all our activities are inline and we especially want to avoid system messages with these.
(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #14) > That's a very good news indeed! Let's do that then :) Thanks to you both. > > Wondering if we have the same issue with activities? Mmm likely not because > all our activities are inline and we especially want to avoid system > messages with these. Yep, we should not have such problem with activities because of the fact that we don't listen for any system messages (except for the activity request of course) when in activity mode + in case we receive new message - "full" Messages instance will be run to handle "sms-received".
Comment on attachment 8602167 [details] [review] [gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2 Hey Julien, How do you feel about this approach? Thanks!
Attachment #8602167 - Flags: feedback?(felash)
Comment on attachment 8602167 [details] [review] [gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2 I left a proposal in github
Attachment #8602167 - Flags: feedback?(felash)
See Also: → 1162899
Comment on attachment 8602167 [details] [review] [gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2 Thanks for feedback! Left some replies at GitHub. Hey Steve, since Julien is on PTO could you please look into it as well? Thanks!
Attachment #8602167 - Flags: feedback?(schung)
Comment on attachment 8602167 [details] [review] [gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2 I left some comment on github and this solution for 2.2 lgtm
Attachment #8602167 - Flags: feedback?(schung) → feedback+
Comment on attachment 8602167 [details] [review] [gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2 Hey Steve, Here is v2.2 patch, it's the same as master one except that it doesn't have integration tests since our test infrastructure (new mocks and etc.) is quite different in master and v2.2. I'll ask for review on master patch a bit later, once Treeherder starts using b2g with patch for bug 1162899. Thanks!
Attachment #8602167 - Flags: review?(schung)
Comment on attachment 8602167 [details] [review] [gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2 Looks great! Just remember to ask for approval and we can fix for 2.2 first.
Attachment #8602167 - Flags: review?(schung) → review+
Comment on attachment 8603347 [details] [review] [gaia] azasypkin:bug-1156464-notification-hash > mozilla-b2g:master Thanks for review! Treeherder is green on master now, so asking for review here as well. It should be the same patch, but with integration test.
Attachment #8603347 - Flags: review?(schung)
Comment on attachment 8603347 [details] [review] [gaia] azasypkin:bug-1156464-notification-hash > mozilla-b2g:master Only 1 question for integration test part but looks good overall, thanks!
Attachment #8603347 - Flags: review?(schung) → review+
Thanks for review, nit fixed! Adding checkin-needed for master patch.
Keywords: checkin-needed
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Hmm, didn't think that autolander would land v2.2 without approval :( Hey Ryan, could you please help to back it since we don't have approval yet? Thanks!
Flags: needinfo?(ryanvm)
Comment on attachment 8602167 [details] [review] [gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): bug 1153808 [User impact] if declined: if user opens app from notification there will be no notifications about future incoming messages until app is restarted. [Testing completed]: yes, manual, unit and integration (for master only) [Risk to taking this patch] (and alternatives if risky): relatively low [String changes made]: no
Attachment #8602167 - Flags: approval-gaia-v2.2?
I'm in TRIBE today, but will backout tomorrow if it hasn't gotten approved by then (or if someone else beats me to it).
status-b2g-v2.2: affected → fixed
status-b2g-master: affected → fixed
Flags: needinfo?(ryanvm)
Target Milestone: --- → 2.2 S12 (15may)
Just a reminder that we will also need gecko patch(bug 1162899) landed on b2g37, but it seems also pended in approval request.
Keywords: verifyme
Comment on attachment 8602167 [details] [review] [gaia] azasypkin:v2.2-bug-1156464-notification-hash > mozilla-b2g:v2.2 Approving and request QA verifyme on both master and 2.2.
Attachment #8602167 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Depends on: 1162899
This bug has been verified as pass on latest build of Flame v2.2&3.0 and Nexus 5 v2.2&3.0 by the STR in Comment 0. Actual results: In step 4,the thread is marked as read successfully.In step 5,SMS notification pops up and the unread SMS also shows on the thread in Message app. See attachment: verified_v2.2&3.0.mp4 Reproduce rate: 0/5 ------------------------------------------------------------------------------- Device: Flame v2.2 build(Pass) Build ID 20150514162500 Gaia Revision 8d478e4df36ace173b3e505b568f1a5077fed7c3 Gaia Date 2015-05-14 17:30:50 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/666b327287d5 Gecko Version 37.0 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150514.201419 Firmware Date Thu May 14 20:14:30 EDT 2015 Bootloader L1TC000118D0 Device: Flame v3.0 build(Pass) Build ID 20150514160201 Gaia Revision 8897e1810aa6426ca483269af76ce2bfd2029d25 Gaia Date 2015-05-14 18:49:06 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/59b6d856160c Gecko Version 41.0a1 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150514.192647 Firmware Date Thu May 14 19:26:58 EDT 2015 Bootloader L1TC000118D0 Device: Nexus 5 v2.2 build (Pass) Build ID 20150514162500 Gaia Revision 8d478e4df36ace173b3e505b568f1a5077fed7c3 Gaia Date 2015-05-14 17:30:50 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/666b327287d5 Gecko Version 37.0 Device Name hammerhead Firmware(Release) 5.1 Firmware(Incremental) eng.cltbld.20150514.214406 Firmware Date Thu May 14 21:44:23 EDT 2015 Bootloader HHZ12f Device: Nexus 5 v3.0 build (Pass) Build ID 20150514160201 Gaia Revision 8897e1810aa6426ca483269af76ce2bfd2029d25 Gaia Date 2015-05-14 18:49:06 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/59b6d856160c Gecko Version 41.0a1 Device Name hammerhead Firmware(Release) 5.1 Firmware(Incremental) eng.cltbld.20150514.192809 Firmware Date Thu May 14 19:28:28 EDT 2015 Bootloader HHZ12f
Status: RESOLVED → VERIFIED
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
Keywords: verifyme
Requesting KanRu to add unit test for patch on bug 1162899 before approving b2g-37.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Depends on: 1166580
(In reply to Oleg Zasypkin [:azasypkin] from comment #28) > Hmm, didn't think that autolander would land v2.2 without approval :( > > Hey Ryan, could you please help to back it since we don't have approval yet? > > Thanks! Oleg, can you file a bug for this ?
(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #36) > (In reply to Oleg Zasypkin [:azasypkin] from comment #28) > > Hmm, didn't think that autolander would land v2.2 without approval :( > > > > Hey Ryan, could you please help to back it since we don't have approval yet? > > > > Thanks! > > Oleg, can you file a bug for this ? Seems we have it already - bug 1150080
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: