1429973 - Make sure HTTP trailers can be received over H2

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Kershaw Chang [:kershaw]]

See bug 1413999 comment #59.

We have to make sure that server-timing trailers also works for h2.

Comment 1

[Kershaw Chang [:kershaw]]

Set the priority to P3, since chrome doesn't support server-timing trailers.

Comment 2

[Patrick McManus [:mcmanus]]

this is a feature we want for server-timing despite chrome's support.

Comment 3

[Patrick McManus [:mcmanus]]

the temptation will be to implement this with the normal h2->h1-semantic stuff that's already going on (i.e. the way we serialize h1 headers from hpack and let the h1 parser reparse them.. but

we currently turn h2 streams into h1 EOF terminated streams for delimiters.. which is really efficient. the temptation might be to turn them into h1 chunked streams so we could just reuse the trailer parser, but I don't really think the overhead of the chunk parser is a great idea (it does a lot of memmoves.. there's a bug open on that, but thankfully its pretty much over taken by events with the rise of h2).

so instead I think just some special case QI needs to be done somewhere.. sorry that's not real helpful - its more of a please "don't do it this way" than a hint on what to do..

Comment 4

[u408661]

I'm going to take this one. I had the same idea as you, Pat - avoid the chunked parser.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this for 59/60. Do you intend to try to fix this or un-whitelist the (or some) tests for 59?

Comment 6

[u408661]

No, this won't be worth trying to get into 59. None of this is exposed up to the DOM yet, everything that's landed (including the HTTPS-limiting) is necko-only. I've only just un-bitrotted the DOM patches enough to start testing to ensure everything's flowing as it should, but that was a very hacky un-bitrotting, and there's one known bug with those patches that I haven't even started in on. So long as this lands before the DOM patches (which it will), there's no need to rush this one.

Comment 7

[u408661]

Attachment: Bug 1429973 part 0 - Update node-http2 to v3.3.8 for required bugfix.

Review commit: https://reviewboard.mozilla.org/r/220656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220656/

Comment 8

[u408661]

Attachment: Bug 1429973 part 1 - plumb through trailers in h2 to support server-timing.

Review commit: https://reviewboard.mozilla.org/r/220658/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220658/

Comment 9

[u408661]

Attachment: Bug 1429973 part 2 - Move server-timing tests into http/2.

Review commit: https://reviewboard.mozilla.org/r/220660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220660/

Comment 10

[u408661]

Attachment: Bug 1429973 part 3 - Remove hidden pref to allow plaintext server-timing.

Review commit: https://reviewboard.mozilla.org/r/220662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220662/

Comment 11

[Code Review Bot [:reviewbot]]

Comment on attachment 8951388 [details]
Bug 1429973 part 1 - plumb through trailers in h2 to support server-timing.

https://reviewboard.mozilla.org/r/220658/#review226630


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/protocol/http/nsHttpTransaction.cpp:2460
(Diff revision 1)
> +            newline = len;
> +        }
> +
> +        int32_t end = aTrailers[newline - 1] == '\r' ? newline - 1 : newline;
> +        nsDependentCSubstring line(aTrailers, cur, end);
> +        nsHttpAtom hdr = {0};

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

        nsHttpAtom hdr = {0};
                          ^
                          nullptr

Comment 12

[u408661]

Comment on attachment 8951388 [details]
Bug 1429973 part 1 - plumb through trailers in h2 to support server-timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220658/diff/1-2/

Comment 13

[u408661]

Comment on attachment 8951389 [details]
Bug 1429973 part 2 - Move server-timing tests into http/2.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220660/diff/1-2/

Comment 14

[u408661]

Comment on attachment 8951390 [details]
Bug 1429973 part 3 - Remove hidden pref to allow plaintext server-timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220662/diff/1-2/

Comment 15

[u408661]

Comment on attachment 8951387 [details]
Bug 1429973 part 0 - Update node-http2 to v3.3.8 for required bugfix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220656/diff/1-2/

Comment 16

[u408661]

Comment on attachment 8951388 [details]
Bug 1429973 part 1 - plumb through trailers in h2 to support server-timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220658/diff/2-3/

Comment 17

[u408661]

Comment on attachment 8951389 [details]
Bug 1429973 part 2 - Move server-timing tests into http/2.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220660/diff/2-3/

Comment 18

[u408661]

Comment on attachment 8951390 [details]
Bug 1429973 part 3 - Remove hidden pref to allow plaintext server-timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220662/diff/2-3/

Comment 19

[Daniel Stenberg [:bagder]]

Comment on attachment 8951387 [details]
Bug 1429973 part 0 - Update node-http2 to v3.3.8 for required bugfix.

https://reviewboard.mozilla.org/r/220656/#review228524

Comment 20

[Daniel Stenberg [:bagder]]

Comment on attachment 8951388 [details]
Bug 1429973 part 1 - plumb through trailers in h2 to support server-timing.

https://reviewboard.mozilla.org/r/220658/#review228526

::: netwerk/protocol/http/Http2Stream.cpp:1153
(Diff revision 3)
> +Http2Stream::ConvertResponseTrailers(Http2Decompressor *decompressor,
> +                                     nsACString &aTrailersIn)
> +{
> +  LOG3(("Http2Stream::ConvertResponseTrailers %p", this));
> +  nsAutoCString flatTrailers;
> +  flatTrailers.SetCapacity(aTrailersIn.Length() + 512);

Can we have a comment about the magic number 512 in here?

Comment 21

[Daniel Stenberg [:bagder]]

Comment on attachment 8951389 [details]
Bug 1429973 part 2 - Move server-timing tests into http/2.

https://reviewboard.mozilla.org/r/220660/#review228528

Comment 22

[Daniel Stenberg [:bagder]]

Comment on attachment 8951390 [details]
Bug 1429973 part 3 - Remove hidden pref to allow plaintext server-timing.

https://reviewboard.mozilla.org/r/220662/#review228530

Comment 23

[u408661]

Comment on attachment 8951388 [details]
Bug 1429973 part 1 - plumb through trailers in h2 to support server-timing.

https://reviewboard.mozilla.org/r/220658/#review228526

> Can we have a comment about the magic number 512 in here?

Heh, it's cargo-culted from the other Convert*Headers methods. I'll add a comment on each one, just for completeness. (To be clear - I don't know *why* we use 512, other than to give some amount of space to expand into without reallocating. Why 512 specifically was chosen is unclear.)

Comment 24

[u408661]

Comment on attachment 8951387 [details]
Bug 1429973 part 0 - Update node-http2 to v3.3.8 for required bugfix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220656/diff/2-3/

Comment 25

[u408661]

Comment on attachment 8951388 [details]
Bug 1429973 part 1 - plumb through trailers in h2 to support server-timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220658/diff/3-4/

Comment 26

[u408661]

Comment on attachment 8951389 [details]
Bug 1429973 part 2 - Move server-timing tests into http/2.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220660/diff/3-4/

Comment 27

[u408661]

Comment on attachment 8951390 [details]
Bug 1429973 part 3 - Remove hidden pref to allow plaintext server-timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220662/diff/3-4/

Comment 28

[u408661]

Comment on attachment 8951389 [details]
Bug 1429973 part 2 - Move server-timing tests into http/2.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220660/diff/4-5/

Comment 29

[u408661]

Comment on attachment 8951390 [details]
Bug 1429973 part 3 - Remove hidden pref to allow plaintext server-timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220662/diff/4-5/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8438cc4db770
part 0 - Update node-http2 to v3.3.8 for required bugfix. r=bagder
https://hg.mozilla.org/integration/autoland/rev/446096b9960d
part 1 - plumb through trailers in h2 to support server-timing. r=bagder
https://hg.mozilla.org/integration/autoland/rev/4bc56ae6d0fe
part 2 - Move server-timing tests into http/2. r=bagder
https://hg.mozilla.org/integration/autoland/rev/69c3b8eeba63
part 3 - Remove hidden pref to allow plaintext server-timing. r=bagder

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8438cc4db770
https://hg.mozilla.org/mozilla-central/rev/446096b9960d
https://hg.mozilla.org/mozilla-central/rev/4bc56ae6d0fe
https://hg.mozilla.org/mozilla-central/rev/69c3b8eeba63