spdy window update via settings does not release blocked stream

VERIFIED FIXED in Firefox 26

Status

()

Core
Networking: HTTP
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

25 Branch
mozilla27
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox24 wontfix, firefox25 wontfix, firefox26 verified, firefox27 verified)

Details

(Whiteboard: [spdy], crash signature)

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 812066 [details] [diff] [review]
spdy/3 settings frame does not restart window starved streams
Attachment #812066 - Flags: review?(hurley)
(Assignee)

Updated

5 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: 922174
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox27: --- → affected
(Assignee)

Updated

5 years ago
Duplicate of this bug: 922174
(Assignee)

Updated

5 years ago
Crash Signature: [@ mozilla::net::SpdySession31::HandleWindowUpdate(mozilla::net::SpdySession31*) ]
(Assignee)

Comment 3

5 years ago
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 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
Attachment #812066 - Flags: review?(hurley) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 812907 [details] [diff] [review]
spdy/3 settings frame does not restart window starved streams - backport for 25/26
(Assignee)

Updated

5 years ago
Attachment #812066 - Flags: checkin+
(Assignee)

Comment 8

5 years ago
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
Attachment #812907 - Attachment description: spdy/3 settings frame does not restart window starved streams → spdy/3 settings frame does not restart window starved streams - backport for 25/26
Attachment #812907 - Flags: approval-mozilla-beta?
Attachment #812907 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/55950ee9ef60
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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.
Attachment #812907 - Flags: approval-mozilla-beta?
Attachment #812907 - Flags: approval-mozilla-beta-
Attachment #812907 - Flags: approval-mozilla-aurora?
Attachment #812907 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 11

5 years ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e09697b6e951
status-firefox24: affected → wontfix
status-firefox25: affected → wontfix
status-firefox26: affected → fixed
status-firefox27: affected → fixed
(Assignee)

Comment 12

5 years ago
this also impacts a form based upload mechanism to youtube

https://code.google.com/p/gdata-issues/issues/detail?id=4919
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
(Assignee)

Comment 14

5 years ago
(In reply to kappaluppa2@gmail.com 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)
Flags: needinfo?(akeybl)
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).
Status: RESOLVED → VERIFIED
status-firefox26: fixed → verified
status-firefox27: fixed → verified
(Assignee)

Updated

4 years ago
Flags: needinfo?(akeybl)
You need to log in before you can comment on or make changes to this bug.