302809 - AJAX regression: POST setRequestHeader causes NS_ERROR_ILLEGAL_VALUE for invalid headers

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Bastian Grupe]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050730 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050730 Firefox/1.0+

Checkin of bug 302263 added a check for invalid headers. If one of them is
found, it throws an exception rather then discarding them silently or outputting
a JavaScript warning.

Since the requests would work without those headers in most cases, and this
checkin breaks almost any AJAX post requests I know, the behaviour should change.


Reproducible: Always

Steps to Reproduce:
1. Go to the bug url
2. click on the POST test
3. check javascript console

Actual Results:  
Exception is thrown.

Expected Results:  
An alert window with the response pops up.

content-length is corpus delicti in this case.

Comment 1

[Ria Klaassen (not reading all bugmail)]

I see no error in the JS Console after clicking on "Click here to run the POST
test!". 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050730
Firefox/1.0+ ID:2005073013

Comment 2

[Darin Fisher]

Ria: The testcase for POST appears to have
setRequestHeader("Content-Length",...) commented out.

Bastian: So, you are saying that it is very common on the web to call
setRequestHeader("Content-Length",...) even though it has never been needed?

Comment 3

[Bastian Grupe]

I accidently left the commented version online, the exception should be back in
place now.

Darin:
When I first learned doing AJAX stuff, I read it in at least 2 tutorials/guides
I think and then I assumed that it was needed and added it to my guide as well.

My point is that if it is not required, why isn't Gecko just silently ignoring it?

Comment 4

[Darin Fisher]

> My point is that if it is not required, why isn't Gecko just silently ignoring it?

The assumption I made is that only someone with malicious intent would set it,
but that is clearly a bad assumption.

Comment 5

[Daniel Veditz [:dveditz]]

If we don't return some kind of error web authors will assume the call worked,
making web app debugging more difficult. On the other hand bgrupe is right: lots
of pages, including msdn, show setting the content-length.

The channel will still error on invalid characters in headers and values, right?
I was mostly concerned about those and they should remain covered. In that case
simply dropping unwanted headers by returning NS_OK might be the most compatible
thing (add an NS_WARNING, or even a PR_LOG warning if we support that in this
module).

Comment 6

[Darin Fisher]

dveditz: yup, invalid characters will still throw.  patch coming up to implement
the NS_OK return w/ NS_WARNING.  no NSPR logging in xmlhttprequest land.

Comment 7

[Darin Fisher]

Attachment: v1 patch

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 191736 [details] [diff] [review]
v1 patch

I'm not sure that this so great. the NS_WARNING will really only be seen by
mozilla developers, who probably don't care so much about what web pages do.
maybe this should show a JS Console warning instead?

Comment 9

[Darin Fisher]

hrm... yeah, i agree.  a JS console warning would be better.  i don't think we
should necessarily hold this patch for that though.  i have higher priority
things to work on, and i probably won't be able to write JS console logging code
until after 1.5 beta.  patches welcome :)

Comment 10

[James Ross]

Attachment: Log refused message to console as a warning

You mean something like this?

Warning: Refusing to set request header Content-length
Source File: http://bazzinet.info/tutorials/js-request/example0_3.js
Line: 14

I can't actually tell if it really helps this example, though, as it still does
nothing, but I can be sure the JS code continues past the disregarded
setRequestHeader as requested.

Comment 11

[Darin Fisher]

That's a lot of code for logging... is there no utility function that can be
called that does all of that for us?

Comment 12

[James Ross]

I can find the following that do similar stuff:
  http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#1121
  http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSUtils.cpp#59

The former is hard-wired for DOM errors, only allows JS warnings, and only gets
messages from the DOM properties file (with no substitution, too), and the
latter only gets the location information for use with other code (irronically,
it's only used in one place).

If you want a slightly more useful utility method somewhere, just tell me where
and I'll see what I can do. Note, however, that I'm away from my computer from
not until Friday, so you'd have to wait until then (or find someone else).

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

http://lxr.mozilla.org/seamonkey/source/content/base/public/nsContentUtils.h#477,
if it weren't in the wrong module.

Comment 14

[James Ross]

That is only half-useful, since it only wraps the actual reporting - not the bit
that gets the location.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 191736 [details] [diff] [review]
v1 patch

r/sr=dveditz for this band-aide. Mystified developers can turn on http header
logging.

Comment 16

[Darin Fisher]

Comment on attachment 191736 [details] [diff] [review]
v1 patch

Let's add better console logging after we get this initial patch checked in.

Comment 17

[Darin Fisher]

fixed-on-trunk

I'll file a follow-up bug about improving the logging.

Comment 18

[Wladimir Palant]

Bug 302263 has been checked in on the aviary branch, I think this should this
should be done with attachment 191736 [details] [diff] [review] as well.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Indeed.

Comment 20

[David Baron :dbaron:]

Comment on attachment 191736 [details] [diff] [review]
v1 patch

Yeah, we need to take this.  Sorry for missing it the first time around.

Comment 21

[David Baron :dbaron:]

Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.

Comment 22

[Mike Schroepfer]

Bastian - can you do final verification?

Comment 23

[Bastian Grupe]

Verified for 1.8 branch