Incorrect "no element found" error when XMLHttpRequest response has no content

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: kuno, Assigned: wisniewskit)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

6 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130512194354

Steps to reproduce:

Perform a POST request using XMLHttpRequest to a server which will respond with "204 No Content".  As the response does not have a body, there is also no Content-Type set.


Actual results:

Because there is no Content-Type set, firefox assumes the response to be XML and tries to parse an empty document.


Expected results:

If there is no response body, firefox should not try to parse it.
Reporter

Comment 1

6 years ago
This bug was reported to firebug at https://code.google.com/p/fbug/issues/detail?id=3100 , they have a test case at https://getfirebug.com/tests/manual/issues/3100/issue3100.html

It was reported to jQuery at http://bugs.jquery.com/ticket/13450 .

Updated

6 years ago
Component: Untriaged → Networking: HTTP
Product: Firefox → Core

Comment 2

6 years ago
> Because there is no Content-Type set, firefox assumes the response to be XML

That sounds like a decision that's being made above necko, in the DOM XHR code.
Component: Networking: HTTP → DOM
FWIW, I can repro this back to Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19 ID:2010031422.

On Firefox 2 it's WFM.
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 521301
This is actually a different issue: that issue is about the content of the document, while this one is about what's reported to the error console.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---

Comment 6

6 years ago
Also causing this issue reported to jQuery: http://bugs.jquery.com/ticket/13450

Comment 7

4 years ago
What's the status of this one? I can see the "No element found" error still in Firefox v39. :'(

Comment 8

4 years ago
I agree. What's the status of this?

Comment 9

4 years ago
+1

Comment 10

4 years ago
Please fix this. Note that the issue is not just with the (somewhat uncommon) 204 No Content response, but also the increasingly common 304 Not Modified response.

Amusingly, this issue seems to go away when e10s is enabled.

Comment 11

4 years ago
Meant to include: it doesn't repro with e10s enabled because the response is interpreted as text/plain instead of application/xml. This should be the behavior without e10s enabled, too.

Comment 13

3 years ago
Can somebody take care of this bug, becouse his existing is generating a lot of work for me to protect sending data (error source is showing crucial data).
Assignee

Comment 14

3 years ago
Note that the fix for bug 289714 has already made it so that a <parsererror> document is not returned for empty documents (the response(XML) will just be null instead). However, a parser failure message is still logged for 204s on the web console.

This patch checks for 204s (and 304s, though it's currently unnecessary*) and cancels the document-parse in those cases, rather than logging a parsing failure. It also cleans up some of the related logic in OnStopRequest, as there are comments indicating that a clean-up is in order, and I see no reason to not do so while I'm here (and also dealing with the XHR code recently).

A try run seems ok; just a typical batch of intermittents which don't seem related: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5a95a643ec2


* we'll of course never really hit the 304 case here as-is, since 304s will already have been converted into 200s. But I feel it's best to leave it there in case we ever decide to change how 304s are handled in XHRs to match how other browsers handle them.
Assignee: nobody → wisniewskit
Status: REOPENED → ASSIGNED
Attachment #8787744 - Flags: review?(bugs)
What happens currently if 204 or 304 *does have* the body?
Comment on attachment 8787744 [details] [diff] [review]
884693-cancel_xhr_parsing_on_http_204_and_304.diff

I think we need some tests for 204 and 304 with and without some data in them
(whether or not data makes sense is different thing, we just don't want to regress the behavior pages see).

So, could you write the tests and ensure then that the patch doesn't change the behavior(, only what is being reported to the web console. No need to add specific test for web console behavior I think).
Attachment #8787744 - Flags: review?(bugs) → review-
Assignee

Updated

3 years ago
Duplicate of this bug: 411060
Assignee

Comment 18

3 years ago
Alright, here is a new patch which just disables the console parse-error logging for the 204/304 case, along with a test which confirms that behavior does not change (aside from the console message being suppressed).

Try seems fine, just unrelated-looking intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea9bd9cf25b8

Oh, and for the record it turns out that the responseText for 204s/304s is set to an empty string even if the server sends a body (and a quick web console test confirms that responseXML remains null as well, even if valid XML is sent).
Attachment #8787744 - Attachment is obsolete: true
Attachment #8792746 - Flags: review?(bugs)
Comment on attachment 8792746 [details] [diff] [review]
884693-do_not_log_console_parser_warnings_for_204_and_304_XHR_statuses.diff

nsExpatDriver.cpp is tiny bit ugly code (already without this patch), but I guess this is fine.
Attachment #8792746 - Flags: review?(bugs) → review+
Assignee

Comment 20

3 years ago
Thanks, requesting check-in.


>nsExpatDriver.cpp is tiny bit ugly code (already without this patch)

Is there anything specific you think we should file a follow-up bug for?
Component: DOM → DOM: Core & HTML
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Version: 21 Branch → Trunk
Not sure. It is just that we end up calling the sink to do something with the error and then possibly use console service to log error. Feels a bit weird setup. Perhaps it is just the method names which are less-clear.

Comment 22

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb622c938de
Do not log console warnings for XHR parse failures if HTTP status is 204 or 304. r=smaug
Keywords: checkin-needed

Comment 23

3 years ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fdb6fff42d1
Backed out changeset 6fb622c938de for crashed @mozilla::dom::XMLHttpRequestMainThread
Sorry had to back out for mozilla::dom::XMLHttpRequestMainThread crashed,e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=36181653&repo=mozilla-inbound#L2548
Flags: needinfo?(wisniewskit)
Assignee

Comment 25

3 years ago
Thanks. Strange that I didn't notice that failure in my own try-runs.

Here's a new version of the patch which fixes that null-dereference issue. A fresh try-run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f196c436fc3

I'll carry over r+ and re-request check-in.
Attachment #8792746 - Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 26

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e71dd3919c
Do not log console warnings for XHR parse failures if HTTP status is 204 or 304. r=smaug
Keywords: checkin-needed

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/67e71dd3919c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 28

3 years ago
Hi all,

I am now also experiencing this when a POST message results in a 201. The POST is made to an Event Hubs server of Microsoft Azure that returns an empty body with Content-Type: "application/xml". Do you consider this to be by-design behavior?

I have also asked a question over at MSDN whether the Content-Type should not actually be "text/plain" instead.
Assignee

Comment 29

3 years ago
Hmm. Per HTTP spec, a 201 response "should" have a body/entity:

>The response SHOULD include an entity containing a list of resource characteristics and location(s) from which the user or user agent can choose the one most appropriate.

If I remember correctly, that would mean that we should also suppress the XML parsing message like we do for 304s. In addition, I'm guessing that 202 and 205 may as well also be added to the list, right bz?
Flags: needinfo?(bzbarsky)
Returning a 201 with a Content-Type but without an entity body is pretty nonsensical, but if they left it out we'd end up assuming it was XML anyway...  That said, if they claim "application/xml" then an empty entity body is certainly bogus, since that's not a valid XML document.

In practical terms, adding 201, 202, 205 to the list here is probably fine.
Flags: needinfo?(bzbarsky)

Comment 31

3 years ago
That sounds like a good solution, is there any indication regarding when this might be picked up?
Assignee

Comment 32

3 years ago
Time permitting, I should be able to get to it over the next week.

Comment 33

3 years ago
Thank you so much Thomas: we, and our customers highly appreciate the effort!
Assignee

Updated

3 years ago
Blocks: 1329365
Assignee

Comment 34

3 years ago
I've just spun off bug 1329365 with a patch to similarly handle 201s, 202s, and 205s.
You need to log in before you can comment on or make changes to this bug.