Closed Bug 302809 Opened 18 years ago Closed 18 years ago
AJAX regression: POST set
Request Header causes NS _ERROR _ILLEGAL _VALUE for invalid headers
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
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → major
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta4
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?
> 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.
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).
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 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?
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 :)
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.
That's a lot of code for logging... is there no utility function that can be called that does all of that for us?
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).
http://lxr.mozilla.org/seamonkey/source/content/base/public/nsContentUtils.h#477, if it weren't in the wrong module.
That is only half-useful, since it only wraps the actual reporting - not the bit that gets the location.
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 on attachment 191736 [details] [diff] [review] v1 patch Let's add better console logging after we get this initial patch checked in.
Attachment #191736 - Flags: approval1.8b4?
fixed-on-trunk I'll file a follow-up bug about improving the logging.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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 on attachment 191736 [details] [diff] [review] v1 patch Yeah, we need to take this. Sorry for missing it the first time around.
Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.
Bastian - can you do final verification?
Verified for 1.8 branch
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.