227976 - nsHttpCompressConv can fail to read any data in OnDataAvailable and return success

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Bob Clary [:bc] (inactive)]

This bug is spun off of bug 223302

The symptom of the problem is an assert

###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error', 
file
c:/work/mozilla_source/trunk/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
line 451

This occurs for me on the above URL using a satellite broadband connection which
is characterised by fast download speads, slow upload speeds and high latency
and potentially effects due to the satellite NOC.  Note that this error did not
occur on a fresh SuSe install with 2003120809 build even on satellite, so this
is WinXP or Windows only I guess.

It does not occur when testing against a local web server. 

With bz's help it turns out that nsHTTPCompressConv::OnDataAvailable can have

aCount > 0 with mStreamEnded == 1 at line 137. 
<http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsHTTPCompressConv.cpp#137>

Comment 1

[Boris Zbarsky [:bzbarsky]]

Just to clarify further, bc's printf at that point showed:

aCount == 2459, mStreamEnded == 0
aCount == 3, mStreamEnded == 1

(so we may be partway through reading the gzip footer or whatever at that point....)

Comment 2

[Bob Clary [:bc] (inactive)]

I am not seeing this with the most recent version of the dom ts. I will close it
out if I can't reproduce it anywhere.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that this is showing up on a real-world web page in bug 245608...

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

only reading no data triggers this assertion; clarifying summary

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

should this code just read the data and throw it away if mStreamEnded (basically
a ReadSegments that sets *aReadCount to aCount)? I don't know what this variable
does...

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'm not quite sure either, but it may just indicate that the data stream is done
and that what's left is either metadata or padding of some sort....  I suspect
that throwing it out is in fact the safe thing to do.

Comment 7

[Darin Fisher]

*** Bug 245608 has been marked as a duplicate of this bug. ***

Comment 8

[Darin Fisher]

Attachment: v1 patch

Implements biesi's suggested fix.  Verified with URL in duplicate bug.	This
seems to do the trick.

Comment 9

[Darin Fisher]

fixed-on-trunk

cvs checkin comment attributed patch suggestion to boris.. sorry biesi! ;-)

Comment 10

[benc]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsHTTPCompressConv.cpp&branch=&root=/cvsroot&subdir=mozilla/netwerk/streamconv/converters&command=DIFF_FRAMESET&rev1=1.21&rev2=1.22

Comment 11

[Boris Zbarsky [:bzbarsky]]

*** Bug 222457 has been marked as a duplicate of this bug. ***

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

would this be a good thing for 1.7.6?

Comment 13

[Uri Bernstein (Google)]

*** Bug 292413 has been marked as a duplicate of this bug. ***

Comment 14

[Bob Clary [:bc] (inactive)]

fyi, i added basic auth to the dom-ts site to keep search engine bots from
eating my bandwidth. the user/password are domts/domts.

Comment 15

[Boris Zbarsky [:bzbarsky]]

*** Bug 297545 has been marked as a duplicate of this bug. ***

Comment 16

[Boris Zbarsky [:bzbarsky]]

*** Bug 225708 has been marked as a duplicate of this bug. ***

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 302541 has been marked as a duplicate of this bug. ***