1010701 - [Tarako][Email][LMK] Email gets killed when attach multiple files to a mail

Status: RESOLVED FIXED

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

Description

[Bingqing Li]

Attachment: logcat

Environment:
---------------------------------------------------
Gaia      dbc2b00f538df1d93960794fbb52aa84f6f19483
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/3b6c97b5fe8b
BuildID   20140514164005
Version   28.1
ro.build.version.incremental=eng.cltbld.20140514.202602
ro.build.date=Wed May 14 20:26:10 EDT 2014

Steps to reproduce:
-------------------------------------------
1. Launch Email
2. Create a new mail
3. Add some attachments to the mail. 
   e.g. add 3~5 videos

Actual results:
-------------------------------------------
After I attached 4 videos, email got killed.
(Those videos are all taken with Camera.) 


Additional info:
-----------------------------------------------
Email gets killed when add multiple pictures/music to the mail.

Comment 1

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

Hi Andrew, would you be able to take a quick look at this please?

Comment 2

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

Based on all of the "SPRDAVCDecoder" lines in the log near the bottom where the email app gets killed, it looks like the most direct causality is from the video app's preview functionality.  Perhaps the videos app should not support previewing when in "pick" mode on a tarako device?

Reporter, did you actively preview the videos or are the entries in the log due to the <video> tag pre-buffering for some reason?

Comment 3

[Bingqing Li]

(In reply to Andrew Sutherland (:asuth) from comment #2)
> Based on all of the "SPRDAVCDecoder" lines in the log near the bottom where
> the email app gets killed, it looks like the most direct causality is from
> the video app's preview functionality.  Perhaps the videos app should not
> support previewing when in "pick" mode on a tarako device?
> 
> Reporter, did you actively preview the videos or are the entries in the log
> due to the <video> tag pre-buffering for some reason?

No，during the test, I did not actual play the videos, just tapped on the check icon on the top right.

BTW, when I add music to the email, I did not play the music either.

Email also got killed when I added more than 3 pictures.

I'll upload those logs about Email being killed when adding multiple pictures and music if needed.

Comment 4

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

Thanks for the clarification.  I filed bug 1011400 on the videos app potentially avoiding any preloading in our "pick" activity case.  I also acquired an OS X laptop while in Mountain View yesterday in order to try and better determine where our memory usage is going in the attaching case.  Especially since the log indicates the email app died because the videos app was actively while we were still asynchronously attaching the attachment and that ideally shouldn't kill us.  We may need to stop-gap this on Tarako by prohibiting triggering another "pick" activity while an attachment is still in the process of being asynchronously attached.

Please do provide the picture/music logs as this will help indicate whether the problem in those cases was also that an activity was active while our (apparently memory-intensive) attaching process was occurring.

I am assigning this bug to myself in the interim for investigation.

Comment 5

[Bingqing Li]

Attachment: logcat-add-music

logcat of attaching music

Comment 6

[Bingqing Li]

Attachment: logcat-add-pic

logcat of attaching pictures

Comment 7

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

Juwei, I need UX input on a potential mitigation here.  I am going to do more investigation into the actual send problem, but in the meantime we need a plan.

When attaching attachments from within the email app, what happens is:
1) User taps attachment icon (paperclip)
2) Web activity picker is triggered, user picks one
3) Other app is launched, user locates thing they want to attach.
4) Other app returns to email app.  Email begins asynchronously processing the attachment for sending in the background.  Email app is still responsive to the user so they can go about typing their message, attach another attachment, etc.

The problem in this bug is that both step 2 and step 4 use non-trivial amounts of memory and in this bug both are happening at the same time.  We are still asynchronously attaching an attachment from step 4 while a new step 2 is occurring.

The best mitigation we can do in the front-end is to make sure that we don't let step 4 and step 2 happen at the same time.  The friendliest option I can think of is to do the following when the user taps the attachment icon:
- Check if we are still attaching something (aka step 4 is still occurring).
  - If yes, show a spinner that conveys to the user that we noticed that they clicked on the attach icon, but we need to finish something first.  My instinct would be to show our transparent-ish black overlay with just a circular spinner.  Because this is v1.3T and we are way past string freeze there would be no related string.
  - Once we finish attaching the attachment, remove the spinner screen
- Then trigger the activity and thereby the picker.

Other alternatives would be doing something like:
- showing a spinner on the attachment while it's being attached and disabling the attach icon while that's happening
- do the modal spinny screen, regrettably interfering with the user's ability to type stuff either.

Comment 8

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

Triage: 1.3T+, this is a partner blocker

Comment 9

[Dylan Oliver [:doliver]]

(In reply to Andrew Sutherland (:asuth) from comment #7)
> Other alternatives would be doing something like:
> - showing a spinner on the attachment while it's being attached and
> disabling the attach icon while that's happening

I like this approach. I think this is more intuitive than creating the overlay and allows them to keep typing while waiting to attach the second item. This also keeps the UI simpler, e.g. no need to create a way to cancel the overlay if you don't want to wait for the first attachment to complete.

Comment 10

[Juwei Huang[:juwei]]

I'm also like this idea, however, my concern is that when the paperclip is disable, users might not know it is because of the uploading attachment. They might think that the message can only attach one file if the upload status is taking too long.

I'd prefer to make the paperclip still clickable, and show a notice to inform users that they have to wait the attachment finish upload.
So how about this
1. When the previous attachment still updating, tap on the paperclip
2. A window pops out and states "Please wait until the attachment has finished uploading."

I know it might not the perfect solution for it, but at least it's the safest way to let users know that they cannot attach another file while the first file is uploading.

Let me know your thought!

Comment 11

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

(In reply to Juwei Huang from comment #10)
> I'm also like this idea, however, my concern is that when the paperclip is
> disable, users might not know it is because of the uploading attachment.
> They might think that the message can only attach one file if the upload
> status is taking too long.

Yeah, I'm worried about disabling buttons and just making them non-responsive for this reason too.  Popping up a toaster seems ideal 

> I'd prefer to make the paperclip still clickable, and show a notice to
> inform users that they have to wait the attachment finish upload.
> So how about this
> 1. When the previous attachment still updating, tap on the paperclip
> 2. A window pops out and states "Please wait until the attachment has
> finished uploading."

As per comment 7, I don't think we can add a string for v1.3T at this point.  I'm not 100% sure on that, but I do know that Mozilla l10n is not handling Tarako, so it's a partner question.  Can't hurt to double-check, though.  Joe?

What if we replaced the attachment paperclip icon when it gets tapped on?  Then the user would know we heard them and we are going to try and do something soon.  In general we are talking about a few seconds of time since attachment sizes are already limited.

Comment 12

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

(In reply to Andrew Sutherland (:asuth) from comment #11)
> What if we replaced the attachment paperclip icon when it gets tapped on? 
> Then the user would know we heard them and we are going to try and do
> something soon.  In general we are talking about a few seconds of time since
> attachment sizes are already limited.

Left out what we'd replace it with: the spinner/progress icon.

Comment 13

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

redirecting the question to Steven for partner management

Comment 14

[Steven Yang [:styang]]

(In reply to Andrew Sutherland (:asuth) from comment #11)
> (In reply to Juwei Huang from comment #10)
> > I'm also like this idea, however, my concern is that when the paperclip is
> > disable, users might not know it is because of the uploading attachment.
> > They might think that the message can only attach one file if the upload
> > status is taking too long.
> 
> Yeah, I'm worried about disabling buttons and just making them
> non-responsive for this reason too.  Popping up a toaster seems ideal 
> 
> > I'd prefer to make the paperclip still clickable, and show a notice to
> > inform users that they have to wait the attachment finish upload.
> > So how about this
> > 1. When the previous attachment still updating, tap on the paperclip
> > 2. A window pops out and states "Please wait until the attachment has
> > finished uploading."
> 
> As per comment 7, I don't think we can add a string for v1.3T at this point.
> I'm not 100% sure on that, but I do know that Mozilla l10n is not handling
> Tarako, so it's a partner question.  Can't hurt to double-check, though. 
> Joe?
  Yes, our partner will handle the translations for 1.3T.
> 
> What if we replaced the attachment paperclip icon when it gets tapped on? 
> Then the user would know we heard them and we are going to try and do
> something soon.  In general we are talking about a few seconds of time since
> attachment sizes are already limited.

Comment 15

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

(In reply to Steven Yang [:styang] from comment #14)
> > As per comment 7, I don't think we can add a string for v1.3T at this point.
> > I'm not 100% sure on that, but I do know that Mozilla l10n is not handling
> > Tarako, so it's a partner question.  Can't hurt to double-check, though. 
> > Joe?
>   Yes, our partner will handle the translations for 1.3T.

Sorry for the ambiguity.  The question is whether we are still allowed to add new strings to v1.3T.

Comment 16

[Steven Yang [:styang]]

(In reply to Andrew Sutherland (:asuth) from comment #15)
> (In reply to Steven Yang [:styang] from comment #14)
> > > As per comment 7, I don't think we can add a string for v1.3T at this point.
> > > I'm not 100% sure on that, but I do know that Mozilla l10n is not handling
> > > Tarako, so it's a partner question.  Can't hurt to double-check, though. 
> > > Joe?
> >   Yes, our partner will handle the translations for 1.3T.
> 
> Sorry for the ambiguity.  The question is whether we are still allowed to
> add new strings to v1.3T.

Yes, 1.3T is only used for Tarako. Our partner has no concern to add new strings if necessary.

Comment 17

[James Zhang (Spreadtrum)]

Agree with Steven.

Comment 18

[James Burke [:jrburke]]

I'm taking this bug with asuth's consent: I will add in a toast message with a localized string if attachment is still in process. 

It may need some tuning and we will need to decide if we want that behavior on master too, as the pull request will be for master, but I will post more details once the pull request is up.

Comment 19

[James Burke [:jrburke]]

Attachment: GitHub pull request

If add attachment button is tapped before the previous attachment has fully been attached, show a toast with the message "One moment, still working on the previous attachment...", and then once the previous attachment is completed, the attachment activity is immediately triggered.

On a hamachi device, for a 380KB image (picture taken from phone camera), it took about 1.6 seconds for the toast to disappear. Factoring in the 600 ms delay used after attachment is complete, and noting that a 4MB file took around 4.7 seconds, a rough guide is about 1 second per MB.

In general, the experience seems fine. For most images like phone pictures, the time seeing the toast so short that by the time user finishes reading, the activity chooser is already up. I think it helps that the code closes the toast as soon as previous attachment is done, and that the next attachment activity automatically proceeds.

Comment 20

[James Burke [:jrburke]]

Attachment: attachment-msg.png

This attachment shows what the toast looks like when it appears.

If you want to try a zip of the app with this change, for use on a hamachi device:

http://jrburke.com/work/gaia/email-attach-1.zip

Juwei: see comment 19 for some background on the behavior. Is this behavior OK for both Tarako and for master? I would prefer to keep the same code for all devices, and the behavior seemed reasonable as the default behavior. But then I'm not a UX person! :)

asuth: if you can try on a Tarako to see if this actually fixes the problem, that is appreciated since I do not have that kind of device.

Comment 21

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

Comment on attachment 8427471 [details] [review]
GitHub pull request

r=asuth.  Thanks very much for the quick turnaround in general and specifically in helping me rev this to be perfect on IRC.

Comment 22

[James Burke [:jrburke]]

Merged to master:
https://github.com/mozilla-b2g/gaia/commit/28af6e4fcb36e794c082e9800b3eb734197e86b4

from pull request:
https://github.com/mozilla-b2g/gaia/pull/19574

I will prep and land the 1.3t pull request next.

Juwei: if you prefer a different default behavior on master, I will do a separate ticket to track that work, but this changeset will allow us to unblock 1.3t, which will need this behavior for the lower memory constraints.

Comment 23

[James Burke [:jrburke]]

Attachment: 1.3t pull request

The 1.3t pull request, for completeness. Carrying forward the r+ from master, merging shortly, waiting on travis.

Comment 24

[James Burke [:jrburke]]

Merge to 1.3t:
https://github.com/mozilla-b2g/gaia/commit/e76fc9fc64a027d84b2ec311fc624f4c3459dca9

from pull request:
https://github.com/mozilla-b2g/gaia/pull/19582

Travis was not green, but it failed in a system notification test, not related to this changeset, so merged.

Comment 25

[Francesco Lodolo [:flod]]

Hi, after looking at the string just landed I wonder if this comes from a spec, and therefore from a copy review.

> compose-attachment-still-working=One moment, still working on the previous attachment...

English always uses "…" (single Unicode character), not "...". Besides that, I've never seen "One moment" in software, "Please wait" is the usual form.

Comment 26

[Juwei Huang[:juwei]]

(In reply to James Burke [:jrburke] from comment #19)
> Created attachment 8427471 [details] [review]
> GitHub pull request
> 
> If add attachment button is tapped before the previous attachment has fully
> been attached, show a toast with the message "One moment, still working on
> the previous attachment...", and then once the previous attachment is
> completed, the attachment activity is immediately triggered.

My original thought is to pop out a confirm window with a OK button. But if you prefer a toast I'm also ok with it:) The only thing I concern here is if it's necessary to jump to attachment activity right after the previous one is completed. I'd prefer to let users press the paperclip button themselves otherwise they might be interrupt by the attachment activity while they are tying things. 


In terms of master behaviour, I actually got another idea if we still have time to do something different in master.The idea is quite like what Andrew said....when the users tap on the paperclip, the paperclip will replaced by another icon that indicate "in progress", so  we don't need to show toast or window and the screen would be clean. But I need visual designer to support it so it takes time.
James, is it possible to have more time to implement it in master?

Comment 27

[James Burke [:jrburke]]

:flod: thank you very much for the feedback. I felt "please wait" was too accusatory of the user, where the app has the problem. However, you have much better experience in this area, thank you for helping me to improve. So in total, it sounds like this would have been better:

    Please wait, still working on the previous attachment…

I apologize for not getting proper copy review. There was urgency to wrap this up for v1.3t, and it was said that a partner would be handling new string translations. Still, I want to put good things in the tree, and seems like I did not do so good in this case. 

If you think it is awkward enough that it should be changed for 1.3t, I will push a change for that. Note that on master, we will fix it by just removing the string, taking the icon approach Juwei suggests.

Juwei: I filed bug 1015286 to track changing the behavior on master to the icon approach that you described. If we sort out a solution for that bug and land it while 1.3t is still accepting changes, then if it was desired by the people managing v1.3t, we could uplift to that branch.

Comment 28

[Francesco Lodolo [:flod]]

(In reply to James Burke [:jrburke] from comment #27)
> If you think it is awkward enough that it should be changed for 1.3t, I will
> push a change for that. Note that on master, we will fix it by just removing
> the string, taking the icon approach Juwei suggests.

Let's see what Matej think, it could also be just me being over critical (and English is not my native language).

Current string: "One moment, still working on the previous attachment...".

I suggested to replace it with "Please wait, still working on the previous attachment…", since I never see the word "Moment" used in software.

Do you think it's worth the fix?

@James
From a technical point of view: if this string stays only on 1.3T, you can fix the string there without changing the ID. If on master it's going to disappear soon, I wouldn't bother updating it.

Comment 29

[Matej Novak [:matej]]

I agree that "Please wait" is better than "One moment," though I wonder if it needs a bigger change. There's something odd about the sentence structure.

Maybe something like:

"Please wait. The previous attachment is still pending."

Comment 30

[James Burke [:jrburke]]

Thanks for the tips on the string! I will rely on bug 1015286 to fix it for master/2.0, and the solution there will just remove the problematic string completely.

Comment 31

[pete siphantong [:Petes]]

Verified on the latest Tarako build

1.3T Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140602014001
Gaia: 335486c42498fa7a93c21e4d6121199728602ab8
Gecko: 55e4d83019e5
Version: 28.1
Firmware Version: SP6821a-Gonk-4.0-4-29

1.3 tarako: Attached videos, email did not gett killed

Comment 32

[James Zhang (Spreadtrum)]

Peipei, this bug verified failed on my side. Please help to check.

Comment 33

[Peipei Cheng (needinfo if you need my action)]

(In reply to James Zhang from comment #32)
> Peipei, this bug verified failed on my side. Please help to check.

Verified on following build. Attaching 11 attachments works fine.

 Gaia      335486c42498fa7a93c21e4d6121199728602ab8                         │
│ Gecko                                                                      │
│ https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/55e4d83019e5       │
│ BuildID   20140602164002                                                   │
│ Version   28.1                                                             │
│ ro.build.version.incremental=eng.cltbld.20140602.202528

Comment 34

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

(In reply to James Zhang from comment #32)
> Peipei, this bug verified failed on my side. Please help to check.

Can you provide a logcat from your failure?  Our fix is fundamentally a workaround and the underlying triggers were very dependent on how the steps taken by the tester.  The log will help shed light on the testing pattern and any nuances.  Please also be sure to specify the steps to reproduce since not everything is in the log.  And some things we didn't test, but you may be trying.  For example, previewing a video by hitting play may result in other system allocations that require us to increase our protective delay, etc.

Comment 35

[James Zhang (Spreadtrum)]

Hi Andrew, I have asked our QA verified on user build, it passed, on userdebug build, it still failed.
Our QA think it's acceptable. But I haven't found LMK/OOM log in kernel log, only gecko "Security problem" log.

{"type":"attachBlobToDraft","longtermId":"0/7","lifecycle":"do","localStatus":"doing","serverStatus":"n/a","tryCount":0,"humanOp":"attachBlobToDraft","existingN)
06-03 03:02:17.220  1686  2032 I Gecko   : [0m
06-03 03:02:17.380    85    85 I Gecko   : Security problem: Content process does not have `device-storage:pictures-read'.  It will be killed.
06-03 03:02:17.410    85   301 I Gecko   : [Parent 85] WARNING: waitpid failed pid:1686 errno:10: file /space/builder/repo/sp6821a_gonk4.0/B2G/gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
06-03 03:02:17.410    85    85 I Gecko   : 
06-03 03:02:17.410    85    85 I Gecko   : ###!!! [Parent][DispatchAsyncMessage] Error: Value error: message was deserialized, but contained an illegal value
06-03 03:02:17.410    85    85 I Gecko   : 
06-03 03:02:17.420    85    85 I Gecko   : 
06-03 03:02:17.420    85    85 I Gecko   : ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv

Comment 36

[James Zhang (Spreadtrum)]

Attachment: Bug 308061_0603.rar

No OOM/LMK log in kernle log.
We can only find Gecko 'Security problem' log.

And it passed on user build.

06-03 03:02:17.380    85    85 I Gecko   : Security problem: Content process does not have `device-storage:pictures-read'.  It will be killed.
06-03 03:02:17.410    85   301 I Gecko   : [Parent 85] WARNING: waitpid failed pid:1686 errno:10: file /space/builder/repo/sp6821a_gonk4.0/B2G/gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
06-03 03:02:17.410    85    85 I Gecko   : 
06-03 03:02:17.410    85    85 I Gecko   : ###!!! [Parent][DispatchAsyncMessage] Error: Value error: message was deserialized, but contained an illegal value
06-03 03:02:17.410    85    85 I Gecko   : 
06-03 03:02:17.420    85    85 I Gecko   : 
06-03 03:02:17.420    85    85 I Gecko   : ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv

Comment 37

[yang.zhao]

Landed on our build by using patch.

Comment 38

[yang.zhao]

(In reply to yang.zhao from comment #37)
> Landed on our build by using patch.
Here means the string in ta/hi-IN/bn-BD languages on our build.