Closed Bug 1253358 Opened 4 years ago Closed 4 years ago

http/2 priority frame after push contains flags

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

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

Attachments

(3 files)

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"
Whiteboard: [spdy][necko-active]
Assignee: nobody → mcmanus
Attachment #8726338 - Flags: review?(hurley)
Attachment #8726340 - Flags: review?(hurley)
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+
(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.
Attachment #8726407 - Flags: review?(hurley) → review+
Attachment #8726340 - Flags: approval-mozilla-aurora?
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+
You need to log in before you can comment on or make changes to this bug.