Last Comment Bug 745379 - Spdy/2 Spec has priority values backwards
: Spdy/2 Spec has priority values backwards
Status: RESOLVED FIXED
[spdy][qa-]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 14 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Patrick McManus [:mcmanus]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 17:24 PDT by Patrick McManus [:mcmanus]
Modified: 2014-12-11 08:11 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
patch 0 (3.29 KB, patch)
2012-04-13 17:32 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2012-04-13 17:24:04 PDT
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] 2012-04-13 17:32:00 PDT
Created attachment 614970 [details] [diff] [review]
patch 0
Comment 2 Honza Bambas (:mayhemer) 2012-04-15 07:30:59 PDT
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] 2012-04-15 09:59:04 PDT
(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] 2012-04-15 10:40:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9196b054d14
Comment 5 Patrick McManus [:mcmanus] 2012-04-15 10:41:17 PDT
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 7 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-16 15:47:24 PDT
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] 2012-04-16 19:18:27 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/f69cf6bc585d
Comment 9 Vignesh Shanmugam 2014-12-11 08:11:46 PST
Checked the same on Firefox Nightly Build 36.0a1. 

The Request Priority for JS and CSS is 7.

Note You need to log in before you can comment on or make changes to this bug.