871852 - [Email] Email sending fails silently when sending large (for an email) files (ex: 25 MB), No limit on max attachments size

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : Email sending with large attachments around 25 MB is not working.
2. Precondition : Email should be working 
3. Tester's Action:  Go to Gallery select images(multiple) of size around 25 MB -> Share via Email -> Enter email address -> Send
4. Detailed Symptom (ENG.) : 
a. Email did not sent and there is no fail popup 
b. Email is not present in Outbox also
5. Expected :
a. Email should be sent successfully
b. There is should be a limit for adding attachments size. Please let us know your opinion on this.
c. If email could not able to send , it should show in Outbox.
6.Reproducibility: Y
           1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: 393b3f57822ae0f34055c6a6060f1433136bafa0
9.Personal email id:  [e-mail]

Comment 1

[psingapati]

Attachment: Logs for large attachments sending

Logs attached for sending email with images around 25MB size from Gallery app.
sending popup was shown continuously. Suddenly Home screen is displayed.
Email does not show either in sent mail or outbox and it did not receive at the recipient email. There is no failure alert also.

Please check

Comment 2

[psingapati]

Hello Andrew,
Please check and let me know if you need any more information.
Thanks.

Comment 3

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

It doesn't look like the out-of-memory killer logs to logcat, but based on your report and what is in the log it sounds like the e-mail app bloated up, all other processes got killed, then the e-mail app bloated further and got killed.  (Running "adb shell dmesg" might get the data, however.)

The GonkMemoryPressure events and the entries that indicated the reload of the home screen and other pre-loaded processes is what I'm going on from the log.

That we die sending a 25 MB file is not terribly surprising.  The file is going to suffer from bloat during the encoding process; the base64 rep will be 137% the size of the original, and both sets of memory will be alive at the same time.  We have to send that data to the main thread from the worker (we don't currently transfer ownership because of node.js semantics we are matching), and then that has to get sent to the parent process from the child process.  So that's about 136 MiB right there that probably happens faster than GC can reap it.

I filed bug 871897 on switching to a streaming implementation for mail sending, although this bug could just end up being about us adding an error message and refusing to add any attachment that causes us to cross a much, much lower threshold than 25 MB.

Comment 4

[psingapati]

So are we going to set threshold for the max attachment size?
If so , we may need to have the popup scenario also to implement this.

Please let us know your opinion on resolving this issue

Comment 5

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

Let's ask Rob MacDonald who seems to be replacing Casey Yee on e-mail UX.

Comment 6

[Stephany Wilkes]

Rob is out this week. Reassigning to Francis, just for this week.

Comment 7

[Alex Keybl [:akeybl]]

Issue with new feature, so blocking.

Comment 8

[Stephany Wilkes]

And... assigning to Rob since this is now Leo+ and Francis is in Hungary for research.

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

Stephany,

Currently, who will take a look at of this bug on UX??

Comment 10

[Stephany Wilkes]

I thought I'd flagged Rob two days ago and that does not seem to have actually happened. This is with Rob now and I've also emailed him about it so it doesn't get missed.

Comment 11

[Rob MacDonald [:robmac]]

Hi everyone and thanks for alerting me to this...

We'll need to use the confirmation dialog (https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/UX/Building_blocks/Confirmation).

As I see it, there are two scenarios for exceeding attachment limits. The user could attach a single file that's too large, or they could attach multiple files that add up to being too large.

In the case of a single file, the draft text, subject to review by Matej, is:

     Attachment too large
     ----------------------
     The selected attachment is too large to send with this message. 

     [OK]

In the case of photos, this dialog would appear immediately after the user selected the image, before the image preview. (For the flow, see https://mozilla.box.com/s/ie6x8zgm8902dveyb5xf pages 5-12) Tapping "OK" returns the user to the picker to allow them to select another file or cancel the attachment. Of course the file is not attached to the email.

In the case where the user selects multiple files, the proposed text is:

     Attachments too large
     ----------------------
     The total size of the selected attachments is too large to send with this message. Try selecting fewer files.

     [OK]


Please flag me if you have any questions or concerns. And, Matej, I'll flag you for the text.

- Rob

Comment 12

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

(In reply to Rob MacDonald [:robmac] from comment #11)
> selected the image, before the image preview. (For the flow, see
> https://mozilla.box.com/s/ie6x8zgm8902dveyb5xf pages 5-12) Tapping "OK"

This wants me to log in... can you change that setting?

In any event, the flow you are proposing sounds like it requires the implementor of the 'pick' activity to enforce size limits.  Which is not necessarily a bad idea, but we can't really enforce that behaviour on pick implementors, so it would also need to be backstopped by the e-mail app itself.  Which suggests that for a v1.1 stop-gap implementation we should only do it in the e-mail app itself.

Comment 13

[psingapati]

Hi Johu,

I want to know, if you already started working on this issue. If not please let me know , can I steal this bug.

Thanks.

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

psingapati, 

Yes, please. I am current stocking on another bug.

Comment 15

[psingapati]

Attachment: Pull Request pointer

Implementation is done as below.
1) Limited max attachment size to 5120000bytes (5MB)
2) When attaching from composer we can attach only one image at a time, so we check whether that total attachment size including the image does not exceed 5MB.
3) From gallery we can attach multiple files, the current implementation inserts the number of attachments that can be allowed in the composer and shows the alert to user.

Notes:
AFAIK gallery supports displaying of images max size 2.5 MB , so we would not get a scenario of single attachment exceeding the max limit. However as per suggested in the above comment #11 string locales are taken care for such case as well.

Please review

Comment 16

[Matej Novak [:matej]]

Could we simplify the second one?

The selected attachments are too large to send with this message. Try selecting fewer files.

It makes the first half less clear, but it's then explained in the second part where it offers a solution.

Otherwise, looks good to me.

Comment 17

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

Comment on attachment 753753 [details]
Pull Request pointer

Works well.  Conditional r=asuth.  Please address the following:
- Fix the lint errors related to syntax
- Please indent the object definition in the call to ConfirmDialog.show
- Please update the "compose-attchments-size-exceeded" string to be "The selected attachments are too large to send with this message. Try selecting fewer files." per Matej in comment 16.

Please let me know when the patch is updated and I can merge it for you.

Comment 18

[psingapati]

I did fix the issues mentioned in comment #17.
Please check the new commit.

Thanks.

Comment 19

[psingapati]

Attachment: Pull Request pointer

New Pull request is raised as there were some more errors.

Comment 20

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

Please squash the commits with "git rebase -i" per gaia policy so I can merge.

Comment 21

[psingapati]

squashed second commit on to the first, please check

Comment 22

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

(In reply to psingapati from comment #21)
> squashed second commit on to the first, please check

I think you may have forgotten to push the rebased commits over top of the existing commits using the "-f" flag for push.  I did check the leob2g repo in case the pull request did not automatically update because you created 2 pull requests, but all I found was two branches (Bug_871852_Email_Composer_Max_Attachment_Size and Bug_871852_Email_Composer_Max_Attachments_Size) but they both had 2 commits.  I think there's a #git channel on mozilla IRC if you need assistance or feel free to ping me when.

Comment 23

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

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/10013
https://github.com/mozilla-b2g/gaia/commit/07b567210f493d39a1b02172db23ad8c184b1bef

Comment 24

[Francesco Lodolo [:flod]]

+composer-attachment-large=Attachment too large
+composer-attachments-large=Attachments too large
+compose-attchment-size-exceeded=The selected attachment is too large to send with this message.
+compose-attchments-size-exceeded=The selected attachments are too large to send with this message. Try selecting fewer files. 

Attachment is misspelled in the last two strings' names, but ignoring that: is this the right way to manage plural forms?

See for example: https://github.com/mozilla-b2g/gaia/blob/07b567210f493d39a1b02172db23ad8c184b1bef/apps/email/locales/email.en-US.properties#L176

Comment 25

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

Oops, didn't notice the id typos, but as you say, it doesn't actually matter since it's an opaque id; when we next change the content or context of the strings we can correct the typo since we'll need to change the id anyways.

Unless I'm misunderstanding, we only need to use the plural forms when the number is placed in the string because various languages have different rules for different numbers.  In this case, we only have the one/many cases so there is no variability to require additional agreement.

Comment 26

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 07b567210f493d39a1b02172db23ad8c184b1bef to:
v1-train: d4ef92e97ee55e924726e07ea080da3808119ad7

Comment 27

[Axel Hecht [:Pike]]

(In reply to Andrew Sutherland (:asuth) from comment #25)
> Oops, didn't notice the id typos, but as you say, it doesn't actually matter
> since it's an opaque id; when we next change the content or context of the
> strings we can correct the typo since we'll need to change the id anyways.
> 
> Unless I'm misunderstanding, we only need to use the plural forms when the
> number is placed in the string because various languages have different
> rules for different numbers.  In this case, we only have the one/many cases
> so there is no variability to require additional agreement.

In fact, using the plural infrastructure without the number would be a bug, as there are languages that use the same grammatical form for 1 and 11. So you get "fix the error" when passing in 11, which is wrong.

Comment 29

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

Bug 880630 which is a duplicate of this bug requested tef?; I am propagating the request to this bug since this fix would need to be uplifted and changing the wontfix to affected.

Comment 30

[Andrew Overholt [:overholt]]

Daniel, can you make a call about uplift here?  Thanks.

Comment 31

[Daniel Coloma:dcoloma]

It would be good to quantify this in terms of a real use case, how many pictures do I need to attach in order to reach the limit that makes sending silently fail?

Comment 32

[Leo]

Hi Daniel,

We restricted the Max attachment size to 5MB. Please see the comment #15 for more information. 
If you have a file around 1MB, with 6 attachments you can see the alert. 

Thanks

Comment 33

[Alex Keybl [:akeybl]]

Moving to leo? since TAs are behind us for 1.0.1 partners and tef? is being shut down.

Comment 34

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

(In reply to Alex Keybl [:akeybl] from comment #33)
> Moving to leo? since TAs are behind us for 1.0.1 partners and tef? is being
> shut down.

Clearing leo? since leo (v1.1) already has the limit of 5 megs and I am actually fixing the mail sending process to use streaming in bug 871897.

Comment 35

[Cody Roesch [:croesch]]

Here are the cases created for this bug.

https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-id=9394
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-id=9397

Comment 36

[buri.blff]

Attachment: 2013-08-15-03-42-50.png

Comment 37

[buri.blff]

Attachment: log

Email exit automatically when sending a mail with about 4M attachments on V184

Comment 38

[Jack Liu]

Dear mozilla, reopen it according to comment#36 & comment#37.

Comment 39

[Andrew Overholt [:overholt]]

(In reply to buri.blff from comment #37)
> Created attachment 791159 [details]
> log
> 
> Email exit automatically when sending a mail with about 4M attachments on
> V184

Please open a new bug as this seems to be a new issue.  Thank you.