Closed Bug 1393736 Opened 2 years ago Closed 2 years ago

The prioritySize of Http2Session::SendHello doesn't include urgent start group

Categories

(Core :: Networking: HTTP, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: kershaw, Assigned: kershaw)

Details

Attachments

(1 file, 1 obsolete file)

Please see [1].
We have 6 priority frames now, but the prioritySize is not updated.

[1] https://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/netwerk/protocol/http/Http2Session.cpp#879
Attached patch Calculate prioritySize correctly (obsolete) — Splinter Review
Please take a look, Nick.

The priority size should be 6 * (kFrameHeaderBytes + 5), am I right?

Thanks.
Attachment #8901147 - Flags: review?(hurley)
Comment on attachment 8901147 [details] [diff] [review]
Calculate prioritySize correctly

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

Nice catch! Fortunately the allocation performed has a large amount of extra space on it (we allocate 4k first time around) so this isn't a security issue, but still better to be correct. Thanks!
Attachment #8901147 - Flags: review?(hurley) → review+
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #2)
> Comment on attachment 8901147 [details] [diff] [review]
> Calculate prioritySize correctly
> 
> Review of attachment 8901147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice catch! Fortunately the allocation performed has a large amount of extra
> space on it (we allocate 4k first time around) so this isn't a security
> issue, but still better to be correct. Thanks!

Thanks for the review.
Carry r+.
Attachment #8901147 - Attachment is obsolete: true
Attachment #8901674 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5590f02da010
Include urgent start group for counting prioritySize. r=hurley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5590f02da010
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.