Last Comment Bug 714669 - nshttpconnection::pushback() asserts incorrectly
: nshttpconnection::pushback() asserts incorrectly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 11 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla12
Assigned To: Patrick McManus [:mcmanus] PTO until Sep 6
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-02 13:24 PST by Patrick McManus [:mcmanus] PTO until Sep 6
Modified: 2012-01-07 19:46 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.03 KB, patch)
2012-01-02 13:30 PST, Patrick McManus [:mcmanus] PTO until Sep 6
no flags Details | Diff | Splinter Review
v2 (859 bytes, patch)
2012-01-06 08:00 PST, Patrick McManus [:mcmanus] PTO until Sep 6
cbiesinger: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] PTO until Sep 6 2012-01-02 13:24:07 PST
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] PTO until Sep 6 2012-01-02 13:30:52 PST
Created attachment 585331 [details] [diff] [review]
patch v1
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2012-01-04 16:17:19 PST
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] PTO until Sep 6 2012-01-06 07:57:37 PST
(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] PTO until Sep 6 2012-01-06 08:00:35 PST
Created attachment 586433 [details] [diff] [review]
v2
Comment 5 Patrick McManus [:mcmanus] PTO until Sep 6 2012-01-06 12:17:29 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9760c10962f
Comment 6 Patrick McManus [:mcmanus] PTO until Sep 6 2012-01-07 19:46:07 PST
cset d9760c10962f

Note You need to log in before you can comment on or make changes to this bug.