Closed Bug 843391 Opened 12 years ago Closed 12 years ago

Gallery crashes with OOM when scanning many images without previews

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-b2g:-, b2g18+ fixed)

RESOLVED FIXED
blocking-b2g -
Tracking Status
b2g18 + fixed

People

(Reporter: djf, Assigned: djf)

Details

(Keywords: crash, Whiteboard: [b2g-crash])

Attachments

(2 files)

When the gallery app finds new images on the sdcard, it needs to create a thumbnail for each one. If the image includes a JPEG embedded preview, it reads the preview image and creates the thumbnail from that so that it doesn't have to decode the full size image. But if the image has no preview, then it must decode the entire image in order to make a thumbnail of it. jhylands recently discovered that if he puts 250 images (without previews) onto the phone and launches the gallery, it crashes. (With an OOM, I think). You can replicate this by unzipping the attached file and pushing the resulting images to the sdcard. We've been through this before with the gallery app. Apparently gecko does not free its image memory right away when I do image.removeAttribute('src'). In order to avoid the crash, I have to insert a 300ms delay between the processing of each of the images. (200ms is not enough). That is going to slow the gallery scan process way down when a users puts lots of images without previews on the phone. But slow is better than crashing I suppose.
Cc'ing Jon because he found the bug and Cc;ing Chris and Justin because I think I discussed the underlying gecko problem (not freeing image memory promptly) with them months ago. Anyone have any better ideas than just inserting a 300ms after processing an image that does not have an embedded preview? Did we fix this in Gecko somehow months ago and then just recently regress it recently in a quest for speed? I vaguely recall some discussion of changing some default in the way gecko handled images. They were going to be pinned or fixed or something like that... Does that ring any bells and does it have any relevance here?
Ah. Bug 792139 is related. Did we ever set image.mem.max_decoded_image_kb to 0? IIRC that was the proposed solution last time this came up. I have no idea what the other performance implications of changing that setting might be, so I'm reluctant to change it for gallery. Actually, given that that setting is only 5mb, that's not the issue here, because we exceed that by decoding just a single image. I wonder about this line in b2g.js: pref("image.mem.min_discard_timeout_ms", 10000); Ideas? I'm setting the crash keyword and nominating this as a leo blocker.
blocking-b2g: --- → leo?
Keywords: crash
Jon, I'm asking you for review because you found the bug and will be best able to determine whether this patch fixes the crash. Chris: I'm asking you for feedback because of the performance implications (scanning each image that does not have a preview will take 400ms longer than it does now). That doubles or triples the scan time. It doesn't affect images with previews like the ones our camera app generates, though.
Assignee: nobody → dflanagan
Status: NEW → ASSIGNED
Attachment #716295 - Flags: review?(jhylands)
Attachment #716295 - Flags: feedback?(jones.chris.g)
> pref("image.mem.min_discard_timeout_ms", 10000); We could try setting this to a small value. I'd be much more comfortable making that change than setting max_decoded_image_kb to 0, since lowering that value has caused all sorts of problems on desktop. Someone needs to understand exactly what path imagelib goes through when you unset an image's src, and how quickly it should be discarding that data. We should do that instead of tweaking prefs without first understanding what's going on.
Severity: normal → critical
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [b2g-crash]
Comment on attachment 716295 [details] link to patch on github As a workaround I'm ok with this. If images with previews are the common case we'll be in better shape. f? -> jlebar on whether we should hack here or make an eleventh hour effort to tweak the discard pref.
Attachment #716295 - Flags: feedback?(justin.lebar+bug)
Attachment #716295 - Flags: feedback?(jones.chris.g)
Attachment #716295 - Flags: feedback+
Whatever we can do to avoid changing image things at the eleventh hour sounds good to me. It's really hard to change anything in imagelib without regressing things.
Attachment #716295 - Flags: feedback?(justin.lebar+bug) → feedback+
(In reply to Justin Lebar [:jlebar] from comment #4) > Someone needs to understand exactly what path imagelib goes through when you > unset an image's src, and how quickly it should be discarding that data. We > should do that instead of tweaking prefs without first understanding what's > going on. Does removing 'src' behave differently than 'src=""'? I seem to remember that it does.
Comment on attachment 716295 [details] link to patch on github It looks good - I've been running with it all day, and haven't been able to reproduce the issue at all.
Attachment #716295 - Flags: review?(jhylands) → review+
Strangely, I can't reproduce the crash anymore with the latest OTA update. Has something changed in Gecko or Gonk to make apps more robust against OOM errors? Using adb shell b2g-ps, I can still observe better memory behavior (fewer other apps will get killed) with the patch than without it, so I'm still going to land it. Kyle: the app currently used removeAttribute('src') and this seems to give better memory behavior than src=''. With removeAttribute, memory usage doesn't go over 200,000 (whatever the b2g-ps VSIZE units are). If I use src='' instead, I get values as high as 220,000. Setting src=null might be better than src='', but does not seem to be as good as removeAttribute('src') (though I haven't tested this as carefully).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
> Someone needs to understand exactly what path imagelib goes through when you unset an > image's src, and how quickly it should be discarding that data. FYI, if you set img.src='', that should immediately discard the image. At least, I'm pretty sure, based on how this code in nsImageLoadingContent looks... :)
blocking-b2g: leo? → -
tracking-b2g18: --- → +
Uplifted commit 6f39ed14aa44b072157188e51ef3bb9ce9ba098d as: v1-train: 096deb01d0b78360e6a3158fbb0c3d6928992542
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: