745379 - Spdy/2 Spec has priority values backwards

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

The spdy/2 spec defines 2 bits for priority and says that the highest value is the highest priority.

That turns out to be a mistake in the spec, 0 is the highest priority and 3 is the lowest.

The fact that we were doing the opposite of chrome was brought to my attention by a 3rd party doing a server side implementation, and indeed the "0 is the highest priority" matches chrome source and a note outside of the v2 spec on the spdy website.

Draft 3 notes here https://sites.google.com/a/chromium.org/dev/spdy/spdy-protocol/
are the best guide to the mistake (yes, draft 3 noting draft 2 errata.). Also see  GetLowestPriority() and GetHighestPriority() in spdy_framer.h of chromium source.. they make a distinction of v3/2 on the lowest priority because v3 ranges from 0..7 instead of v2's 0..3, but in each case the highest priority is 0.

The easiest thing to do is just reverse our behavior and push it to FF13 where this will be on by default for the first time.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 614970 [details] [diff] [review]
patch 0

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

r=honzab

::: netwerk/protocol/http/SpdySession.h
@@ +102,5 @@
> +
> +  const static PRUint8 kPri00   = 0 << 6; // highest
> +  const static PRUint8 kPri01   = 1 << 6;
> +  const static PRUint8 kPri02   = 2 << 6;
> +  const static PRUint8 kPri03   = 3 << 6; // lowest

Could we name this kPriorityHighest, *Higher, *Lower, *Lowest?

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #2)

> Could we name this kPriorityHighest, *Higher, *Lower, *Lowest?

hmm.. v3 has 8 values instead of 4, and I'm not sure I can come up with 8 names that are clearer than 0..7 - but I'll give it some thought when I fork v3 (after vacation next week).

(the extra bit of priority will actually help us as we use at least 6 or 7 different priority values with some regularity.. I suspect there is also work we can do setting those better, but I have not looked too hard at that yet - right now I just map whatever priority is given to the channel onto a spdy priority.)

thanks!

Comment 4

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9196b054d14

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 614970 [details] [diff] [review]
patch 0

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: incorrect implementation of priority scheme when using spdy
Testing completed (on m-c, etc.): verified behavior matches chrome
Risk to taking this patch (and alternatives if risky): I suppose priorities could still be more wrong but that risk is very low
String changes made by this patch:

This is a bug in the spdy implementation that will be on by default for the first time in FF13. The bug was caused by an inaccuracy in the protocol specification that was just recently discovered - the patch changes our implemntation to match the defacto behavior of chrome, google.com, and twitter.com.

Comment 6

[(no longer active)]

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

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 614970 [details] [diff] [review]
patch 0

[Triage Comment]
Results on m-c look good, let's get this landed on aurora to make the implementation right.

Comment 8

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f69cf6bc585d

Comment 9

[Vignesh Shanmugam]

Checked the same on Firefox Nightly Build 36.0a1. 

The Request Priority for JS and CSS is 7.