Closed Bug 745379 Opened 11 years ago Closed 11 years ago

Spdy/2 Spec has priority values backwards


(Core :: Networking: HTTP, defect)

14 Branch
Not set



Tracking Status
firefox13 --- fixed
firefox14 --- fixed


(Reporter: mcmanus, Assigned: mcmanus)


(Whiteboard: [spdy][qa-])


(1 file)

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
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.
Assignee: nobody → mcmanus
Attached patch patch 0Splinter Review
Attachment #614970 - Flags: review?(honzab.moz)
Comment on attachment 614970 [details] [diff] [review]
patch 0

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


::: 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.)

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,, and
Attachment #614970 - Flags: approval-mozilla-aurora?
Closed: 11 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+
Whiteboard: [spdy] → [spdy][qa-]
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.