945606 - [email/back] regression in snippet creation for multi-bodypart messages on IMAP, POP3 possibly affected too

Status: RESOLVED FIXED

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

Description

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

Message on dev-gaia with a glodebug (see http://blog.xulforum.org/index.php?post/2011/03/14/Basic-MimeMessage-demo) that looks like this:

===
			/---------------------------\
			|       Mime  results       |
			\---------------------------/

Size of the message: 814004
Structure of the message:
Message (814004 bytes): [b2g] 12/03/2013 TWCI Hamachi Gaia-ui Automation v1.3.0 Mozilla RIL Build - 120/123 tests
  1 Container (814004 bytes): multipart/mixed
    1 Body: text/plain (1390 bytes)
    2 Attachment (812473 bytes): 2013-12-03-twci-hamachi-mc.html, text/html
    3 Body: text/plain (141 bytes)
Number of attachments: 1
This message is from: Askeing Yen <[e-mail]>
===

We end up with no snippet.  I think the source of badness looks like this:
- While processing part 1.3, our call to imapchew.updateMessageWithFetch from ImapFolderConn._lazyDownloadBodyReps sets createSnippet to false in its 'request' object.
- updateMessageWithFetch calls mailchew.processMessageContent with this value set to false, so the returned 'data' object has no snippet.
- updateMessageFetch unilaterally sets header.snippet.

I believe this is a regression from POP3.  Specifically, I remember saying that we probably always want to update the snippet, but I fear I missed this nuance.

We should probably briefly revisit the review feedback to double-check what I was saying there, then add a GELAM unit test that duplicates the failure case, then fix.

I suspect we want to go back to using req.createSnippet to decide whether to assign to header.snippet and just want to make sure that the POP3 path is similarly only requesting a snippet from the body-part identified by $imapchew.selectSnippetBodyRep, just like IMAP is doing.

Comment 1

[Marcus Cavanaugh [:mcav]]

Attachment: pull.html

I remember the following comment about always updating bytesToDownloadForBodyDisplay, but it's not quite the same thing:

https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/258#discussion-diff-7646949R187

You're right that it was caused by POP3 (via `git bisect`), but I'm not sure what specifically caused the regression. The line `header.snippet = data.snippet` was unguarded even before the POP3 patch, so it must be an indirect change that caused it. Seems like a side effect maybe.

I couldn't find a comment in the review about always updating the snippet in this affected portion specifically, but it's possible you said that on IRC and I missed it.

In any case, the test case I added failed without the patch, succeeds with it, and succeeded before the POP3 patch.

Comment 2

[Jason Smith [:jsmith]]

Does this need to be 1.3? if this is a POP3 regression?

Comment 3

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

(In reply to Jason Smith [:jsmith] from comment #2)
> Does this need to be 1.3? if this is a POP3 regression?

Yes, this needs to land in 1.3 since POP3 is 1.3.

Comment 4

[Marcus Cavanaugh [:mcav]]

Landed:

https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/fc6ed79cd3169420cb6f83c1d0622623028c0848
https://github.com/mozilla-b2g/gaia/commit/ececa25fbcfce0f36af9acf328bd6d602da085ed

Comment 5

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

(landed in 1.3 as non-feature bugfix prior to branching, no need to triage further)