1095818 - [email] app startup by notification goes to message list instead of message reader

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[James Burke [:jrburke]]

STR:

1) Set up account to receive notifications. To enable faster notifications (at the expense of your battery, network and sanity) you can enable a 20 second or 60 second sync interval for testing via the secret debug menu:
https://wiki.mozilla.org/Gaia/Email/SecretDebugMode#Getting_to_the_Secret_Debug_Menu

2) Completely close the email app by removing it from the recent apps UI.

3) Send email to the account, and wait for the sync to happen in the background.

4) When the notification pops up for the 1 message received, tap on it.

Expected:
You should go to the message reader card for that one message.

Actual:
You end up in the message list for the account.

Affects 2.0 and 2.1. Reproduces on flame. Does not affect master, 2.2+.

Hilarious Cause:

app_messages received two mozSetMessageHandler 'notification' callbacks, one for the click and one for the close. These come in the correct order, click first, close second. 

These get queued inside evt.js via emitWhenListener, because there are no listeners yet, mail_app module has not been executed yet. The queues inside evt.js are event ID based, not a globally ordered queue.

mail_app finally starts up, and registers two listeners, in this order:

appMessages.on('notificationClosed', ...);
appMessages.on('notification', ....);

Since notificationClosed was registered first, it processes its queue first. The default action for 'closed' is to show the message list. We do this in case the notification system starts up email for a 'close' event and then email gets shown in the process, we have something to show.

We gate these sorts of notification and activity notifications via the `gateEntry` functionality inside of mail_app. This was to fix double click entries mainly via activities, in bug 1010867.

The gate discards the 'notification' message, and the UI ends up on the message_list.

However, if appMessages.on('notificationClosed', ...) is moved after the other .on call, it all works out.

So that will be the fix. Funny enough, this does not happen on master. I am still investigating the precise reason, but I expect it is because the evt.emit infrastructure now async notifies, and so I expect actually looks for listeners after mail_app has registered listeners, so it works out.

I will still do the fix on master though, just to keep all the code as similar as possible. There will be a separate pull request for 2.1, and that one should be upliftable to 2.0.

Comment 1

[James Burke [:jrburke]]

Note on the master/2.2 behavior: it was not the evt changes. The app gets the notification message, the gets all the way to mail_app before the notification closed event comes in. So it could be either from changes in how the system sends events, maybe with a delay, or the alameda loader using native promises, so perhaps mail_app gets all set up before the next message comes into the app.

So for safety reasons, still useful to do the same code change on master.

Comment 2

[James Burke [:jrburke]]

Attachment: GitHub pull request

Comment 3

[James Burke [:jrburke]]

For reference, the v2.1 pull request, that should also apply cleanly for v2.0 if it is wanted:
https://github.com/mozilla-b2g/gaia/pull/25965

Cannot use the master pull request due to the cards refactor on master.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8519320 [details] [review]
GitHub pull request

(IRC in-joke reference:) That IS one weird trick.  And JS event emitters DO hate it!

Comment 5

[James Burke [:jrburke]]

Merged in master:
https://github.com/mozilla-b2g/gaia/commit/81201e97c502111eddfa506fd934b2a52898c96e

from pull request:
https://github.com/mozilla-b2g/gaia/pull/25964

Comment 6

[James Burke [:jrburke]]

Attachment: v2.1 Gaia pull request

Carrying r=asuth from the master pull request to this pull request. This is the pull request that should be applied to 2.1. 

Master has drifted from 2.1/2.0 due to the card refactoring on trunk, but this change is equivalent, just moving the one listen binding below the other.

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Bug 1001629 introduced handling a notification close event, and that coupled with the gateEntry limit on too many mozMessageHandler events entering email at once, resulted in the behavior seen.

[User impact] if declined:
User will not go directly to the email message when tapping on a notification for a single message, but instead end up on the message list.

[Testing completed]:
On device on 2.1, confirms the fix.

[Risk to taking this patch] (and alternatives if risky):
Extremely low. Just a movement of a listener below the other listener.

[String changes made]:
none

Comment 7

[James Burke [:jrburke]]

Merged in v2.1:
https://github.com/mozilla-b2g/gaia/commit/2866e1de1bcf07ae729fb1a8246fb89568bace6a

from pull request:
https://github.com/mozilla-b2g/gaia/pull/25965

Comment 8

[James Burke [:jrburke]]

Comment on attachment 8520200 [details] [review]
v2.1 Gaia pull request

[v2.0 Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Bug 1001629 introduced handling a notification close event, and that coupled with the gateEntry limit on too many mozMessageHandler events entering email at once, resulted in the behavior seen.

[User impact] if declined:
User will not go directly to the email message when tapping on a notification for a single message, but instead end up on the message list.

Nominating for 2.0 given that a related partner bug has asked for a 2.0? blocking request.

Comment 7 has the commit that was applied to 2.1, and should also apply cleanly to 2.0.

[Testing completed]:
On device on 2.1, confirms the fix.

[Risk to taking this patch] (and alternatives if risky):
Extremely low. Just a movement of a listener below the other listener.

[String changes made]:
none

Comment 9

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Please help to cherry pick to 2.0M.
Thanks!

Comment 11

[Kai-Zhen Li [:seinlin][:kli]]

https://github.com/mozilla-b2g/gaia/commit/87b23fa81c3b59f2ba24b84f7d686f4160d4e7cb

Comment 12

[Alissa（leave from Mozilla）]

This issue has been successfully verified on Flame 2.1, 2.2
Reproducing rate: 0/5
1.Launch email and set Every 5 minutes to check for new message.
2.Send one email to test account.
3.5minutes later,new message notification pop up on top of screen.
4.Tap the notification.
**Email details view dislayed.Tap the notification in drop down list,message details view also be displayed.

Flame 2.1 build:
Gaia-Rev        1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db893274d9a6
Build-ID        20141125001201
Version         34.0

Flame 2.2 new build:
Gaia-Rev        824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/acde07cb4e4d
Build-ID        20141125040209
Version         36.0a1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/8d1e868864c8a8f1e037685f0656d1da70d08c06

Comment 14

[Shine(Leave from Mozilla)]

Attachment: video

This issue has been verified successfully on woodduck 2.0.
Verify steps: 
1.Launch email and set Every 5 minutes to check for new message.
2.Completely close the email app by removing it from the recent apps UI
3.Send one email to test account.
4.5minutes later,new message notification pop up on top of screen.
4.Tap the notification.
**Email details view displayed.
See attachment:  Woodduck_video.MP4
Reproducing rate: 0/3
Woodduck2.0 build:
Gaia-Rev        d742e375aca6dc1bf3a36638000ad7f5338ef457
Gecko-Rev       d049d4ef127844121c9cf14d2e8ca91fd9045fcb
Build-ID        20141126050313
Version         32.0

Comment 15

[Coler]

This issue has been verified successfully on Flame 2.0.
Verify steps: 
1.Launch email and set Every 5 minutes to check for new message.
2.Completely close the email app by removing it from the recent apps UI
3.Send one email to test account.
4.5 minutes later,new message notification pop up on top of screen.
4.Tap the notification.
**Email details view displayed.
See attachment:  Verify_Flame2.0.MP4
Reproducing rate: 0/5

build version:

Gaia-Rev        f76014fd2c7528493b90d759c68ec3070233d094
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/53ff92e647a0
Build-ID        20150107000201
Version         32.0

Comment 16

[Coler]

Attachment: Verify_Flame2.0.MP4