1068116 - [email/POP3] email.js POP3 attachment regressions: binary attachments are corrupted on download and partially downloaded attachments are saved to disk

Status: VERIFIED FIXED

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

Description

[Seth Miko [:SMiko]]

Attachment: pop3.txt

Description: An error message is received when a pop3 email account attempts to open or view an attachment.

Repro Steps:
1: Update a Flame to 20140915000203
2: Create a POP3 Account using the following credentials:

Username: [e-mail] -- Password: firefoxos
POP3 Settings -- Username: ffos.qa -- Hostname: pop.aol.com -- SSL = 995 (default)
SMTP Settings -- Username: ffos.qa -- Hostname: smtp.aol.com -- SSL = 465 (default)

4: Send the POP3 account an email with a picture attached.
5: Receive the email and attempt to view the picture

Actual: The following error message is displayed, "Image is invalid or corrupt and cannot be displayed."

Expected: The user can view the image

Environmental Variables:

Flame 2.1(319mb)

Environmental Variables:
Device: Flame 2.1 (319mb)
Build ID: 20140915000203
Gaia: 944e5b4582c4efa1b67cd33245dbb8f6aa25d09f
Gecko: 7546fedad918
Version: 34.0a2 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Notes:
1: File types tested: .txt, .jpg, .png
2: Possible regression of bug 1022036

Repro frequency: 100%

Link to failed test case: https://moztrap.mozilla.org/manage/case/10875/

See attached: logcat

Video clip: http://youtu.be/TF1vtFBO5KM

Comment 1

[Seth Miko [:SMiko]]

Please disregard the above version in the environmental variables. 
2.1 is not Master. The correct version is 34.0a2

Comment 2

[Seth Miko [:SMiko]]

This issue DOES repro on Flame 2.2(319mb), Open C 2.2, Flame 2.1(512mb), and Open C 2.1

Flame 2.2 (319mb)

Environmental Variables:
Device: Flame Master
Build ID: 20140915040203
Gaia: 855be6ade407c26e0596e7306a44deebc3f60933
Gecko: f27ff178807d
Version: 35.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Open C 2.2

Environmental Variables:
Device: Open_C Master
BuildID: 20140915040203
Gaia: 855be6ade407c26e0596e7306a44deebc3f60933
Gecko: f27ff178807d
Version: 35.0a1 (Master)
Firmware: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1 (512 mb)

Environmental Variables:
Device: Flame 2.1
Build ID: 20140915000203
Gaia: 944e5b4582c4efa1b67cd33245dbb8f6aa25d09f
Gecko: 7546fedad918
Version:34.0a2 (2.1)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Open C 2.1

Environmental Variables:
Device: Open_C 2.1
BuildID: 20140915000203
Gaia: 944e5b4582c4efa1b67cd33245dbb8f6aa25d09f
Gecko: 7546fedad918
Version: 34.0a2 (2.1)
Firmware: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

This issue does NOT repro on Flame 2.0(319mb/KK base), Flame 2.0(319mb/JB base), or Open C 2.0

Flame 2.0 KitKat Base (319mb)

Enviromental Variables:
Device: Flame 2.0 (319mb)
BuildID: 20140914183002
Gaia: 7edd3b0b9f65c3dde235c732d270e43e055a1254
Gecko: 13e04ab68621
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Flame 2.0 JellyBean Base (319mb)

Environmental Variables:
Device: Flame 2.0(319mb)
Build ID: 20140915000202
Gaia:7edd3b0b9f65c3dde235c732d270e43e055a1254
Gecko: 13e04ab68621
Version: 32.0 (2.0)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile;rv:32.0) Gecko/32.0 Firefox/32.0

Open_C 2.0

Enviromental Variables:
Device: Open_C 2.0
BuildID: 20140915000202
Gaia: 7edd3b0b9f65c3dde235c732d270e43e055a1254
Gecko: 13e04ab68621
Version: 32.0 (2.0)
Firmware: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 3

[Seth Miko [:SMiko]]

This issue DOES repro on Flame 2.2(319mb), Open C 2.2, Flame 2.1(512mb), and Open C 2.1
Actual result: The user receives an error message when attempting to view attachments.

This issue does NOT repro on Flame 2.0(319mb/KK base), Flame 2.0(319mb/JB base), or Open C 2.0
Actual result: The user is able to view attachments.

Comment 4

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

[Blocking Requested - why for this release]:

This is a regression from 2.0 so nominating this 2.1? The user should be able to view attachments in their emails without issue.

Comment 5

[Candice Serran (:cserran)]

regression. incorrect functionality, must fix

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20140828202701
Gaia: c0fa9fe2f91ea76191d4e7c58db1dbf2993f76c8
Gecko: 9e1d80308ac1
Version: 34.0a1 (2.1 Master)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20140828211200
Gaia: 8d965e7182500fd1849e8eec5ae2aca35a55af22
Gecko: efb4f3f291a4
Version: 34.0a1 (2.1 Master)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First broken gaia & Last working gecko - issue DOES repro
Gaia: 8d965e7182500fd1849e8eec5ae2aca35a55af22
Gecko: 9e1d80308ac1

First broken gecko & Last working gaia - issue does NOT repro
Gaia: c0fa9fe2f91ea76191d4e7c58db1dbf2993f76c8
Gecko: efb4f3f291a4

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/c0fa9fe2f91ea76191d4e7c58db1dbf2993f76c8...8d965e7182500fd1849e8eec5ae2aca35a55af22

Caused by Bug 885110.

Comment 7

[Joshua Mitchell (Inactive)]

Broken by Bug 885110 - Marcus, can you take a look?

Comment 8

[Marcus Cavanaugh [:mcav]]

Andrew has graciously volunteered to take a look at this one.

Comment 9

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

So an intersection of bad things happened here:

- POP3 had no downloaded contents verification coverage because test_compose.js and test_compose_blob.js were our only coverage for downloads in general.  This was bad news for POP3 because test_compose.js only verified the attachments in the 'sent' folder (not the inbox) and for POP3 we don't actually keep the attachments around in sent so the test explicitly bailed.  And test_compose_blob.js is only for IMAP.

- POP3's parseMime implementation was utf-8 decoding the Uint8Array contents into a string in all cases.  That's right for body parts, but not right for relatedParts/attachments.  This was corrupting the images and any other attachment that had characters with values >= 128 with high probability.  (Obviously, valid utf-8 or bits that looked like valid utf-8 would get passed through.)

- The partial node logic was broken because the 'onbody' hack was not actually finding the lastNode correctly.  This would result in us saving a lot of 0-byte image files to disk, I believe because the node would not be closed out so the content would be null and so our base-case 0-byte file would be created.

Patch going up (once I submit my updated bug subject) with the following fixes/changes:

- test_compose now verifies the downloaded attachment in both the 'sent' folder and the 'inbox'.  This did result in some rearrangement of logic in the file that looks more extreme than it is.  There are, of course, also various horrible parametrizations relating to POP3

- a cascading result of that is we now also validate our filename collision logic on IMAP since we'll download a file with the same name twice.

- the backend now makes its bodyRepsDownloaded determination for POP3 by also checking whether all known attachments have been downloaded.  This better conforms to our POP3 invariants and makes getBody({ withBodyReps: true }) both do what tests want and avoid horrible edge-cases where we'd do the wrong thing if the snippet had downloaded all the known body parts already.

- lastBodyNode / partial nodes computed correctly

- we don't try and utf-8 decode binary attachments

Comment 10

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

Attachment: GELAM patch and tests

Uh, and I tested this on a trunk flame device.  Sad before, happy afterwards.  (Nuking the /sdcard contents between and nefariously bumping the db revision so I wouldn't have to redo manual pop3 config; so much typing for manual config!)

Comment 11

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8497588 [details] [review]
GELAM patch and tests

Looks solid to me, thanks for handling this.

Comment 12

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

Note that Travis is experiencing deterministic failures on some activesync tests.  I have isolated this to a platform regression in the POSTing of IndexedDB-backed blobs which is why it (only) affects ActiveSync mail sending.  I filed bug 1075302 on that.

Since this is POP3 and orthogonal to this, I'll probably go ahead and land this a little later tonight.  I do have some additional debugging support I came up with while looking into the ActiveSync failure but I think it likely makes sense to do that on a separate bug, possibly as a v2.2-only thing.

Comment 13

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

landed on gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/342
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/3903ef170059f987c50b3725efd0474516471c3e

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/24593
https://github.com/mozilla-b2g/gaia/commit/a23d2c490b39c4699c9375e25c4acdf396a2fa85

Comment 14

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

Attachment: v2.2 gaia/master landing of the GELAM changes

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): This was fallout from the bug  	885110 refactoring/switch to email.js where we had insufficient test coverage (technical debt from v1.3 time pressure landings) but it wasn't obvious because of POP3 implementation nuances and the test being a little dense with conditionals.  Backing out that bug is not technically possible or desirable at this time.
[User impact] if declined: Binary attachments will be corrupt for POP3 and a bunch of zero-length attachments will be created.
[Testing completed]: Tested on v2.2 flame device, new email backend coverage added that detects the failures (without the fix present) and passes (with the fix present).  The new test coverage is a little cleaner (we still have some test refactoring we'd like to do), but definitely has the needed coverage.
[Risk to taking this patch]: Extremely low risk given the improved test coverage.  No alternatives other than melodramatic things like disabling POP3 support wholesale.
[String changes made]: No string changes.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/9b7f66999769ae5fd4d92b0e5932ed6a867f24e9

Comment 16

[Edward Chen[:edchen]]

[Environment]
Gaia-Rev        778ebac47554e1c4b7e9a952d73e850f58123914
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/aa98eb8873a2
Build-ID        20141005160206
Version         34.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20140925.192608
FW-Date         Thu Sep 25 19:26:18 EDT 2014
Bootloader      L1TC10011800

[Result]
PASS

Comment 17

[Shally Li（leave from Mozilla）]

Hi,
    This bug has been verified successfully on Flame v2.1.
    Attch:jpg_1.png,jpg_2.png,png_1.png and png_2.png.

Flame 2.1 build:
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
Build-ID        20141123001201
Version         34.0

Comment 18

[Shally Li（leave from Mozilla）]

Attachment: jpg_1.png

Comment 19

[Shally Li（leave from Mozilla）]

Attachment: jpg_2.png

Comment 20

[Shally Li（leave from Mozilla）]

Attachment: png_1.png

Comment 21

[Shally Li（leave from Mozilla）]

Attachment: png_2.png

Comment 22

[Shally Li（leave from Mozilla）]

This issue has been successfully verified on Flame 2.2.
See attachment: verified_v2.1.MP4.
Reproducing rate: 0/5

Flame 2.2 build:
Gaia-Rev        824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/acde07cb4e4d
Build-ID        20141125040209
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141125.113029
FW-Date         Tue Nov 25 11:30:43 EST 2014
Bootloader      L1TC00011880