1413999 - Add support for Server-Timing HTTP trailing header

Status: RESOLVED FIXED

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

Description

[Jason Duell]

This bug is specifically for the necko work involved here, not the devtools (and/or JS exposure) of the Server-timing data (see bug 1403051 and bug 1337684).

  https://w3c.github.io/server-timing/

Server-Timing is an HTTP trailer, not a header. :mcmanus tells me we currently parse trailers, but then silently throw them away.  We don't want to change that behavior in general (we don't want to encourage trailers), so we'll want to whitelist the Server-Timing trailer, store it somewhere (probably even just a mServerTiming header will work for now, since it's the only trailer we support) and then make it available via some new channel.getTrailers() call. That call should fail with NS_NOT_AVAIL unless the channel has hit OnStopRequest().  (I.e we can't provide a trailer header value until we've gotten all the data from the channel).

Note: we're fine having devtools show the Server-Timing intertwined with regular HTTP headers:  we just want a separate API internally for getting trailers (that feels safer than modifying the original HTTP header array that we make available at OnStartRequest time).

Comment 1

[cvazac]

Server-Timing is a header AND a trailer (https://w3c.github.io/server-timing/#dfn-server-timing-header-field). 

The "MUST" word around the reference to trailers in the spec is a little strong and will likely be downgraded to a "SHOULD" or "MAY". Header parsing is in Chrome behind experimental-flag.

Comment 2

[u408661]

Bad Jason, marking this triaged without a priority! :) (I assume you had a priority in mind, so I'll let you fix that.)

Comment 3

[Jason Duell]

Guilty as charged.  Also, thanks for inspiring me to notice that we have 30 other priority-less "triaged" necko bugs :)

Leaving this ni'd to me while I find an owner.

Comment 4

[Dragana Damjanovic [:dragana]]

this is on my plate :)

Comment 5

[Jason Duell]

Kershaw is going to code this and Dragana will mentor/review.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Part1: Serve-Timing header parser

It seems there is no existed parser that can be used directly to parse server-timing header, so I think the first step is to create a parser.

Patch summary:
 - A server-timing header parser
 - A test case which is most copied from chromium

Hi Dragana,
Could you take a look at this patch? Thanks.

Comment 7

[cvazac]

FYI there is an imminent spec change for shorter named parameters in the response header:
https://github.com/w3c/server-timing/pull/41

`duration` -> `dur`
`description` -> `desc`

Web interface (`PerformanceServerTiming`) remains unchanged.

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Kershaw Chang [:kershaw] from comment #6)
> Created attachment 8928947 [details] [diff] [review]
> Part1: Serve-Timing header parser
> 
> It seems there is no existed parser that can be used directly to parse
> server-timing header, so I think the first step is to create a parser.
> 
> Patch summary:
>  - A server-timing header parser
>  - A test case which is most copied from chromium
> 
> Hi Dragana,
> Could you take a look at this patch? Thanks.

Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList? Maybe extend it if needed? It looks like it is doing a similar parsing.

Comment 9

[cvazac]

FWIW, the format of the server-timing header is very similar to "Link":
https://tools.ietf.org/html/rfc5988#section-5

Comment 10

[Kershaw Chang [:kershaw]]

> Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList?
> Maybe extend it if needed? It looks like it is doing a similar parsing.

Thanks for the suggestion.
Actually, ParsedHeaderValueListList doesn't parse quoted string well. For example, it doesn't take care of backslash escape (RFC 2616 Section 2.2). So, I think I should merge my code with ParsedHeaderValueListList to make it work correctly.

I'll update a patch to do this later.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Part1: Serve-Timing header parser - v2

Summary:
1. Modify ParsedHeaderValueListList
  - Use nsCCharSeparatedTokenizer to split string by ','.
2. Modify ParsedHeaderValueList
  - Use nsCCharSeparatedTokenizer to split string by ';'.
  - Function ParseNameAndValue() is for getting name and value from the string containing '='.
3. Handle backslash escapes for quoted string.
4. Reuse ParsedHeaderValueListList to parse server-timing header.
5. Create a test case.

Thanks.

Comment 12

[Kershaw Chang [:kershaw]]

(In reply to cvazac from comment #7)
> FYI there is an imminent spec change for shorter named parameters in the
> response header:
> https://github.com/w3c/server-timing/pull/41
> 
> `duration` -> `dur`
> `description` -> `desc`
> 
> Web interface (`PerformanceServerTiming`) remains unchanged.

Thanks for the information.
This change has been included in my patch.

Comment 13

[cvazac]

Delimiting characters (; and ,) are legal in quoted strings.
See `qdtext` here: https://tools.ietf.org/html/rfc7230#section-3.2.6

This header:
Server-Timing: metric;desc="descr;,=iption";dur=123.4

Should yield entry: 
name = metric
duration = 123.4
description = descr;,=iption

Without running the code, I worry that splitting on commas and semi-colons won't handle this correctly.

Comment 14

[Kershaw Chang [:kershaw]]

(In reply to cvazac from comment #13)
> Delimiting characters (; and ,) are legal in quoted strings.
> See `qdtext` here: https://tools.ietf.org/html/rfc7230#section-3.2.6
> 
> This header:
> Server-Timing: metric;desc="descr;,=iption";dur=123.4
> 
> Should yield entry: 
> name = metric
> duration = 123.4
> description = descr;,=iption
> 
> Without running the code, I worry that splitting on commas and semi-colons
> won't handle this correctly.

Yes, you are right. I should not use tokenizer to split string.
Thanks for your great help!

Comment 15

[Kershaw Chang [:kershaw]]

Comment on attachment 8930838 [details] [diff] [review]
Part1: Serve-Timing header parser - v2

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

Cancel the review based on comment 14.

Comment 16

[Honza Bambas (:mayhemer)]

Just in case you might want to use it: https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/Tokenizer.h

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Part1: Serve-Timing header parser - v3

Summary:
  - Create a custom function to split the input string that contains quoted-string and quoted-pair.
 - The rest is the same as comment #11.

Thanks.

Comment 18

[Kershaw Chang [:kershaw]]

Attachment: Part2: Take http trailers from chunk decoder and add server timing attribute

Summary:
1. Save http trailers in the header array in chunk decoder.
2. Take trailers from chunk decoder in OnStopRequest().
3. Add serverTiming attribute in nsITimedChannel.
4. Parse server timing header when GetServerTiming() is called.

Thanks.

Comment 19

[Kershaw Chang [:kershaw]]

Attachment: Part3: IPC part

Summary:
 - Send http trailers to child process

Thanks.

Comment 20

[Dragana Damjanovic [:dragana]]

(In reply to Kershaw Chang [:kershaw] from comment #10)
> > Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList?
> > Maybe extend it if needed? It looks like it is doing a similar parsing.
> 
> Thanks for the suggestion.
> Actually, ParsedHeaderValueListList doesn't parse quoted string well. For
> example, it doesn't take care of backslash escape (RFC 2616 Section 2.2).
> So, I think I should merge my code with ParsedHeaderValueListList to make it
> work correctly.
> 
> I'll update a patch to do this later.

RFC 2616 is deprecated please refer to a new one rfc7231 or one of rfc-s 7230, 7232, 7233, 7234 or 7235 depends what are you looking for.

Comment 21

[Kershaw Chang [:kershaw]]

(In reply to Dragana Damjanovic [:dragana] from comment #20)
> (In reply to Kershaw Chang [:kershaw] from comment #10)
> > > Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList?
> > > Maybe extend it if needed? It looks like it is doing a similar parsing.
> > 
> > Thanks for the suggestion.
> > Actually, ParsedHeaderValueListList doesn't parse quoted string well. For
> > example, it doesn't take care of backslash escape (RFC 2616 Section 2.2).
> > So, I think I should merge my code with ParsedHeaderValueListList to make it
> > work correctly.
> > 
> > I'll update a patch to do this later.
> 
> RFC 2616 is deprecated please refer to a new one rfc7231 or one of rfc-s
> 7230, 7232, 7233, 7234 or 7235 depends what are you looking for.

It seems that the rules for quoted-string in RFC 2616 is almost the same as RFC 7230.
So, I think my patch still works.

Comment 22

[Dragana Damjanovic [:dragana]]

(In reply to Kershaw Chang [:kershaw] from comment #21)
> (In reply to Dragana Damjanovic [:dragana] from comment #20)
> > (In reply to Kershaw Chang [:kershaw] from comment #10)
> > > > Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList?
> > > > Maybe extend it if needed? It looks like it is doing a similar parsing.
> > > 
> > > Thanks for the suggestion.
> > > Actually, ParsedHeaderValueListList doesn't parse quoted string well. For
> > > example, it doesn't take care of backslash escape (RFC 2616 Section 2.2).
> > > So, I think I should merge my code with ParsedHeaderValueListList to make it
> > > work correctly.
> > > 
> > > I'll update a patch to do this later.
> > 
> > RFC 2616 is deprecated please refer to a new one rfc7231 or one of rfc-s
> > 7230, 7232, 7233, 7234 or 7235 depends what are you looking for.
> 
> It seems that the rules for quoted-string in RFC 2616 is almost the same as
> RFC 7230.
> So, I think my patch still works.

yes it is, I was just informing you for a future work, so that you know that is deprecated and not a valid standard.

Comment 23

[Dragana Damjanovic [:dragana]]

Comment on attachment 8932033 [details] [diff] [review]
Part1: Serve-Timing header parser - v3

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

please address my comments.

::: netwerk/protocol/http/nsHttp.cpp
@@ +585,5 @@
> +    if (nameLen > 0) {
> +        mName.Rebind(name, name + nameLen);
> +    }
> +    if (valLen > 0) {
> +        mValue.Rebind(val, val + valLen);

you can avoid Rebind() twice if the string is a quoted string. Rebind only make a dependent string so it is not very expensive. You can decide if you want to change this.

@@ +636,2 @@
>  
> +    const char *last = input;

is 'first' a better name here? While as was reading the code I was wondering why it is called 'last' :)

@@ +694,5 @@
> +    int32_t nameLen = nameEnd - nameStart + 1;
> +
> +    if (!nameLen || nameLen == inputLen) {
> +        mValues.AppendElement(ParsedHeaderPair(nameStart, nameLen,
> +                                               valueStart, 0, false));

use nullpt instead of valueStart, it makes more sense.

@@ +699,5 @@
> +        return;
> +    }
> +
> +    for (; nameEnd >= nameStart; --nameEnd) {
> +        if (!nsCRT::IsAsciiSpace(*nameEnd)) {

a not correctly formatted header, for example 'Server-Timing: something something2'

will return 'something something2' as a token, which is not a token because it has a space in it. I think it is ok, or should we return only the first one?

There is a text in the spec only regarding multiple server-timing-param-name.

Can you add a test for this case?
Also can you add a test for multiple server-timing-param-name as well?

@@ +717,5 @@
> +    if (*input != '"') {
> +        // The value is a token, not a quoted string.
> +        valueStart = input;
> +        for (valueEnd = input;
> +             *valueEnd && !nsCRT::IsAsciiSpace (*valueEnd) &&

you guarantee that this is a 0-ended string or you want to limit iteration until inputLen is reached?

In some cases you are checking inputLen.

@@ +787,5 @@
> +        // According to spec, the first ParsedHeaderPair's name is metric-name.
> +        auto timingHeader = mServerTimingHeaders.AppendElement();
> +        timingHeader->name.Assign(parsedHeader.mValues[index].mValues[0].mName);
> +
> +        if (parsedHeader.mValues[index].mValues.Length() < 1) {

should here be == 1 ?

Comment 24

[Dragana Damjanovic [:dragana]]

Comment on attachment 8932429 [details] [diff] [review]
Part2: Take http trailers from chunk decoder and add server timing attribute

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

Are you going to implement webIDL as well?

Chrome implements this already? Does Chrome request to have "Trailer: Server-Timing" header?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +4395,5 @@
>  }
>  
> +namespace {
> +
> +class nsServerTiming final : public nsIServerTiming

do we really need nsServerTiming and ServerTimingStruct?

Thinking of it do we really need ServerTimingParser class. You can also make all of this simpler:
GetServerTiming->ParseServerTimingHeader->ServerTimingParser parser(serverTimingHeader);->parser.Parse();->parser.TakeServerTimingHeaders();->append to an array->convert to another array.

::: netwerk/protocol/http/nsHttpChunkedDecoder.cpp
@@ +125,5 @@
> +                nsAutoCString headerNameOriginal;
> +                nsAutoCString val;
> +                if (NS_SUCCEEDED(mTrailers->ParseHeaderLine(nsDependentCSubstring(buf, count),
> +                                                            &hdr, &headerNameOriginal, &val))) {
> +                    Unused << mTrailers->SetHeaderFromNet(hdr, headerNameOriginal,

Please ignore all other headers except Server-Timing.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1758,5 @@
>  
>      // check for end-of-file
>      if ((mContentRead == mContentLength) ||
>          (mChunkedDecoder && mChunkedDecoder->ReachedEOF())) {
> +        mForTakeResponseTrailers = mChunkedDecoder

this should happen under a lock.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +368,5 @@
>  
>      // protected by nsHttp::GetLock()
>      nsHttpResponseHead             *mForTakeResponseHead;
>      bool                            mResponseHeadTaken;
> +    nsHttpHeaderArray              *mForTakeResponseTrailers;

Can we have a autoPtr here, anything better than raw pointer.

Also you have forgot to delete it in ~nsHttpTransaction() :)

Comment 25

[Kershaw Chang [:kershaw]]

(In reply to Dragana Damjanovic [:dragana] from comment #24)
> Comment on attachment 8932429 [details] [diff] [review]
> Part2: Take http trailers from chunk decoder and add server timing attribute
> 
> Review of attachment 8932429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thanks for the review.

> Are you going to implement webIDL as well?
> 

I am not sure, but that should be happened in another bug.

> Chrome implements this already? Does Chrome request to have "Trailer:
> Server-Timing" header?
> 

If I read their code correctly, the answer is no. It looks like that they don't even parse trailer in their chunked decoder.
https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.h?type=cs&sq=package:chromium&l=78

> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +4395,5 @@
> >  }
> >  
> > +namespace {
> > +
> > +class nsServerTiming final : public nsIServerTiming
> 
> do we really need nsServerTiming and ServerTimingStruct?
> 

nsIServerTiming is just for easily testing server timing headers in JS. Not sure if we want to let httpd.js support sending server timing header in trailers.
Perhaps only ServerTimingStruct is enough, if we don't have to test this in JS.

> Thinking of it do we really need ServerTimingParser class.

I think you mean maybe we can just provide header string to devtool or dom?
IMO, the server timing header string eventually need to be parsed, so I think it would be better only happened once in necko.

> You can also make all of this simpler:
> GetServerTiming->ParseServerTimingHeader->ServerTimingParser
> parser(serverTimingHeader);->parser.Parse();->parser.
> TakeServerTimingHeaders();->append to an array->convert to another array.
> 

Assume we don't need nsIServerTiming. The flow would be like:
GetServerTiming->ParseServerTimingHeader->ServerTimingParser parser(serverTimingHeader);->parser.Parse();
->parser.TakeServerTimingHeaders();->append to an array.

Or I can try to just pass nsHttpHeaderArray to ServerTimingParser, so the flow would be like:
GetServerTiming->ServerTimingParser parser(headerArray);->parser.Parse();
->parser.TakeServerTimingHeaders();->append to an array.

Not sure if there other way to make it simpler.

> ::: netwerk/protocol/http/nsHttpChunkedDecoder.cpp
> @@ +125,5 @@
> > +                nsAutoCString headerNameOriginal;
> > +                nsAutoCString val;
> > +                if (NS_SUCCEEDED(mTrailers->ParseHeaderLine(nsDependentCSubstring(buf, count),
> > +                                                            &hdr, &headerNameOriginal, &val))) {
> > +                    Unused << mTrailers->SetHeaderFromNet(hdr, headerNameOriginal,
> 
> Please ignore all other headers except Server-Timing.
> 

Will do.

> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +1758,5 @@
> >  
> >      // check for end-of-file
> >      if ((mContentRead == mContentLength) ||
> >          (mChunkedDecoder && mChunkedDecoder->ReachedEOF())) {
> > +        mForTakeResponseTrailers = mChunkedDecoder
> 
> this should happen under a lock.
> 

Will do.

> ::: netwerk/protocol/http/nsHttpTransaction.h
> @@ +368,5 @@
> >  
> >      // protected by nsHttp::GetLock()
> >      nsHttpResponseHead             *mForTakeResponseHead;
> >      bool                            mResponseHeadTaken;
> > +    nsHttpHeaderArray              *mForTakeResponseTrailers;
> 
> Can we have a autoPtr here, anything better than raw pointer.
> 
> Also you have forgot to delete it in ~nsHttpTransaction() :)

Will do.

Comment 26

[Dragana Damjanovic [:dragana]]

(In reply to Kershaw Chang [:kershaw] from comment #25)
> (In reply to Dragana Damjanovic [:dragana] from comment #24)
> > Comment on attachment 8932429 [details] [diff] [review]
> > Part2: Take http trailers from chunk decoder and add server timing attribute
> > 
> > Review of attachment 8932429 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> 
> Thanks for the review.
> 
> > Are you going to implement webIDL as well?
> > 
> 
> I am not sure, but that should be happened in another bug.
> 

As you prefer, but please file a bug if you are not going to implement it here.


> > Chrome implements this already? Does Chrome request to have "Trailer:
> > Server-Timing" header?
> > 
> 
> If I read their code correctly, the answer is no. It looks like that they
> don't even parse trailer in their chunked decoder.
> https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.
> h?type=cs&sq=package:chromium&l=78
> 


I just saw some tweets and have not look deep to figure out if they have implemented a trailer parsing as well. I think we do not need to enforce it.

> > ::: netwerk/protocol/http/HttpBaseChannel.cpp
> > @@ +4395,5 @@
> > >  }
> > >  
> > > +namespace {
> > > +
> > > +class nsServerTiming final : public nsIServerTiming
> > 
> > do we really need nsServerTiming and ServerTimingStruct?
> > 
> 
> nsIServerTiming is just for easily testing server timing headers in JS. Not
> sure if we want to let httpd.js support sending server timing header in
> trailers.
> Perhaps only ServerTimingStruct is enough, if we don't have to test this in
> JS.

if you implement webIDL interface you can use that for testing.
how would interface to dev-tools look like? you will need an interface to js (the dev-tools patch from bug 1403051 parse s-t header in js, but they do not need to do that if we are doing it for them)? The performance timing dom code uses nsITimedChannel to get timing info. and dev-tools use it as well.

if you do not implement trailers in httpd.js (or use that interface Jason was talking about) how are you planning to automatically test the trailer code?

> 
> > Thinking of it do we really need ServerTimingParser class.
> 
> I think you mean maybe we can just provide header string to devtool or dom?
> IMO, the server timing header string eventually need to be parsed, so I
> think it would be better only happened once in necko.
> 
> > You can also make all of this simpler:
> > GetServerTiming->ParseServerTimingHeader->ServerTimingParser
> > parser(serverTimingHeader);->parser.Parse();->parser.
> > TakeServerTimingHeaders();->append to an array->convert to another array.
> > 
> 
> Assume we don't need nsIServerTiming. The flow would be like:
> GetServerTiming->ParseServerTimingHeader->ServerTimingParser
> parser(serverTimingHeader);->parser.Parse();
> ->parser.TakeServerTimingHeaders();->append to an array.
> 
> Or I can try to just pass nsHttpHeaderArray to ServerTimingParser, so the
> flow would be like:
> GetServerTiming->ServerTimingParser parser(headerArray);->parser.Parse();
> ->parser.TakeServerTimingHeaders();->append to an array.
> 
> Not sure if there other way to make it simpler.

This is only helper function. WebIDL and dev-tools are the only one using it. Therefore it  would be good to implement exactly what they need. Probably our tests will need the same interface.

Comment 27

[Kershaw Chang [:kershaw]]

> As you prefer, but please file a bug if you are not going to implement it
> here.
> 

I've filed bug 1423495 for this.

> 
> > > Chrome implements this already? Does Chrome request to have "Trailer:
> > > Server-Timing" header?
> > > 
> > 
> > If I read their code correctly, the answer is no. It looks like that they
> > don't even parse trailer in their chunked decoder.
> > https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.
> > h?type=cs&sq=package:chromium&l=78
> > 
> 
> 
> I just saw some tweets and have not look deep to figure out if they have
> implemented a trailer parsing as well. I think we do not need to enforce it.
> 

Do you mean we don't have to parse trailers or?

> if you implement webIDL interface you can use that for testing.
> how would interface to dev-tools look like? you will need an interface to js
> (the dev-tools patch from bug 1403051 parse s-t header in js, but they do
> not need to do that if we are doing it for them)? The performance timing dom
> code uses nsITimedChannel to get timing info. and dev-tools use it as well.
> 

I already ask Honza about the requirements from dev-tool in bug 1403051.

> if you do not implement trailers in httpd.js (or use that interface Jason
> was talking about) how are you planning to automatically test the trailer
> code?
> 

That's true. It is necessary to implement trailer in httpd.js.

> This is only helper function. WebIDL and dev-tools are the only one using
> it. Therefore it  would be good to implement exactly what they need.
> Probably our tests will need the same interface.

Let's ask them first before deciding what to do.

Comment 28

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #27)
> > As you prefer, but please file a bug if you are not going to implement it
> > here.
> > 
> 
> I've filed bug 1423495 for this.
> 
> > 
> > > > Chrome implements this already? Does Chrome request to have "Trailer:
> > > > Server-Timing" header?
> > > > 
> > > 
> > > If I read their code correctly, the answer is no. It looks like that they
> > > don't even parse trailer in their chunked decoder.
> > > https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.
> > > h?type=cs&sq=package:chromium&l=78
> > > 
> > 
> > 
> > I just saw some tweets and have not look deep to figure out if they have
> > implemented a trailer parsing as well. I think we do not need to enforce it.
> > 
> 
> Do you mean we don't have to parse trailers or?
> 
> > if you implement webIDL interface you can use that for testing.
> > how would interface to dev-tools look like? you will need an interface to js
> > (the dev-tools patch from bug 1403051 parse s-t header in js, but they do
> > not need to do that if we are doing it for them)? The performance timing dom
> > code uses nsITimedChannel to get timing info. and dev-tools use it as well.
> > 
> 
> I already ask Honza about the requirements from dev-tool in bug 1403051.
> 

It looks to me that we really need a array of nsIServerTiming in nsITimedChannel, so dev-tool and dom can get server timing info from nsITimedChannel.
I think I can get rid off ServerTimingStruct and let the parser return nsTArray<nsCOMPtr<nsIServerTiming>>. This could make the code a bit simpler. What do you think, Dragana?

Comment 29

[Dragana Damjanovic [:dragana]]

(In reply to Kershaw Chang [:kershaw] from comment #28)
> (In reply to Kershaw Chang [:kershaw] from comment #27)
> 
> It looks to me that we really need a array of nsIServerTiming in
> nsITimedChannel, so dev-tool and dom can get server timing info from
> nsITimedChannel.
> I think I can get rid off ServerTimingStruct and let the parser return
> nsTArray<nsCOMPtr<nsIServerTiming>>. This could make the code a bit simpler.
> What do you think, Dragana?


That is what I was thinking about :)

Comment 30

[Yoav Weiss]

(In reply to Dragana Damjanovic [:dragana] from comment #26)
> (In reply to Kershaw Chang [:kershaw] from comment #25)
> > (In reply to Dragana Damjanovic [:dragana] from comment #24)
> > > Comment on attachment 8932429 [details] [diff] [review]
> > > Part2: Take http trailers from chunk decoder and add server timing attribute
> > > 
> 
> > > Chrome implements this already? Does Chrome request to have "Trailer:
> > > Server-Timing" header?
> > > 
> > 
> > If I read their code correctly, the answer is no. It looks like that they
> > don't even parse trailer in their chunked decoder.
> > https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.
> > h?type=cs&sq=package:chromium&l=78
> > 
> 
> 

The Chromium implementation does not support Server-Timing trailers, only headers. cvazac (from previous comments) is the Chromium implementer as well as the spec editor, so you can probably ping him for questions related to that implementation.

Comment 31

[Kershaw Chang [:kershaw]]

Attachment: Part1: Serve-Timing header parser - v4

Please see below for the changes between v3 and v4.

> ::: netwerk/protocol/http/nsHttp.cpp
> @@ +585,5 @@
> > +    if (nameLen > 0) {
> > +        mName.Rebind(name, name + nameLen);
> > +    }
> > +    if (valLen > 0) {
> > +        mValue.Rebind(val, val + valLen);
> 
> you can avoid Rebind() twice if the string is a quoted string. Rebind only
> make a dependent string so it is not very expensive. You can decide if you
> want to change this.
> 

Fixed.

> @@ +636,2 @@
> >  
> > +    const char *last = input;
> 
> is 'first' a better name here? While as was reading the code I was wondering
> why it is called 'last' :)
> 

Agree. This should be "first".

> @@ +694,5 @@
> > +    int32_t nameLen = nameEnd - nameStart + 1;
> > +
> > +    if (!nameLen || nameLen == inputLen) {
> > +        mValues.AppendElement(ParsedHeaderPair(nameStart, nameLen,
> > +                                               valueStart, 0, false));
> 
> use nullpt instead of valueStart, it makes more sense.
> 

Done.

> @@ +699,5 @@
> > +        return;
> > +    }
> > +
> > +    for (; nameEnd >= nameStart; --nameEnd) {
> > +        if (!nsCRT::IsAsciiSpace(*nameEnd)) {
> 
> a not correctly formatted header, for example 'Server-Timing: something
> something2'
> 
> will return 'something something2' as a token, which is not a token because
> it has a space in it. I think it is ok, or should we return only the first
> one?
>

I am not sure what is right to do. It seems that this is not specified.
But, I think returning the first one makes more sense, so I also made this change in this patch.
 
> There is a text in the spec only regarding multiple server-timing-param-name.
> 
> Can you add a test for this case?
> Also can you add a test for multiple server-timing-param-name as well?
> 

Already added in TestServerTimingHeader.cpp.

> @@ +717,5 @@
> > +    if (*input != '"') {
> > +        // The value is a token, not a quoted string.
> > +        valueStart = input;
> > +        for (valueEnd = input;
> > +             *valueEnd && !nsCRT::IsAsciiSpace (*valueEnd) &&
> 
> you guarantee that this is a 0-ended string or you want to limit iteration
> until inputLen is reached?
> 
> In some cases you are checking inputLen.
> 

Right now I check 0-ended string for all places.

> @@ +787,5 @@
> > +        // According to spec, the first ParsedHeaderPair's name is metric-name.
> > +        auto timingHeader = mServerTimingHeaders.AppendElement();
> > +        timingHeader->name.Assign(parsedHeader.mValues[index].mValues[0].mName);
> > +
> > +        if (parsedHeader.mValues[index].mValues.Length() < 1) {
> 
> should here be == 1 ?

Yes.

Please note that I also copy all test cases from the Chromium [1] to TestServerTimingHeader.cpp.
The only failed test case is:
testServerTimingHeader("metric=foo;dur;dur=123.4,metric2",
                         {{"metric", "0", ""}});

In my current patch, the result is {{"metric", "123.4", ""}, {"metric2", "0", ""}}.
I think this should be fine, since this is not covered by spec?

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp?type=cs&sq=package:chromium&l=510

Comment 32

[Kershaw Chang [:kershaw]]

Attachment: Diff for v3 and v4 of part1.

Just in case you might want to compare.

Comment 33

[Kershaw Chang [:kershaw]]

Attachment: Part2: Take http trailers from chunk decoder and add server timing attribute - v2

Summary:
 - Address previous comments.
 - Move ServerTimingParser from nsHttp.h to nsSerTiming.h.

Thanks.

Comment 34

[Kershaw Chang [:kershaw]]

Attachment: Part4: Test case

Summary:
 - To check whether the serving timing header can be parsed successfully from response and trailer headers.

Thanks.

Comment 35

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6db274042d77b4182bd924319744ac98f3281a5

Comment 36

[Dragana Damjanovic [:dragana]]

(In reply to Kershaw Chang [:kershaw] from comment #31)
> Created attachment 8936932 [details] [diff] [review]
> Part1: Serve-Timing header parser - v4
> 
> Please note that I also copy all test cases from the Chromium [1] to
> TestServerTimingHeader.cpp.
> The only failed test case is:
> testServerTimingHeader("metric=foo;dur;dur=123.4,metric2",
>                          {{"metric", "0", ""}});
> 
> In my current patch, the result is {{"metric", "123.4", ""}, {"metric2",
> "0", ""}}.

you accept 'metric=foo' as a name 'metric' that is ok, but you then have 'dur' 2 times and the spec says that you should only take the first one... although 'dur' is not a valid server-timing-param it is missing '=" and server-timing-param-value.

The spec says "individual server-timing-param-name SHOULD NOT appear multiple times within a server-timing-metric. If any parameter is specified more than once, only the first instance is to be considered."

it is talking only about the multiple server-timing-param-name, so the result should be {{"metric", "0", ""}, {"metric2", "0", ""}}


> I think this should be fine, since this is not covered by spec?
> 
> [1]
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/
> network/HTTPParsersTest.cpp?type=cs&sq=package:chromium&l=510

Comment 37

[Kershaw Chang [:kershaw]]

(In reply to Dragana Damjanovic [:dragana] from comment #36)
> (In reply to Kershaw Chang [:kershaw] from comment #31)
> > Created attachment 8936932 [details] [diff] [review]
> > Part1: Serve-Timing header parser - v4
> > 
> > Please note that I also copy all test cases from the Chromium [1] to
> > TestServerTimingHeader.cpp.
> > The only failed test case is:
> > testServerTimingHeader("metric=foo;dur;dur=123.4,metric2",
> >                          {{"metric", "0", ""}});
> > 
> > In my current patch, the result is {{"metric", "123.4", ""}, {"metric2",
> > "0", ""}}.
> 
> you accept 'metric=foo' as a name 'metric' that is ok, but you then have
> 'dur' 2 times and the spec says that you should only take the first one...
> although 'dur' is not a valid server-timing-param it is missing '=" and
> server-timing-param-value.
> 
> The spec says "individual server-timing-param-name SHOULD NOT appear
> multiple times within a server-timing-metric. If any parameter is specified
> more than once, only the first instance is to be considered."
> 
> it is talking only about the multiple server-timing-param-name, so the
> result should be {{"metric", "0", ""}, {"metric2", "0", ""}}
> 

According to the tests [1] from chromium, the result of "metric;dur;dur=123.4" should be {"metric", "123.4", ""}.
I think it makes sense to take the first "valid" server-timing-param.


[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp?type=cs&sq=package:chromium&l=651-653

Comment 38

[Kershaw Chang [:kershaw]]

Attachment: Part5: Fix-test_altsvc.js

Summary:
 - Use quoted-string as the header value of "Alt-Svc".
 - For example, the original header value of Alt-Svc in test_altsvc.js h2=foo.example.com:62933 is wrong. The correct value should be h2="foo.example.com:62933".

Thanks.

Comment 39

[Dragana Damjanovic [:dragana]]

(In reply to Kershaw Chang [:kershaw] from comment #38)
> Created attachment 8937925 [details] [diff] [review]
> Part5: Fix-test_altsvc.js
> 
> Summary:
>  - Use quoted-string as the header value of "Alt-Svc".
>  - For example, the original header value of Alt-Svc in test_altsvc.js
> h2=foo.example.com:62933 is wrong. The correct value should be
> h2="foo.example.com:62933".
> 
> Thanks.

Does the old version, h2=foo.example.com:62933, still works?

Let's try not to regress alt-svc.

Comment 40

[Kershaw Chang [:kershaw]]

(In reply to Dragana Damjanovic [:dragana] from comment #39)
> (In reply to Kershaw Chang [:kershaw] from comment #38)
> > Created attachment 8937925 [details] [diff] [review]
> > Part5: Fix-test_altsvc.js
> > 
> > Summary:
> >  - Use quoted-string as the header value of "Alt-Svc".
> >  - For example, the original header value of Alt-Svc in test_altsvc.js
> > h2=foo.example.com:62933 is wrong. The correct value should be
> > h2="foo.example.com:62933".
> > 
> > Thanks.
> 
> Does the old version, h2=foo.example.com:62933, still works?
> 
> Let's try not to regress alt-svc.

Unfortunately, "h2=foo.example.com:62933" does not work.
So, should we allow invalid token for parsing "Alt-Svc" and "Server-Timing" headers? Or only allow "Alt-Svc" to have an invalid token?

IMO, we should allow for both headers. This makes the code look a bit clear.

Comment 41

[Dragana Damjanovic [:dragana]]

(In reply to Kershaw Chang [:kershaw] from comment #40)
> (In reply to Dragana Damjanovic [:dragana] from comment #39)
> > (In reply to Kershaw Chang [:kershaw] from comment #38)
> > > Created attachment 8937925 [details] [diff] [review]
> > > Part5: Fix-test_altsvc.js
> > > 
> > > Summary:
> > >  - Use quoted-string as the header value of "Alt-Svc".
> > >  - For example, the original header value of Alt-Svc in test_altsvc.js
> > > h2=foo.example.com:62933 is wrong. The correct value should be
> > > h2="foo.example.com:62933".
> > > 
> > > Thanks.
> > 
> > Does the old version, h2=foo.example.com:62933, still works?
> > 
> > Let's try not to regress alt-svc.
> 
> Unfortunately, "h2=foo.example.com:62933" does not work.
> So, should we allow invalid token for parsing "Alt-Svc" and "Server-Timing"
> headers? Or only allow "Alt-Svc" to have an invalid token?
> 
> IMO, we should allow for both headers. This makes the code look a bit clear.


Let's ask Patrick, it is his draft and he have seen more of alt-svc headers.

Patrick, the current code allows alt-svc header:
Alt-svc: h2=foo.example.com:62933
without '"'
The spec says the value of "alt-authority" should be quoted-string, i.e. Alt-svc: h2="foo.example.com:62933"

The new code will enforce quoted-string. Do you think that will cause a regression?

Comment 42

[Patrick McManus [:mcmanus]]

you should not regress alt-svc but implement server-timings the stricter way. alt-svc should have been done that way initially but fixing it is all risk and no reward

Comment 43

[cvazac]

I opened up this issue against the spec to clarify the handling of params with unspecified or invalid values. Would love your participation!

https://github.com/w3c/server-timing/issues/46

Comment 44

[Kershaw Chang [:kershaw]]

Attachment: Add a flag to allow invalid token value

As Patrick said, this patch adds a flag to allow invalid token value when parsing Alt-Svc header.

Comment 45

[Yoav Weiss]

Note that the tendancy on the spec issue and in the related PR[1] is to modify the parsing to not check for the values' validity.
Patrick - do you think this is a mistake and we should take validity into account? (that's how I read #42, but would love clarifications)

[1] https://github.com/w3c/server-timing/pull/47

Comment 46

[Dragana Damjanovic [:dragana]]

(In reply to Yoav Weiss from comment #45)
> Note that the tendancy on the spec issue and in the related PR[1] is to
> modify the parsing to not check for the values' validity.
> Patrick - do you think this is a mistake and we should take validity into
> account? (that's how I read #42, but would love clarifications)
> 
> [1] https://github.com/w3c/server-timing/pull/47


For Service-Timing - this the first implementation in Firefox and it would be good to well define everything in the spec and have appropriate tests so that there is no confusion afterwords.

In my opinion the PR looks good, just one thing

"If any server-timing-param-name is specified more than once, only the first instance is to be considered, even if it is incomplete..."

actually "even if it is incomplete.." should it be "even if its specification is..." in the example server-timing-params are incomplete  or invalid not server-timing-param-names.

Just a note:
Firefox will interpret 'dur=123.4 567.8;desc=...' (this is invalid) in the same way as 'dur=123.4;desc=...'

similar 'desc=something1 something2;' (note a non-quoted string and it is not a token, therefore invalid) will be the same as 'desc=something1;'

Comment 47

[Dragana Damjanovic [:dragana]]

Comment on attachment 8938308 [details] [diff] [review]
Add a flag to allow invalid token value

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

::: netwerk/protocol/http/nsHttp.h
@@ +292,5 @@
>  
>  class ParsedHeaderValueList
>  {
>  public:
> +    ParsedHeaderValueList(const char *t, uint32_t len, bool allowInvalidValue);

Can you please add a longer comment explaining what allowInvalidValue does and where and why it is used.

Comment 48

[cvazac]

A related but different issue for invalid params:
https://github.com/w3c/server-timing/issues/48

Comment 49

[Kershaw Chang [:kershaw]]

Attachment: Part1: Serve-Timing header parser, r=dragana

unbitrotting and carry r+.

Comment 50

[Kershaw Chang [:kershaw]]

Attachment: Part2: Take http trailers from chunk decoder and add server timing attribute, r=dragana

unbitrotting

Comment 51

[Kershaw Chang [:kershaw]]

Attachment: Part3: IPC part, r=dragana

Carry r+.

Comment 52

[Kershaw Chang [:kershaw]]

Attachment: Part4: Test case, r=dragana

Carry r+.

Comment 53

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8d4a85ca6775250d6d672a175f6fb7940ef52ee

Comment 54

[Kershaw Chang [:kershaw]]

Attachment: Part4: Test case, r=dragana

Fix the compiler error of gtest.

Comment 55

[Kershaw Chang [:kershaw]]

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b7557b5519c943d99e3da726d8cffc36ad1fb93

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b226b0cff55a
Part1: Parse Serve-Timing header r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/c92b27c7f6bd
Part2: Take http trailers from chunk decoder  and add serverTiming attribute in nsITimedChannel r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/c29cf020715f
Part3: IPC part, send trailers to child  process r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ad893078b2
Part4: Test case r=dragana

Comment 57

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b226b0cff55a
https://hg.mozilla.org/mozilla-central/rev/c92b27c7f6bd
https://hg.mozilla.org/mozilla-central/rev/c29cf020715f
https://hg.mozilla.org/mozilla-central/rev/d6ad893078b2

Comment 58

[u408661]

So I'm fairly certain this won't work for resources served over h2 - see https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/netwerk/protocol/http/Http2Session.cpp#1436 - h2 just flat-out ignores trailers right now (it silently drops them on the floor after decompressing them, in order to keep our compression context in sync with the server).

Comment 59

[Dragana Damjanovic [:dragana]]

(In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail]) from comment #58)
> So I'm fairly certain this won't work for resources served over h2 - see
> https://searchfox.org/mozilla-central/rev/
> 03877052c151a8f062eea177f684a2743cd7b1d5/netwerk/protocol/http/Http2Session.
> cpp#1436 - h2 just flat-out ignores trailers right now (it silently drops
> them on the floor after decompressing them, in order to keep our compression
> context in sync with the server).

Kershaw, can you take a look and open another bug for this?

Comment 60

[Kershaw Chang [:kershaw]]

(In reply to Dragana Damjanovic [:dragana] from comment #59)
> (In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail])
> from comment #58)
> > So I'm fairly certain this won't work for resources served over h2 - see
> > https://searchfox.org/mozilla-central/rev/
> > 03877052c151a8f062eea177f684a2743cd7b1d5/netwerk/protocol/http/Http2Session.
> > cpp#1436 - h2 just flat-out ignores trailers right now (it silently drops
> > them on the floor after decompressing them, in order to keep our compression
> > context in sync with the server).
> 
> Kershaw, can you take a look and open another bug for this?

Thanks. I've filed bug 1429973 for this.