nshttpconnection::pushback() asserts incorrectly

RESOLVED FIXED in mozilla12

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

11 Branch
mozilla12
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 585331 [details] [diff] [review]
patch v1
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?
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 586433 [details] [diff] [review]
v2
Attachment #585331 - Attachment is obsolete: true
Attachment #585331 - Flags: review?(cbiesinger)
Attachment #586433 - Flags: review?(cbiesinger)
Attachment #586433 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9760c10962f
Target Milestone: --- → mozilla12
(Assignee)

Comment 6

5 years ago
cset d9760c10962f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.