460767 - Crash in imgRequest.cpp [@ imgRequest::NotifyProxyListener]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: checks frame with NS_ENSURE_TRUE

the frame we get back from GetCurrentFrame is null and we crash when we attempt to use it.  I suspect this is due to an out of memeory condition as I'm seeing it when loading a large bmp on a mobile device.  

putting a NS_ENSURE_TRUE around frame allows us to fail soft instead of crash.

Comment 1

[Doug Turner (:dougt)]

brad, should we be testing for low memory (IsLowMemory()) in the image decoder?  We are testing this predicate in nsThebesImage.cpp.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Risk: pretty low, just adds a null check
Reward: avoids potential crash in low memory situations

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 343913 [details] [diff] [review]
checks frame with NS_ENSURE_TRUE

Risk: pretty low, just adds a null check
Reward: avoids potential crash in low memory situations

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 343913 [details] [diff] [review]
checks frame with NS_ENSURE_TRUE

Keep joe in the loop on imglib bugs, pls!

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 343913 [details] [diff] [review]
checks frame with NS_ENSURE_TRUE

I'm not 100% on having both an NS_ASSERTION and NS_ENSURE_TRUE, but there's no obvious harm in it either.

Comment 7

[timeless]

NS_ASSERTIONs are supposed to be fatal in debug builds. so this patch really isn't ok :)

Comment 8

[Joe Drew (not getting mail)]

In that case, Brad, just swap the NS_ENSURE_TRUE for the NS_ASSERTION.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

We should better mark this blocking bug 457496 as duping it against. It doesn't make sense to have a patch on a duped bug which needs check-in.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: swapped NS_ASSERTION for NS_ENSURE_TRUE

carrying reviews from pav and joe

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349750 [details] [diff] [review]
swapped NS_ASSERTION for NS_ENSURE_TRUE

a191b2=beltzner for fixing a topcrasher

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Brad, please respect for the next time to not create a new bug for own patches. It's really important to search on Bugzilla first for already existent bug reports. You should attach your patch there. Thanks.

Adding keyword so it will be checked-in asap.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

the patch has been pushed to m-c, I'm waiting for it to cycle through the tinderboxes before closing the bug

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

http://hg.mozilla.org/mozilla-central/rev/66aec2d9ab08

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Crash reporter doesn't list any crashes anymore on trunk. Marking as verified and adjusting some flags.

Comment 16

[Samuel Sidler (old account; do not CC)]

Marked blocking for bug 457496, which is presumably the topcrash we see on 1.9.0.

Brad, can you please attach a 1.9.0 patch? (That's CVS trunk. :) )

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: cvs patch

Comment 18

[Daniel Veditz [:dveditz]]

My only concern is that the original coder thought frame could never be null, and was obviously wrong. While this change fixes the crash at this location, are we sure that we haven't violated some basic assumption in the code and are just kicking the can of failure down the road? Could this turn a safe null-deref crash into an unsafe crash somewhere else?

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

the original coder is pav, adding him to the CC

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/modules/libpr0n/src&command=DIFF_FRAMESET&file=imgRequest.cpp&rev2=1.39&rev1=1.38

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 353610 [details] [diff] [review]
cvs patch

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 21

[Stuart Parmenter]

it is possible for that stuff to be null only in out of memory cases.  should be safe to error out there

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Checking in modules/libpr0n/src/imgRequest.cpp;
/cvsroot/mozilla/modules/libpr0n/src/imgRequest.cpp,v  <--  imgRequest.cpp
new revision: 1.110; previous revision: 1.109

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Samuel, I've taken a look at Socorro and there is one crash listed within the last two weeks, It occured with a build after the check-in on 1.9.0 branch: bp-b0af2029-7a19-4d0f-bdbe-e6caa2090112

Shall we mark this bug as verified1.9.0.6 and open a new bug if the crash will continue after the release? 3.1 and 3.2 still looks fine.

Comment 24

[Samuel Sidler (old account; do not CC)]

This wasn't a topcrash so it's unlikely that you'll see many crashes at all. I don't think there's any easy way to verify this (and that's probably okay).

Comment 25

[timeless]

fwiw, a friend hit this crash while using:
http://timeless.justdave.net/stress/stress.html

yes, he was using 3.0.5, he eventually got an upgrade notice and upgraded to 3.0.19 and then was told (by the landing page) to upgrade to 3.6.3.