Closed
Bug 302809
Opened 18 years ago
Closed 18 years ago
AJAX regression: POST setRequestHeader causes NS_ERROR_ILLEGAL_VALUE for invalid headers
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
VERIFIED
FIXED
mozilla1.8beta4
People
(Reporter: bgrupe, Assigned: darin.moz)
References
()
Details
(4 keywords)
Attachments
(2 files)
1.20 KB,
patch
|
dveditz
:
review+
dveditz
:
superreview+
dbaron
:
approval-aviary1.0.7+
dbaron
:
approval1.7.12+
cbeard
:
approval1.8b4+
|
Details | Diff | Splinter Review |
3.85 KB,
patch
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
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
Assignee | ||
Comment 2•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Blocks: 302263
Severity: normal → major
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta4
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.8b4?
Reporter | ||
Comment 3•18 years ago
|
||
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?
Assignee | ||
Comment 4•18 years ago
|
||
> 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•18 years ago
|
||
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).
Keywords: regression
Updated•18 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Assignee | ||
Comment 6•18 years ago
|
||
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.
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #191736 -
Flags: superreview?(dveditz)
Attachment #191736 -
Flags: review?(dveditz)
Comment 8•18 years ago
|
||
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?
Assignee | ||
Comment 9•18 years ago
|
||
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•18 years ago
|
||
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.
Assignee | ||
Comment 11•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
http://lxr.mozilla.org/seamonkey/source/content/base/public/nsContentUtils.h#477, if it weren't in the wrong module.
Comment 14•18 years ago
|
||
That is only half-useful, since it only wraps the actual reporting - not the bit that gets the location.
Comment 15•18 years ago
|
||
Comment on attachment 191736 [details] [diff] [review] v1 patch r/sr=dveditz for this band-aide. Mystified developers can turn on http header logging.
Attachment #191736 -
Flags: superreview?(dveditz)
Attachment #191736 -
Flags: superreview+
Attachment #191736 -
Flags: review?(dveditz)
Attachment #191736 -
Flags: review+
Assignee | ||
Comment 16•18 years ago
|
||
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?
Updated•18 years ago
|
Attachment #191736 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 17•18 years ago
|
||
fixed-on-trunk I'll file a follow-up bug about improving the logging.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 18•18 years ago
|
||
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.
Updated•18 years ago
|
Flags: blocking-aviary1.0.7?
![]() |
||
Comment 19•18 years ago
|
||
Indeed.
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+
![]() |
||
Updated•18 years ago
|
Attachment #191736 -
Flags: approval1.7.12?
Attachment #191736 -
Flags: approval-aviary1.0.7?
Comment 20•18 years ago
|
||
Comment on attachment 191736 [details] [diff] [review] v1 patch Yeah, we need to take this. Sorry for missing it the first time around.
Attachment #191736 -
Flags: approval1.7.12?
Attachment #191736 -
Flags: approval1.7.12+
Attachment #191736 -
Flags: approval-aviary1.0.7?
Attachment #191736 -
Flags: approval-aviary1.0.7+
Comment 21•18 years ago
|
||
Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.
Keywords: fixed-aviary1.0.7,
fixed1.7.12
Comment 22•18 years ago
|
||
Bastian - can you do final verification?
You need to log in
before you can comment on or make changes to this bug.
Description
•