Closed Bug 460767 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: ImageLib, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Keywords: crash, fixed1.9.0.6, mobile)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached patch checks frame with NS_ENSURE_TRUE (obsolete) — Splinter Review
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.
Attachment #343913 - Flags: review?(pavlov) → review+
Risk: pretty low, just adds a null check
Reward: avoids potential crash in low memory situations
Flags: blocking1.9.1?
Attachment #343913 - Flags: approval1.9.1b2?
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 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.
Blocks: 457496
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Whiteboard: [needs landing]
Status: REOPENED → ASSIGNED
carrying reviews from pav and joe
Assignee: nobody → blassey
Attachment #343913 - Attachment is obsolete: true
Attachment #349750 - Flags: review+
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+
Attachment #349750 - Attachment is patch: true
Attachment #349750 - Attachment mime type: application/octet-stream → text/plain
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.
Keywords: checkin-needed
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
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/66aec2d9ab08
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 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
Flags: blocking1.9.1?
Keywords: crash
Target Milestone: --- → mozilla1.9.1b2
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.6+
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. :) )
Attached patch cvs patchSplinter Review
Attachment #353610 - Flags: approval1.9.0.6?
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 on attachment 353610 [details] [diff] [review]
cvs patch

Approved for 1.9.0.6, a=dveditz for release-drivers.
Attachment #353610 - Flags: approval1.9.0.6? → approval1.9.0.6+
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
Keywords: fixed1.9.0.6
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.
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.