1191981 - "ASSERTION: cancel with non-failure status code" with XMLHttpRequest, moz-blob

Status: RESOLVED DUPLICATE of bug 1337746

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: cancel with non-failure status code: 'NS_FAILED(status)', file netwerk/base/nsInputStreamPump.cpp, line 210

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Aidin Gharibnavaz]

The bug interestes me, and I investigate its history.

The problem is here:
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#1965
When the response type is Blob or MozBlob, it cancel the request with NS_OK state. Which cause the assertion.

It's done in Bug 689008. In comment five of that bug, it said that we support "Cancel(NS_OK)". But seems the assertion was exist long before that. I couldn't understand this.

Some more info:
1. No one else did "Cancel(NS_OK)" on an XHR request:
https://dxr.mozilla.org/mozilla-central/search?q=Cancel%28NS_OK%29

2.Seems that others saw this assertion before, and just ignore it:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_video_discovery.html?offset=100#125

Comment 4

[Masatoshi Kimura [:emk]]

I think we should just drop support for moz-blob once we've implemented the Streams spec.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Could you check if the assertion is also triggered with "blob" rather than "moz-blob"?

Comment 6

[Masatoshi Kimura [:emk]]

Hm, you're right. But I don't know if the assertion is valid or not. Maybe netwerk peers would have more insight.

Comment 7

[Jason Duell]

My understanding is that necko clients may use "Cancel(NS_OK)" when they want to tell necko that the data transfer could be stopped (and the network connection could be closed, etc), but that the channel was not to be considered a failure.  (in the Blob case it looks like we don't need to serve the actual data immediately?  Or we're getting it from elsewhere?)

I've never been a huge fan of this semantic--it makes the necko code a little tricker (IIRC there are lots of places where we check mStatus and equate !NS_OK with cancelation/failure).  But I'm willing to support it if it's needed. And from a glance at the code in question (now living in XMLHttpRequestMainThread::OnDataAvailable(), it looks like we're only reading from the channel because the nsIStreamListener contract mandates it, and then we cancel.

So my hunch is that we should remove the assertion in nsInputStreamPump::Cancel().  Honza, what do you think?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #7)
> So my hunch is that we should remove the assertion in
> nsInputStreamPump::Cancel().  Honza, what do you think?

Hard to say.  But:

https://dxr.mozilla.org/mozilla-central/rev/f4f374622111022d41dd8d5eb9220624135c534a/netwerk/base/nsInputStreamPump.cpp#207

And mAsyncStream->CloseWithStatus expects that too.  So, no we can't.

the caller has to migrate to NS_ERROR_SOMETHING and just ignore the error in OnStopRequest, if that is the reason why NS_OK was used.  Apparently, there is no test for this, since it would be perma-crashing on try....

> now living in XMLHttpRequestMainThread::OnDataAvailable() 
https://dxr.mozilla.org/mozilla-central/rev/f4f374622111022d41dd8d5eb9220624135c534a/dom/xhr/XMLHttpRequestMainThread.cpp#1785

Comment 9

[Andrea Marchesini [:baku]]

I mark this bug as duplicate of bug 1337746. I already wrote a patch for XHR.
Important to know that this assertion is triggered only when the XHR opens a file: URL and the responseType is blob/moz-blob.

About removing moz-blob, I added a telemetry ID in order to know if it's used or not. I'm planning to remove telemetry/moz-blob in 1 release.

Comment 10

[Jason Duell]

OK, now that we're not going to support it, should we throw an error for Cancel(NS_OK)?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #10)
> OK, now that we're not going to support it, should we throw an error for
> Cancel(NS_OK)?

"throw an error" - like let Cancel(NS_OK) return some NS_ERROR_FAILURE and do nothing?  Not sure...  maybe add MOZ_ASSERT(NS_FAILED(aStatus)) there (and convert some NS_ASSERTIONS below to it too!) and we are done here.