996232 - [B2G][Email] Downloading large attachments will result in the e-mail app being killed

Status: VERIFIED FIXED

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

Description

[mkruml]

Attachment: music.txt

Description:
If a user's email account was sent a mp3 audio file, the user will not be given a warning when they open the attachment via the Tarako Email app. Instead, the file will appear to download, and the email app with force close.
This also repros if the account is sent a h.264 mp4 video file.


Repro Steps:
1) Update a tarako to BuildID: 20140414004001
2) Open the email app and add an account
3) Compose a new email, and attach one of the files found in the Music app to the email, then send it to the test account.
4) Open the email and select the download link for the attachment

Actual:
After a time, the email app will force close

Expected:
The file will either download and be available to play, or an error will appear stating the inability to do so.

1.3t Environmental Variables:
Device: tarako 1.3t MOZ
BuildID: 20140414004001
Gaia: 0e7f21e61625b75a9149480cd5a259211549f020
Gecko: 3b02811c314a
Version: 28.1
Firmware Version: sp6821a


Notes: Test account used: [e-mail] & [e-mail]
This does not occur in Buri 1.3
Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/6561/
See attached: logcat = music.txt

Comment 1

[mkruml]

Attachment: Firewatch log

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Looks like it got killed by the system.

04-14 14:14:06.470: I/Gecko(83): [Parent 83] WARNING: waitpid failed pid:738 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
04-14 14:14:06.470: I/Gecko(83): [Parent 83] WARNING: Failed to deliver SIGKILL to 738!(3).: file ../../../gecko/ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118

Comment 3

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

As Naoki indicates, e-mail got killed.  I think this log line says it better though:
  04-14 14:14:03.540: I/log(818): <4>0[ 309.868877] lowmem_shrink select 738 (E-Mail), adj 2, size 14606, to kill

What's interesting is that our download successfully completed:
  04-14 14:14:01.360: I/Gecko(738): WLOG: Downloaded 3148030 bytes of attachment data.

And we had handed things off to DeviceStorage to save:
  04-14 14:14:02.020: I/GeckoDump(738): DeviceStorage: process save

But we did not complete; if we had, we would have had a log that looked like "WLOG: saved attachment to sdcard blahblah.mp3 type: audio/mp3".

I do see the homescreen had already died for us, so this may be a question of the file just being too big for what we can handle low-hanging fruit-wise.

The cited file was basically exactly 3 MiB.  This seems like it might be an argument for us simply capping downloads on Tarako to files no larger than 1 MiB.


Note that bug 991567 already improved the attachment download situation significantly.  Marking 1.3T? mainly so we can consider just capping the downloads at 1 meg unless :mcav has some thoughts on how to easily improve things further.  (Stitching together multiple DeviceStorage-stored files is unattractive because of the garbage problem if we are killed before we stitch the parts together and delete them in favor of the super file.)

Comment 4

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

Reporter, can you clarify if the screen turned off or anything like that?  If it's a question of holding the screen and cpu wake-locks, that may be the lowest hanging fruit for us.  (Although that's also the best way for us to cause system problems if we screw it up.)

Comment 5

[mkruml]

Attachment: Email App force close

Screen does not appear to turn off. See attached video.

Comment 6

[Marvin Khoo [:Marvin_Khoo]]

May i know when downloading the file, do we keep it all in RAM until download completed? or when exceed download buffer we moved downloaded content to flash?

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

(In reply to Andrew Sutherland (:asuth) from comment #3)
> 
> Note that bug 991567 already improved the attachment download situation
> significantly.  Marking 1.3T? mainly so we can consider just capping the
> downloads at 1 meg unless :mcav has some thoughts on how to easily improve
> things further.  (Stitching together multiple DeviceStorage-stored files is
> unattractive because of the garbage problem if we are killed before we
> stitch the parts together and delete them in favor of the super file.)

triage: either we need to be able to download or block download to avoid crashing. 1.3T+
can you provide more information on capping @ 1MB? do you mean any attachment greater than 1MB will not have the option for users to downlaod? thanks

Comment 8

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

(In reply to Marvin Khoo [:Marvin_Khoo] from comment #6)
> May i know when downloading the file, do we keep it all in RAM until
> download completed? or when exceed download buffer we moved downloaded
> content to flash?

Yes, we keep it in RAM since there is no way to stream attachments to disk.  Bug 943798 is our e-mail bug on doing that, but is blocked on either IndexedDB or DeviceStorage becoming capable of streaming.  There are potential workarounds discussed there, but they are inadvisable in general and almost inconceivable as something to be backported to the 1.3 series.

(In reply to Joe Cheng [:jcheng] from comment #7)
> can you provide more information on capping @ 1MB? do you mean any
> attachment greater than 1MB will not have the option for users to downlaod?

Well, there are 2 main UX options here:
- Just not display the download button if the file is above 1MiB / a specific size.  This may confuse and infuriate the users.
- Display a message like "The file is too big to download on this device." when the user hits the download button.  This is less confusing but probably more infuriating.  It also adds a new string.  Not sure where we are on 1.3T strings.

Of course, infuriating the user before we waste their time/bandwidth and then crash is probably a big win either way.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The fix for the underlying memory issue is bug 943798, so we're not going to track wallpapering over the symptoms here.

Comment 10

[Marcus Cavanaugh [:mcav]]

(In reply to Andrew Sutherland (:asuth) from comment #8)
> - Display a message like "The file is too big to download on this device."
> when the user hits the download button. 

Worth noting that the Gallery app presents a similar message when an image is too large to be viewed on the device: "Image is too large to display on this device." So at least there's precedent there.

Comment 11

[Evan Tseng [:evanxd]]

> - Display a message like "The file is too big to download on this device."
> when the user hits the download button.  This is less confusing but probably
> more infuriating.  It also adds a new string.  Not sure where we are on 1.3T
> strings.

Hi Andrew,

It is good idea.
But after email is received in our Email app and assume that there is no the backup email in server. How could user get the attachments in the email? Forward the email?

Could you have time to help here?
Thanks.

Comment 12

[Evan Tseng [:evanxd]]

Hi Juwei,
How do you think about the UX designs at Comment 8?

Comment 13

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

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #11)
> But after email is received in our Email app and assume that there is no the
> backup email in server. How could user get the attachments in the email?
> Forward the email?

There's no in-app workarounds.  We don't support forwarding attachments right now; even if we did, in order to avoid the download memory-size problem we'd need support for CATENATE on IMAP, we might be able to do smart-reply on ActiveSync, POP3 has no server-side assistance.

The best option for the user would be if they also have a webmail UI to access their e-mail.  In that case maybe the browser app's download-manager could do us one better assuming it uses the native/C++ support for asynchronously streaming to DeviceStorage.

Comment 14

[Juwei Huang[:juwei]]

The idea what Andrew proposed seems like a good idea, I prefer the first idea with minor changes:
1. Disable the download button
2. A error description would be displayed right below the attachment, like the UI below:
https://www.dropbox.com/s/0nqw9szam6bqfry/bug996232.pdf
"The file is too large to download"

In this case, the download button is disable so that the user cannot tap to download the file.
Let me know if there's any other concern:)

Comment 15

[Joe Cheng [:jcheng] (please needinfo)]

ni? Andrew, do you mind taking the bug? thanks 

this will introduce late l10n, ni? Axel

Comment 16

[Mason Chang [Inactive] [:mchang]]

Triage: Dilan, can you find an assignee for this please?

Comment 17

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18387

This patch applies to 1.3t, as it sounded like this would be a 1.3t-specific restriction. Screenshot attached below, Juwei.

Comment 18

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18426

This is a screenshot from the Peak showing one attachment being too large, and the other as usual.

Comment 19

[Juwei Huang[:juwei]]

Hi Marcus! Thank you for having the screenshot to make review easier :)
Could you grey-out the download button and make it untappable rather than disappear the button?
Thanks!!

Comment 20

[Marcus Cavanaugh [:mcav]]

Just to confirm (since you're here) -- right now, if the attachment is undownloadable for another reason, such as an unsupported filetype, we currently do not show the button. Do you want this to be a special case?

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Marcus Cavanaugh [:mcav] <[e-mail]> from comment #17)
> This patch applies to 1.3t, as it sounded like this would be a 1.3t-specific
> restriction. Screenshot attached below, Juwei.

Please patch master and make it configurable as a build time option. You don't want to revisit this in the future.

Comment 22

[Juwei Huang[:juwei]]

Hi Marcus, please make it as a special case now. We'll modify both unsupported filetype & oversize file to have disable button later in the future.

Comment 23

[Marcus Cavanaugh [:mcav]]

Cool, thanks for the clarification.

Comment 24

[Marvin Khoo [:Marvin_Khoo]]

just curious, with this solution, what happen if i'm using POP3?

e.g: A mail (with Attached more than 1MB) received on phone and it's deleted from server, i can't download it, and i can't even find it from server.

Comment 25

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

(In reply to Marvin Khoo [:Marvin_Khoo] from comment #24)
> just curious, with this solution, what happen if i'm using POP3?
> 
> e.g: A mail (with Attached more than 1MB) received on phone and it's deleted
> from server, i can't download it, and i can't even find it from server.

Due to protocol limitations (POP3 is horrible), POP3 messages are downloaded in their entirety when you go to display them, this brings the attachments along with them.  As a result, there is never any download UI for attachments on POP3 messages.

This fix does not address POP3 which will continue to crash if a message, including its attachments, are too large.  Note that our POP3 support already includes a mechanism that warns you if a message exceeds 1 megabyte in size.  It just currently happens if you select 'yes', there's a good chance the app will crash.

The mitigation for POP3 has two real options here:
1) Just refuse to download the message at all.

2) Download only the first megabyte (or whatever arbitrary constant we decide), similar to how we process things for the initial snippet fetch.  This adds some new edge-cases we previously avoided since it becomes possible for us to have:
- no downloaded user-displayable body-parts
- partially downloaded body-parts
- partially-downloaded attachments

We can deal with the edge-cases more or less, although some may require UX.  For example, we may need to:
- Have a string to indicate there was no usable body-part in the portion of the message we downloaded.
- Indicate that we only partially downloaded an attachment and so it's not usable.

I would suggest we decouple such a fix from this fix.

Comment 26

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #21)
> (In reply to Marcus Cavanaugh [:mcav] <[e-mail]> from comment #17)
> > This patch applies to 1.3t, as it sounded like this would be a 1.3t-specific
> > restriction. Screenshot attached below, Juwei.
> 
> Please patch master and make it configurable as a build time option. You
> don't want to revisit this in the future.

I had trouble finding docs on this, but it looks like we basically want to do something along the lines of what calendar does at:
https://github.com/mozilla-b2g/gaia/blob/master/apps/calendar/build/build.js

But we would integrate it with our r.js optimization so that the code gets properly inlined.

However, judging from the history of that file, there have been a tremendous number of changes to the build infrastructure in trunk that are not present on v1.3T, which seems troublesome:
https://github.com/mozilla-b2g/gaia/commits/master/apps/calendar/build/build.js

What if we started out by landing the patch as-is on trunk, but with a really high value (say 20 megs), and then on v1.3T just keep the value at 1 megabyte?  And then we spin-off a bug to implement that build mechanism on trunk and change the constant to slurp from there for now?

I do believe we'll see more things like this in the future, so it makes sense to support, but our build mechanism is not exactly bulletproof as is, so I'd be wary about backporting such a thing.  For example, bug 997652 was just filed in e-mail where apparently our locales got broken somehow on v1.3T for at least one build.

Comment 27

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18426

Comment 28

[Marcus Cavanaugh [:mcav]]

Attachment: Screen Shot 2014-04-05 at 09.22.54.png

Comment 29

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18426

Comment 30

[Marcus Cavanaugh [:mcav]]

(In reply to Andrew Sutherland (:asuth) from comment #26)
> What if we started out by landing the patch as-is on trunk, but with a
> really high value (say 20 megs), and then on v1.3T just keep the value at 1
> megabyte?  And then we spin-off a bug to implement that build mechanism on
> trunk and change the constant to slurp from there for now?

Given the build system churn, I think that’s a more reasonable
approach. It’s not like we’re going to go back and automatically
forwardport all 1.3T commits.

Regardless, the latest patch is against master, and shows the faded
icon as Juwei requested. We can't automatically merge this with 1.3T
due to CSS changes, but it will be a simpleish tweak and manual uplift.

Comment 31

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

Comment on attachment 8408351 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18426

Accessibility-wise, we either need to be setting disabled on the button or aria-disabled.  Doing pointer-events: none is insufficient.  Since it's a button, I suggest we use disabled (which has a magic JS property that is bidirectionally bound to mess with the attribute).

This does preclude us just using CSS rules to be clever.  I suggest we may have a "buttonDisabled" variable we set in our if-cascade (which you can fix-up the lint of while you're there, although it may bitrot :jrburke's virtual scroll stuff a teeeny bit) that we then set appropriately on the button.

Alternately, I think accessibility will pick up on display: none, so we could have two physically distinct <button>s, the disabled one being eternally disabled.  I think that might be somewhat more confusing, however.

Clearing review for now.

Comment 32

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8408351 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18426

Right, I forgot about the accessibility side of things. Updated the patch as you suggested.

Comment 33

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

Comment on attachment 8408351 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18426

Let's put a "margin-right: 6rem;" on .msg-attachment-too-large in case localizations want to spill to a second line.  Without this, the text can overlap with the button and look weird.  (If the localization spills to 3 lines it will overlap that divider line which is bad, but that would be one long translation!)

I picked 6rem because the .msg-attachment-download is 3.5rem wide with 1.5rem padding on both sides.  That gives us 6.5rem, but that seems like slightly too much.

I tested on a trunk-ish hamachi/buri and used the app manager to try forcing the string to be stupidly long for comparison purposes.

Also, here's what the diff looks like in reviewboard:
https://reviewboard.allizom.org/r/67/diff/

Comment 34

[Axel Hecht [:Pike]]

It seems that we're not in charge of 1.3T localization.

We don't have infra for the 1.3 locales to cover T, too. So how that works is probably something that the tarako folks need to add to the stuff they need to figure out.

Also CCing Chris, who's been on that conversation.

Comment 35

[chris hofmann]

Its my understanding that our partner is taking on responsibility for doing all needed localization work for 1.3T, and they have sent localization files to their contractor.  If that's true this has missed their string freeze cuttoff.

They probably need a change order to the contractor schedule of work in order accommodate this string if it lands.  These changes can be costly.  The partner is in the best position to evaluate if that's possible and how they would like to proceed.  [e-mail], [e-mail], [e-mail], [e-mail] are the contacts for communication with the partner, and can alert them about the bug and the need for decisions and direction on how to handle this and any other late-l10n bug that might surface on 1.3T.

As for infra to support sting landing, tracking of localization work and production builds on 1.3T we don't know what process the partner and their localization contractors intend to use, and haven't received any requests for help on that.  bd folks above may have more on that in the coming days.

So as Axel mentions there is not much help we can provide here at this point.  Hopeful I've at least outlined some steps to move forward, things that need to be done, and decisions that need to be made.

Comment 36

[Marcus Cavanaugh [:mcav]]

Landed the fix (with r=asuth + margin-right:6rem) on master:

https://github.com/mozilla-b2g/gaia/commit/af6cc92abaa8b0eb6020ef66afd97eb2bb39ffdd

Still awaiting more input from l10n folks on whether or not we can actually land a string change on 1.3T.

Comment 37

[Axel Hecht [:Pike]]

Marcus, you're waiting on input from [e-mail], [e-mail], [e-mail], [e-mail], as Chris laid out. Not from the l10n team.

Comment 38

[Marcus Cavanaugh [:mcav]]

Sorry, I have very little visibility into the organizational structure of the non-engineering groups, both internal and partner-related. You are correct, Axel; I meant waiting for input about l10n and whether or not this patch will be feasible with a new string.

Comment 39

[Joe Cheng [:jcheng] (please needinfo)]

Bengali was done by our Moz and the community so we'd need l10n feedback on Bengali
Hindi/Tamil, Steven will talk to partner directly.
Thanks

Comment 40

[Axel Hecht [:Pike]]

Forwarding this to Chris, who's leading this conversation.

Comment 41

[Dylan Oliver [:doliver]]

Marking as fixed since this is landed on master. Moving back to 1.3T? so we don't land this prematurely. 

Joe, please re-block for 1.3T if we are clear to uplift.

Comment 42

[Jason Smith [:jsmith]]

I think you meant to flag 1.3T?, not 1.3?

Comment 43

[Dylan Oliver [:doliver]]

(In reply to Jason Smith [:jsmith] from comment #42)
> I think you meant to flag 1.3T?, not 1.3?

Yep, thanks.

Comment 44

[Steven Yang [:styang]]

Hi Ying, please uplift the patch to 1.3T. thanks.

Comment 45

[Keven Kuo [:kkuo]]

Hi! Marcus,

Could you help to uplift to 1.3T? Thanks

--
Keven

Comment 46

[Keven Kuo [:kkuo]]

That PR is for master. I am afraid conflict occurs while uplifting to 1.3T.

--
Keven


(In reply to Steven Yang [:styang] from comment #44)
> Hi Ying, please uplift the patch to 1.3T. thanks.

Comment 47

[ying.xu]

v1.3t pr based on https://github.com/mozilla-b2g/gaia/commit/af6cc92abaa8b0eb6020ef66afd97eb2bb39ffdd

https://github.com/mozilla-b2g/gaia/commit/440cbdb5c7fec10e424985e7844ae4cbedc38808

Comment 48

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

The uplifted patch (https://github.com/mozilla-b2g/gaia/commit/440cbdb5c7fec10e424985e7844ae4cbedc38808) left the maximum download size constant at 20 MiB.  It needs to be lowered to 1 MiB for v1.3t, as discussed in comment 26.

Comment 49

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18695

Constant fix.

Comment 50

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

Comment on attachment 8412772 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18695

Good comment!

Comment 51

[Marcus Cavanaugh [:mcav]]

Everyone loves a good comment.

v1.3t: https://github.com/mozilla-b2g/gaia/commit/d99ec0efdfc39931d224571ac8de80708526ab48

Comment 52

[[:fabrice] Fabrice Desré]

So, this is only on 1.3t and we'll have the same issue once tarako moves back to master. We need a setting instead of hard coding constants.

Comment 53

[Marcus Cavanaugh [:mcav]]

I have filed followup bug 1001577 (per Comment 26) for moving the constant into a build-time setting.

Comment 54

[James Zhang (Spreadtrum)]

Can we config max size by setting.json?

Comment 55

[James Zhang (Spreadtrum)]

And I think 2MB is better, the same as gallery.

Comment 56

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

(In reply to James Zhang from comment #54)
> Can we config max size by setting.json?

No.  Bug 1001577 was filed about making this configurable, but that will only happen on trunk because it requires changes to the email app's build process and those are considered risky for uplift.


(In reply to James Zhang from comment #55)
> And I think 2MB is better, the same as gallery.

In https://bugzilla.mozilla.org/show_bug.cgi?id=991567#c26 it was reported that a 1.5 MiB attachment caused us to crash.  While it may be worth tweaking the number further, the number obviously needs to be set low enough so that we don't crash.  Gallery's limit has to do with memory used in displaying the image, our limit has to do with the memory used during the download process.

Please feel free to change the constant like was done in https://github.com/mozilla-b2g/gaia/commit/d99ec0efdfc39931d224571ac8de80708526ab48 and test to determine a better number.  It is fine to change this number without consulting the email team on this as long as you test thoroughly.

Comment 57

[Mike Lien[:mlien]]

verified and fixed with today's daily build (file size exceeds 1MB will unable to download)
Gaia      8895b180ed636069473703d0e7b73086989601ce
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/7caf4b5abfce
BuildID   20140428014001
Version   28.1