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

RESOLVED DUPLICATE of bug 1337746

Status

()

RESOLVED DUPLICATE of bug 1337746
3 years ago
2 years ago

People

(Reporter: jruderman, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Created attachment 8644547 [details]
testcase

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

Comment 1

3 years ago
Created attachment 8644549 [details]
stack

Comment 2

3 years ago
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
(Reporter)

Updated

3 years ago
Flags: needinfo?(VYV03354)
I think we should just drop support for moz-blob once we've implemented the Streams spec.
Depends on: 1128959
Flags: needinfo?(VYV03354)
Could you check if the assertion is also triggered with "blob" rather than "moz-blob"?
Flags: needinfo?(VYV03354)
Hm, you're right. But I don't know if the assertion is valid or not. Maybe netwerk peers would have more insight.
Flags: needinfo?(VYV03354) → needinfo?(jduell.mcbugs)
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?
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
(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
Flags: needinfo?(honzab.moz)
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1337746
OK, now that we're not going to support it, should we throw an error for Cancel(NS_OK)?
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.