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

Status: RESOLVED FIXED

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

Description

[kuno]

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.

Comment 1

[kuno]

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 .

Comment 2

[Jason Duell]

> 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.

Comment 3

[(mostly gone) XtC4UaLL [:xtc4uall]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Eldmannen]

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

Comment 7

[bn]

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

Comment 8

[Matthew Pavkov]

I agree. What's the status of this?

Comment 9

[Jonathan Allard]

+1

Comment 10

[none11given]

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

[none11given]

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 12

[hlcs]

C'mon https://developers.google.com/apis-explorer/?hl=en_US#p/youtube/v3/youtube.videos.rate?id=tRlmP6xWdw0&rating=dislike&_h=5&

Comment 13

[Maniek]

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).

Comment 14

[Thomas Wisniewski]

Attachment: 884693-cancel_xhr_parsing_on_http_204_and_304.diff

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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

What happens currently if 204 or 304 *does have* the body?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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).

Comment 18

[Thomas Wisniewski]

Attachment: 884693-do_not_log_console_parser_warnings_for_204_and_304_XHR_statuses.diff

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).

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 20

[Thomas Wisniewski]

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?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fdb6fff42d1
Backed out changeset 6fb622c938de for crashed @mozilla::dom::XMLHttpRequestMainThread

Comment 24

[Iris Hsiao [:ihsiao]]

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

Comment 25

[Thomas Wisniewski]

Attachment: 884693-do_not_log_console_parser_warnings_for_204_and_304_XHR_statuses.diff

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.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67e71dd3919c

Comment 28

[Remco Huijser]

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.

Comment 29

[Thomas Wisniewski]

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?

Comment 30

[Boris Zbarsky [: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.

Comment 31

[Remco Huijser]

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

Comment 32

[Thomas Wisniewski]

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

Comment 33

[Remco Huijser]

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

Comment 34

[Thomas Wisniewski]

I've just spun off bug 1329365 with a patch to similarly handle 201s, 202s, and 205s.