714669 - nshttpconnection::pushback() asserts incorrectly

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

from bug 713400 comment 4 and comment 9

Before websockets, nshttpconnection::pushback() just dropped any data. HTTP Pipelines are handled separately - and non pipelined transactions should only generate pushback in the case of websockets or server data errors.

Websockets required storing up to 1 buffer of over-read data because the server can legitimately begin writing websocket protocol data before the client has read the 101 HTTP response. The read handlers are not installed once the end of the response headers are found so we only need to worry about storing up to 1 buffer of data.

Server errors happen when the server writes data beyond the delimited transaction - commonly this is extra whitespace beyond the content-length or actual data incorrectly added to a no-content response. (e.g. a 304 response that also contains a response body). In either case that data can be ignored.

The code added for websockets asserted that pushback was only called once - because we should only see one buffer of data. (and if we were seeing more than that a more aggressive use-and-free policy would be needed). But its buggy in the sense that the buffer is a property of the connection and pushback can be called once per transaction (therefore multiple times per connection) without indicating a problem.

The solution is just to overwrite the buffer with each new call - a new call indicates we have moved onto a new transaction and won't need that old data for a websockets upgrade.

from bug 713400 the successful 304 revalidations of both /static/LAB.js and /favicon.ico occur on the same persistent connection and both contain incorrect message bodies as 304's should never have bodies.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch v1

Comment 2

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

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

The previous mInputOverflow should've been cleared by TakeTransport. Why'd that not happen? Should we just clear out mInputOverflow in Activate or something?

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #2)
> Comment on attachment 585331 [details] [diff] [review]
> patch v1
> 
> The previous mInputOverflow should've been cleared by TakeTransport. Why'd
> that not happen?

TakeTransport() is only called by websockets - and that isn't a websockets test case. The input overflow in that case is just http message bodies on a 304 response which should be no content (a server error we ignore).

> Should we just clear out mInputOverflow in Activate or
> something?

we can do that. originally I didn't do that because activate isn't a clean boundary between transactions in the pipeline case, but duh - pipelining of course has its own pushback implementation. so that's better.

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: v2

Comment 5

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9760c10962f

Comment 6

[Patrick McManus [:mcmanus]]

cset d9760c10962f