37773 - HTTPChannel adds POST stream directly after the headers, with no extra CRLF.

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Martijn Pieters]

Creating a HTTP Channel object using XPConnect, then using it to request a
remote document fails.

Comment 1

[Martijn Pieters]

Attachment: testcase #1, async GET.

Comment 2

[Martijn Pieters]

Reproduce:
1/ Save attachent testcase #1 as asyncGet.js
2/ Run this testcase with xpcshell

Output:
Stop request
Status 0x80004005
Err: null
Response status not available

Expected output:
Start request
Stop Request
Status 0x0
Err: null
Response status: 200

Comment 3

[Martijn Pieters]

Attachment: testcase #2, sync GET

Comment 4

[Martijn Pieters]

openInputStream (synchronous usage) fails as well:
Reproduce:
1/ Save attachent testcase #2 as syncGet.js
2/ Run this testcase with xpcshell

Output:
uncaught exception: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIHTTPChannel.openInputStream]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame :: /home/mj/projects/syncGet.js ::
<TOP_LEVEL> :: line 13"  data: no]

Expected:
Response status: 200

Comment 5

[Martijn Pieters]

Attachment: testcase #3, async POST

Comment 6

[Martijn Pieters]

Attachment: testcase #3, async POST, _correct version_

Comment 7

[Martijn Pieters]

Please disregard the 3rd attachment, it contains some left over debug code and
the wrong URL.

Async POST never calls onStopRequest.
Reproduce
1/ Save attachent testcase #3 as asyncPost.js
2/ Run this testcase with xpcshell

Output:
None

Expected output:
Start request
Stop Request
Status 0x0
Err: null
Response status: 200

Comment 8

[Martijn Pieters]

Attachment: testcase #3, async POST, (DAMN!)

Comment 9

[Martijn Pieters]

I hate it when linewrap masls the fact that there is a newline missing. the call
to AsyncRead never got executed in #3.

Output now is identical to #1.

Comment 10

[Martijn Pieters]

Attachment: testcase #4, sync POST

Comment 11

[Martijn Pieters]

Sync POST broken in the same way as sync GET.
Same exception, see testcase #4.

Comment 12

[Martijn Pieters]

After talking with ruslan, the behaviour is consistent with using HTTPChannel
from xpconnect, as xpconnect doesn't have a event queue, which is needed for
async calls. Sync calls are implemented async under the hood, so they won't work
either.

When trying to use HTTPChannel from XUL, GET requests work. However, POST
requests do _not_ work. No StreamListener methods get called, nor is there any
indication of any error.

I changed the summary to reflect the new situation.

Comment 13

[Martijn Pieters]

Attachment: testcase #5, XUL for both a GET and a POST.

Comment 14

[Martijn Pieters]

To reproduce:
1/ Save testcase #5 as 8202.xul in chrome/test/content (create dir as needed).
2/ fire up this XUL file with './mozilla -chrome chrome://test/content/8202.xul
3/ Press the POST button
4/ Press the GET button

Output:
- When pressing the POST button: Nothing happens. No errors, nothing.
- When pressing the GET button: The following information is printed to the
console:
Start request
Stop request
Status 0x0
Err: null
Response status: 405

Expected:
- Output of the GET button is expected, including the 405 HTTP status, the URL
retrieved doesn't support GET =)
- Output of the POST button should be:
Start request
Stop request
Status 0x0
Err: null
Response status: 200

Comment 15

[Martijn Pieters]

After some scouring around on LXR, I discovered that the UploadStream is added
_directly_ after the headers, with no extra CRLF in between.

If I either prepend the stream with the extra headers I want (content-type and
-length) and a extra CRLF, or just a CRLF (and add the headers with
SetRequestHeader), POST works as well.
See
http://lxr.mozilla.org/seamonkey/source/xpfe/components/search/src/nsInternetSearchService.cpp#3071
for a current use of POST where headers are included in the stream.

Either this has to be made clear from the documentation (including the comments
in the .idl file), or a CRLF should be inserted into the stream. I'd prefer the
latter.

Adjusting summary again.

Comment 16

[ruslan]

Gagan,

that's true. We're checking for input stream being present and deliberately not
adding CRLF. Do you know the reason why is it this way?
-----------
in nsHTTPRequest.cpp:

    // This is under the assumpsion that the POST/PUT headers are all done
    if (!mInputStream)
        requestBuffer->Append(CRLF);
-----------

Comment 17

[ruslan]

Well. After discussions with Gagan latering this to "LATER" (M20) unless there 
are major objections. It's fixable in necko, however all the consumers will have 
to be changes/tested which is deemed to be too late at this point.

Comment 18

[Dawn Endico]

moving from architecture to browser product

Comment 19

[Blake Ross]

*spam* changing qa contact from [e-mail] to me ([e-mail]) 
on 121 open or resolved (but not verified) bugs.  sorry for the spam everybody, 
but most of these bugs would just remain dormant and not checked by QA 
otherwise.  I'm not sure how so many bugs have nobody as their QA contact, but 
I suspect this is the fault of some sort of bugzilla corruption that happened 
at some point (most of these bugs are in the 20000-26000 range, and I don't see 
where in the activity log that QA contact explicitly changed to 
[e-mail])

Anyways, sorry again for spam.  If you really get annoyed, I'm usually 
available in #mozilla on IRC for torture.

Comment 20

[ruslan]

Ok. This is arch. fix for some next release.

Comment 21

[Gagan]

pulling in ruslan's necko bugs ->darin

Comment 22

[Gagan]

http bugs to "Networking::HTTP"

Comment 23

[Guy Alfandary]

You can't post a file "as is" you must insert CRLF at the beginning.
Header data doesn't belong in the upload stream!

Comment 24

[Mike Lee]

is this bug fixed already? I can't see this anymore, adding extra CRLF manually
create error for some xmprpc calls.

Comment 25

[Darin Fisher]

this bug is actually fixed now.  see nsIUploadChannel::SetUploadStream.
remember to set the HTTP request method after calling SetUploadStream, since the
call to SetUploadStream may alter the request method.

Comment 26

[Jeff Walden [:Waldo]]

Any number of Mochitests use XHR with POST, and httpd.js will return a 400 on an incoming request that doesn't have the requisite extra CRLF (at least assuming the request body doesn't look like well-formed HTTP header lines, which is generally true).