1136633 - [Messages] Blank page when launching the SMS application in some conditions

Status: VERIFIED FIXED

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

Description

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

STR:
1. receive a SMS
2. force close the Messages application using the Task Manager. (note: in real life, we can have the same bug by rebooting the phone -- but force closing the application is faster to reproduce)
3. swipe right the notification to remove it (or click on "clear all")
4. launch the Messages application.

=> Blank page.

QAwanted for branch checks.
Please also find a regression window.
A possible culprit is bug 1112135.

Comment 1

[Yeojin Chung [:YeojinC]]

This issue reproduces on Flame Master.

Result: Contacts app appears blank after force closing the app and clearing the notification.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150225030650
Gaia: cc235a867161e0000ea55a4f009b3be19021f066
Gecko: 6608e0605dfc
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0
-----------------------------------
This issue does NOT reproduce on Flame 2.1.

Result: Contacts app launches properly.

Environmental Variables:
Device: Flame 2.1
BuildID: 20150223194120
Gaia: 86af0ca427adad12c3109124f31bef2fd9614e47
Gecko: a275f2c05ca6
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
==========================

I'll continue to find the regression window.

Comment 2

[Yeojin Chung [:YeojinC]]

Initial Regression Window:

Last Working Environmental Variables:
Device: Flame 2.2
BuildID: 20141231114359
Gaia: 334532c0fcb809ac1c8bcd64a8eb9357967acf36
Gecko: b3e6101aa94b
Version: 37.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First Broken Environmental Variables:
Device: Flame 2.2
BuildID: 20141231164358
Gaia: fce979e88240aba9f106c41b299151d3676bed99
Gecko: 271f40e3c3ad
Version: 37.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Last Working Gaia First Broken Gecko: Issue does NOT reproduce 
Gaia: 334532c0fcb809ac1c8bcd64a8eb9357967acf36
Gecko: 271f40e3c3ad

First Broken Gaia Last Working Gecko: Issue NOT reproduce
Gaia: fce979e88240aba9f106c41b299151d3676bed99
Gecko: b3e6101aa94b

https://github.com/mozilla-b2g/gaia/compare/334532c0fcb809ac1c8bcd64a8eb9357967acf36...fce979e88240aba9f106c41b299151d3676bed99

As Julien suspected, this issue is caused by bug 1112135

Comment 3

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Thanks a lot !

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Oleg, can you take look at this please? Looks like the work done on bug 1112135 might have caused this to occur.

Comment 5

[Oleg Zasypkin [:azasypkin]]

(In reply to KTucker [:KTucker] from comment #4)
> Oleg, can you take look at this please? Looks like the work done on bug
> 1112135 might have caused this to occur.

Sure, thanks for discovering the issue! 

Just for the record, this issue happens because of the fact that removing notification from notification tray causes System to run responsible app to handle "notification" system message (see discussion in bug 1068564). When Messages is run to handle such "close" notification message ("clicked" property of message is "false") it just does nothing with it.

What changed here is that after bug 1112135, we initialize ThreadList only once we receive "navigated" event that is fired for "open" system notification messages via navigating to the appropriate message thread. For the "close" notification message we don't do anything and "navigated" event isn't fired that leads to uninitialized ThreadList.

We should take care of this case in Messages app until we get system support for dedicated "close" notification event.

Will provide patch soon.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1136633-close-notification > mozilla-b2g:master

Comment 7

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8569790 [details] [review]
[gaia] azasypkin:bug-1136633-close-notification > mozilla-b2g:master

Hey Julien,

Could you please take a look at the patch?

Thanks!

Comment 8

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8569790 [details] [review]
[gaia] azasypkin:bug-1136633-close-notification > mozilla-b2g:master

Looks good but I don't feel comfortable with doing a full launch if the user merely closed a notification... I made an alternative proposition on github.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

Comment 10

[Oleg Zasypkin [:azasypkin]]

Hey Michael,

Not sure if you're right person to ask about it, but will try :) In this bug we're trying to deal with the fact that Messages app is run to handle "notification" system message when notification is removed from notification tray. Julien pointed me to the related discussion in bug 1068564 where this question was discussed a bit, but no changes were made.

I've just read through the latest spec [1], [2] and [3] and it looks like "onclose" event is going to be removed from both persistent and non-persistent notifications. 

So the question is do you know if we're going to stop dispatching of "notification" system when notification is removed any time soon? I suspect that it may depend on some platform bug to remove "Notification.onclose" first, but "unexpected" system message is more important problem for us at the moment :)

Thanks!

[1] https://notifications.spec.whatwg.org/
[2] https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Oct/0135.html
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1098223#c3

Comment 11

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8569958 [details] [review]
[gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

Hey Julien,

As we discussed offline it would be safer to stick with the old behaviour for v2.2 branch (the same behaviour as we have in v2.1 branch), so here is the same patch, but for v2.2 branch-only.

For the master patch, I'll wait for Michael answer before doing any workaround improvements. 

Thanks!

Comment 12

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

(In reply to Oleg Zasypkin [:azasypkin] from comment #10)
> So the question is do you know if we're going to stop dispatching of
> "notification" system when notification is removed any time soon? I suspect
> that it may depend on some platform bug to remove "Notification.onclose"
> first, but "unexpected" system message is more important problem for us at
> the moment :)

There is no timeline currently for removing the onclose event. However, I will say this: it looks like the Notification API has stabilized enough that we can remove the onclose event without conflicting with any upcoming changes based on Service Worker notification persistence (I hope!). That being the case, this will most likely be something Robert Bindar will work on when he returns for his second internship this summer.

That said, you should probably put a workaround in place in master in the meantime. Judging from the 2.2 patch, the footprint of the workaround seems small and it's best for dogfooders if this bug doesn't exist on master in the meantime.

Comment 13

[Oleg Zasypkin [:azasypkin]]

Hey Michael,

Thanks for your reply! 

I have one more quick question for you, we see that when user clicks on notification we receive 2 consequent "notification" system messages, the first one with "clicked === true" and the second one with "clicked === false" (presumably because of the fact that notification is automatically removed by system when user clicks on it). 

Is it expected behaviour?

Comment 14

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8569958 [details] [review]
[gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

Julien, as you found out during review, this solution unfortunately doesn't work in some cases, so I've updated PR with another idea that should cover all cases I can think of including the one mentioned in comment 13.

What do you think about it?

Comment 15

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8569958 [details] [review]
[gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

Yep, I like this idea !

Comment 16

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

(In reply to Oleg Zasypkin [:azasypkin] from comment #13)
> Hey Michael,
> 
> Thanks for your reply! 
> 
> I have one more quick question for you, we see that when user clicks on
> notification we receive 2 consequent "notification" system messages, the
> first one with "clicked === true" and the second one with "clicked ===
> false" (presumably because of the fact that notification is automatically
> removed by system when user clicks on it). 
> 
> Is it expected behaviour?

Hmm, this is not exactly expected behavior. A notification should not be automatically removed (and therefore closed) by the system when a user clicks on it. It is up to the app to decide to close it when receiving the click event. However, I believe the sms app will close the notification once it opens the correct thread panel, and so the system will send the close event at this point (which I believe will be a system message since there is no "live" notification to broadcast onclose to).

If, however, you are seeing the close system message before actually closing the notification (or just receiving another unknown notification system message with the click one), this is a bug in gecko.

Comment 17

[Oleg Zasypkin [:azasypkin]]

(In reply to Michael Henretty [:mhenretty] from comment #16)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #13)
> > Hey Michael,
> > 
> > Thanks for your reply! 
> > 
> > I have one more quick question for you, we see that when user clicks on
> > notification we receive 2 consequent "notification" system messages, the
> > first one with "clicked === true" and the second one with "clicked ===
> > false" (presumably because of the fact that notification is automatically
> > removed by system when user clicks on it). 
> > 
> > Is it expected behaviour?
> 
> Hmm, this is not exactly expected behavior. A notification should not be
> automatically removed (and therefore closed) by the system when a user
> clicks on it. It is up to the app to decide to close it when receiving the
> click event. However, I believe the sms app will close the notification once
> it opens the correct thread panel, and so the system will send the close
> event at this point (which I believe will be a system message since there is
> no "live" notification to broadcast onclose to).
> 

Ah, you're right, I completely forgot about this. Sorry for the noise!

Comment 18

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8569958 [details] [review]
[gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

Thanks for feedback! Added unit tests for the new approach, should be ready for review.

Comment 19

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8569958 [details] [review]
[gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

I think we should go ahead and implement your transitionPromise idea so that we can return a proper promise from ensureCurrentPanel and have more solid tests.

So I'm defering the r+.

Please do your changes in a separate commit so that we can decide which direction we want in the end :)

Comment 20

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8569958 [details] [review]
[gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

Changed Navigation.transitioning to Navigation.transitionPromise + some other nits.

Could you please review it again?

Thanks!

Comment 21

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8569958 [details] [review]
[gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

r=me with a nit

thanks, good work !

Comment 22

[Oleg Zasypkin [:azasypkin]]

Thanks for review, nit fixed! Now I'll think how can we do better for master.

Can't say that Treeherder is green, but nothing related to the patch :)

v2.2: https://github.com/mozilla-b2g/gaia/commit/4361b3965acfc4094e23ea4001c5b49da51f5f86

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Why are we landing patches on v2.2 without approval?
https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_6

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] mozilla-b2g:revert-28503-v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

Comment 25

[Oleg Zasypkin [:azasypkin]]

Oops, sorry, reverted.

Reverted from v2.2: https://github.com/mozilla-b2g/gaia/commit/166491b92278dc9e648f8d49ab02d9ca00d74421

Comment 26

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

Comment 27

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8574081 [details] [review]
[gaia] azasypkin:v2.2-bug-1136633-close-notification > mozilla-b2g:v2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1112135
[User impact] if declined: user may stuck in half-initialized app after removing message notification(s)
[Testing completed]: yes, manual and unit tests
[Risk to taking this patch] (and alternatives if risky): relatively low
[String changes made]: n/a

Comment 28

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

BTW Oleg, to make things easier, I think we should land on master too, and file a separate bug for any further improvement.

Comment 29

[bhavana bajaj [:bajaj]]

+1 for comment#28, and I am waiting for master landing before approving on the branch.

Comment 30

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (PTO March 7th -> 15th) from comment #28)
> BTW Oleg, to make things easier, I think we should land on master too, and
> file a separate bug for any further improvement.

(In reply to bhavana bajaj [:bajaj] from comment #29)
> +1 for comment#28, and I am waiting for master landing before approving on
> the branch.

Sure, let me prepare PR for the master branch

Comment 31

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8569790 [details] [review]
[gaia] azasypkin:bug-1136633-close-notification > mozilla-b2g:master

Carrying over r+ flag from v2.2 patch, code is the same, no conflicts in v2.2 -> master cherry-pick.

Comment 32

[Oleg Zasypkin [:azasypkin]]

Treeherder is green, manual tests performed successfully.

Filed bug 1141014 for further improvements on master.

Master: https://github.com/mozilla-b2g/gaia/commit/3528adf67c15c83a07a85cd9053a714afd15b6d9

Comment 33

[Oleg Zasypkin [:azasypkin]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/f02ae51924c1cd3d649d7ec299b25edf139d6978

Comment 34

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame Master and 2.2.

Result: The Messages app launches properly following STR in comment 0. 
 
Device: Flame Master (KK, 319mb, full flash)
Build ID: 20150317073344
Gaia: 738987bd80b0ddb4ccf853855388c2627e19dcc1
Gecko: 008b3f65a7e0
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Device: Flame 2.2 (KK, 319mb, full flash)
Build ID: 20150317002504
Gaia: d0e09d5e6367e558824f9cbf691da99cedf63037
Gecko: 793d61bb0bd4
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0