912825 - [email] Message reader appears to duplicate body contents when buildBodyDom is called multiple times

Status: RESOLVED FIXED

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

Description

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

There's a class of errors with duplicated contents/attachments that occur because buildBodyDom populates the page using the available information, in its entirety, every time it is called.  This is mainly a problem in that handleBodyChange (hooked up to onchange) can apparently be called multiple times in the case the back-end 

This is arguably a combination of front-end and back-end issues.

It appears that in a just-reported bug 912808 (that has other issues, conflated, so I'm not using it), an ActiveSync account ran afoul of the fact that ActiveSyncFolderConn.downloadBodyReps is not quite as clever as ImapFolderConn.downloadBodyReps.  The IMAP implementation notices when there is no work to do and returns without triggering a change notification.  The ActiveSync notification will always download the body and always generate a change notification.

I would propose the following solution:

- Stop the back-end from doing the double-fetch.  This can be handled in ActiveSync, and/or we can push the logic currently in IMAP up into jobmixin.js' do_downloadBodyReps implementation.  It's worth noting that our ActiveSync implementation currently only ever produces a single body part, so it lives in a somewhat simpler world than IMAP.

- Declare our contract that we protect the front-end from silly things like this.  This mainly means a unit test where we enqueue a second downloadBodyReps job before the first one has executed, then verify that we only get one set of notifications.  We should add a comment to that effect.

- Change buildBodyDom to be a little bit smarter about bodies.  The 'bodyReps' notification does populate its 'indexes' field so we know what part was downloaded/updated.  We could have the routine due an idempotent thing where it creates one div per expected body part and nukes the contents of the div and replaces them if it hears about an update for that div.

- Change buildBodyDom so it doesn't entirely regenerate the attachments container every time.  My streaming download patch (to come soon) helps clean up our handling of attachments (I think) so we will actually get some events related to this.

Comment 2

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

(propagating koi? from the bug about duplicated ActiveSync attachments to get this in the backlog)

Comment 3

[Dylan Oliver [:doliver]]

Moved into Pivotal backlog.

Comment 4

[Dylan Oliver [:doliver]]

Moved into Pivotal backlog.

Comment 6

[lecky]

Hi Wayne:
   Could you tell me that whether this issue will be fixed on V1.1HD or not?

Comment 7

[Wayne Chang [:wchang]]

It is not part of v1.1 or v1.2 given by the comments above.

Comment 8

[lecky]

Hi
Bug 914007 is caused by this issue, and the issue will not be fixed on V1.1HD.
Could you accept it?

Comment 9

[Beatriz Rodríguez [:brg]]

(In reply to lecky from comment #8)
> Hi
> Bug 914007 is caused by this issue, and the issue will not be fixed on
> V1.1HD.
> Could you accept it?
Yes, please read comment 7.

Comment 10

[Dylan Oliver [:doliver]]

Tagging this bug to put it in the productivity backlog

Comment 11

[Marcus Cavanaugh [:mcav]]

Attachment: gelam.html

The POP3 work addressed the last two bullets in your recommendation for fixing this (by being smarter in the frontend at rerendering the message bodies).

This patch moves the logic for checking to see if the bodyReps are already downloaded (as used in IMAP) up to the do_downloadBodyReps job layer. The unit test checks to ensure that "onchange" only fires when the body has actually changed, and that we therefore only download the body once.

Comment 12

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

Comment on attachment 8346769 [details]
gelam.html

fix looks great.  note I have some minor cleanup requests for the test on the pull request.  I did inspect the logs and everything seems to be doing the right thing, just the modified expectations I think would make the logs a little easier to read and are more likely to detect wacky regressions.

Comment 13

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8346769 [details]
gelam.html

Aside: I know you're on PTO this week. I'm probably going to keep flagging you on things for review, but please don't bother looking at any of this now -- go enjoy your vacation. I don't expect or need you to step away from PTO to look at any of these things, it can all wait until after break. It's just easier to track what's left for me if I mark them as I go.

Anyway, I'm re-setting the review flag because I'd like to have you take one more look at the test fixes to make sure it catches everything you intended. Plus, there's a comment in there about POP3 test assertions that I want to see if it makes sense to you.

Comment 14

[Marcus Cavanaugh [:mcav]]

Merged:

https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/c348db85fa667ab0d370df070d1220887571de3d
https://github.com/mozilla-b2g/gaia/commit/46a03bc40375a8cb1b2f43a4b429085253535de2