959422 - [Messages] The Messages app crashes when attaching too big images

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Josh Schmitt [Joshs]]

Description:
While the user was in a call, the user tried to send a picture message and the messaging app force closed

Repro Steps:
1) Updated Buri to BuildID: 20140113004002
2) Using 2 devices, make a call from device 1 to device 2
3) Once connected, on device 1 proceed to the message app
4) Select the attachment icon
5) Select camera
6) Take a picture and use that picture

Actual:
The messaging app force closes

Expected:
The messaging app converts to mms

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140113004002
Gaia: b3fc4f712562ee92b0ed0bd17abc61be9a36a8da
Gecko: 5bb1837de7c0
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Comment 1

[Josh Schmitt [Joshs]]

Issue did not occur in 1.2 buri build. The messaging app did not force close.

Environmental Variables:
Device: Buri 1.2 COM
BuildID: 20140113004002
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: e672faf1e6a1
Version: 26.0
RIL Version: 01.02.00.019.102
Firmware Version: V1.2-device.cfg

Comment 2

[Julien Wajsberg [:julienw]]

Can we try with (separately):
* SkiaGL disabled (set: "pref("gfx.canvas.azure.accelerated", false);")
* APZC disabled (longpress power button, select "disable APZ for all applications", kill all apps

And does it happen 100% of tries?

Thanks!

Comment 3

[Jason Smith [:jsmith]]

It isn't going to be Skia GL related - the problem would have likely reproduced in 1.2 if that was the case. It also isn't likely going to be APZC related - those issues don't usually cause force close behavior. And I don't see evidence that the reporter thinks this is intermittent based on the above comment, so I'm clearing the flag.

Comment 4

[Julien Wajsberg [:julienw]]

Please, I'd still want to know what happens with SkiaGL disabled, as this points to a memory-related issue (I don't reproduce on Peak).

Comment 5

[Maria Angeles Oteo (:oteo)]

umm, it's not a problem related with the call in progress...

I am reproducing the bug in v1.3 while the call is in progress and I attach two photos (not one) taken from the camera in the same message, then the messaging app force closes.
But I am testing without the call in progress now and the messaging app is closing too after attaching two pictures from the camera or the gallery... size photos problem in MMS, perhaps

Build info:
Buri
eng
v1.3
B-33.
Gecko-a037f1d
Gaia-96c05de

Comment 6

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Please, I'd still want to know what happens with SkiaGL disabled, as this
> points to a memory-related issue (I don't reproduce on Peak).

I think I made it pretty clear that this isn't going to be related to SkiaGL likely, as we would have saw this bug on 1.2.

Comment 7

[J Zimbrick]

Regression Window:

Last Working Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20131123040202
Gaia: e48c2025b97f26db6e23c16d72095143d731b1fa
Gecko: 98aa428a392c
Version: 28.0a1
Base Image: V1.2-device.cfg

First Broken Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20131124040202
Gaia: c736d91f6a1aada7b81a3d7b7df2635f2f9a655a
Gecko: 74ab61b8d0f0
Version: 28.0a1
Base Image: V1.2-device.cfg

Note: On the the 11/23/13 build the issue in comment 0 does not occur, but the issue noted in comment 5 can be reproduced.

Comment 8

[Jason Smith [:jsmith]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=98aa428a392c&tochange=74ab61b8d0f0

Comment 9

[Julien Wajsberg [:julienw]]

No commit in the SMS app in this range.

Comment 10

[David Scravaglieri [:scravag]]

Gabriele please, could you check if there is something we can do or if Buri is definitely to low in memory to achieve this scenario ?

Comment 11

[Julien Wajsberg [:julienw]]

From bug 940729 comment 20:

1. Create a contact in the contacts app.
2. Select that contact and press the message icon.
3. Press the attachment button to select a picture.
4. Crop the picture and accept the changes to add it to the MMS.
5. Press the attachment button a second time and select and crop a picture
6. Observe that the user is returned to the Contacts screen rather than remaining on the SMS thread.

See also attachment 8349705 [details] and attachment 8349706 [details] for that bug.


From bug 956226 comment 11:

1. Share one pic (200k~300k) to messages
2. Tapping on Home button immediately after SMS is launched and new message page appears.
3. Share another pic or the same pic to messages again
   --> SMS is forced to stop. But if you wait for some time before sending SMS to background, everything works fine.

The difference with this bug is that the pic is smaller, but the resulting behavior is the same.


Gabriele, I'm requesting needinfo from Jerry here but if you can move forward without his help, please do!

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

Smells like OOM to me. Here's my theory:

- you receive an incoming call -> launches the communications app and gives it FOREGROUND_HIGH priority, this pretty much guarantees that the communication app will not be killed if we run out of memory

- launch the SMS app, all fine

- launch the camera app, again all fine, we've got four open applications and we should be able to fit them

- take a picture and use it, this in general causes a spike in memory consumption due to the creation of the blob with its backing array and all, since the camera app is in the foreground to make room for more memory there's only the SMS app to be killed...

- ... hence the SMS app gets killed

I have to try it to be sure though so that I can examine in real-time what's going on. If my theory proves true then there's no easy solution for this; I've got a couple of ideas to reduce memory consumption when creating blobs but they require significant platform work.

Comment 13

[Jason Smith [:jsmith]]

QA Wanted - Can we get a dmesg log with this bug?

Comment 14

[Julien Wajsberg [:julienw]]

Gabriele: thanks to Jerry we managed to reduce the memory consumption in the Gallery app, so maybe we can do some easy adjustment to the SMS app. For information, the code is in [1].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/utils.js#L296-L388

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Gabriele: thanks to Jerry we managed to reduce the memory consumption in the
> Gallery app, so maybe we can do some easy adjustment to the SMS app. For
> information, the code is in [1].
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/utils.js#L296-
> L388

I'm trying to figure out how to use that to help us in this situation. The problem in general is caused by the generation of the blob on the gallery side; if the original image is too large then the resulting blob eats enough memory to trigger the OOM condition. The code in your link deals with shrinking the image once it's been received by the SMS app. I'm I missing something? I'll try to reproduce it now, I've been a bit stuck with reviews today and I'm a bit late :-(

Comment 16

[J Zimbrick]

Attachment: 959422 dmesg log

Attached dmesg log taken immediately after completing the steps from Comment 0.

Comment 17

[Jason Smith [:jsmith]]

I'm assuming the renomination was by accident, so setting the blocking flag again.

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Jason Smith [:jsmith] from comment #17)
> I'm assuming the renomination was by accident, so setting the blocking flag
> again.

Yes, I changed it accidentally; I hadn't even noticed in fact :(

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Julien,
Did you see the bug 939962?
In comment 14,maybe you can try to use "willReadFrequently".
var context = canvas.getContext('2d', { willReadFrequently: true });

With this option, we will use skia as canvas backend instead of skiaGL.
You can compare the performance between skia and skiaGL.


--
Hi Jason,

"I think I made it pretty clear that this isn't going to be related to SkiaGL likely, as we would have saw this bug on 1.2."

In comment 1, Josh said that it is not occur on 1.2(maybe there is no skiagl in 1.2), so can we make sure that isn't going to be related to SkiaGL?
Use skia(software rendering) can reduce some memory usage.

Comment 21

[Julien Wajsberg [:julienw]]

I could make it crash with just one picture.

Gabriele, I'm stealing this, at least temporarily.

Comment 22

[Julien Wajsberg [:julienw]]

Disabling skiagl make this bug disappear.

I'll reenable skiagl and add "willReadFrequently" to our canvas code.

If I understand correctly,it's better to use skiagl for canvases that are just displayed, but not necessarily for canvases that are used to calculate and then thrown away.

Comment 23

[Julien Wajsberg [:julienw]]

Attachment: patch v1

see also PR at https://github.com/mozilla-b2g/gaia/pull/15681

This adds willReadFrequently to all our canvas contexts that are not displayed.
---
 apps/sms/js/attachment.js | 2 +-
 apps/sms/js/utils.js      | 2 +-
 apps/sms/js/wbmp.js       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)


Asking review to Gabriele because he's good at performance and memory stuff ;)

Comment 24

[Julien Wajsberg [:julienw]]

So, all our canvases are used to read back data after writing to them, therefore using "willReadFrequently" everywhere makes a lot of sense (even if this feels like a hack to me).

Comment 25

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8365041 [details] [diff] [review]
patch v1

Review of attachment 8365041 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the stealing. LGTM but have you tried to reproduce the issue with this patch applied? I can try myself if you'd like as soon as I'm finished with bug 963475 (which means later today basically).

Comment 26

[Julien Wajsberg [:julienw]]

Yes of course I did :) I don't request review before trying ;)

Without the patch, I was able to make the app crash when attaching _one_ picture with the camera. With the patch I couldn't make it crash.

Comment 27

[Julien Wajsberg [:julienw]]

master: 177e4c8c6459d8b85a840354c27e0d3b37ee4329

Comment 28

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> Yes of course I did :) I don't request review before trying ;)

OK, cool :) I was asking because lately I'm churning out so many patches that I've got a test backlog so I work on a patch while the (slow) tests for the previous one are running so I'm never sure what I've tested already and what I didn't ;) And BTW, thanks for fixing this.

Comment 29

[Jason Smith [:jsmith]]

(In reply to Jerry Shih[:jerry] from comment #19)
> Hi Julien,
> Did you see the bug 939962?
> In comment 14,maybe you can try to use "willReadFrequently".
> var context = canvas.getContext('2d', { willReadFrequently: true });
> 
> With this option, we will use skia as canvas backend instead of skiaGL.
> You can compare the performance between skia and skiaGL.
> 
> 
> --
> Hi Jason,
> 
> "I think I made it pretty clear that this isn't going to be related to
> SkiaGL likely, as we would have saw this bug on 1.2."
> 
> In comment 1, Josh said that it is not occur on 1.2(maybe there is no skiagl
> in 1.2), so can we make sure that isn't going to be related to SkiaGL?
> Use skia(software rendering) can reduce some memory usage.

Skia GL is enabled on 1.2. So this if reproducing on 1.3 and not on 1.2, then it's a regression with Skia GL in the 1.3 timeframe.

Milan - Does the gfx team agree with the approach given in this bug for the Messages app? Or should we consider not using willReadFrequently & get this fixed in gecko?

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, willReadFrequently is the way to go (for now - see bug 963559 for some longer term thoughts.)  1.2 sometimes gets away with things as we don't have pmem to shmem fallback (although I didn't see that in this log.)  And while it does feel a wee bit like a hack, in lots of cases (probably all these included), it'll probably be faster, as well as use less memory, so it's hard to say no to it.

Comment 31

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

Uplifted 177e4c8c6459d8b85a840354c27e0d3b37ee4329 to:
v1.3: 7aabd6692c5d20edf90c9fdcc20d35a951d416fe

Comment 32

[Julien Wajsberg [:julienw]]

(In reply to Milan Sreckovic [:milan] from comment #30)
> Yes, willReadFrequently is the way to go (for now - see bug 963559 for some
> longer term thoughts.)  1.2 sometimes gets away with things as we don't have
> pmem to shmem fallback (although I didn't see that in this log.)  And while
> it does feel a wee bit like a hack, in lots of cases (probably all these
> included), it'll probably be faster, as well as use less memory, so it's
> hard to say no to it.

Before doing this change I asked Nical to understand more deeply the change. In all the cases we have in SMS, we use Canvas either to resize images (write a big image + read a small one in a small amount of time) or decode a binary image format (write invididual pixels).

Maybe the name "willReadFrequently" is not good enough, or maybe we should have more terms that would also bypass the GPU, eg "willWritePixels" or "willNotDisplay".