286 bytes, patch
|Details | Diff | Splinter Review|
While browsing, I triggered this assertion: http://lxr.mozilla.org/seamonkey/source/gfx/src/nsImageNetContextAsync.cpp#367 The error was generated here: http://lxr.mozilla.org/seamonkey/source/netwerk/cache/memcache/nsMemCacheChannel .cpp#164 Unfortunately I can't reproduce the assertion, but I think the problem is clear. Diagnosis: the memcache stream had been aborted (don't know why), nsImageNetContextAsync tries to Read the stream, gets the NS_ERROR_ABORT result, then checks to see if any data was lost, but nb is uninitialized so it thinks data's been lost when it really hasn't. Recommendation: I looked at several of the other stream implementations and they don't put a meaningful value into aBytesRead unless the Read is successful. Therefore the code in nsImageNetContextAsync should not be relying on aBytesRead being initialized in the failure case. I suggest that ImageConsumer::OnDataAvailable should initialize nb to 0.
Created attachment 6345 [details] [diff] [review] patch to initialize nb in ImageConsumer::OnDataAvailable
19 years ago
This hadn't been fixed as of 20000511. Confirming. Robert O'Callahan - thanks :-) firstname.lastname@example.org - this one's a one-line fix for you :-) Gerv
Status: UNCONFIRMED → NEW
Ever confirmed: true
As the fix is is in image code I am going to reassign. The fix looks good to me if anyone cares.
Component: Networking: Cache → ImageLib
forgot to reassign
Assignee: gordon → pnunn
QA Contact: tever → elig
Sounds pretty straight forward to me. -P
Status: NEW → ASSIGNED
Target Milestone: --- → M16
checked in. Thanks for catching it Robert. -p
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
Robert, might you care to mark this as verified if you're satisfied with the solution? Thanks!
I'm not sure if the fix is correct or not. It seems to me that the patch that pnunn checked in could still lead to problems: suppose we go once around the DO loop, setting "nb" to some positive number, then fail in pIStream->Read: should we then assert, or not? I don't really know this code well enough to know when the assertions should fire. I'm not even sure why they're there at all. It seems to me that if we fail in pIStream->Read with NS_BASE_STREAM_WOULD_BLOCK, we will safely break out of the loop and the rest of the data can be read later. If we fail with some other error, we give up on the image so it doesn't matter if we lost data or not. However, I don't have a smoking gun pointing to a bug here, so if someone comes up with a plausible explanation for this, or just tells me to VERIFY this bug, I will :-).
Pam says that this is fixed, and that you might be seen other cache bugs, of which several are already known to exist. So, I'm marking this as verified for lack of any other resolution that I can justify. ;)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.