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

VERIFIED FIXED in 2.2 S12 (15may)

Status

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: pcheng, Assigned: azasypkin)

Tracking

unspecified
2.2 S12 (15may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(4 attachments)

Posted 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?]
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
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.
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
Status: ASSIGNED → RESOLVED
Closed: 4 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).
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
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.