1124393 - [Email] Unread E-mail count does not properly count down when reading unread messages

Status: VERIFIED FIXED

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

Description

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Attachment: Email Counter Logcat

Description:
The Unread E-mail counter will go down by 2 when the user reads an unread E-mail. Once the user gets the counter below 0, the counter will go into the negatives, but the counter will appear as though it has disappeared. This issue persists even after force closing the app and re opening.

Prerequisites: Have an email account with at least 10 unread emails

Repro Steps:
1) Update a Flame to 20150121010204
2) Sign into a valid Email account (ex: Gmail, or Outlook account)
3) Observe unread Email counter next to the "Inbox" Header
4) Select an unread Email, then go back to Inbox
5) Select another unread Email, then go back  to Inbox


Actual:
Now every time an Unread Email is selected, the unread counter at the top goes down by 2.


Expected:
The Unread Email counter will go down by 1 for every Email that is read.

Environmental Variables:
Device: Flame 3.0
Build ID: 20150121010204
Gaia: 5e98dc164b17fd6decb48a9eaddef0e55b82e249
Gecko: 540077a30866
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Repro frequency: 15/15 100%
See attached: Logcat, Video - https://www.youtube.com/watch?edit=vd&v=UHYgpGkRYrI

Comment 1

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

The tests done on Flame 3.0 in comment 0 were done with 319MB Memory, Kitkat Base, and the device was updated by a Full Flash

This issue DOES occur on Flame 2.2

The Unread Email counter will go down by 2 after viewing an unread Email

Device: Flame 2.2 (319MB)(Kitkat)(Full Flash)
BuildID: 20150121002607
Gaia: e4f9b5da3751798f9cc5d95f302c30722cc11fca
Gecko: 75a462a58d7a
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


This issue does NOT occur on Flame 2.1

The Unread Email counter will go down by 1 after viewing an unread Email

Device: Flame 2.1
BuildID: 20150121001510
Gaia: 77c57eb8a985d5cbd34a597fb1b978ba6e205af6
Gecko: 4c28bb3be0c6
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Functional regression of a core feature can lead to a broken state of counter.

Requesting a window.

Comment 3

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

So the underlying cause of the bad math is bug 1107749.  (No matter what the front-end does, it shouldn't be able to mess up the unread count like that.)  But it looks like (from the log) the message reader is calling setRead(true) multiple times, which is triggering that bug more than it should.

While, again, the back-end shouldn't be screwing up like this, it suggests that the message_reader may be inefficiently be rebuilding the UI state too much.  Specifically, setRead(true) is likely to be happening because onCurrentMessage is calling _setHeader, at which point we'll also be calling clearDom() and buildHeaderDom and rebuilding the body and such.  Besides perhaps increasing the logging in message_reader to help figure out what's going on, it probably wouldn't be unreasonable for onCurrentMessage to an idempotent fast-bail if it's being told about the same message it already knows about (and it wasn't doing _afterInDomMessage).

Also, I know we had some discussion about hackMutationHeader in the last bug touching this code area (kgrandon's mark unread button?) which I'm not immediately recalling 100%, but going on my current code-reading, we really should have a comment at hackMutationHeader's instantiation that explains WTF.  (I think we maybe decided we didn't need it but kept it around to avoid having to make a bigger change?)

I'll fix the mathy bug, but let's leave this around.

Comment 4

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

b2g-inbound Regression Window:

Last Working Environmental Variables:
Device: Flame 2.2
BuildID: 20141102105539
Gaia: 8a13593f860f863c2c9f9c1a860a39971657367c
Gecko: dc0694997f61
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 36.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame 2.2
BuildID: 20141102115338
Gaia: 7fc32b16fabbe0ec33135f8864066cbde702f1c0
Gecko: cd1e1d3d9179
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 36.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Last Working Gaia First Broken Gecko: Issue does NOT reproduce 
Gaia: 8a13593f860f863c2c9f9c1a860a39971657367c
Gecko: cd1e1d3d9179

First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: 7fc32b16fabbe0ec33135f8864066cbde702f1c0 
Gecko: dc0694997f61

https://github.com/mozilla-b2g/gaia/compare/8a13593f860f863c2c9f9c1a860a39971657367c...7fc32b16fabbe0ec33135f8864066cbde702f1c0

This may have been caused by Bug 1005446

Comment 5

[Peter Bylenga [:PBylenga]]

NI on James Burke, although Andrew is already working on a fix in Comment 3.  Can you still take a look since it appears to have been caused by the patch landing in bug 1005446?

Comment 6

[James Burke [:jrburke]]

The front end ends up calling setRead twice in message_reader because emit() in evt.js as of bug 1005446 now async notifies listeners. Before that change, the sequence was

* header_cursor synchronously emits of currentMessage. Only message_list is listening at this point, message_reader does not exist yet.
* message_reader is created. As part of that it asks header_cursor.latest('currentMessage'). This calls setRead.

After the evt.js change:

* header_cursor async emits of currentMessage. No card is notified yet.
* message_reader is created. As part of that it asks header_cursor.latest('currentMessage'). This calls setRead.
* The emit in header_cursor finally contacts is listeners. Now both message_list and message_reader are listening. message_reader does its onCurrentMessage work again, which calls setRead again.

So while :asuth says the backend should be (and now is on master) robust to these multiple calls, message_reader can also avoid doing extra work if onCurrentMessage is called more than once with the same header. So I create a pull request to do that.

Longer term, I want to move evt.js back to sync emit()s to match other event emitters better, but that is likely a 3.0 thing, would be a harder thing to uplift. Plus the robustness change for message_reader should still be there just for defensive/perf reasons.

Comment 7

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

(I think a midair conflict caused me to mistakenly not add bug 1107749 as a dep in comment 3.)

Comment 8

[James Burke [:jrburke]]

Attachment: GitHub pull request

Comment 9

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

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

Yup yup yup!  (https://www.youtube.com/watch?v=cAEVzJnHv2c)

Comment 10

[James Burke [:jrburke]]

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

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

Comment 11

[James Burke [:jrburke]]

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

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Bug 1005446. See comment 6 in this bug for more details.

[User impact] if declined:
The unread count in the message list header for the folder could be incorrect, confusing.

[Testing completed]:
Tested on flame device.

[Risk to taking this patch] (and alternatives if risky):
Very low. It just avoids doing work if the message reader is already showing the current message.

[String changes made]:
none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Cola, OOO till Oct. 8th]

Attachment: Verify1_Flame_v2.2.3gp

This bug has been verified as pass on latest build of Flame v2.2 & master and Nexus5 v2.2 & master by the STR in Comment 0.

Actual results: The Unread Email counter goes down by 1 for every Email that is read.

See attachment:Verify1_Flame_v2.2.3gp

Reproduce rate: 0/10

Device: Flame 2.2 build(Pass)
Build ID               20150714162501
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a5db6d9850f6
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150714.200955
Firmware Date          Tue Jul 14 20:10:07 EDT 2015
Bootloader             L1TC000118D0

Device: Flame master build(Pass)
Build ID               20150714160203
Gaia Revision          803d04e3829fd4fe9261211aa0ddca6b79d4e328
Gaia Date              2015-07-14 17:54:44
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d5025c151d17
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150714.193145
Firmware Date          Tue Jul 14 19:31:57 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus5 2.2 build(Pass)
Build ID               20150714002501
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a5db6d9850f6
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150714.042449
Firmware Date          Tue Jul 14 04:25:08 EDT 2015
Bootloader             HHZ12f

Device: Nexus5 master build(Pass)
Build ID               20150714160203
Gaia Revision          803d04e3829fd4fe9261211aa0ddca6b79d4e328
Gaia Date              2015-07-14 17:54:44
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d5025c151d17
Gecko Version          42.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150714.194243
Firmware Date          Tue Jul 14 19:43:01 EDT 2015
Bootloader             HHZ12f