1100557 - [email/IMAP] v2.0-specific (/ pre v2.1) closed-over shared-variable hazard may corrupt embedded attachments when there are 2 or more of them

Status: VERIFIED FIXED

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

Description

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

Partner-specific bug 1093442 (product/marketing derived) reported a situation where hitting "show embedded images" for an email with with 2 embedded images would result in one image apparently being shown both times.

== Investigation notes:

Mail info:
- relatedParts (in part order per our data structure, and consistent with the raw rfc2822 which is (related (alternative (plain html)) image002.jpg image0001.jpg):
  - image002.jpg: company logo
  - image001.jpg: interesting picture
- bodyRep: html, with the image001.jpg actually coming first.

Protocol trace:
- 2 Independent fetch requests issued in the expected order image002, image001.  We overlapped them per optimizations we had done.
- The fetch requests did process in the right order

Files saved to device storage:
- image001.jpg ended up as image002.jpg with the actual image001.jpg that concatenated on the end of that.  Not quite what we wanted.

Which quickly led to looking at the implementation and noticing that the "mparser" was closed over outside the "partInfos.forEach" loop which 100% explains the problem.

== The fix:

I've moved the forEach logic up so that everything gets closed over appropriately.  I have not changed the indentation since this makes the diff much easier to process.  I also used vim this time for the purposes of whitespace sanity.  Patch momentarily once I have a bug number.

Comment 1

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

Attachment: v2.0-specific fix: move partInfos up so we close over all the things

:mcav, since you made the problem go away originally, I think you are probably the logical reviewer for this.  I have tested on v2.0 b2g-desktop with the 2nd set of 163.com credentials listed at https://intranet.mozilla.org/Gaia/Email and scrolled down to the "3333" email that was the original STR in the private bug.

Comment 3

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

Comment on attachment 8524076 [details] [review]
v2.0-specific fix: move partInfos up so we close over all the things

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Unclear what the permutation of code race / regression / etc. is.  The code is clearly wrong though, so it's not particularly worth looking into.
[User impact] if declined:  Users who have messages with more than one embedded image (an image that is included with the message for display in the message rather than as a separately downloadable attachment) may find that only one of the images looks right and all the other images look identical to that first image.
[Testing completed]: Tested on b2g-desktop with v2.0 on the 163.com account provided by the bug reporter on the private bug by me, asuth.  More significantly, the code flaw is obvious by inspection and the fix is also obvious by inspection. 
[Risk to taking this patch] (and alternatives if risky):  No risk.  The control flow is the same, there's just no clobbering of variables.
[String changes made]: No string changes made.

Comment 4

[bhavana bajaj [:bajaj]]

Brian, can QA help apply this patch on 2.0 codebase and verify before we uplift. Given this is a branch specific landing, it will get me more confidence to approve once verified.

Comment 5

[Brian Huang [:brianhuang][:brhuang]]

William, please help on this. Thx.

Comment 6

[William Hsu [:whsu]]

(In reply to Brian Huang [:brianhuang][:brhuang] from comment #5)
> William, please help on this. Thx.

Hi, Brian,

I will verify it as soon as possible after I go back to office.
Thanks.

Comment 7

[bhavana bajaj [:bajaj]]

(In reply to William Hsu [:whsu] from comment #6)
> (In reply to Brian Huang [:brianhuang][:brhuang] from comment #5)
> > William, please help on this. Thx.
> 
> Hi, Brian,
> 
> I will verify it as soon as possible after I go back to office.
> Thanks.

William, any update on this ?

Comment 8

[William Hsu [:whsu]]

Hi, Andrew, Bhavana, and Brian,

Sorry to keep you waiting since I need time to check the code and side effect.

After I verified this patch on private (local) branch, I don't find any side effect.
Attach the screenshot - "Bug_1100557_Verification.png"

But, I find another bug (Bug 1109503). FYI.
Thanks.

* Tested case:
  1. Download attached images and signature
  2. Volume test (30 images)
  3. Test between different email accounts

* Build information:
 - Gaia-Rev        054b9d4351a6e7c188bb4c87dee2f13746fd5ae0 (Local Patch)
 - Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2d0860bd0225
 - Build-ID        20141209160202
 - Version         32.0
 - Device-Name     flame
 - FW-Release      4.4.2
 - FW-Incremental  eng.cltbld.20141209.192800
 - FW-Date         Tue Dec  9 19:28:11 EST 2014
 - Bootloader      L1TC00011880

Comment 9

[William Hsu [:whsu]]

Attachment: Bug_1100557_Verification.png

Comment 11

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

Setting a needinfo flag to make sure this approval request has visibility since the approval request has been stalled for a while and this bug just got a dupe filed.

Comment 12

[Norry.L.F(Leave from Mozilla)]

According to a duplicated bug 1118153, this bug can be repro on woodduck 2.0M.

Comment 13

[Josh Cheng [:josh]]

Hi Bhavana,
I am making this as 2.0M+ since partner also reported this issue in bug 1118153. Please feel free to make it 2.0+ or stay in 2.0M+ upon your decision. Thanks!

Comment 14

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

landed on gaia/2.0:
https://github.com/mozilla-b2g/gaia/pull/26201
https://github.com/mozilla-b2g/gaia/commit/c59f021a082ec34fdad5746f11231a47501bba4e

This should make it way to the 2.0m branch via the regular merges described at https://wiki.mozilla.org/Release_Management/B2G_Landing#v2.0M

Comment 15

[Shally Li（leave from Mozilla）]

This bug has been successfully verified on Woodduck v2.0 and Flame v2.0.
See attachment: verified_v2.0&2.0m.mp4
Reproduce rate: 0/5.

STR：
1.Sign in a IMAP email account on device.
2.Send a mail from Outlook(PC) to device with signature and two embedded images as mail content.
3.Receive the mail and open it on device.
4.Tap on "Click to download" at the bottom.
**It displays all images normally on mail content page.

Flame 2.0 build:
Gaia-Rev        31d6c9422cd0a8213df9f96019c9ab7168ec3ab3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a05a5378cb1f
Build-ID        20150111000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150111.034429
FW-Date         Sun Jan 11 03:44:40 EST 2015
Bootloader      L1TC000118D0

Woodduck 2.0 build:
aia-Rev        7e55f20bc0f82397207cac6b5b477948652da62c
Gecko-Rev       5e2c8611f0705e5ee656e3bfad8e8ee9bde228fe
Build-ID        20150112050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2
FW-Incremental  1421010380
FW-Date         Mon Jan 12 05:06:48 CST 2015

Comment 16

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.0&2.0m.MP4

Comment 17

[Sue]

Accroding to comment 15, this bug had been verified, change the Status to VERIFIED.