1249379 - http2 push not transmitting stream flow control bump on channel that is already open when headers arrive

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

This is very similar to the bug that was dealt with in bug 1228822

you can see it with https://nghttp2.org/bug1228822/ and https://http2.undertow.io/ (thanks to the charles proxy folks for reporting the latter). (these are confirmed with e10s, untested at this point in single process)

Comment 1

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9395df2055ef

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: H2 Push WindowUpdate not written to network

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8721010 [details] [diff] [review]
H2 Push WindowUpdate not written to network

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

The basic problem was that by using the consumerstream's ::ReadSegments it wouldn't find the actual window update that was queued in the inline frame of the pushed stream. This shuffles some code around, but I think it makes the flow simpler.

::: netwerk/test/unit/test_http2.js
@@ +1000,5 @@
>  var altsvcpref1;
>  var altsvcpref2;
>  var loadGroup;
>  var serverPort;
> +var speculativeLimit;

this is a workaround for a different bug where the pushed resource and the request for it wound up on different sessions (which we can't 100% prevent, but it happened to my test a lot).

Comment 4

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeed35b886f452118984cb7bff0509608e9c8e35
Bug 1249379 - H2 Push WindowUpdate not written to network r=hurley

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eeed35b886f4

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8721010 [details] [diff] [review]
H2 Push WindowUpdate not written to network

I would like to uplift 1253358 and 1249379 as part of making h2 push more robust.

Approval Request Comment
[Feature/regressing bug #]: original h2 bug
[User impact if declined]: problems loading sites with h2 push - google, facebook, and twitter are all starting to use more push 
[Describe test coverage new/current, TreeHerder]: new xpcsell
[Risks and why]: medium low. This is well understood and targetted, I would normally classify it as low except it does touch a fair number of LOC
[String/UUID change made/needed]: none

Comment 7

[Liz Henry (:lizzard)]

Comment on attachment 8721010 [details] [diff] [review]
H2 Push WindowUpdate not written to network

This is a big patch, seems a bit risky but there is some new test coverage. 
Let's uplift this before the merge. 
Patrick can you explain how/if QA can manually test the fix in 46?

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)

> Patrick can you explain how/if QA can manually test the fix in 46?

as long as this and 1253358 are both landed on 46, https://h2-dot-io-webapp-staging.appspot.com/io2016/ should illustrate the problem and fix. (it often blank without the patches).

Comment 9

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/4dc00107f2b9

Comment 10

[Liz Henry (:lizzard)]

Andrei if your team can verify this in 46 some time during beta that would be great.

Comment 11

[Andrei Vaida [:avaida]]

I'll make sure we have a test case written and added in our Beta Regression Test Suite for this issue, based on Comment 8. Keeping the ni? as a reminder.

Comment 12

[Andrei Vaida [:avaida]]

Reproduced the issue with 46.0a2 (2016-03-02), using the URL from Comment 8.

Confirmed fixed on Windows 10 x64, Mac OS X 10.10.5 and Ubuntu 12.04 x86, using:
 - 46.0b10-build1 (20160411042519),
 - 47.0a2 (2016-04-14).