1046892 - http/2 draft 14

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

Update our implementation to draft14

Comment 1

[u408661]

Attachment: part 1 - client implementation

Comment 2

[u408661]

Attachment: part 2 - tests

Comment 3

[u408661]

https://tbpl.mozilla.org/?tree=Try&rev=0a058da6b577

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8465602 [details] [diff] [review]
part 1 - client implementation

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

awesome.

the two existing places that screen out :colon-header-names from h1 should be updated to look for preceding whitespace now that what they are doing is actually required by the spec.

I think we need to enforce the pseudo-headers first requirement when receiving on pain of PROTOCOL_ERROR

::: netwerk/protocol/http/Http2Session.h
@@ +119,5 @@
>      SETTINGS_TYPE_HEADER_TABLE_SIZE = 1, // compression table size
>      SETTINGS_TYPE_ENABLE_PUSH = 2,     // can be used to disable push
>      SETTINGS_TYPE_MAX_CONCURRENT = 3,  // streams recvr allowed to initiate
> +    SETTINGS_TYPE_INITIAL_WINDOW = 4,  // bytes for flow control default
> +    SETTINGS_TYPE_MAX_FRAME_SIZE = 5   // max frame size we can send

"max frame size settings sender allows receipt of"

@@ +155,5 @@
> +  const static uint8_t kFrameLengthBytes = 3;
> +  const static uint8_t kFrameStreamIDBytes = 4;
> +  const static uint8_t kFrameFlagBytes = 1;
> +  const static uint8_t kFrameTypeBytes = 1;
> +  const static uint8_t kFrameHeaderBytes = kFrameLengthBytes + kFrameFlagBytes +

const static double pi = 3.1415927; // in case it changes

Comment 5

[u408661]

Attachment: Colon header handling update

Just to sanity check myself (this works fine against an updated node implementation which I will release shortly), here's the interdiff to make the colon-header handling better.

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8466414 [details] [diff] [review]
Colon header handling update

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

lgtm

Comment 7

[u408661]

Updated try push for review updates and new version of node-http2 that interops w/the review updates: https://tbpl.mozilla.org/?tree=Try&rev=2f7b12ce15ba

Comment 8

[u408661]

Attachment: 0001-Bug-1046892-part-1-HTTP-2-draft-14-client-implementation-r-mcmanus.patch

For posterity - the updated version

Comment 9

[u408661]

Attachment: 0002-Bug-1046892-part-2-HTTP-2-draft-14-test-updates-r-mcmanus.patch

Also updated for posterity

Comment 10

[u408661]

For the record, we have positive interop with both node-http2 (not independent, since I'm updating it) and nghttp2 (which is an independent implementation)

Comment 11

[u408661]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f442730a6e7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac28ef70b931

Comment 12

[u408661]

Sheriffs - if for some reason you end up having to back this out, please also back out bug 1047594. It's not strictly dependent on this (nothing will break if you don't), but we don't want it without this bug. Thanks!

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f442730a6e7
https://hg.mozilla.org/mozilla-central/rev/ac28ef70b931