782428 - [otoro] b2g process getting killled when viewing images in gallery app

Status: RESOLVED WONTFIX

(Core :: Graphics: CanvasWebGL, defect)

Description

[[e-mail] [:onecyrenus]]

Otoro: 

Noticed an issue where if you view images too quickly in the gallery app the phone will crash. 

The only output that makes it to stdout/stderr / logcat is 

"Killed" 

This is reproducible by opening the camera app selecting an image, and then repeatedly & rapidly clicking on a thumbnail above to load an image.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is probably the b2g master process being OOM-killed.

It's because of problems like this that we need to move the gallery et al. OOP.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

dhylands has a patch to move gallery OOP.  That should prevent the master-process crashes.  Please retest after that lands.

The fundamental problem here is gecko being dumb about decoded images.  There are bugs on file to actually fix that, and a gaia bug on working around gecko dumb-ness.

Comment 3

[[e-mail] [:onecyrenus]]

Chris do you have a bug #

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

dhylands do you have a PR up?

Comment 5

[[e-mail] [:onecyrenus]]

Sounds like there are several fixes in the work, and we need a usability bug to track those fixes. 

Might want to add those bugs as blockers to this functionality.

Comment 6

[Dave Hylands [:dhylands]]

https://github.com/mozilla-b2g/gaia/pull/3416

I added s bunch more bugs. Gallery is the only app I could remove from the blacklist.

Comment 7

[Justin Lebar (not reading bugmail)]

> The fundamental problem here is gecko being dumb about decoded images.

Overall, bug 683284.  In particular, bug 689623.  You'll notice these are both old bug; they're unresolved because imagelib and layout resources were taken away from MemShrink priorities and directed towards native Fennec.  It is not feasible at this point to fix these issues for V1.

Although we can fix the gallery app, we can't fix random websites which display images and cause us to OOM.  At this point, the only feasible fix to this general problem for B2G v1 that I'm aware of is to add more RAM to our device.

Comment 8

[Andreas Gal :gal]

Justin, help me understand why having more memory solves this problem. Can't content just as well OOM 512MB and 512GB, if the page is dumb enough? Isn't Chris' suggestion the right one here, moving content OOP and ideally also fixing the MemShrink issues.

Comment 9

[Justin Lebar (not reading bugmail)]

> Justin, help me understand why having more memory solves this problem.  Can't content just as well 
> OOM 512MB and 512GB, if the page is dumb enough? 

Yes, of course.

But my point is that even if all apps are OOP, the content process will still crash if it OOMs.  That's better than a whole-device crash, but it's still a bad experience.  Adding more RAM to the device, particularly considering how memory-starved it is at the moment, would likely mean fewer fewer content process crashes.

Comment 10

[Andrew Overholt [:overholt]]

David, have you been seeing gallery crashes during your testing?

Comment 11

[David Flanagan [:djf]]

I haven't been seeing it, but I haven't been trying to reproduce this, either. Most of my recent testing has been on the desktop.

Gallery just went OOP yesterday, which may have helped with this, but now it has a bunch of indexeddb problems that might be masking symptoms of this issue.

cjones: I've lost track of the gaia bug you reference in comment #2.  Do you have a link?  iirc, the workaround was to reuse <img> elements and change their src attribute rather than creating new img elements when the user pans to a new page.  I'm pretty sure the gallery app does that.  Maybe there's something more I need to do to avoid the "gecko being dumb" issue?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, I can't find it again either.

jlebar is the local expert on this.

Comment 13

[Justin Lebar (not reading bugmail)]

> I've lost track of the gaia bug you reference in comment #2.  Do you have a link?

I searched for "gallery jlebar" and found [1].  Is that what you were looking for?

https://github.com/mozilla-b2g/gaia/issues/2109

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That's the one!  Thanks.

Comment 15

[David Flanagan [:djf]]

Thanks jlebar.  Supposedly, I fixed that bug... But rereading it reminds me about adb shell procrank.  Maybe we can use that to determine whether it is a memory issue that is causing this crash.

dclarke: in your original report you first mention the gallery app and then mention the camera when describing how to reproduce. Was that a typo? Please confirm that this is a gallery bug not a camera bug.

Comment 16

[[e-mail] [:onecyrenus]]

I meant gallery.  This is a gallery browsing bug. I wasn't sure if this was an oom issue specifically as I found that if i clicked slower, then the crash did not surface.

Comment 17

[[e-mail] [:onecyrenus]]

My gut feeling is that this bug is probably due to us trying to gather or render multiple images at the same time.

Comment 18

[Justin Lebar (not reading bugmail)]

(I presume you didn't mean to change the summary back?)

Comment 19

[David Flanagan [:djf]]

Now that gallery is OOP, I can't crash the phone, but I can crash the gallery app by following dclarke's steps.  It takes a while, but eventually the screen goes blank white.  adb shell dmesg shows [844: plugin-containe]send sigkill to 844 (plugin-containe), adj 1, size 24669

This bug on github is related: https://github.com/mozilla-b2g/gaia/issues/3407
It produces the same white screen and sigkill message in a different way

Comment 20

[David Flanagan [:djf]]

Using procrank seems to support the idea that this is an OOM situation.  I saw the usage of plugin_container spike to 90mb shortly before the whitescreen.

Since the steps to reproduce this involve rapidly switching images, I suspect the problem is that I end up requesting images from device storage faster than they can be returned. I'm guess that is where the leak is... not in image handling, but in having too many blobs in the air at once.  The related github bug occurs when switching in and out of the gallery, causing (I think) multiple device storage scans to be in progress at once.  So again, lots of blobs flying around.

The multiple scan issue will go away now that device storage is sending change events. Maybe I'll see if I can make this bug go away by cancelling pending requests when a new one comes in, or by just locking the UI so that the user can't switch images so quickly.  Maybe it just shouldn't repond to touches while it is still trying to handle a previous touch.

Comment 21

[David Flanagan [:djf]]

I've modified the code in Gallery so it doesn't got out and try to load a bunch of images in parallel if you click on thumbnails quickly. But I can still crash the process to a white screen...

Comment 22

[David Flanagan [:djf]]

Attachment: reduced test case

The attached patch modifies the test_apps/template app to an app named Thrash.
When you start it, it looks for all ".jpg" files in /sdcard/DCIM (or in ~/Pictures on macos) and displays them one at a time as fast as it can.

It crashes after about 30 images for me.

If I add a 500ms delay between images it does not crash.

If I make the image display:none, it does not crash

If I just call Object.createObjectURL() once for each file and always use the same blob URL without ever calling revokeObjectURL() it does not crash... Possibly a sign that the blob is getting cached somewhere?

I don't see any memory leaks in my JavaScript code, so I think that the issue is in the platform.  There are known problems with images and memory that won't be fixed for v1, and if this is one of those, I'll have to figure out a workaround.

I'm using the onload event on an image before I go start loading the next.  If there was some other event that meant "the image is actually displayed now" I could wait for that instead, and that might work better than inserting a 500ms delay....

Comment 23

[David Flanagan [:djf]]

Anyone see a memory leak in the crash.js file in the attached patch file?  Am I doing something wrong, or is this a platform bug?

Anyone qualified to figure out why it crashes if I do createObjectURL/revokeObjectURL each time but doesn't if I just cache teh blob:// URL of each image?  Who needs to be cc'ed to look into that?

Comment 24

[Justin Lebar (not reading bugmail)]

Kyle touched this code last; maybe he has some insight.

One thing we want to determine is what is taking up all this memory.  The obvious candidate is decoded images, but with a recent gecko (past day or two) plus image.mem.max_decoded_image_kb == 20480, we should drop decoded images almost immediately on memory pressure.

But if it crashes after 30 images and your /encoded/ images are 3mb each, it's possible that we're crashing due to storing those encoded JPEGs in memory (maybe in a Necko cache).

Basically, I would want to see an about:memory dump from as close to a crash as you can get it.  Unfortunately about:memory doesn't work on b2g atm (bug 767191).

In terms of working around the issue, I'd consider putting an artificial delay into the gallery app after you've gone quickly through a few images.

Comment 25

[Andreas Gal :gal]

How large is our in-memory network cache?

Comment 26

[Andreas Gal :gal]

Answering my own question here. I think something else is up here. I would bet JS keeps a pointer alive to a XPCOM Image element and we don't GC in time and the content process dies.

pref("browser.cache.memory.enable", true);
pref("browser.cache.memory.capacity", 1024); // kilobytes                                                                                                                       

/* image cache prefs */
pref("image.cache.size", 1048576); // bytes

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #24)
> Basically, I would want to see an about:memory dump from as close to a crash
> as you can get it.  Unfortunately about:memory doesn't work on b2g atm (bug
> 767191).
> 

Can we dump that to stdout/stderr/logcat?  That would be easier to work with anyway.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

... especially given that about:memory uses a significant amount of memory itself.

Comment 29

[Justin Lebar (not reading bugmail)]

Yes, dumping to text would be relatively straighforward.  See bug 768470.

Comment 30

[David Flanagan [:djf]]

(In reply to Andreas Gal :gal from comment #26)
> Answering my own question here. I think something else is up here. I would
> bet JS keeps a pointer alive to a XPCOM Image element and we don't GC in
> time and the content process dies.
> 

The test app has only one Image element, and just keeps changing its src property.  So no GC on the element itself.  I converted the gallery app to use static image elements some time ago to fix a slow memory leak.

> pref("browser.cache.memory.enable", true);
> pref("browser.cache.memory.capacity", 1024); // kilobytes                   
> 
> 
> /* image cache prefs */
> pref("image.cache.size", 1048576); // bytes

Are these prefs that I should set or experiment with?

Comment 31

[David Flanagan [:djf]]

(In reply to Justin Lebar [:jlebar] from comment #24)
> Kyle touched this code last; maybe he has some insight.
> 
> One thing we want to determine is what is taking up all this memory.  The
> obvious candidate is decoded images, but with a recent gecko (past day or
> two) plus image.mem.max_decoded_image_kb == 20480, we should drop decoded
> images almost immediately on memory pressure.

I now have today's nightly and have that pref set, and am still seeing the same crash.

> 
> But if it crashes after 30 images and your /encoded/ images are 3mb each,
> it's possible that we're crashing due to storing those encoded JPEGs in
> memory (maybe in a Necko cache).
> 

The images are all < 3mb as .jpeg files, so that's the the source.

The slideshow that does this stress test just cycles through all of the images on the device.  If I create a blob: url for each image in advance and always refer to the image by that blob url, then I don't get the crash. Like maybe the image is being copied from its blob into a cache or something?

But if I create a new blob: URL for each image, display the image and then release the URL, I get the crash.

I'm not sure what to make of that, but it seems interesting...

> Basically, I would want to see an about:memory dump from as close to a crash
> as you can get it.  Unfortunately about:memory doesn't work on b2g atm (bug
> 767191).
> 
> In terms of working around the issue, I'd consider putting an artificial
> delay into the gallery app after you've gone quickly through a few images.

I wonder if there is any way to detect when the image element repaints itself with the image pixels.  The onload event handler comes too early...  It makes me nervous to fix this with a timing hack...

Comment 32

[Justin Lebar (not reading bugmail)]

> I wonder if there is any way to detect when the image element repaints itself with the 
> image pixels.

I'm pretty sure we don't expose this to webpages.

>> But if it crashes after 30 images and your /encoded/ images are 3mb each,
>> it's possible that we're crashing due to storing those encoded JPEGs in
>> memory (maybe in a Necko cache).
> 
>
> The images are all < 3mb as .jpeg files, so that's the the source.

Sorry, do you mean the images are all smaller than 3mb or larger than 3mb, compressed?

Comment 33

[David Flanagan [:djf]]

(In reply to Justin Lebar [:jlebar] from comment #32)
> > I wonder if there is any way to detect when the image element repaints itself with the 
> > image pixels.
> 
> I'm pretty sure we don't expose this to webpages.
> 
> >> But if it crashes after 30 images and your /encoded/ images are 3mb each,
> >> it's possible that we're crashing due to storing those encoded JPEGs in
> >> memory (maybe in a Necko cache).
> > 
> >
> > The images are all < 3mb as .jpeg files, so that's the the source.
> 
> Sorry, do you mean the images are all smaller than 3mb or larger than 3mb,
> compressed?

Oops.  The images are all smaller than .5mb, actually, so that's not the source.  Serious brain/finger disconnect when I tried to type that the first time...

Justin, if I'm going to introduce an artificial timing delay into my app, is there a bug # (other than this one) I can cite in the code?

Comment 34

[Justin Lebar (not reading bugmail)]

> Justin, if I'm going to introduce an artificial timing delay into my app, is there a bug 
> # (other than this one) I can cite in the code?

I don't think so.  It's looking like you have found a legit Gecko bug here, but it's hard to say for sure without the about:memory dump.

Comment 35

[David Flanagan [:djf]]

I can still reproduce this crash with the attached test.  If this is a gecko bug as jlebar suggests, then I'm not the right one for it to be assigned to. Anyone want to take it?

Meanwhile, in Gaia, I'll insert an artificial delay, or alter the UX in some other way so that user's can't switch images nearly so quickly so that we don't trigger this bug.

Comment 36

[David Flanagan [:djf]]

Okay, I'm addressing the Gaia symptoms here https://github.com/mozilla-b2g/gaia/issues/4475 and have unassigned myself from this bug, so we need someone else to investigate the underlying Gecko cause.

Comment 37

[Justin Lebar (not reading bugmail)]

If we work around this in Gaia, I don't think it should be a basecamp blocker.

Comment 38

[Andrew Overholt [:overholt]]

David Flanagan, will the Gaia workarounds be sufficient?

Comment 39

[[e-mail] [:onecyrenus]]

Agreed this is no longer a blocker in the short term, but others might want to build a gallery app, so we should try to fix, or at least put the limitation at the api layer.

Comment 40

[David Flanagan [:djf]]

Yes, we can work around this in Gaia.  This bug does not need to block.  (In fact, I have just worked around it as a side effect of removing the "filmstrip" from the Gallery.  That was the only UX element that allowed the user to switch photos quickly enough to trigger the bug.)

Comment 41

[Andrew Overholt [:overholt]]

(In reply to David Flanagan [:djf] from comment #40)
> Yes, we can work around this in Gaia.  This bug does not need to block.

Thanks, David.

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Mass closing as we are no longer working on b2g/firefox os.

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Mass closing as we are no longer working on b2g/firefox os.