213005 - ASSERTION: OnDataAvailable implementation consumed no data

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

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

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

this happens when the user clicks cancel in the helper app dialog... the reason
is that we don't actually cancel the request, we just set a member variable and
cancel it next time OnDataAvailable is called.

Comment 1

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

the reason in comment 0 was actually wrong. the Cancel() call affected only
nsHTTPChannel, but not the InputStreamPump which asserts.

Comment 2

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

-> Networking

I shouldn't get that assertion if I cancel the channel, imho... (even though
OnDataAvailable returns NS_OK)

Comment 3

[Darin Fisher]

if you got that assertion, it means that you did not call Cancel with a failure
code.  see nsInputStreamPump::Cancel... it sets mStatus to the status value that
you pass in.  the assertion is only hit if NS_SUCCEEDED(mStatus) is true.

back to file handling

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is NS_BINDING_ABORTED a success code then?  The code in question over in
nsExternalAppHandler::OnDataAvailable does:

1560   if (mCanceled) // then go cancel our underlying channel too
1561     return request->Cancel(NS_BINDING_ABORTED);

Comment 5

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

when I traced through this code, nsInputStreamPump::Cancel was never called...
only nsHttpChannel and one or two other cancel impls. an event was posted
somewhere which might've been supposed to cancel the pump; but that's of course
async.

Comment 6

[Darin Fisher]

ok, back to networking:http then ;)

Comment 7

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

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

Comment 8

[Aaron Leventhal]

Is this a dup of bug 223202?

Comment 9

[Aaron Leventhal]

I meant bug 223302.

Comment 10

[Aaron Leventhal]

This might be fixed by the checkin to bug 223302.

Comment 11

[timeless]

 	necko.dll!nsHttpChannel::Cancel(unsigned int status=2152398850)  Line 2880	C++
 	docshell.dll!nsExternalAppHandler::OnDataAvailable(nsIRequest *
request=0x03534b48, nsISupports * aCtxt=0x00000000, nsIInputStream *
inStr=0x036c9c50, unsigned int sourceOffset=0, unsigned int count=65536)  Line
1891 + 0x11	C++
 	docshell.dll!nsDocumentOpenInfo::OnDataAvailable(nsIRequest *
request=0x03534b48, nsISupports * aCtxt=0x00000000, nsIInputStream *
inStr=0x036c9c50, unsigned int sourceOffset=0, unsigned int count=65536)  Line
349 + 0x2e	C++
 	necko.dll!nsStreamListenerTee::OnDataAvailable(nsIRequest *
request=0x03534b48, nsISupports * context=0x00000000, nsIInputStream *
input=0x078c1618, unsigned int offset=0, unsigned int count=65536)  Line 97 +
0x33	C++
 	necko.dll!nsHttpChannel::OnDataAvailable(nsIRequest * request=0x03719c98,
nsISupports * ctxt=0x00000000, nsIInputStream * input=0x078c1618, unsigned int
offset=0, unsigned int count=65536)  Line 3918 + 0x44	C++
>	necko.dll!nsInputStreamPump::OnStateTransfer()  Line 437 + 0x46	C++
 	necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *
stream=0x078c1618)  Line 340 + 0xb	C++
 	xpcom_core.dll!nsInputStreamReadyEvent::EventHandler(PLEvent *
plevent=0x0799d544)  Line 119	C++
 	xpcom_core.dll!PL_HandleEvent(PLEvent * self=0x0799d544)  Line 698 + 0xa	C

Comment 12

[timeless]

Attachment: honor mstatus

Comment 13

[Darin Fisher]

Perhaps nsHttpChannel::Cancel should call Cancel on mTransactionPump instead, or
perhaps nsHttpChannel::OnDataAvailable should return mStatus.  Either of those
seems like a better choice than calling Cancel as in the current patch.

Comment 14

[timeless]

Attachment: propagate Cancel

Comment 15

[Darin Fisher]

Comment on attachment 178095 [details] [diff] [review]
propagate Cancel

my concern with this patch is that there are edge cases in which
mTransactionPump may be null-valued when mTransaction is not.  I'm thinking
about cases where SetupTransaction fails to allocate mTransactionPump, and then
for some reason the consumer still calls Cancel.

I think it might be better to just run down the list of objects to cancel and
cancel any that are non-null.

Comment 16

[Darin Fisher]

Attachment: v2.1 patch

Here's the patch I had in mind.

Comment 17

[timeless]

Comment on attachment 178095 [details] [diff] [review]
propagate Cancel

i'm running w/:
--
RCS file: /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v
retrieving revision 1.250
@@ -3003,8 +3015,10 @@
     mStatus = status;
     if (mProxyRequest)
	 mProxyRequest->Cancel(status);
-    else if (mTransaction)
+    else if (mTransaction) {
+	 mTransactionPump->Cancel(status);
	 gHttpHandler->CancelTransaction(mTransaction, status);
+    }
     else if (mCachePump)
	 mCachePump->Cancel(status);
     return NS_OK;
--
and i'm getting this assert, 

uri = "http://www.metanexus.net/conference2003/pdf/WOLPaper_Uelmen_Amelia.pdf"

i'm pretty sure it's simple to reproduce using mfcembed on a system w/o a pdf
plugin (just load a pdf and click cancel in the what do you want to do thing)

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

timeless, do you think this patch should go in, or not? I'm not sure of the
relative seriousness of the new assert you're talking about.

Comment 19

[timeless]

it's the same assert, i think that the patch is an improvement since it gives 
certain objects a notification, i am just uncertain that it fixes all flavors 
of the problem.

Comment 20

[Alex Vincent [:WeirdAl]]

The patch appears to have not been checked in yet; what's the status?

Comment 21

[Darin Fisher]

fixed-on-trunk

Comment 22

[Darin Fisher]

fixed1.8.1