82873 - HTTP trailers not supported

Status: VERIFIED FIXED

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

Description

[Andreas M. "Clarence" Schneider]

Trailers in responses with "Transfer-Encoding: chunked" are treated as chunks
(if their name begins with [A-F]) or lead to an error (otherwise).
Note that some trailers are allowed in HTTP/1.1 even if we don't send
"TE: trailers".

I'll attach a patch, but it is probably not complete. It does not add support
for the "Trailer" header and I do not know if problems are caused by adding
headers after the page itself has arrived.

While writing the patch I noticed a related problem with
nsHttpTransaction::Read(): I am not sure, but I think we loose data if
pipelining is enabled and data of different responses is contained within a
single chunk arriving over the network.

Comment 1

[Andreas M. "Clarence" Schneider]

Attachment: patch (first draft)

Comment 2

[Darin Fisher]

lack of support for trailers is a regression from the http branch landing.

pipelining is currently not implemented, but that being said the problem you
have described will need to be solved once pipelining is activated.  my approach
to that problem is simply to have a PushBack method on nsHttpConnection, so
data can be in a sense given back to the connection (and later given out to the
next transaction in the pipeline).  so, in short, this "bug" is really just a
prerequisit for pipelining in the new world.

Comment 3

[Darin Fisher]

Attachment: minimal patch

Comment 4

[Darin Fisher]

clarence: i'm not completely sold on the design put forth by your patch.  i
think the changes could be more isolated to either just the chunked decoder
or just the http transaction code.  in the old code, we let the chunked decoder
eat the trailing headers and only report EOF after they had been consumed.  this
is probably the right thing now as well, but for some time i have considered
doing it in nsHttpTransaction (in much the same manner as regular headers are
parsed).  this may just add clutter to nsHttpTransaction, so for that reason
i'm leaning toward the former solution.

at any rate, i've gone ahead and attached a very simple work around for this bug 
just in case.

Comment 5

[Gagan]

I am assuming you made the DontReuse function so you could call it from 
elsewhere as well (in future) r=gagan

Comment 6

[Doug Turner (:dougt)]

how hard would be it to consume the last bytes on the socket. Note that ftp is 
going to want something that takes a nsSocketTransport and consumes any extra 
data on the socket.  

If it is too hard to do now, that is fine.  (s)r=d

Comment 7

[Darin Fisher]

yeah.. i just didn't want to slap something together, and trailers are a VERY
rare occurance.  until getting the right patch, i just wanted to land something
to make us at least not fail to load the next page.

Comment 8

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 9

[Darin Fisher]

minimal patch checked in... futuring this bug.

Comment 10

[Darin Fisher]

-> moz 0.9.5 since this will need to be fixed before the pipelining code can land.

Comment 11

[Darin Fisher]

Attachment: v1.0 patch

Comment 12

[Darin Fisher]

Comment on attachment 50655 [details] [diff] [review]
v1.0 patch


>+    else {
>+        // save the partial line; wait for more data
>+        mLineBuf.Append(buf, count);
>+        *bytesConsumed = count;
>+    }

this is no good... it actually needs to skip a trailing CR.

Comment 13

[Darin Fisher]

Attachment: v1.1 revised to better handled CRLF line delimiters

Comment 14

[Gagan]

Comment on attachment 50656 [details] [diff] [review]
v1.1 revised to better handled CRLF line delimiters

r=gagan

Comment 15

[Scott MacGregor]

Comment on attachment 50656 [details] [diff] [review]
v1.1 revised to better handled CRLF line delimiters

sr=mscott

Comment 16

[Darin Fisher]

fixed-on-trunk

Comment 17

[John Unruh]

Verified per darin's comment.