Closed Bug 714669 Opened 8 years ago Closed 8 years ago

nshttpconnection::pushback() asserts incorrectly

Categories

(Core :: Networking: HTTP, defect)

11 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #585331 - Flags: review?(cbiesinger)
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?
(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.
Attached patch v2Splinter Review
Attachment #585331 - Attachment is obsolete: true
Attachment #585331 - Flags: review?(cbiesinger)
Attachment #586433 - Flags: review?(cbiesinger)
Attachment #586433 - Flags: review?(cbiesinger) → review+
cset d9760c10962f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.