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

RESOLVED WORKSFORME

Status

()

Core
DOM
RESOLVED WORKSFORME
3 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

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

Trunk
assertion, testcase
Points:
---

Firefox Tracking Flags

(firefox42 affected)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Created attachment 8644524 [details]
testcase

###!!! ASSERTION: mChannel must be valid if we're OPENED.: 'mChannel', file dom/base/nsXMLHttpRequest.cpp, line 3037
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky) → needinfo?(bugs)

Comment 2

2 years ago
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().
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(bugs)
Resolution: --- → WORKSFORME
> 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?
Flags: needinfo?(wisniewskit)

Comment 4

2 years ago
>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).
Flags: needinfo?(wisniewskit)
OK, and nothing else has a problem with the "mChannel null but state is OPENED" state?

Comment 6

2 years ago
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).
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

2 years ago
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.
You need to log in before you can comment on or make changes to this bug.