810152 - spdy 0 length no fin data frame handling

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

a legit data frame of length 0 and no fin bit causes the stream to loop without end and the page load to hang. This frame has no purpose (it carries no information) but is legal.

A 0 length frame bearing a fin bit is handled fine.

The problem is that the eventual call to nshttpconnection->OnWriteSegment() returns an error for size 0 and this causes the check that would determine the frame was done reading in spdysession::onwritesegment() to be bypassed - and no progress is made to just scan for the next frame header.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 679917 [details] [diff] [review]
patch 0

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

::: netwerk/protocol/http/SpdySession2.cpp
@@ +368,5 @@
> +  if (!count) {
> +    *countWritten = 0;
> +    return NS_OK;
> +  }
> +    

White space (see splinter).

Comment 3

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/823b82e784f5

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/823b82e784f5

Should this have a test?

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 679917 [details] [diff] [review]
patch 0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): original spdy bug
User impact if declined: interop problems with new spdy implementations
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): a new service deploying spdy would not be interoperable with firefox. The server can work around this. This behavior does not occur with any of the current high profile spdy deployments, but this was reported to me by another site doing a spdy implementation and they found this issue.
String or UUID changes made by this patch: none

we're at a stage with spdy that there are a lot more implementations coming on line and that is resulting in broader interop testing, so more corner cases like this one are coming to light.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2b9339453bd0