1191956 - "ASSERTION: mChannel must be valid if we're OPENED" in nsXMLHttpRequest::SetRequestHeader

Status: RESOLVED WORKSFORME

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

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: mChannel must be valid if we're OPENED.: 'mChannel', file dom/base/nsXMLHttpRequest.cpp, line 3037

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like this needs to happen via a file:// URI.

What's happening here is that we null out mChannel when AsyncOpen in Send() throws.  We should probably change the state at that point too.  To XML_HTTP_REQUEST_UNSENT or something else?

Comment 2

[Thomas Wisniewski]

The XHR code has changed lately, and the attached code no longer triggers any assertions.

Over the http protocol, it sends the request, gets the (server-specific) response, and then throws an InvalidStateError as expected by spec on line 19 (you can't setRequestHeader after send()).

Over the file protocol, it throws an NS_ERROR_BAD_URI upon hitting send().

Comment 3

[Boris Zbarsky [:bzbarsky]]

> the attached code no longer triggers any assertions.

The reason for that is that the assertion involved was simply removed, no?  Specifically, this was done in https://hg.mozilla.org/mozilla-central/rev/15171e9168c4

> Over the file protocol, it throws an NS_ERROR_BAD_URI upon hitting send().

Yes, but the assert was under setRequestHeader _after_ that exception was thrown.  In this case the XHR was left in a bad state that it wasn't expecting to be in: null mChannel but mState set to "opened".  Is that not the case anymore?  Why was it OK to remove that assertion?

Comment 4

[Thomas Wisniewski]

>Why was it OK to remove that assertion?

It was alright in the sense that SetRequestHeaders doesn't touch the channel anymore. Headers are now collected in a RequestHeaders object, and are only set on the channel during send() and on redirect (both of which do their own checks, with send throwing NS_ERROR_DOM_INVALID_STATE_ERR early as always if !mChannel).

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, and nothing else has a problem with the "mChannel null but state is OPENED" state?

Comment 6

[Thomas Wisniewski]

There may be other endpoints that still hit the "mChannel null but state is OPENED" case, but I didn't see any problems at the time related to dropping this specific assertion in SetRequestHeader. That patch did clash with a couple of tests, but not because of this assertion.

Bear in mind though that I'm not arguing against putting the assertion back. I'd rather play it safe if you're getting bad vibes from this. I also don't mind taking another look at this bug if you think it hasn't yet been properly solved (I'm too tired at the moment to know for sure).

Comment 7

[Boris Zbarsky [:bzbarsky]]

I don't think we should readd the assertion per se.  I think we should be clear on what our invariants are and document them and maintain them.  The assertion sounds like we thought we had invariants, but we didn't really, because it was firing.  Now we don't have the assertion, but it seems to me that we still have the invariant-violation.  We should at least have a followup bug to sort that out and figure out whether the invariants we have should be changed or whether we need to better maintain them or what.

Comment 8

[Thomas Wisniewski]

Oh definitely. I was hoping to accomplish that as part of my effort to help refactor the XHR code and align it with the spec as much as possible. It's honestly a shame we can't get rid of the XHR.channel API for the foreseeable future, or I would gladly tackle that right away.