Last Comment Bug 758625 - 0 length spdy responses not always terminated properly
: 0 length spdy responses not always terminated properly
Status: RESOLVED FIXED
[spdy]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 12 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-25 07:54 PDT by Patrick McManus [:mcmanus]
Modified: 2012-05-30 09:57 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
fixed


Attachments
patch 0 (3.72 KB, patch)
2012-05-25 07:56 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2012-05-25 07:54:23 PDT
Our friends at webtide (makers of jetty) reported this.

a spdy stream with a fin on the syn-reply might not have its termination detected if the http data in the syn-reply does not also signal that there is no body (e.g. via a content-length: 0 or by using a response code that is defined to have no body like 304).

this can be worked around on the server side by either including a content-length:0 or by placing the fin on a 0 length data frame instead of directly on the syn-reply.

Fixing it is just a matter of applying the same mNeedsCleanup flag that data frame processing uses for signaling EOF to the http layer.
Comment 1 Patrick McManus [:mcmanus] 2012-05-25 07:56:51 PDT
Created attachment 627231 [details] [diff] [review]
patch 0
Comment 2 Honza Bambas (:mayhemer) 2012-05-25 08:37:29 PDT
Comment on attachment 627231 [details] [diff] [review]
patch 0

Review of attachment 627231 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/SpdySession.cpp
@@ +2042,5 @@
> +  NS_ABORT_IF_FALSE(!mNeedsCleanup, "mNeedsCleanup unexpectedly set");
> +  mNeedsCleanup = mInputFrameDataStream;
> +
> +  *aCountWritten = 0;
> +  ResetDownstreamState();

Please let *aCountWritten = 0; be set explicitly outside this method (by the caller).
Comment 3 Patrick McManus [:mcmanus] 2012-05-25 14:25:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/004e123be159

(checkin comment has wrong bug number 758725)
Comment 4 Patrick McManus [:mcmanus] 2012-05-25 14:28:57 PDT
Comment on attachment 627231 [details] [diff] [review]
patch 0

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  original spdy bug

User impact if declined: 
interop problem with compliant spdy/2 servers.. just discovered in the devlopment of a new server. we risk problems with other emerging servers in a fast moving space by not fixing.

Testing completed (on m-c, etc.): 
on m-c

Risk to taking this patch (and alternatives if risky):  low.. the path it uses is already used on data frames, this allows control frames to trigger the same code
Comment 5 Patrick McManus [:mcmanus] 2012-05-26 06:39:58 PDT
https://hg.mozilla.org/mozilla-central/rev/004e123be159
Comment 6 Alex Keybl [:akeybl] 2012-05-29 11:18:51 PDT
Comment on attachment 627231 [details] [diff] [review]
patch 0

[Triage Comment]
Approved for Aurora 14 for sake of compatibility given the low risk evaluation.
Comment 7 Patrick McManus [:mcmanus] 2012-05-30 09:57:40 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/210ca2935a9a

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