http/2 priority frame after push contains flags

RESOLVED FIXED in Firefox 46

Status

()

Core
Networking: HTTP
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed)

Details

(Whiteboard: [spdy][necko-active])

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
If an in-progress push stream is matched up with a pull request, a new priority frame is sent to the server to indicate this change in circumstance.

It had the 0x20 PRIORITY flag set on it, like you would for HEADERS that contained priority information. the PRIORITY frame does not define any flags.

The google front end (over?) reacts to this by closing the session. I believe they have a fix underway for that, but firefox also should not send it - so this is the fix for our side.

for now, you can see this on
https://h2-dot-io-webapp-staging.appspot.com/io2016/

with the pushed "example.js"
(Assignee)

Updated

2 years ago
Whiteboard: [spdy][necko-active]
(Assignee)

Updated

2 years ago
Assignee: nobody → mcmanus
(Assignee)

Comment 1

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d55390d466f6
(Assignee)

Comment 2

2 years ago
Created attachment 8726338 [details] [diff] [review]
minor h2 push fixes
Attachment #8726338 - Flags: review?(hurley)
(Assignee)

Comment 3

2 years ago
Created attachment 8726340 [details] [diff] [review]
no flags on priority frames
Attachment #8726340 - Flags: review?(hurley)
(Assignee)

Comment 4

2 years ago
this is split into 2 patches to make a targetted uplift easier.
Attachment #8726338 - Flags: review?(hurley) → review+
Comment on attachment 8726340 [details] [diff] [review]
no flags on priority frames

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

r=me with one change

::: netwerk/protocol/http/Http2Session.cpp
@@ +631,5 @@
>                                  uint32_t streamID)
>  {
>    MOZ_ASSERT(frameLength <= kMaxFrameData, "framelength too large");
>    MOZ_ASSERT(!(streamID & 0x80000000));
> +  MOZ_ASSERT(!frameFlags || (frameType != FRAME_TYPE_PRIORITY));

RST_STREAM, GOAWAY, & WINDOW_UPDATE also do not define any flags - assert for them, as well, since we're adding this check.
Attachment #8726340 - Flags: review?(hurley) → review+
(Assignee)

Comment 6

2 years ago
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #5)
> 
> 
> RST_STREAM, GOAWAY, & WINDOW_UPDATE also do not define any flags - assert
> for them, as well, since we're adding this check.

its all true - but I haven't done the leg work to make sure we don't make that mistake :) So we'll do it somewhere independent we aren't uplifting..
(In reply to Patrick McManus [:mcmanus] from comment #6)
> its all true - but I haven't done the leg work to make sure we don't make
> that mistake :) So we'll do it somewhere independent we aren't uplifting..

I'm pretty confident we don't - I did a quick look at all CreateFrameHeader calls before making that comment, and they all look sane (modulo the one you're fixing here). But yes, let's do that as a separate, non-uplifted followup.
(Assignee)

Comment 8

2 years ago
Created attachment 8726407 [details] [diff] [review]
Assert no flags on several h2 frame types
Attachment #8726407 - Flags: review?(hurley)
Attachment #8726407 - Flags: review?(hurley) → review+
(Assignee)

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b82610b71cde4661b5e8519f1f5ea52a41c9612d
Bug 1253358 - minor h2 push fixes r=hurley

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4556ef0897778dfb06c290d3b57999f3d6b3cae
Bug 1253358 - no flags on priority frames r=hurley

https://hg.mozilla.org/integration/mozilla-inbound/rev/7488a41b7ca5e04cdbd970c82b21bd1aa38706c5
Bug 1253358 - Assert no flags on several h2 frame types r=hurley

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b82610b71cde
https://hg.mozilla.org/mozilla-central/rev/f4556ef08977
https://hg.mozilla.org/mozilla-central/rev/7488a41b7ca5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

2 years ago
Attachment #8726340 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 11

2 years ago
Comment on attachment 8726340 [details] [diff] [review]
no flags on priority frames

Approval Request Comment
[Feature/regressing bug #]: original h2 push issue
[User impact if declined]: race condition in failure to load some sites using h2 push. facebook, twitter, and google are starting to experiment with push for the first time and we're discovering interop issues.
[Describe test coverage new/current, TreeHerder]: contains runtime test for sending wrong flag.
[Risks and why]: extremely low - targeted one line fix removes sending a flag bit that was not legal to send.
[String/UUID change made/needed]: none.
Comment on attachment 8726340 [details] [diff] [review]
no flags on priority frames

Fix for an issue with push, it would be good to get this to release quickly. ok to uplift.
Attachment #8726340 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/68a781b27044
status-firefox46: --- → fixed
You need to log in before you can comment on or make changes to this bug.