0 length spdy responses not always terminated properly

RESOLVED FIXED in Firefox 14



Networking: HTTP
5 years ago
5 years ago


(Reporter: mcmanus, Assigned: mcmanus)


12 Branch

Firefox Tracking Flags

(firefox12 wontfix, firefox13 wontfix, firefox14 fixed, firefox15 fixed)


(Whiteboard: [spdy])


(1 attachment)

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.
Created attachment 627231 [details] [diff] [review]
patch 0
Attachment #627231 - Flags: review?(honzab.moz)
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox12: --- → wontfix
status-firefox13: affected → wontfix
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).
Attachment #627231 - Flags: review?(honzab.moz) → review+

(checkin comment has wrong bug number 758725)
status-firefox15: affected → fixed
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
Attachment #627231 - Flags: approval-mozilla-aurora?
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 6

5 years ago
Comment on attachment 627231 [details] [diff] [review]
patch 0

[Triage Comment]
Approved for Aurora 14 for sake of compatibility given the low risk evaluation.
Attachment #627231 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox14: affected → fixed
You need to log in before you can comment on or make changes to this bug.