Closed Bug 745379 Opened 8 years ago Closed 8 years ago
Spdy/2 Spec has priority values backwards
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 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?
Attachment #614970 - Flags: review?(honzab.moz) → review+
(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 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.
Attachment #614970 - Flags: approval-mozilla-aurora?
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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.
Attachment #614970 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Checked the same on Firefox Nightly Build 36.0a1. The Request Priority for JS and CSS is 7.
You need to log in before you can comment on or make changes to this bug.