Hard-to-reproduce assertion, looks like minor cache bug

VERIFIED FIXED in M16

Status

()

Core
ImageLib
P3
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: roc, Assigned: pnunn)

Tracking

Trunk
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
Keywords: patch
This hadn't been fixed as of 20000511. Confirming.

Robert O'Callahan - thanks :-)

gordon@netscape.com - this one's a one-line fix for you :-)

Gerv
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

18 years ago
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

Comment 4

18 years ago
forgot to reassign
Assignee: gordon → pnunn
QA Contact: tever → elig
(Assignee)

Comment 5

18 years ago
Sounds pretty straight forward to me.
-P
Status: NEW → ASSIGNED
Target Milestone: --- → M16
(Assignee)

Comment 6

18 years ago
checked in.
Thanks for catching it Robert.
-p
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 7

18 years ago
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 :-).

Comment 9

18 years ago
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.