Spdy/2 Spec has priority values backwards

RESOLVED FIXED in Firefox 13

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

14 Branch
mozilla14
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed)

Details

(Whiteboard: [spdy][qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → mcmanus
status-firefox13: --- → affected
status-firefox14: --- → affected
(Assignee)

Comment 1

5 years ago
Created attachment 614970 [details] [diff] [review]
patch 0
Attachment #614970 - Flags: review?(honzab.moz)
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+
(Assignee)

Comment 3

5 years ago
(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!
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9196b054d14
(Assignee)

Comment 5

5 years ago
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?
https://hg.mozilla.org/mozilla-central/rev/f9196b054d14
Status: NEW → RESOLVED
Last Resolved: 5 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+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/f69cf6bc585d
status-firefox13: affected → fixed
status-firefox14: affected → fixed
Whiteboard: [spdy] → [spdy][qa-]

Comment 9

2 years ago
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.