Closed Bug 213005 Opened 18 years ago Closed 15 years ago

ASSERTION: OnDataAvailable implementation consumed no data

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: Biesinger, Assigned: darin.moz)

References

()

Details

(Keywords: fixed1.8.1, helpwanted)

Attachments

(1 file, 2 obsolete files)

###!!! 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.
the reason in comment 0 was actually wrong. the Cancel() call affected only
nsHTTPChannel, but not the InputStreamPump which asserts.
-> Networking

I shouldn't get that assertion if I cancel the channel, imho... (even though
OnDataAvailable returns NS_OK)
Assignee: cbiesinger → darin
Component: File Handling → Networking
QA Contact: petersen → benc
Target Milestone: --- → mozilla1.5beta
Target Milestone: mozilla1.5beta → mozilla1.6alpha
Keywords: helpwanted
Target Milestone: mozilla1.6alpha → Future
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
Assignee: darin → file-handling
Component: Networking → File Handling
QA Contact: benc → ian
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);
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.
ok, back to networking:http then ;)
Assignee: file-handling → darin
Component: File Handling → Networking: HTTP
QA Contact: ian → httpqa
*** Bug 267579 has been marked as a duplicate of this bug. ***
Is this a dup of bug 223202?
I meant bug 223302.
This might be fixed by the checkin to bug 223302.
 	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
Attached patch honor mstatus (obsolete) — Splinter Review
Assignee: darin → timeless
Status: NEW → ASSIGNED
Attachment #177829 - Flags: superreview?(darin)
Attachment #177829 - Flags: review?(darin)
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.
Attachment #177829 - Flags: superreview?(darin)
Attachment #177829 - Flags: superreview-
Attachment #177829 - Flags: review?(darin)
Attachment #177829 - Flags: review-
Attached patch propagate Cancel (obsolete) — Splinter Review
Attachment #177829 - Attachment is obsolete: true
Attachment #178095 - Flags: superreview?(darin)
Attachment #178095 - Flags: review?(darin)
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.
Attachment #178095 - Flags: superreview?(darin)
Attachment #178095 - Flags: superreview-
Attachment #178095 - Flags: review?(darin)
Attachment #178095 - Flags: review-
Attached patch v2.1 patchSplinter Review
Here's the patch I had in mind.
Attachment #178095 - Attachment is obsolete: true
Attachment #178643 - Flags: review?(timeless)
Attachment #178643 - Flags: superreview?(roc)
Attachment #178643 - Flags: review?(timeless)
Attachment #178643 - Flags: review+
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)
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.
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.
Assignee: timeless → darin
Status: ASSIGNED → NEW
Attachment #178643 - Flags: superreview?(roc) → superreview+
The patch appears to have not been checked in yet; what's the status?
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.9alpha
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #178643 - Flags: branch-1.8.1?(bzbarsky)
Attachment #178643 - Flags: branch-1.8.1?(bzbarsky) → branch-1.8.1+
fixed1.8.1
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.