Closed Bug 460767 Opened 13 years ago Closed 13 years ago
Crash in img
Request .cpp [@ img Request::Notify Proxy Listener]
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.
Attachment #343913 - Flags: review?(pavlov)
brad, should we be testing for low memory (IsLowMemory()) in the image decoder? We are testing this predicate in nsThebesImage.cpp.
Risk: pretty low, just adds a null check Reward: avoids potential crash in low memory situations
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
Attachment #343913 - Flags: review?(joe)
Comment on attachment 343913 [details] [diff] [review] checks frame with NS_ENSURE_TRUE Keep joe in the loop on imglib bugs, pls!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 457496
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.
Attachment #343913 - Flags: review?(joe) → review+
NS_ASSERTIONs are supposed to be fatal in debug builds. so this patch really isn't ok :)
In that case, Brad, just swap the NS_ENSURE_TRUE for the NS_ASSERTION.
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Whiteboard: [needs landing]
carrying reviews from pav and joe
Comment on attachment 349750 [details] [diff] [review] swapped NS_ASSERTION for NS_ENSURE_TRUE a191b2=beltzner for fixing a topcrasher
Attachment #349750 - Flags: approval1.9.1b2+
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.
Whiteboard: [needs landing]
the patch has been pushed to m-c, I'm waiting for it to cycle through the tinderboxes before closing the bug
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Crash reporter doesn't list any crashes anymore on trunk. Marking as verified and adjusting some flags.
Severity: normal → critical
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.1b2
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. :) )
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?
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 on attachment 353610 [details] [diff] [review] cvs patch Approved for 22.214.171.124, a=dveditz for release-drivers.
Attachment #353610 - Flags: approval126.96.36.199? → approval188.8.131.52+
it is possible for that stuff to be null only in out of memory cases. should be safe to error out there
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
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 verified184.108.40.206 and open a new bug if the crash will continue after the release? 3.1 and 3.2 still looks fine.
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).
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.
Summary: Crash in imgRequest.cpp → Crash in imgRequest.cpp [@ imgRequest::NotifyProxyListener]
Crash Signature: [@ imgRequest::NotifyProxyListener]
You need to log in before you can comment on or make changes to this bug.