922109 - spdy window update via settings does not release blocked stream

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

a settings frame updates all the existing remote stream flow control windows in the same way that a window update frame updates just 1 particular stream.

however the settings update path is missing the logic that restarts a stream that had been blocked on a closed window. That code exists in the window update path. The result is a stream with an open window but that is stalled anyhow.

The patch centralizes that code. It also removes a stream check from the 3.1 code that should have been removed when the session handling was put it - it could now point at the wrong stream and potentially even segv.

a large pre-rollout test informed me of this bug - they were seeing upload stalls with spdy/3 - basically stream 0x1 was being used for the POST and had already stalled by the time the server settings frame rolled in.. the server didn't send window updates for the stream because there was plenty of room in it (when the settings frame was taken into account). Subsequent streams of course worked fine.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: spdy/3 settings frame does not restart window starved streams

Comment 3

[Patrick McManus [:mcmanus]]

this patch is a little confusing.. it contains 2 bits that I'd rather not split up.. but the crash signature only applies to firefox 27.. a less serious problem (stalled upload) applies to <= 27.

Comment 4

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=3a0b6695e4bd

Comment 5

[u408661]

Comment on attachment 812066 [details] [diff] [review]
spdy/3 settings frame does not restart window starved streams

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

::: netwerk/protocol/http/SpdyStream31.cpp
@@ +1329,5 @@
> +void
> +SpdyStream31::UpdateRemoteWindow(int32_t delta)
> +{
> +  mRemoteWindow += delta;
> +  

nit: trailing whitespace

Comment 6

[Patrick McManus [:mcmanus]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/55950ee9ef60

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: spdy/3 settings frame does not restart window starved streams - backport for 25/26

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 812907 [details] [diff] [review]
spdy/3 settings frame does not restart window starved streams - backport for 25/26

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 737470
User impact if declined: ccasional failed uploads
Testing completed (on m-c, etc.): on m-i, confirmed with reported, and automated test coverate
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55950ee9ef60

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 812907 [details] [diff] [review]
spdy/3 settings frame does not restart window starved streams - backport for 25/26

Not critical enough to warrant uplift to Beta.

Comment 11

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e09697b6e951

Comment 12

[Patrick McManus [:mcmanus]]

this also impacts a form based upload mechanism to youtube

https://code.google.com/p/gdata-issues/issues/detail?id=4919

Comment 13

[[e-mail]]

This fix solves the problem of videos not being uploaded via FireFox using the YouTube Upload Direct API v3. I tested it via Aurora 26.x. How do I go about renominating this for firefox25? I'll have to redirect members who upload to use another browser - Currently Chrome & IE 10, 9, 8 work. 

Thanks,
K

Comment 14

[Patrick McManus [:mcmanus]]

(In reply to [e-mail] from comment #13)
>
> about renominating this for firefox25? I'll have to redirect members who

I'll cc alex who triaged this the first time to see if he has a new opinion based on comment 13. but I suspect it is just too late in the beta cycle. (otoh the patch has been well baked on aurora)

Comment 15

[Tracy Walker [:tracy]]

There are no crash reports from builds created after the fix here was landed.


note: The only reports of this crash signature over the past 28 days are on Nightly (Fx27).